netdev - Re: [intel-net] ice: remove unnecessary CONFIG_ICE

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]

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."