| Message ID | 20191210203710.2987983-1-arnd@arndb.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: ocelot: add NET_VENDOR_MICROSEMI dependency | expand |
Hi Arnd, On Tue, 10 Dec 2019 at 22:37, Arnd Bergmann <arnd@arndb.de> wrote: > > Selecting MSCC_OCELOT_SWITCH is not possible when NET_VENDOR_MICROSEMI > is disabled: > > WARNING: unmet direct dependencies detected for MSCC_OCELOT_SWITCH > Depends on [n]: NETDEVICES [=y] && ETHERNET [=n] && NET_VENDOR_MICROSEMI [=n] && NET_SWITCHDEV [=y] && HAS_IOMEM [=y] > Selected by [m]: > - NET_DSA_MSCC_FELIX [=m] && NETDEVICES [=y] && HAVE_NET_DSA [=y] && NET_DSA [=y] && PCI [=y] > > Add a Kconfig dependency on NET_VENDOR_MICROSEMI, which also implies > CONFIG_NETDEVICES. > > Fixes: 56051948773e ("net: dsa: ocelot: add driver for Felix switch family") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- This has been submitted before, here [0]. It isn't wrong, but in principle I agree with David that it is strange to put a "depends" relationship between a driver in drivers/net/dsa and the Kconfig vendor umbrella from drivers/net/ethernet/mscc ("why would the user care/need to enable NET_VENDOR_MICROSEMI to see the DSA driver" is a valid point to me). This is mainly because I don't understand the point of CONFIG_NET_VENDOR_* options, they're a bit tribalistic to my ears. Nonetheless, alternatives may be: - Move MSCC_OCELOT_SWITCH core option outside of the NET_VENDOR_MICROSEMI umbrella, and make it invisible to menuconfig, just selectable from the 2 driver instances (MSCC_OCELOT_SWITCH_OCELOT and NET_DSA_MSCC_FELIX). MSCC_OCELOT_SWITCH has no reason to be selectable by the user anyway. - Remove NET_VENDOR_MICROSEMI altogether. There is a single driver under drivers/net/ethernet/mscc and it's already causing problems, it's ridiculous. - Leave it as it is. I genuinely ask: if the build system tells you that the build dependencies are not met, does it matter if it compiles or not? [0]: https://www.spinics.net/lists/netdev/msg614325.html Regards, -Vladimir
On Tue, Dec 10, 2019 at 10:37 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > Hi Arnd, > > On Tue, 10 Dec 2019 at 22:37, Arnd Bergmann <arnd@arndb.de> wrote: > > > > Selecting MSCC_OCELOT_SWITCH is not possible when NET_VENDOR_MICROSEMI > > is disabled: > > > > WARNING: unmet direct dependencies detected for MSCC_OCELOT_SWITCH > > Depends on [n]: NETDEVICES [=y] && ETHERNET [=n] && NET_VENDOR_MICROSEMI [=n] && NET_SWITCHDEV [=y] && HAS_IOMEM [=y] > > Selected by [m]: > > - NET_DSA_MSCC_FELIX [=m] && NETDEVICES [=y] && HAVE_NET_DSA [=y] && NET_DSA [=y] && PCI [=y] > > > > Add a Kconfig dependency on NET_VENDOR_MICROSEMI, which also implies > > CONFIG_NETDEVICES. > > > > Fixes: 56051948773e ("net: dsa: ocelot: add driver for Felix switch family") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > This has been submitted before, here [0]. > > It isn't wrong, but in principle I agree with David that it is strange > to put a "depends" relationship between a driver in drivers/net/dsa > and the Kconfig vendor umbrella from drivers/net/ethernet/mscc ("why > would the user care/need to enable NET_VENDOR_MICROSEMI to see the DSA > driver" is a valid point to me). This is mainly because I don't > understand the point of CONFIG_NET_VENDOR_* options, they're a bit > tribalistic to my ears. > > Nonetheless, alternatives may be: > - Move MSCC_OCELOT_SWITCH core option outside of the > NET_VENDOR_MICROSEMI umbrella, and make it invisible to menuconfig, > just selectable from the 2 driver instances (MSCC_OCELOT_SWITCH_OCELOT > and NET_DSA_MSCC_FELIX). MSCC_OCELOT_SWITCH has no reason to be > selectable by the user anyway. You still need 'depends on NETDEVICES' in that case, otherwise this sounds like a good option. > - Remove NET_VENDOR_MICROSEMI altogether. There is a single driver > under drivers/net/ethernet/mscc and it's already causing problems, > it's ridiculous. It's only there for consistency with the other directories under drivers/net/ethernet/. > - Leave it as it is. I genuinely ask: if the build system tells you > that the build dependencies are not met, does it matter if it compiles > or not? We try very hard to allow all randconfig builds to complete without any output from the build process when building with 'make -s'. Random warnings like this just clutter up the output, even if it's harmless there is a risk of missing something important. Yet another option is - Change NET_DSA_MSCC_FELIX to use 'depends on MSCC_OCELOT_SWITCH' instead of 'select NET_DSA_MSCC_FELIX'. Arnd
On Wed, 11 Dec 2019 at 00:04, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Dec 10, 2019 at 10:37 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > > > Hi Arnd, > > > > On Tue, 10 Dec 2019 at 22:37, Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > Selecting MSCC_OCELOT_SWITCH is not possible when NET_VENDOR_MICROSEMI > > > is disabled: > > > > > > WARNING: unmet direct dependencies detected for MSCC_OCELOT_SWITCH > > > Depends on [n]: NETDEVICES [=y] && ETHERNET [=n] && NET_VENDOR_MICROSEMI [=n] && NET_SWITCHDEV [=y] && HAS_IOMEM [=y] > > > Selected by [m]: > > > - NET_DSA_MSCC_FELIX [=m] && NETDEVICES [=y] && HAVE_NET_DSA [=y] && NET_DSA [=y] && PCI [=y] > > > > > > Add a Kconfig dependency on NET_VENDOR_MICROSEMI, which also implies > > > CONFIG_NETDEVICES. > > > > > > Fixes: 56051948773e ("net: dsa: ocelot: add driver for Felix switch family") > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > --- > > > > This has been submitted before, here [0]. > > > > It isn't wrong, but in principle I agree with David that it is strange > > to put a "depends" relationship between a driver in drivers/net/dsa > > and the Kconfig vendor umbrella from drivers/net/ethernet/mscc ("why > > would the user care/need to enable NET_VENDOR_MICROSEMI to see the DSA > > driver" is a valid point to me). This is mainly because I don't > > understand the point of CONFIG_NET_VENDOR_* options, they're a bit > > tribalistic to my ears. > > > > Nonetheless, alternatives may be: > > - Move MSCC_OCELOT_SWITCH core option outside of the > > NET_VENDOR_MICROSEMI umbrella, and make it invisible to menuconfig, > > just selectable from the 2 driver instances (MSCC_OCELOT_SWITCH_OCELOT > > and NET_DSA_MSCC_FELIX). MSCC_OCELOT_SWITCH has no reason to be > > selectable by the user anyway. > > You still need 'depends on NETDEVICES' in that case, otherwise this sounds > like a good option. > I don't completely understand this. Looks like NETDEVICES is another one of those options that don't enable any code. I would have expected that NET_SWITCHDEV depended on it already? But anyway, it's still a small compromise and not a problem. > > - Remove NET_VENDOR_MICROSEMI altogether. There is a single driver > > under drivers/net/ethernet/mscc and it's already causing problems, > > it's ridiculous. > > It's only there for consistency with the other directories under > drivers/net/ethernet/. > > > - Leave it as it is. I genuinely ask: if the build system tells you > > that the build dependencies are not met, does it matter if it compiles > > or not? > > We try very hard to allow all randconfig builds to complete without > any output from the build process when building with 'make -s'. > Random warnings like this just clutter up the output, even if it's > harmless there is a risk of missing something important. > > Yet another option is > - Change NET_DSA_MSCC_FELIX to use 'depends on > MSCC_OCELOT_SWITCH' instead of 'select NET_DSA_MSCC_FELIX'. > That's strange too. MSCC_OCELOT_SWITCH just enables a common driver core which is used by both NET_DSA_MSCC_FELIX and MSCC_OCELOT_SWITCH_OCELOT (possibly by more in the future). I don't see any reason why the common library (purely an implementation detail) should even be user-visible, let alone why it should hide a DSA driver. So, if you agree, I can take care of this tomorrow by reworking the Kconfig in the 1st proposed way. I hope you don't mind that I'm volunteering to do it, but the change will require a bit of explaining which is non-trivial, and I don't expect that you really want to know these details, just that it compiles with no issue from all angles. > > Arnd Thanks, -Vladimir
On Tue, Dec 10, 2019 at 11:33 PM Vladimir Oltean <olteanv@gmail.com> wrote: > On Wed, 11 Dec 2019 at 00:04, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Dec 10, 2019 at 10:37 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > > Nonetheless, alternatives may be: > > > - Move MSCC_OCELOT_SWITCH core option outside of the > > > NET_VENDOR_MICROSEMI umbrella, and make it invisible to menuconfig, > > > just selectable from the 2 driver instances (MSCC_OCELOT_SWITCH_OCELOT > > > and NET_DSA_MSCC_FELIX). MSCC_OCELOT_SWITCH has no reason to be > > > selectable by the user anyway. > > > > You still need 'depends on NETDEVICES' in that case, otherwise this sounds > > like a good option. > > > > I don't completely understand this. Looks like NETDEVICES is another > one of those options that don't enable any code. I would have expected > that NET_SWITCHDEV depended on it already? But anyway, it's still a > small compromise and not a problem. My mistake: When I had tried it, I did run into this problem, but it was CONFIG_ETHERNET, not CONFIG_NETDEVICES. NET_SWITCHDEV depends only on INET, NET_DSA also depends on NETDEVICES, but neither of them depends on ETHERNET, which only controls drivers under drivers/net/ethernet, but not support for ethernet itself. > > So, if you agree, I can take care of this tomorrow by reworking the > Kconfig in the 1st proposed way. I hope you don't mind that I'm > volunteering to do it, but the change will require a bit of explaining > which is non-trivial, and I don't expect that you really want to know > these details, just that it compiles with no issue from all angles. Sounds good, thanks! If you like, I can give your patch a spin on my randconfig build system before you submit it for inclusion, in case there is another problem we both missed. Arnd
diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig index 0031ca814346..6f9804093150 100644 --- a/drivers/net/dsa/ocelot/Kconfig +++ b/drivers/net/dsa/ocelot/Kconfig @@ -2,6 +2,7 @@ config NET_DSA_MSCC_FELIX tristate "Ocelot / Felix Ethernet switch support" depends on NET_DSA && PCI + depends on NET_VENDOR_MICROSEMI select MSCC_OCELOT_SWITCH select NET_DSA_TAG_OCELOT help
Selecting MSCC_OCELOT_SWITCH is not possible when NET_VENDOR_MICROSEMI is disabled: WARNING: unmet direct dependencies detected for MSCC_OCELOT_SWITCH Depends on [n]: NETDEVICES [=y] && ETHERNET [=n] && NET_VENDOR_MICROSEMI [=n] && NET_SWITCHDEV [=y] && HAS_IOMEM [=y] Selected by [m]: - NET_DSA_MSCC_FELIX [=m] && NETDEVICES [=y] && HAVE_NET_DSA [=y] && NET_DSA [=y] && PCI [=y] Add a Kconfig dependency on NET_VENDOR_MICROSEMI, which also implies CONFIG_NETDEVICES. Fixes: 56051948773e ("net: dsa: ocelot: add driver for Felix switch family") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/net/dsa/ocelot/Kconfig | 1 + 1 file changed, 1 insertion(+)