netdev - RE: [PATCH net-next v2] ethernet/intel: fix PTP_1588

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CO1PR11MB50892EAF3C871F6934B85852D6F09@CO1PR11MB5089.namprd11.prod.outlook.com>
Date:   Tue, 3 Aug 2021 17:18:06 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Arnd Bergmann <arnd@...nel.org>,
        Richard Cochran <richardcochran@...il.com>
CC:     Nicolas Pitre <nico@...xnic.net>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        "Jakub Kicinski" <kuba@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        "Saleem, Shiraz" <shiraz.saleem@...el.com>,
        "Ertman, David M" <david.m.ertman@...el.com>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK
 dependencies



> -----Original Message-----
> From: Arnd Bergmann <arnd@...nel.org>
> Sent: Tuesday, August 03, 2021 10:01 AM
> To: Richard Cochran <richardcochran@...il.com>
> Cc: Nicolas Pitre <nico@...xnic.net>; Keller, Jacob E <jacob.e.keller@...el.com>;
> Brandeburg, Jesse <jesse.brandeburg@...el.com>; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; David S. Miller <davem@...emloft.net>; Jakub
> Kicinski <kuba@...nel.org>; Arnd Bergmann <arnd@...db.de>; Kurt
> Kanzenbach <kurt@...utronix.de>; Saleem, Shiraz <shiraz.saleem@...el.com>;
> Ertman, David M <david.m.ertman@...el.com>; intel-wired-lan@...ts.osuosl.org;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK
> dependencies
> 
> On Tue, Aug 3, 2021 at 6:14 PM Richard Cochran <richardcochran@...il.com>
> 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