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

Commit Message

Arnd Bergmann Aug. 2, 2021, 2:59 p.m. UTC

From: Arnd Bergmann <[e-mail]>

The 'imply' keyword does not do what most people think it does, it only
politely asks Kconfig to turn on another symbol, but does not prevent
it from being disabled manually or built as a loadable module when the
user is built-in. In the ICE driver, the latter now causes a link failure:

aarch64-linux-ld: drivers/net/ethernet/intel/ice/ice_main.o: in function `ice_eth_ioctl':
ice_main.c:(.text+0x13b0): undefined reference to `ice_ptp_get_ts_config'
ice_main.c:(.text+0x13b0): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `ice_ptp_get_ts_config'
aarch64-linux-ld: ice_main.c:(.text+0x13bc): undefined reference to `ice_ptp_set_ts_config'
ice_main.c:(.text+0x13bc): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `ice_ptp_set_ts_config'
aarch64-linux-ld: drivers/net/ethernet/intel/ice/ice_main.o: in function `ice_prepare_for_reset':
ice_main.c:(.text+0x31fc): undefined reference to `ice_ptp_release'
ice_main.c:(.text+0x31fc): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `ice_ptp_release'
aarch64-linux-ld: drivers/net/ethernet/intel/ice/ice_main.o: in function `ice_rebuild':

For the other Intel network drivers, there is no link error when the
drivers are built-in and PTP is a loadable module, because
linux/ptp_clock_kernel.h contains an IS_REACHABLE() check, but this
just changes the compile-time failure to a runtime failure, which is
arguably worse.

Change all the Intel drivers to use the 'depends on PTP_1588_CLOCK ||
!PTP_1588_CLOCK' trick to prevent the broken configuration, as we
already do for several other drivers. To avoid circular dependencies,
this also requires changing the IGB driver back to using the normal
'depends on I2C' instead of 'select I2C'.

This is a recurring problem in many drivers, and we have discussed
it several times befores, without reaching a consensus. I'm providing
a link to the previous email thread for reference, which discusses
some related problems, though I can't find what reasons there were
against the approach with the extra Kconfig dependency.

Fixes: 06c16d89d2cb ("ice: register 1588 PTP clock device object for E810 devices")
Link: https://lore.kernel.org/netdev/CAK8P3a3=[e-mail]/
Link: https://lore.kernel.org/netdev/[e-mail]/
Cc: Richard Cochran <[e-mail]>
Signed-off-by: Arnd Bergmann <[e-mail]>
---
Changes in v2:
- include a missing patch hunk
- link to a previous discussion with Richard Cochran
---
 drivers/net/ethernet/intel/Kconfig | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Richard Cochran Aug. 2, 2021, 4:49 p.m. UTC | #1

On Mon, Aug 02, 2021 at 04:59:33PM +0200, Arnd Bergmann wrote:

> This is a recurring problem in many drivers, and we have discussed
> it several times befores, without reaching a consensus. I'm providing
> a link to the previous email thread for reference, which discusses
> some related problems, though I can't find what reasons there were
> against the approach with the extra Kconfig dependency.

Quoting myself in the thread from 12 Nov 2020:

   This whole "implies" thing turned out to be a colossal PITA.

   I regret the fact that it got merged.  It wasn't my idea.

This whole thing came about because Nicolas Pitre wanted to make PHC
core support into a module and also to be able to remove dynamic posix
clock support for tinification.  It has proved to be a never ending
source of confusion.

Let's restore the core functionality and remove "implies" for good.

Thanks,
Richard

Jacob Keller Aug. 2, 2021, 7:54 p.m. UTC | #2

On 8/2/2021 9:49 AM, Richard Cochran wrote:
> On Mon, Aug 02, 2021 at 04:59:33PM +0200, Arnd Bergmann wrote:
> 
>> This is a recurring problem in many drivers, and we have discussed
>> it several times befores, without reaching a consensus. I'm providing
>> a link to the previous email thread for reference, which discusses
>> some related problems, though I can't find what reasons there were
>> against the approach with the extra Kconfig dependency.
> 
> Quoting myself in the thread from 12 Nov 2020:
> 
>    This whole "implies" thing turned out to be a colossal PITA.
> 
>    I regret the fact that it got merged.  It wasn't my idea.
> 
> This whole thing came about because Nicolas Pitre wanted to make PHC
> core support into a module and also to be able to remove dynamic posix
> clock support for tinification.  It has proved to be a never ending
> source of confusion.
> 
> Let's restore the core functionality and remove "implies" for good.
> 
> Thanks,
> Richard
> 

So go back to "select"?

It looks like Arnd proposed in the thread a solution that did a sort of
"please enable this" but still let you disable it.

An alternative (unfortunately per-driver...) solution was to setup the
drivers so that they gracefully fall back to disabling PTP if the PTP
core support is not reachable.. but that obviously requires that drivers
do the right thing, and at least Intel drivers have not tested this
properly.

I'm definitely in favor of removing "implies" entirely. The semantics
are unclear, and the fact that it doesn't handle the case of "i'm
builtin, so my implies can't be modules"...

I don't really like the syntax of the double "depends on A || !A".. I'd
prefer if we had some keyword for this, since it would be more obvious
and not run against the standard logic (A || !A is a tautology!)

Thanks,
Jake

Arnd Bergmann Aug. 2, 2021, 8:32 p.m. UTC | #3

On Mon, Aug 2, 2021 at 9:54 PM Keller, Jacob E <[e-mail]> wrote:
>
> So go back to "select"?
>
> It looks like Arnd proposed in the thread a solution that did a sort of
> "please enable this" but still let you disable it.
>
> An alternative (unfortunately per-driver...) solution was to setup the
> drivers so that they gracefully fall back to disabling PTP if the PTP
> core support is not reachable.. but that obviously requires that drivers
> do the right thing, and at least Intel drivers have not tested this
> properly.
>
> I'm definitely in favor of removing "implies" entirely. The semantics
> are unclear, and the fact that it doesn't handle the case of "i'm
> builtin, so my implies can't be modules"...
>
> I don't really like the syntax of the double "depends on A || !A".. I'd
> prefer if we had some keyword for this, since it would be more obvious
> and not run against the standard logic (A || !A is a tautology!)

I think the main reason we don't have a keyword for it is that nobody
so far has come up with an English word that expresses what it is
supposed to mean.

You can do something like it for a particular symbol though, such as

config MAY_USE_PTP_1588_CLOCK
       def_tristate PTP_1588_CLOCK || !PTP_1588_CLOCK

 config E1000E
        tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
        depends on PCI && (!SPARC32 || BROKEN)
+       depends on MAY_USE_PTP_1588_CLOCK
        select CRC32
-       imply PTP_1588_CLOCK


          Arnd

Jacob Keller Aug. 2, 2021, 8:46 p.m. UTC | #4

> -----Original Message-----
> From: Arnd Bergmann <[e-mail]>
> Sent: Monday, August 02, 2021 1:32 PM
> To: Keller, Jacob E <[e-mail]>
> Cc: Richard Cochran <[e-mail]>; Nicolas Pitre
> <[e-mail]>; Brandeburg, Jesse <[e-mail]>;
> Nguyen, Anthony L <[e-mail]>; David S. Miller
> <[e-mail]>; Jakub Kicinski <[e-mail]>; Arnd Bergmann
> <[e-mail]>; Kurt Kanzenbach <[e-mail]>; Saleem, Shiraz
> <[e-mail]>; Ertman, David M <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK
> dependencies
> 
> On Mon, Aug 2, 2021 at 9:54 PM Keller, Jacob E <[e-mail]>
> wrote:
> >
> > So go back to "select"?
> >
> > It looks like Arnd proposed in the thread a solution that did a sort of
> > "please enable this" but still let you disable it.
> >
> > An alternative (unfortunately per-driver...) solution was to setup the
> > drivers so that they gracefully fall back to disabling PTP if the PTP
> > core support is not reachable.. but that obviously requires that drivers
> > do the right thing, and at least Intel drivers have not tested this
> > properly.
> >
> > I'm definitely in favor of removing "implies" entirely. The semantics
> > are unclear, and the fact that it doesn't handle the case of "i'm
> > builtin, so my implies can't be modules"...
> >
> > I don't really like the syntax of the double "depends on A || !A".. I'd
> > prefer if we had some keyword for this, since it would be more obvious
> > and not run against the standard logic (A || !A is a tautology!)
> 
> I think the main reason we don't have a keyword for it is that nobody
> so far has come up with an English word that expresses what it is
> supposed to mean.
> 

Right. I don't have a great example that's a single word either.

> You can do something like it for a particular symbol though, such as
> 
> config MAY_USE_PTP_1588_CLOCK
>        def_tristate PTP_1588_CLOCK || !PTP_1588_CLOCK
> 
>  config E1000E
>         tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
>         depends on PCI && (!SPARC32 || BROKEN)
> +       depends on MAY_USE_PTP_1588_CLOCK
>         select CRC32
> -       imply PTP_1588_CLOCK

What about "integrates"?

Or.. what if we just changed "implies" to also include the dependencies automatically? i.e. "implies PTP_1588_CLOCK" also means the depends trick which ensures that you can't have it as module if this is built-in.

I.e. we still get the nice "this will turn on automatically in the menu if you enable this" and we enforce that you can't have it as a module since it would be a dependency if it's on"?

> 
> 
>           Arnd

Arnd Bergmann Aug. 2, 2021, 8:59 p.m. UTC | #5

On Mon, Aug 2, 2021 at 10:46 PM Keller, Jacob E
<[e-mail]> wrote:

> > You can do something like it for a particular symbol though, such as
> >
> > config MAY_USE_PTP_1588_CLOCK
> >        def_tristate PTP_1588_CLOCK || !PTP_1588_CLOCK
> >
> >  config E1000E
> >         tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
> >         depends on PCI && (!SPARC32 || BROKEN)
> > +       depends on MAY_USE_PTP_1588_CLOCK
> >         select CRC32
> > -       imply PTP_1588_CLOCK
>
> What about "integrates"?

Maybe, we'd need to look at whether that fits for the other users of the
"A || !A" trick.

> Or.. what if we just changed "implies" to also include the dependencies
> automatically? i.e. "implies PTP_1588_CLOCK" also means the depends
> trick which ensures that you can't have it as module if this is built-in.
>
> I.e. we still get the nice "this will turn on automatically in the menu if you
> enable this" and we enforce that you can't have it as a module since it
> would be a dependency if it's on"?

I don't want to mess with the semantics of the keyword any further.
The original meaning was meant to avoid circular dependencies
by making it a softer version of 'select' that would not try to select
anything that has unmet dependencies. The current version made
it even softer by only having an effect during 'make defconfig'
and 'make oldconfig' but not preventing it from being soft-disabled
any more. Changing it yet again is guarantee to break lots of the
existing users, while probably also bringing back the original problem
of the circular dependencies.

         Arnd

Jacob Keller Aug. 2, 2021, 9:09 p.m. UTC | #6

> -----Original Message-----
> From: Arnd Bergmann <[e-mail]>
> Sent: Monday, August 02, 2021 1:59 PM
> To: Keller, Jacob E <[e-mail]>
> Cc: Richard Cochran <[e-mail]>; Nicolas Pitre
> <[e-mail]>; Brandeburg, Jesse <[e-mail]>;
> Nguyen, Anthony L <[e-mail]>; David S. Miller
> <[e-mail]>; Jakub Kicinski <[e-mail]>; Arnd Bergmann
> <[e-mail]>; Kurt Kanzenbach <[e-mail]>; Saleem, Shiraz
> <[e-mail]>; Ertman, David M <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK
> dependencies
> 
> I don't want to mess with the semantics of the keyword any further.
> The original meaning was meant to avoid circular dependencies
> by making it a softer version of 'select' that would not try to select
> anything that has unmet dependencies. The current version made
> it even softer by only having an effect during 'make defconfig'
> and 'make oldconfig' but not preventing it from being soft-disabled
> any more. Changing it yet again is guarantee to break lots of the
> existing users, while probably also bringing back the original problem
> of the circular dependencies.
> 
>          Arnd

Yea ok that makes sense. Better to use a new keyword if we do at all.

Jacob Keller Aug. 2, 2021, 9:10 p.m. UTC | #7

> -----Original Message-----
> From: Arnd Bergmann <[e-mail]>
> Sent: Monday, August 02, 2021 1:59 PM
> To: Keller, Jacob E <[e-mail]>
> Cc: Richard Cochran <[e-mail]>; Nicolas Pitre
> <[e-mail]>; Brandeburg, Jesse <[e-mail]>;
> Nguyen, Anthony L <[e-mail]>; David S. Miller
> <[e-mail]>; Jakub Kicinski <[e-mail]>; Arnd Bergmann
> <[e-mail]>; Kurt Kanzenbach <[e-mail]>; Saleem, Shiraz
> <[e-mail]>; Ertman, David M <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK
> dependencies
> 
> On Mon, Aug 2, 2021 at 10:46 PM Keller, Jacob E
> <[e-mail]> wrote:
> 
> > > You can do something like it for a particular symbol though, such as
> > >
> > > config MAY_USE_PTP_1588_CLOCK
> > >        def_tristate PTP_1588_CLOCK || !PTP_1588_CLOCK
> > >
> > >  config E1000E
> > >         tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
> > >         depends on PCI && (!SPARC32 || BROKEN)
> > > +       depends on MAY_USE_PTP_1588_CLOCK
> > >         select CRC32
> > > -       imply PTP_1588_CLOCK
> >
> > What about "integrates"?
> 
> Maybe, we'd need to look at whether that fits for the other users of the
> "A || !A" trick.
> 

Sure.  I just know from reading it other places it really causes a "huh?" reaction.

Nicolas Pitre Aug. 2, 2021, 9:22 p.m. UTC | #8

On Mon, 2 Aug 2021, Arnd Bergmann wrote:

> On Mon, Aug 2, 2021 at 10:46 PM Keller, Jacob E
> <[e-mail]> wrote:
> 
> > > You can do something like it for a particular symbol though, such as
> > >
> > > config MAY_USE_PTP_1588_CLOCK
> > >        def_tristate PTP_1588_CLOCK || !PTP_1588_CLOCK
> > >
> > >  config E1000E
> > >         tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
> > >         depends on PCI && (!SPARC32 || BROKEN)
> > > +       depends on MAY_USE_PTP_1588_CLOCK
> > >         select CRC32
> > > -       imply PTP_1588_CLOCK
> >
> > What about "integrates"?
> 
> Maybe, we'd need to look at whether that fits for the other users of the
> "A || !A" trick.

I implemented "conditional dependencies" at the time, which is syntactic 
sugar to express the above as:

	depends on A if A != n

	depends on A if A

etc.

http://lkml.iu.edu/hypermail/linux/kernel/2004.2/09783.html

But Masahiro shut it down.


Nicolas

Richard Cochran Aug. 2, 2021, 11:09 p.m. UTC | #9

On Mon, Aug 02, 2021 at 07:54:20PM +0000, Keller, Jacob E wrote:
> So go back to "select"?

Why not keep it simple?

PTP core:
   Boolean PTP_1588_CLOCK

drivers:
   depends on PTP_1588_CLOCK

Also, make Posix timers always part of the core.  Tinification is a
lost cause.

Thanks,
Richard

Jacob Keller Aug. 2, 2021, 11:45 p.m. UTC | #10

> -----Original Message-----
> From: Richard Cochran <[e-mail]>
> Sent: Monday, August 02, 2021 4:09 PM
> To: Keller, Jacob E <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>; Nicolas Pitre <[e-mail]>;
> Brandeburg, Jesse <[e-mail]>; Nguyen, Anthony L
> <[e-mail]>; David S. Miller <[e-mail]>; Jakub
> Kicinski <[e-mail]>; Arnd Bergmann <[e-mail]>; Kurt
> Kanzenbach <[e-mail]>; Saleem, Shiraz <[e-mail]>;
> Ertman, David M <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK
> dependencies
> 
> On Mon, Aug 02, 2021 at 07:54:20PM +0000, Keller, Jacob E wrote:
> > So go back to "select"?
> 
> Why not keep it simple?
> 
> PTP core:
>    Boolean PTP_1588_CLOCK
> 
> drivers:
>    depends on PTP_1588_CLOCK
> 
> Also, make Posix timers always part of the core.  Tinification is a
> lost cause.
> 
> Thanks,
> Richard

Ok, so basically: if any driver that needs PTP core is on, PTP core is on, with no way to disable it.

Thanks,
Jake

Richard Cochran Aug. 3, 2021, 12:03 a.m. UTC | #11

On Mon, Aug 02, 2021 at 11:45:09PM +0000, Keller, Jacob E wrote:
> Ok, so basically: if any driver that needs PTP core is on, PTP core is on, with no way to disable it.

Right.  Some MAC drivers keep the PTP stuff under a second Kconfig option.

IIRC, we (davem and netdev) decided not to do that going forwards.  If
a MAC has PTP features, then users will sure want it enabled.

So, let the MACs use "depends" or "select" PTP core.  I guess that
"select" is more user friendly.

And Posix timers: never disable this.  After all, who wants an
embedded system without timer_create()?  Seriously?

Thanks,
Richard

Arnd Bergmann Aug. 3, 2021, 6:59 a.m. UTC | #12

On Tue, Aug 3, 2021 at 1:09 AM Richard Cochran <[e-mail]> wrote:
>
> On Mon, Aug 02, 2021 at 07:54:20PM +0000, Keller, Jacob E wrote:
> > So go back to "select"?
>
> Why not keep it simple?
>
> PTP core:
>    Boolean PTP_1588_CLOCK
>
> drivers:
>    depends on PTP_1588_CLOCK
>
> Also, make Posix timers always part of the core.  Tinification is a
> lost cause.

It may well be a lost cause, but a build fix is not the time to nail down
that decision. The fix I proposed (with the added MAY_USE_PTP_1588_CLOCK
symbol) is only two extra lines and leaves everything else working for the
moment. I would suggest we merge that first and then raise the question
about whether to give up on tinyfication on the summit list, there are a few
other things that have come up that would also benefit from trying less hard,
but if we overdo this, we can get to the point of hurting even systems that are
otherwise still well supported (64MB MIPS/ARMv5 SoCs, small boot partitions,
etc.).

        Arnd

Richard Cochran Aug. 3, 2021, 3:55 p.m. UTC | #13

On Tue, Aug 03, 2021 at 08:59:02AM +0200, Arnd Bergmann wrote:
> It may well be a lost cause, but a build fix is not the time to nail down
> that decision. The fix I proposed (with the added MAY_USE_PTP_1588_CLOCK
> symbol) is only two extra lines and leaves everything else working for the
> moment.

Well, then we'll have TWO ugly and incomprehensible Kconfig hacks,
imply and MAY_USE.

Can't we fix this once and for all?

Seriously, "imply" has been nothing but a major PITA since day one,
and all to save 22 kb.  I can't think of another subsystem which
tolerates so much pain for so little gain.

Thanks,
Richard


> I would suggest we merge that first and then raise the question
> about whether to give up on tinyfication on the summit list, there are a few
> other things that have come up that would also benefit from trying less hard,
> but if we overdo this, we can get to the point of hurting even systems that are
> otherwise still well supported (64MB MIPS/ARMv5 SoCs, small boot partitions,
> etc.).
> 
>         Arnd

Richard Cochran Aug. 3, 2021, 4:14 p.m. UTC | #14

On Tue, Aug 03, 2021 at 08:55:56AM -0700, Richard Cochran wrote:
> On Tue, Aug 03, 2021 at 08:59:02AM +0200, Arnd Bergmann wrote:
> > It may well be a lost cause, but a build fix is not the time to nail down
> > that decision. The fix I proposed (with the added MAY_USE_PTP_1588_CLOCK
> > symbol) is only two extra lines and leaves everything else working for the
> > moment.
> 
> Well, then we'll have TWO ugly and incomprehensible Kconfig hacks,
> imply and MAY_USE.
> 
> Can't we fix this once and for all?
> 
> Seriously, "imply" has been nothing but a major PITA since day one,
> and all to save 22 kb.  I can't think of another subsystem which
> tolerates so much pain for so little gain.

Here is what I want to have, in accordance with the KISS principle:

config PTP_1588_CLOCK
	bool "PTP clock support"
	select NET
	select POSIX_TIMERS
	select PPS
	select NET_PTP_CLASSIFY

# driver variant 1:

config ACME_MAC
	select PTP_1588_CLOCK

# driver variant 2:

config ACME_MAC

config ACME_MAC_PTP
	depends on ACME_MAC
	select PTP_1588_CLOCK

Hm?	

Thanks,
Richard

Arnd Bergmann Aug. 3, 2021, 5 p.m. UTC | #15

On Tue, Aug 3, 2021 at 6:14 PM Richard Cochran <[e-mail]> wrote:
> On Tue, Aug 03, 2021 at 08:55:56AM -0700, Richard Cochran wrote:
> > On Tue, Aug 03, 2021 at 08:59:02AM +0200, Arnd Bergmann wrote:
> > > It may well be a lost cause, but a build fix is not the time to nail down
> > > that decision. The fix I proposed (with the added MAY_USE_PTP_1588_CLOCK
> > > symbol) is only two extra lines and leaves everything else working for the
> > > moment.
> >
> > Well, then we'll have TWO ugly and incomprehensible Kconfig hacks,
> > imply and MAY_USE.

I'm all in favor of removing imply elsewhere as well, but that needs much
broader consensus than removing it from PTP_1588_CLOCK.

It has already crept into cryto/ and sound/soc/codecs/, and at least in
the latter case it does seem to even make sense, so they are less
likely to remove it.

> > Can't we fix this once and for all?
> >
> > Seriously, "imply" has been nothing but a major PITA since day one,
> > and all to save 22 kb.  I can't think of another subsystem which
> > tolerates so much pain for so little gain.
>
> Here is what I want to have, in accordance with the KISS principle:
>
> config PTP_1588_CLOCK
>         bool "PTP clock support"
>         select NET
>         select POSIX_TIMERS
>         select PPS
>         select NET_PTP_CLASSIFY
>
> # driver variant 1:
>
> config ACME_MAC
>         select PTP_1588_CLOCK
>
> # driver variant 2:
>
> config ACME_MAC
>
> config ACME_MAC_PTP
>         depends on ACME_MAC
>         select PTP_1588_CLOCK
>
> Hm?

Selecting a subsystem (NET, POSIX_TIMES, PPS, NET_PTP_CLASSIFY)
from a device driver is the nightmare that 'imply' was meant to solve (but did
not): this causes dependency loops, and unintended behavior where you
end up accidentally enabling a lot more drivers than you actually need
(when other symbols depend on the selected ones, and default to y).

If you turn all those 'select' lines into 'depends on', this will work, but it's
not actually much different from what I'm suggesting. Maybe we can do it
in two steps: first fix the build failure by replacing all the 'imply'
statements
with the correct dependencies, and then you send a patch on top that
turns PPS and PTP_1588_CLOCK into bool options.

     Arnd

Jacob Keller Aug. 3, 2021, 5:18 p.m. UTC | #16

> -----Original Message-----
> From: Arnd Bergmann <[e-mail]>
> Sent: Tuesday, August 03, 2021 10:01 AM
> To: Richard Cochran <[e-mail]>
> Cc: Nicolas Pitre <[e-mail]>; Keller, Jacob E <[e-mail]>;
> Brandeburg, Jesse <[e-mail]>; Nguyen, Anthony L
> <[e-mail]>; David S. Miller <[e-mail]>; Jakub
> Kicinski <[e-mail]>; Arnd Bergmann <[e-mail]>; Kurt
> Kanzenbach <[e-mail]>; Saleem, Shiraz <[e-mail]>;
> Ertman, David M <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK
> dependencies
> 
> On Tue, Aug 3, 2021 at 6:14 PM Richard Cochran <[e-mail]>
> wrote:
> > On Tue, Aug 03, 2021 at 08:55:56AM -0700, Richard Cochran wrote:
> > > On Tue, Aug 03, 2021 at 08:59:02AM +0200, Arnd Bergmann wrote:
> > > > It may well be a lost cause, but a build fix is not the time to nail down
> > > > that decision. The fix I proposed (with the added
> MAY_USE_PTP_1588_CLOCK
> > > > symbol) is only two extra lines and leaves everything else working for the
> > > > moment.
> > >
> > > Well, then we'll have TWO ugly and incomprehensible Kconfig hacks,
> > > imply and MAY_USE.
> 
> I'm all in favor of removing imply elsewhere as well, but that needs much
> broader consensus than removing it from PTP_1588_CLOCK.
> 
> It has already crept into cryto/ and sound/soc/codecs/, and at least in
> the latter case it does seem to even make sense, so they are less
> likely to remove it.
> 
> > > Can't we fix this once and for all?
> > >
> > > Seriously, "imply" has been nothing but a major PITA since day one,
> > > and all to save 22 kb.  I can't think of another subsystem which
> > > tolerates so much pain for so little gain.
> >
> > Here is what I want to have, in accordance with the KISS principle:
> >
> > config PTP_1588_CLOCK
> >         bool "PTP clock support"
> >         select NET
> >         select POSIX_TIMERS
> >         select PPS
> >         select NET_PTP_CLASSIFY
> >
> > # driver variant 1:
> >
> > config ACME_MAC
> >         select PTP_1588_CLOCK
> >
> > # driver variant 2:
> >
> > config ACME_MAC
> >
> > config ACME_MAC_PTP
> >         depends on ACME_MAC
> >         select PTP_1588_CLOCK
> >
> > Hm?
> 
> Selecting a subsystem (NET, POSIX_TIMES, PPS, NET_PTP_CLASSIFY)
> from a device driver is the nightmare that 'imply' was meant to solve (but did
> not): this causes dependency loops, and unintended behavior where you
> end up accidentally enabling a lot more drivers than you actually need
> (when other symbols depend on the selected ones, and default to y).
> 
> If you turn all those 'select' lines into 'depends on', this will work, but it's
> not actually much different from what I'm suggesting. Maybe we can do it
> in two steps: first fix the build failure by replacing all the 'imply'
> statements
> with the correct dependencies, and then you send a patch on top that
> turns PPS and PTP_1588_CLOCK into bool options.
> 
>      Arnd

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.

This would make it work correctly for users who want tinification, *and* it would make there be no strong dependency between anything, while still allowing optionally defaulting to yes.

That being said, I don't think saving 22kb is worth the chance to get things wrong (as we've seen).

Thanks,
Jake

Arnd Bergmann Aug. 3, 2021, 6:27 p.m. UTC | #17

On Tue, Aug 3, 2021 at 7:19 PM Keller, Jacob E <[e-mail]> wrote:
> > On Tue, Aug 3, 2021 at 6:14 PM Richard Cochran <[e-mail]> 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.

      Arnd

Arnd Bergmann Aug. 3, 2021, 8:29 p.m. UTC | #18

On Mon, Aug 2, 2021 at 10:32 PM Arnd Bergmann <[e-mail]> wrote:

> config MAY_USE_PTP_1588_CLOCK
>        def_tristate PTP_1588_CLOCK || !PTP_1588_CLOCK
>
>  config E1000E
>         tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
>         depends on PCI && (!SPARC32 || BROKEN)
> +       depends on MAY_USE_PTP_1588_CLOCK
>         select CRC32
> -       imply PTP_1588_CLOCK

I've written up the patch to do this all over the kernel now, and started an
overnight randconfig build session with this applied:

https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/commit/?h=ptp-1588-optional&id=3f69b7366cfd4b2c048c76be5299b38066933ee1

       Arnd

Richard Cochran Aug. 3, 2021, 8:54 p.m. UTC | #19

On Tue, Aug 03, 2021 at 07:00:49PM +0200, Arnd Bergmann wrote:

> If you turn all those 'select' lines into 'depends on', this will work, but it's
> not actually much different from what I'm suggesting.

"depends" instead of "select" works for me.  I just want it simple and clear.

> Maybe we can do it
> in two steps: first fix the build failure by replacing all the 'imply'
> statements
> with the correct dependencies, and then you send a patch on top that
> turns PPS and PTP_1588_CLOCK into bool options.

Sounds good.

Thanks,
Richard

Jacob Keller Aug. 3, 2021, 11:25 p.m. UTC | #20

> -----Original Message-----
> From: Arnd Bergmann <[e-mail]>
> Sent: Tuesday, August 03, 2021 11:27 AM
> To: Keller, Jacob E <[e-mail]>
> Cc: Richard Cochran <[e-mail]>; Nicolas Pitre
> <[e-mail]>; Brandeburg, Jesse <[e-mail]>; Nguyen,
> Anthony L <[e-mail]>; David S. Miller
> <[e-mail]>; Jakub Kicinski <[e-mail]>; Arnd Bergmann
> <[e-mail]>; Kurt Kanzenbach <[e-mail]>; Saleem, Shiraz
> <[e-mail]>; Ertman, David M <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK
> dependencies
> 
> On Tue, Aug 3, 2021 at 7:19 PM Keller, Jacob E <[e-mail]> wrote:
> > > On Tue, Aug 3, 2021 at 6:14 PM Richard Cochran
> <[e-mail]> 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.
> 
>       Arnd

Fair enough. I am also fine with just "depends". We can make most of the drivers simply always enable it, and if a specific driver is used in some embedded setup that has requirements on minimizing things that driver can be setup to use a 2nd config symbol, and all of the other drivers that aren't used can be disabled (as that minimizer is probably already doing!)

I think we've found the best route to go then!

Thanks,
Jake

Arnd Bergmann Aug. 4, 2021, 11:18 a.m. UTC | #21

On Tue, Aug 3, 2021 at 8:27 PM Arnd Bergmann <[e-mail]> wrote:
>
> On Tue, Aug 3, 2021 at 7:19 PM Keller, Jacob E <[e-mail]> wrote:
> > > On Tue, Aug 3, 2021 at 6:14 PM Richard Cochran <[e-mail]> 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

Jacob Keller Aug. 4, 2021, 8:53 p.m. UTC | #22

> -----Original Message-----
> From: Richard Cochran <[e-mail]>
> Sent: Tuesday, August 03, 2021 1:55 PM
> To: Arnd Bergmann <[e-mail]>
> Cc: Nicolas Pitre <[e-mail]>; Keller, Jacob E <[e-mail]>;
> Brandeburg, Jesse <[e-mail]>; Nguyen, Anthony L
> <[e-mail]>; David S. Miller <[e-mail]>; Jakub
> Kicinski <[e-mail]>; Arnd Bergmann <[e-mail]>; Kurt
> Kanzenbach <[e-mail]>; Saleem, Shiraz <[e-mail]>;
> Ertman, David M <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK
> dependencies
> 
> 
> On Tue, Aug 03, 2021 at 07:00:49PM +0200, Arnd Bergmann wrote:
> 
> > If you turn all those 'select' lines into 'depends on', this will work, but it's
> > not actually much different from what I'm suggesting.
> 
> "depends" instead of "select" works for me.  I just want it simple and clear.
> 
> > Maybe we can do it
> > in two steps: first fix the build failure by replacing all the 'imply'
> > statements
> > with the correct dependencies, and then you send a patch on top that
> > turns PPS and PTP_1588_CLOCK into bool options.
> 
> Sounds good.
> 
> Thanks,
> Richard

Works for me too.

Thanks,
Jake

Patch

diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 2aa84bd97287..3ddadc147d45 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -58,8 +58,8 @@  config E1000
 config E1000E
 	tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
 	depends on PCI && (!SPARC32 || BROKEN)
+	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
 	select CRC32
-	imply PTP_1588_CLOCK
 	help
 	  This driver supports the PCI-Express Intel(R) PRO/1000 gigabit
 	  ethernet family of adapters. For PCI or PCI-X e1000 adapters,
@@ -87,8 +87,8 @@  config E1000E_HWTS
 config IGB
 	tristate "Intel(R) 82575/82576 PCI-Express Gigabit Ethernet support"
 	depends on PCI
-	imply PTP_1588_CLOCK
-	select I2C
+	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
+	depends on I2C
 	select I2C_ALGOBIT
 	help
 	  This driver supports Intel(R) 82575/82576 gigabit ethernet family of
@@ -159,9 +159,9 @@  config IXGB
 config IXGBE
 	tristate "Intel(R) 10GbE PCI Express adapters support"
 	depends on PCI
+	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
 	select MDIO
 	select PHYLIB
-	imply PTP_1588_CLOCK
 	help
 	  This driver supports Intel(R) 10GbE PCI Express family of
 	  adapters.  For more information on how to identify your adapter, go
@@ -239,7 +239,7 @@  config IXGBEVF_IPSEC
 
 config I40E
 	tristate "Intel(R) Ethernet Controller XL710 Family support"
-	imply PTP_1588_CLOCK
+	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
 	depends on PCI
 	select AUXILIARY_BUS
 	help
@@ -295,11 +295,11 @@  config ICE
 	tristate "Intel(R) Ethernet Connection E800 Series Support"
 	default n
 	depends on PCI_MSI
+	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
 	select AUXILIARY_BUS
 	select DIMLIB
 	select NET_DEVLINK
 	select PLDMFW
-	imply PTP_1588_CLOCK
 	help
 	  This driver supports Intel(R) Ethernet Connection E800 Series of
 	  devices.  For more information on how to identify your adapter, go
@@ -317,7 +317,7 @@  config FM10K
 	tristate "Intel(R) FM10000 Ethernet Switch Host Interface Support"
 	default n
 	depends on PCI_MSI
-	imply PTP_1588_CLOCK
+	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
 	help
 	  This driver supports Intel(R) FM10000 Ethernet Switch Host
 	  Interface.  For more information on how to identify your adapter,