[net-next PATCH] net: phy: Validate PHY LED OPs presence before registering

[net-next PATCH] net: phy: Validate PHY LED OPs presence before registering

[Christian Marangi]

Validate PHY LED OPs presence before registering and parsing them.
Defining LED nodes for a PHY driver that actually doesn't supports them
is wrong and should be reported.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy_device.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index af088bf00bae..ce154a54bfa4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3426,6 +3426,16 @@ static int of_phy_leds(struct phy_device *phydev)
 	if (!leds)
 		return 0;
 
+	/* Check if the PHY driver have at least an OP to
+	 * set the LEDs.
+	 */
+	if (!phydev->drv->led_brightness_set &&
+	    !phydev->drv->led_blink_set &&
+	    !phydev->drv->led_hw_control_set) {
+		phydev_err(phydev, "ignoring leds node defined with no PHY driver support\n");
+		goto exit;
+	}
+
 	for_each_available_child_of_node_scoped(leds, led) {
 		err = of_phy_led(phydev, led);
 		if (err) {
@@ -3435,6 +3445,7 @@ static int of_phy_leds(struct phy_device *phydev)
 		}
 	}
 
+exit:
 	of_node_put(leds);
 	return 0;
 }
-- 
2.45.2

Re: [net-next PATCH] net: phy: Validate PHY LED OPs presence before registering

[Vladimir Oltean]

On Fri, Oct 04, 2024 at 12:12:48AM +0200, Christian Marangi wrote:
> Validate PHY LED OPs presence before registering and parsing them.
> Defining LED nodes for a PHY driver that actually doesn't supports them
> is wrong and should be reported.

What about the case where a PHY driver gets LED support in the future?
Shouldn't the current kernel driver work with future device trees which
define LEDs, and just ignore that node, rather than fail to probe?

Re: [net-next PATCH] net: phy: Validate PHY LED OPs presence before registering

[Christian Marangi]

On Fri, Oct 04, 2024 at 01:24:00AM +0300, Vladimir Oltean wrote:
> On Fri, Oct 04, 2024 at 12:12:48AM +0200, Christian Marangi wrote:
> > Validate PHY LED OPs presence before registering and parsing them.
> > Defining LED nodes for a PHY driver that actually doesn't supports them
> > is wrong and should be reported.
> 
> What about the case where a PHY driver gets LED support in the future?
> Shouldn't the current kernel driver work with future device trees which
> define LEDs, and just ignore that node, rather than fail to probe?

Well this just skip leds node parse and return 0, so no fail to probe.
This just adds an error. Maybe I should use warn instead?

(The original idea was to return -EINVAL but it was suggested by Daniel
that this was too much and a print was much better)

-- 
	Ansuel

Re: [net-next PATCH] net: phy: Validate PHY LED OPs presence before registering

[Andrew Lunn]

On Fri, Oct 04, 2024 at 12:33:17AM +0200, Christian Marangi wrote:
> On Fri, Oct 04, 2024 at 01:24:00AM +0300, Vladimir Oltean wrote:
> > On Fri, Oct 04, 2024 at 12:12:48AM +0200, Christian Marangi wrote:
> > > Validate PHY LED OPs presence before registering and parsing them.
> > > Defining LED nodes for a PHY driver that actually doesn't supports them
> > > is wrong and should be reported.
> > 
> > What about the case where a PHY driver gets LED support in the future?
> > Shouldn't the current kernel driver work with future device trees which
> > define LEDs, and just ignore that node, rather than fail to probe?
> 
> Well this just skip leds node parse and return 0, so no fail to probe.
> This just adds an error. Maybe I should use warn instead?

Yes, a phydev_warn() would be better.

	Andrew

Re: [net-next PATCH] net: phy: Validate PHY LED OPs presence before registering

[Vladimir Oltean]

On Fri, Oct 04, 2024 at 12:51:32AM +0200, Andrew Lunn wrote:
> On Fri, Oct 04, 2024 at 12:33:17AM +0200, Christian Marangi wrote:
> > On Fri, Oct 04, 2024 at 01:24:00AM +0300, Vladimir Oltean wrote:
> > > On Fri, Oct 04, 2024 at 12:12:48AM +0200, Christian Marangi wrote:
> > > > Validate PHY LED OPs presence before registering and parsing them.
> > > > Defining LED nodes for a PHY driver that actually doesn't supports them
> > > > is wrong and should be reported.
> > > 
> > > What about the case where a PHY driver gets LED support in the future?
> > > Shouldn't the current kernel driver work with future device trees which
> > > define LEDs, and just ignore that node, rather than fail to probe?
> > 
> > Well this just skip leds node parse and return 0, so no fail to probe.
> > This just adds an error. Maybe I should use warn instead?
> 
> Yes, a phydev_warn() would be better.

I'm thinking even KERN_WARN is too much. There's nothing actionable
about the message.

Re: [net-next PATCH] net: phy: Validate PHY LED OPs presence before registering

[Vladimir Oltean]

On Fri, Oct 04, 2024 at 12:33:17AM +0200, Christian Marangi wrote:
> On Fri, Oct 04, 2024 at 01:24:00AM +0300, Vladimir Oltean wrote:
> > On Fri, Oct 04, 2024 at 12:12:48AM +0200, Christian Marangi wrote:
> > > Validate PHY LED OPs presence before registering and parsing them.
> > > Defining LED nodes for a PHY driver that actually doesn't supports them
> > > is wrong and should be reported.
> > 
> > What about the case where a PHY driver gets LED support in the future?
> > Shouldn't the current kernel driver work with future device trees which
> > define LEDs, and just ignore that node, rather than fail to probe?
> 
> Well this just skip leds node parse and return 0, so no fail to probe.
> This just adds an error. Maybe I should use warn instead?
> 
> (The original idea was to return -EINVAL but it was suggested by Daniel
> that this was too much and a print was much better)

Ok, the "exit" label returns 0, not a probe failure, but as you say,
there's still the warning message printed to dmesg. What's its intended
value, exactly?

What would you do if you were working on a board which wasn't supported
in mainline but instead you only had the DTB for it, and you had to run
a git bisect back to when the driver didn't support parsing the PHY LED
nodes? What would you do, edit the DTB to add/remove the node at each
bisect step, so that the kernel gets what it understands in the device
tree and nothing more?

Why would the kernel even act so weird about it and print warnings or
return errors in the first place? Nobody could possibly develop anything
new with patches like this, without introducing some sort of mishap in
past kernels. Is there some larger context around this patch which I'm
missing?

Re: [net-next PATCH] net: phy: Validate PHY LED OPs presence before registering

[Christian Marangi]

On Fri, Oct 04, 2024 at 02:03:57AM +0300, Vladimir Oltean wrote:
> On Fri, Oct 04, 2024 at 12:33:17AM +0200, Christian Marangi wrote:
> > On Fri, Oct 04, 2024 at 01:24:00AM +0300, Vladimir Oltean wrote:
> > > On Fri, Oct 04, 2024 at 12:12:48AM +0200, Christian Marangi wrote:
> > > > Validate PHY LED OPs presence before registering and parsing them.
> > > > Defining LED nodes for a PHY driver that actually doesn't supports them
> > > > is wrong and should be reported.
> > > 
> > > What about the case where a PHY driver gets LED support in the future?
> > > Shouldn't the current kernel driver work with future device trees which
> > > define LEDs, and just ignore that node, rather than fail to probe?
> > 
> > Well this just skip leds node parse and return 0, so no fail to probe.
> > This just adds an error. Maybe I should use warn instead?
> > 
> > (The original idea was to return -EINVAL but it was suggested by Daniel
> > that this was too much and a print was much better)
> 
> Ok, the "exit" label returns 0, not a probe failure, but as you say,
> there's still the warning message printed to dmesg. What's its intended
> value, exactly?
> 
> What would you do if you were working on a board which wasn't supported
> in mainline but instead you only had the DTB for it, and you had to run
> a git bisect back to when the driver didn't support parsing the PHY LED
> nodes? What would you do, edit the DTB to add/remove the node at each
> bisect step, so that the kernel gets what it understands in the device
> tree and nothing more?
> 
> Why would the kernel even act so weird about it and print warnings or
> return errors in the first place? Nobody could possibly develop anything
> new with patches like this, without introducing some sort of mishap in
> past kernels. Is there some larger context around this patch which I'm
> missing?

No larger context. I posted 2 other patch in net that fix some problem
and found it strange that this change wasn't in place. Looks wrong to
define leds node for a PHY ID with the driver not having OPs for it.

I posted this in net-next detached from the other 2 as I wasn't sure if
it was that correct, just taste. The problematic part is registering a
LED that is actually not usable cause as you described I can totally see
someone proposing downstream DTS with LED in the PHY node and never
actually care to implement LED support for it. User might get confused
and think there is some kind of bug in the driver as he would have LED
present but everything gives errors, while in reality is just the
feature missing in the driver.

Totally ok to ignore this but aside from the bisec scenario that can
produce false-warning, the warning might be useful in some situation.

-- 
	Ansuel