| Message ID | 20230222223558.2328428-1-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [intel-net] ice: remove unnecessary CONFIG_ICE_GNSS | expand |
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? 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.. ?
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."
On Thu, 23 Feb 2023 14:55:07 -0800 Jacob Keller wrote: > > 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." Just to be crystal clear we're talking about the GNSS=m ICE=y case. I'm suggesting that it should be disallowed at the Kconfig level. ICE=m/y GNSS=n will still work as expected.
On 2/23/2023 4:13 PM, Jakub Kicinski wrote: > On Thu, 23 Feb 2023 14:55:07 -0800 Jacob Keller wrote: >>> 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." > > Just to be crystal clear we're talking about the GNSS=m ICE=y case. > I'm suggesting that it should be disallowed at the Kconfig level. > ICE=m/y GNSS=n will still work as expected. Fair enough. I guess I would expect "ICE=y, GNSS=m" to just have ice not support GNSS. But disallowing it is fine as well. I can see how that might be confusing to others. I can make that change with the dependency. Thanks, Jake
diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig index a3c84bf05e44..3facb55b7161 100644 --- a/drivers/net/ethernet/intel/Kconfig +++ b/drivers/net/ethernet/intel/Kconfig @@ -337,9 +337,6 @@ config ICE_HWTS the PTP clock driver precise cross-timestamp ioctl (PTP_SYS_OFFSET_PRECISE). -config ICE_GNSS - def_bool GNSS = y || GNSS = ICE - config FM10K tristate "Intel(R) FM10000 Ethernet Switch Host Interface Support" default n diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile index f269952d207d..5d89392f969b 100644 --- a/drivers/net/ethernet/intel/ice/Makefile +++ b/drivers/net/ethernet/intel/ice/Makefile @@ -47,4 +47,4 @@ ice-$(CONFIG_DCB) += ice_dcb.o ice_dcb_nl.o ice_dcb_lib.o ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o -ice-$(CONFIG_ICE_GNSS) += ice_gnss.o +ice-$(CONFIG_GNSS) += ice_gnss.o diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h b/drivers/net/ethernet/intel/ice/ice_gnss.h index 31db0701d13f..d453987492f0 100644 --- a/drivers/net/ethernet/intel/ice/ice_gnss.h +++ b/drivers/net/ethernet/intel/ice/ice_gnss.h @@ -45,7 +45,7 @@ struct gnss_serial { struct list_head queue; }; -#if IS_ENABLED(CONFIG_ICE_GNSS) +#if IS_REACHABLE(CONFIG_GNSS) void ice_gnss_init(struct ice_pf *pf); void ice_gnss_exit(struct ice_pf *pf); bool ice_gnss_is_gps_present(struct ice_hw *hw); @@ -56,5 +56,5 @@ static inline bool ice_gnss_is_gps_present(struct ice_hw *hw) { return false; } -#endif /* IS_ENABLED(CONFIG_ICE_GNSS) */ +#endif /* IS_REACHABLE(CONFIG_GNSS) */ #endif /* _ICE_GNSS_H_ */
CONFIG_ICE_GNSS was added by commit c7ef8221ca7d ("ice: use GNSS subsystem instead of TTY") as a way to allow the ice driver to optionally support GNSS features without forcing a dependency on CONFIG_GNSS. The original implementation of that commit at [1] used IS_REACHABLE. This was rejected by Olek at [2] with the suggested implementation of CONFIG_ICE_GNSS. Eventually after merging, Linus reported a .config which had CONFIG_ICE_GNSS = y when both GNSS = n and ICE = n. This confused him and he felt that the config option was not useful, and commented about it at [3]. CONFIG_ICE_GNSS is defined to y whenever GNSS = ICE. This results in it being set in cases where both options are not enabled. The goal of CONFIG_ICE_GNSS is to ensure that the GNSS support in the ice driver is enabled when GNSS is enabled, while ensuring that ICE = y and GNSS = m don't break. The complaint from Olek about the original IS_REACHABLE was due to the required IS_REACHABLE checks throughout the ice driver code and the fact that ice_gnss.c was compiled regardless of GNSS support. This can be fixed in the Makefile by using ice-$(CONFIG_GNSS) += ice_gnss.o If GNSS = m, then this will add ice_gnss.o to ice-m, which will be ignored if we're not compiling ice as a module, and thus we will skip compiling GNSS code just as with CONFIG_ICE_GNSS. Drop CONFIG_ICE_GNSS, and replace the IS_ENABLED checks for it with IS_REACHABLE checks for GNSS. Update the Makefile to add the ice_gnss.o object based on CONFIG_GNSS. This works on my system to ensure that GNSS support is optionally included without any need for additional config options, and it works correctly in at least the following configurations: 1. CONFIG_ICE = m, CONFIG_GNSS = m 2. CONFIG_ICE = y, CONFIG_GNSS = m 3. CONFIG_ICE = m, CONFIG_GNSS = y This solution should resolve the complains Olek made regarding compilation of ice_gnss.o and additional unnecessary IS_REACHABLE checks, while also avoiding extra config flags. [1] https://lore.kernel.org/intel-wired-lan/20221019095603.44825-1-arkadiusz.kubalewski@intel.com/ [2] https://lore.kernel.org/intel-wired-lan/20221028165706.96849-1-alexandr.lobakin@intel.com/ [3] https://lore.kernel.org/all/CAHk-=wi_410KZqHwF-WL5U7QYxnpHHHNP-3xL=g_y89XnKc-uw@mail.gmail.com/ Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Fixes: c7ef8221ca7d ("ice: use GNSS subsystem instead of TTY") Acked-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Acked-by: Alexander Lobakin <alexandr.lobakin@intel.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Anthony Nguyen <anthony.l.nguyen@intel.com> --- 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. drivers/net/ethernet/intel/Kconfig | 3 --- drivers/net/ethernet/intel/ice/Makefile | 2 +- drivers/net/ethernet/intel/ice/ice_gnss.h | 4 ++-- 3 files changed, 3 insertions(+), 6 deletions(-) base-commit: 5b7c4cabbb65f5c469464da6c5f614cbd7f730f2