LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Keller, Jacob E" <email@example.com>
To: Arnd Bergmann <firstname.lastname@example.org>,
Richard Cochran <email@example.com>
Cc: Nicolas Pitre <firstname.lastname@example.org>,
"Brandeburg, Jesse" <email@example.com>,
"Nguyen, Anthony L" <firstname.lastname@example.org>,
"David S. Miller" <email@example.com>,
"Jakub Kicinski" <firstname.lastname@example.org>, Arnd Bergmann <email@example.com>,
Kurt Kanzenbach <firstname.lastname@example.org>,
"Saleem, Shiraz" <email@example.com>,
"Ertman, David M" <firstname.lastname@example.org>,
Subject: RE: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK dependencies
Date: Tue, 3 Aug 2021 17:18:06 +0000 [thread overview]
Message-ID: <CO1PR11MB50892EAF3C871F6934B85852D6F09@CO1PR11MB5089.namprd11.prod.outlook.com> (raw)
> -----Original Message-----
> From: Arnd Bergmann <email@example.com>
> Sent: Tuesday, August 03, 2021 10:01 AM
> To: Richard Cochran <firstname.lastname@example.org>
> Cc: Nicolas Pitre <email@example.com>; Keller, Jacob E <firstname.lastname@example.org>;
> Brandeburg, Jesse <email@example.com>; Nguyen, Anthony L
> <firstname.lastname@example.org>; David S. Miller <email@example.com>; Jakub
> Kicinski <firstname.lastname@example.org>; Arnd Bergmann <email@example.com>; Kurt
> Kanzenbach <firstname.lastname@example.org>; Saleem, Shiraz <email@example.com>;
> Ertman, David M <firstname.lastname@example.org>; email@example.com;
> firstname.lastname@example.org; email@example.com
> Subject: Re: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK
> On Tue, Aug 3, 2021 at 6:14 PM Richard Cochran <firstname.lastname@example.org>
> > 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
> > > > 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'
> with the correct dependencies, and then you send a patch on top that
> turns PPS and PTP_1588_CLOCK into bool options.
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).
next prev parent reply other threads:[~2021-08-03 17:18 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-02 14:59 Arnd Bergmann
2021-08-02 16:49 ` Richard Cochran
2021-08-02 19:54 ` Keller, Jacob E
2021-08-02 20:32 ` Arnd Bergmann
2021-08-02 20:46 ` Keller, Jacob E
2021-08-02 20:59 ` Arnd Bergmann
2021-08-02 21:09 ` Keller, Jacob E
2021-08-02 21:10 ` Keller, Jacob E
2021-08-02 21:22 ` Nicolas Pitre
2021-08-03 20:29 ` Arnd Bergmann
2021-08-02 23:09 ` Richard Cochran
2021-08-02 23:45 ` Keller, Jacob E
2021-08-03 0:03 ` Richard Cochran
2021-08-03 6:59 ` Arnd Bergmann
2021-08-03 15:55 ` Richard Cochran
2021-08-03 16:14 ` Richard Cochran
2021-08-03 17:00 ` Arnd Bergmann
2021-08-03 17:18 ` Keller, Jacob E [this message]
2021-08-03 18:27 ` Arnd Bergmann
2021-08-03 23:25 ` Keller, Jacob E
2021-08-04 11:18 ` Arnd Bergmann
2021-08-03 20:54 ` Richard Cochran
2021-08-04 20:53 ` Keller, Jacob E
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='RE: [PATCH net-next v2] ethernet/intel: fix PTP_1588_CLOCK dependencies' \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).