[PATCH net-next] net: phy: drop PHYLIB_LEDS knob

[PATCH net-next] net: phy: drop PHYLIB_LEDS knob

[Paolo Abeni]

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(-)

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:
-- 
2.40.0

Re: [PATCH net-next] net: phy: drop PHYLIB_LEDS knob

[Arnd Bergmann]

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

Re: [PATCH net-next] net: phy: drop PHYLIB_LEDS knob

[Andrew Lunn]

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

Re: [PATCH net-next] net: phy: drop PHYLIB_LEDS knob

[Arnd Bergmann]

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

Re: [PATCH net-next] net: phy: drop PHYLIB_LEDS knob

[Jakub Kicinski]

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.

Re: [PATCH net-next] net: phy: drop PHYLIB_LEDS knob

[Jakub Kicinski]

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?

Re: [PATCH net-next] net: phy: drop PHYLIB_LEDS knob

[Andrew Lunn]

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

Re: [PATCH net-next] net: phy: drop PHYLIB_LEDS knob

[Paolo Abeni]

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