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

From: Arnd Bergmann <arnd@arndb.de>
To: "Keller, Jacob E" <jacob.e.keller@intel.com>
Cc: Richard Cochran <richardcochran@gmail.com>,
	Nicolas Pitre <nico@fluxnic.net>,
	"Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
	"Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	Kurt Kanzenbach <kurt@linutronix.de>,
	"Saleem, Shiraz" <shiraz.saleem@intel.com>,
	"Ertman, David M" <david.m.ertman@intel.com>,
	"intel-wired-lan@lists.osuosl.org"
	<intel-wired-lan@lists.osuosl.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Yangbo Lu <yangbo.lu@nxp.com>
Subject: Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK dependencies
Date: Wed, 4 Aug 2021 13:18:48 +0200	[thread overview]
Message-ID: <CAK8P3a2=pLt9iwE1xw5yhk=DA_i5HLsf5q94GQ2BCG48rS45jQ@mail.gmail.com> (raw)
In-Reply-To: <CAK8P3a06enZOf=XyZ+zcAwBczv41UuCTz+=0FMf2gBz1_cOnZQ@mail.gmail.com>

On Tue, Aug 3, 2021 at 8:27 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Tue, Aug 3, 2021 at 7:19 PM Keller, Jacob E <jacob.e.keller@intel.com> wrote:
> > > On Tue, Aug 3, 2021 at 6:14 PM Richard Cochran <richardcochran@gmail.com> wrote:
>
> > 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