[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7af17cfa-ae15-f548-1a1b-01397a766066@intel.com>
Date: Thu, 23 Feb 2023 14:55:07 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>,
<netdev@...r.kernel.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Alexander Lobakin <alexandr.lobakin@...el.com>,
Anthony Nguyen <anthony.l.nguyen@...el.com>
Subject: Re: [intel-net] ice: remove unnecessary CONFIG_ICE_GNSS
On 2/22/2023 9:17 PM, Jakub Kicinski wrote:
> On Wed, 22 Feb 2023 14:35:58 -0800 Jacob Keller wrote:
>> I'm sending to both Intel-wired-lan and netdev lists since this was
>> discussed publicly on the netdev list. I'm not sure how we want to queue it
>> up, so I currently have it tagged as intel-net to go through Tony's IWL
>> tree. I'm happy however it gets pulled. I believe this is the best solution
>> as the total number of #ifdefs is the same as with CONFIG_ICE_GNSS, as is
>> the Makefile line. As far as I can tell the Kbuild just does the right thing
>> here so there is no need for an additional flag.
>>
>> I'm happy to respin with a "depends" check if we think the flag has other
>> value.
>
> Sorry for late response. Do you mean depends as in keeping the separate
> Kconfig? IS_REACHABLE() is a bit of a hack, makes figuring out what
> gets built a lot harder for users. How about we keep the IS_ENABLED()
> but add a dependency to ICE as a whole?
>
IS_ENABLED's problem is that it can break if CONFIG_GNSS = m while
CONFIG_ICE = y (not that many people would build it as a builtin...)
unless there is a dependency involved, but the original code allowed
building ICE without GNSS.
We did the CONFIG_ICE_GNSS but it lacked any dependency on ice = Y, so
it was getting set regardless of whether ice was building.
The solution with depends I was referring to is Linus' suggestion with
adding depends on ICE to CONFIG_ICE_GNSS, or otherwise putting it in an
"if ICE" block in Kconfig.
> I mean instead of s/IS_ENABLED/IS_REACHABLE/ do this:
>
> index 3facb55b7161..198995b3eab5 100644
> --- a/drivers/net/ethernet/intel/Kconfig
> +++ b/drivers/net/ethernet/intel/Kconfig
> @@ -296,6 +296,7 @@ config ICE
> default n
> depends on PCI_MSI
> depends on PTP_1588_CLOCK_OPTIONAL
> + depends on GNSS || GNSS=n
> select AUXILIARY_BUS
> select DIMLIB
> select NET_DEVLINK
>
> Or do you really care about building ICE with no GNSS.. ?
This would probably also work, but you'd still need #if IS_ENABLED in
ice_gnss.h to split the stub functions when GNSS is disabled.
The original author, Arkadiusz, can comment on whether we care about
building without GNSS support.
My guess its a "we don't need it for core functionality, so we don't
want to block building ice if someone doesn't want GNSS for whatever
reason."
Powered by blists - more mailing lists