Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK dependencies

On Tue, Aug 3, 2021 at 8:27 PM Arnd Bergmann [off-list ref] wrote:

On Tue, Aug 3, 2021 at 7:19 PM Keller, Jacob E [off-list ref] wrote:

quoted

quoted

On Tue, Aug 3, 2021 at 6:14 PM Richard Cochran [off-list ref] wrote:

quoted

There is an alternative solution to fixing the imply keyword:

Make the drivers use it properly by *actually* conditionally enabling the feature only when IS_REACHABLE, i.e. fix ice so that it uses IS_REACHABLE instead of IS_ENABLED, and so that its stub implementation in ice_ptp.h actually just silently does nothing but returns 0 to tell the rest of the driver things are fine.

I would consider IS_REACHABLE() part of the problem, not the solution, it makes
things magically build, but then surprises users at runtime when they do not get
the intended behavior.

Case in point: two patches from Yangbo Lu that call into the ptp core
from built-in
network code look like they could never have worked with CONFIG_PTP_1588_CLOCK,
but did not cause a link failure because of the IS_REACHABLE() check, see
commit d7c088265588 ("net: socket: support hardware timestamp conversion to
PHC bound") and c156174a6707 ("ethtool: add a new command for getting PHC
virtual clocks").

I found that by testing my patch that turns the IS_REACHABLE() back into
IS_ENABLED() and got

aarch64-linux-ld: net/socket.o: in function `__sock_recv_timestamp':
socket.c:(.text+0x1f20): undefined reference to `ptp_convert_timestamp'
aarch64-linux-ld: net/ethtool/common.o: in function `ethtool_get_phc_vclocks':
common.c:(.text+0x35c): undefined reference to `ptp_get_vclocks_index'

I added a workaround to my patch now to keep it working as before, but this
needs to be fixed properly. The easiest way would be to no longer support
modular PTP at all as Richard would like to do anyway, but I have not
attempted other fixes.

       Arnd