| Message ID | c783f6b8d8cc08100b13ce50a1330913dd95dbce.1682457539.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: drop PHYLIB_LEDS knob | expand |
On Tue, Apr 25, 2023, at 22:19, Paolo Abeni wrote: > --- > @Andrew, @Arnd: the rationale here is to avoid the new config knob=y, > which caused in the past a few complains from Linus. In this case I > think the raised exception is not valid, for the reason mentioned above. > > If you have different preferences or better solutions to address that, > please voice them :) I think using IS_REACHABLE() is generally much worse than having another explicit option, because it makes it harder for users to figure out why something does not work as they had expected it to. Note that I'm the one who introduced IS_REACHABLE() to start with, but the intention at the time was really to replace open-coded logic doing the same thing, not to have it as a generic way to hide broken dependencies. Arnmd
On Tue, Apr 25, 2023 at 11:19:11PM +0200, Paolo Abeni wrote: > commit 4bb7aac70b5d ("net: phy: fix circular LEDS_CLASS dependencies") > solved a build failure, but introduces a new config knob with a default > 'y' value: PHYLIB_LEDS. > > The latter is against the current new config policy. The exception > was raised to allow the user to catch bad configurations without led > support. > > Anyway the current definition of PHYLIB_LEDS does not fit the above > goal: if LEDS_CLASS is disabled, the new config will be available > only with PHYLIB disabled, too. > > Instead of building a more complex and error-prone dependency definition > it looks simpler and more in line with the mentioned policies use > IS_REACHABLE(CONFIG_LEDS_CLASS) instead of the new symbol. > > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > @Andrew, @Arnd: the rationale here is to avoid the new config knob=y, > which caused in the past a few complains from Linus. In this case I > think the raised exception is not valid, for the reason mentioned above. > > If you have different preferences or better solutions to address that, > please voice them :) Arnd did mention making it an invisible option. That would have the advantage of keeping the hundreds of randcomfig builds which have been done. How much time do you have now to do that before sending Linus the pull request? > --- > drivers/net/phy/Kconfig | 9 --------- > drivers/net/phy/phy_device.c | 2 +- > 2 files changed, 1 insertion(+), 10 deletions(-) > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 2f3ddc446cbb..f83420b86794 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -44,15 +44,6 @@ config LED_TRIGGER_PHY > <Speed in megabits>Mbps OR <Speed in gigabits>Gbps OR link > for any speed known to the PHY. > > -config PHYLIB_LEDS > - bool "Support probing LEDs from device tree" I don't know Kconfig to well, but i think you just need to remove the text, just keep the bool. - bool "Support probing LEDs from device tree" + bool Andrew
On Tue, Apr 25, 2023, at 22:38, Andrew Lunn wrote: >> >> -config PHYLIB_LEDS >> - bool "Support probing LEDs from device tree" > > I don't know Kconfig to well, but i think you just need to remove the > text, just keep the bool. > > - bool "Support probing LEDs from device tree" > + bool Right, that should work, or it can become def_bool y or even def_bool OF for brevity. Arnd
On Tue, 25 Apr 2023 22:35:46 +0100 Arnd Bergmann wrote: > > @Andrew, @Arnd: the rationale here is to avoid the new config knob=y, > > which caused in the past a few complains from Linus. In this case I > > think the raised exception is not valid, for the reason mentioned above. > > > > If you have different preferences or better solutions to address that, > > please voice them :) > > I think using IS_REACHABLE() is generally much worse than having another > explicit option, because it makes it harder for users to figure out why > something does not work as they had expected it to. > > Note that I'm the one who introduced IS_REACHABLE() to start with, > but the intention at the time was really to replace open-coded > logic doing the same thing, not to have it as a generic way to > hide broken dependencies. Agreed :( But that kind of presupposed that user knows what they are looking for, right? My thinking was this: using "depends on" instead and preventing the bad configuration from occurring is a strongly preferred alternative to IS_REACHABLE(). But an extra third option which will be hidden from nconfig will not prevent user from doing LEDS=m PHYS=y.
On Tue, 25 Apr 2023 22:44:34 +0100 Arnd Bergmann wrote: > On Tue, Apr 25, 2023, at 22:38, Andrew Lunn wrote: > >> > >> -config PHYLIB_LEDS > >> - bool "Support probing LEDs from device tree" > > > > I don't know Kconfig to well, but i think you just need to remove the > > text, just keep the bool. > > > > - bool "Support probing LEDs from device tree" > > + bool > > Right, that should work, or it can become > > def_bool y > > or even > > def_bool OF > > for brevity. Hm, I think Paolo was concerned that we'd get PHYLIB_LEDS=y if PHYLIB=n and LEDS_CLASS=n. But that's not possible because the option is in the "if PHYLIB" section. Is that right?
On Tue, Apr 25, 2023 at 03:01:11PM -0700, Jakub Kicinski wrote: > On Tue, 25 Apr 2023 22:44:34 +0100 Arnd Bergmann wrote: > > On Tue, Apr 25, 2023, at 22:38, Andrew Lunn wrote: > > >> > > >> -config PHYLIB_LEDS > > >> - bool "Support probing LEDs from device tree" > > > > > > I don't know Kconfig to well, but i think you just need to remove the > > > text, just keep the bool. > > > > > > - bool "Support probing LEDs from device tree" > > > + bool > > > > Right, that should work, or it can become > > > > def_bool y > > > > or even > > > > def_bool OF > > > > for brevity. > > Hm, I think Paolo was concerned that we'd get PHYLIB_LEDS=y if PHYLIB=n > and LEDS_CLASS=n. But that's not possible because the option is in the > "if PHYLIB" section. > > Is that right? Seems correct to me. But a randconfig test bot is who you really want conformation from. The bot is probably harder to keep happy then Linus. Andrew
On Tue, 2023-04-25 at 23:38 +0200, Andrew Lunn wrote: > On Tue, Apr 25, 2023 at 11:19:11PM +0200, Paolo Abeni wrote: > > commit 4bb7aac70b5d ("net: phy: fix circular LEDS_CLASS dependencies") > > solved a build failure, but introduces a new config knob with a default > > 'y' value: PHYLIB_LEDS. > > > > The latter is against the current new config policy. The exception > > was raised to allow the user to catch bad configurations without led > > support. > > > > Anyway the current definition of PHYLIB_LEDS does not fit the above > > goal: if LEDS_CLASS is disabled, the new config will be available > > only with PHYLIB disabled, too. > > > > Instead of building a more complex and error-prone dependency definition > > it looks simpler and more in line with the mentioned policies use > > IS_REACHABLE(CONFIG_LEDS_CLASS) instead of the new symbol. > > > > Suggested-by: Jakub Kicinski <kuba@kernel.org> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > @Andrew, @Arnd: the rationale here is to avoid the new config knob=y, > > which caused in the past a few complains from Linus. In this case I > > think the raised exception is not valid, for the reason mentioned above. > > > > If you have different preferences or better solutions to address that, > > please voice them :) > > Arnd did mention making it an invisible option. That would have the > advantage of keeping the hundreds of randcomfig builds which have been > done. How much time do you have now to do that before sending Linus > the pull request? Very little, I would say. > > --- > > drivers/net/phy/Kconfig | 9 --------- > > drivers/net/phy/phy_device.c | 2 +- > > 2 files changed, 1 insertion(+), 10 deletions(-) > > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > > index 2f3ddc446cbb..f83420b86794 100644 > > --- a/drivers/net/phy/Kconfig > > +++ b/drivers/net/phy/Kconfig > > @@ -44,15 +44,6 @@ config LED_TRIGGER_PHY > > <Speed in megabits>Mbps OR <Speed in gigabits>Gbps OR link > > for any speed known to the PHY. > > > > -config PHYLIB_LEDS > > - bool "Support probing LEDs from device tree" > > I don't know Kconfig to well, but i think you just need to remove the > text, just keep the bool. > > - bool "Support probing LEDs from device tree" > + bool > > Andrew I read the following discussion as substantial agreement with the above. I'll share and use a formal patch with that. Thanks, Paolo
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 2f3ddc446cbb..f83420b86794 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -44,15 +44,6 @@ config LED_TRIGGER_PHY <Speed in megabits>Mbps OR <Speed in gigabits>Gbps OR link for any speed known to the PHY. -config PHYLIB_LEDS - bool "Support probing LEDs from device tree" - depends on LEDS_CLASS=y || LEDS_CLASS=PHYLIB - depends on OF - default y - help - When LED class support is enabled, phylib can automatically - probe LED setting from device tree. - config FIXED_PHY tristate "MDIO Bus/PHY emulation with fixed speed/link PHYs" select SWPHY diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 17d0d0555a79..fd28d389b00f 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -3288,7 +3288,7 @@ static int phy_probe(struct device *dev) /* Get the LEDs from the device tree, and instantiate standard * LEDs for them. */ - if (IS_ENABLED(CONFIG_PHYLIB_LEDS)) + if (IS_REACHABLE(CONFIG_LEDS_CLASS)) err = of_phy_leds(phydev); out:
commit 4bb7aac70b5d ("net: phy: fix circular LEDS_CLASS dependencies") solved a build failure, but introduces a new config knob with a default 'y' value: PHYLIB_LEDS. The latter is against the current new config policy. The exception was raised to allow the user to catch bad configurations without led support. Anyway the current definition of PHYLIB_LEDS does not fit the above goal: if LEDS_CLASS is disabled, the new config will be available only with PHYLIB disabled, too. Instead of building a more complex and error-prone dependency definition it looks simpler and more in line with the mentioned policies use IS_REACHABLE(CONFIG_LEDS_CLASS) instead of the new symbol. Suggested-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- @Andrew, @Arnd: the rationale here is to avoid the new config knob=y, which caused in the past a few complains from Linus. In this case I think the raised exception is not valid, for the reason mentioned above. If you have different preferences or better solutions to address that, please voice them :) --- drivers/net/phy/Kconfig | 9 --------- drivers/net/phy/phy_device.c | 2 +- 2 files changed, 1 insertion(+), 10 deletions(-)