[net PATCH 1/2] net: phy: Remove LED entry from LEDs list on unregister

[net PATCH 1/2] net: phy: Remove LED entry from LEDs list on unregister

[Christian Marangi]

Commit c938ab4da0eb ("net: phy: Manual remove LEDs to ensure correct
ordering") correctly fixed a problem with using devm_ but missed
removing the LED entry from the LEDs list.

This cause kernel panic on specific scenario where the port for the PHY
is torn down and up and the kmod for the PHY is removed.

On setting the port down the first time, the assosiacted LEDs are
correctly unregistered. The associated kmod for the PHY is now removed.
The kmod is now added again and the port is now put up, the associated LED
are registered again.
On putting the port down again for the second time after these step, the
LED list now have 4 elements. With the first 2 already unregistered
previously and the 2 new one registered again.

This cause a kernel panic as the first 2 element should have been
removed.

Fix this by correctly removing the element when LED is unregistered.

Reported-by: Daniel Golle <daniel@makrotopia.org>
Tested-by: Daniel Golle <daniel@makrotopia.org>
Cc: stable@vger.kernel.org
Fixes: c938ab4da0eb ("net: phy: Manual remove LEDs to ensure correct ordering")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy_device.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 560e338b307a..499797646580 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3326,10 +3326,11 @@ static __maybe_unused int phy_led_hw_is_supported(struct led_classdev *led_cdev,
 
 static void phy_leds_unregister(struct phy_device *phydev)
 {
-	struct phy_led *phyled;
+	struct phy_led *phyled, *tmp;
 
-	list_for_each_entry(phyled, &phydev->leds, list) {
+	list_for_each_entry_safe(phyled, tmp, &phydev->leds, list) {
 		led_classdev_unregister(&phyled->led_cdev);
+		list_del(&phyled->list);
 	}
 }
 
-- 
2.45.2

[net PATCH 2/2] net: phy: Skip PHY LEDs OF registration for Generic PHY driver

[Christian Marangi]

It might happen that a PHY driver fails to probe or is not present in
the system as it's a kmod. In such case the Device Tree might have LED
entry but the Generic PHY is probed instead.

In this scenario, PHY LEDs OF registration should be skipped as
controlling the PHY LEDs is not possible.

Tested-by: Daniel Golle <daniel@makrotopia.org>
Cc: stable@vger.kernel.org
Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy_device.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 499797646580..af088bf00bae 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3411,6 +3411,11 @@ static int of_phy_leds(struct phy_device *phydev)
 	struct device_node *leds;
 	int err;
 
+	/* Skip LED registration if we are Generic PHY */
+	if (phy_driver_is_genphy(phydev) ||
+	    phy_driver_is_genphy_10g(phydev))
+		return 0;
+
 	if (!IS_ENABLED(CONFIG_OF_MDIO))
 		return 0;
 
-- 
2.45.2

Re: [net PATCH 1/2] net: phy: Remove LED entry from LEDs list on unregister

[Andrew Lunn]

On Fri, Oct 04, 2024 at 12:10:04AM +0200, Christian Marangi wrote:
> Commit c938ab4da0eb ("net: phy: Manual remove LEDs to ensure correct
> ordering") correctly fixed a problem with using devm_ but missed
> removing the LED entry from the LEDs list.
> 
> This cause kernel panic on specific scenario where the port for the PHY
> is torn down and up and the kmod for the PHY is removed.
> 
> On setting the port down the first time, the assosiacted LEDs are
> correctly unregistered. The associated kmod for the PHY is now removed.
> The kmod is now added again and the port is now put up, the associated LED
> are registered again.
> On putting the port down again for the second time after these step, the
> LED list now have 4 elements. With the first 2 already unregistered
> previously and the 2 new one registered again.
> 
> This cause a kernel panic as the first 2 element should have been
> removed.
> 
> Fix this by correctly removing the element when LED is unregistered.
> 
> Reported-by: Daniel Golle <daniel@makrotopia.org>
> Tested-by: Daniel Golle <daniel@makrotopia.org>
> Cc: stable@vger.kernel.org
> Fixes: c938ab4da0eb ("net: phy: Manual remove LEDs to ensure correct ordering")
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [net PATCH 2/2] net: phy: Skip PHY LEDs OF registration for Generic PHY driver

[Andrew Lunn]

On Fri, Oct 04, 2024 at 12:10:05AM +0200, Christian Marangi wrote:
> It might happen that a PHY driver fails to probe or is not present in
> the system as it's a kmod. In such case the Device Tree might have LED
> entry but the Generic PHY is probed instead.
> 
> In this scenario, PHY LEDs OF registration should be skipped as
> controlling the PHY LEDs is not possible.
> 
> Tested-by: Daniel Golle <daniel@makrotopia.org>
> Cc: stable@vger.kernel.org
> Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/phy_device.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 499797646580..af088bf00bae 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3411,6 +3411,11 @@ static int of_phy_leds(struct phy_device *phydev)
>  	struct device_node *leds;
>  	int err;
>  
> +	/* Skip LED registration if we are Generic PHY */
> +	if (phy_driver_is_genphy(phydev) ||
> +	    phy_driver_is_genphy_10g(phydev))
> +		return 0;

Why fix it link this, when what you propose for net-next, that the drv
ops must also exist, would fix it.

I don't see any need to special case genphy.

	Andrew

Re: [net PATCH 2/2] net: phy: Skip PHY LEDs OF registration for Generic PHY driver

[Christian Marangi]

On Fri, Oct 04, 2024 at 12:50:39AM +0200, Andrew Lunn wrote:
> On Fri, Oct 04, 2024 at 12:10:05AM +0200, Christian Marangi wrote:
> > It might happen that a PHY driver fails to probe or is not present in
> > the system as it's a kmod. In such case the Device Tree might have LED
> > entry but the Generic PHY is probed instead.
> > 
> > In this scenario, PHY LEDs OF registration should be skipped as
> > controlling the PHY LEDs is not possible.
> > 
> > Tested-by: Daniel Golle <daniel@makrotopia.org>
> > Cc: stable@vger.kernel.org
> > Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/phy/phy_device.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 499797646580..af088bf00bae 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -3411,6 +3411,11 @@ static int of_phy_leds(struct phy_device *phydev)
> >  	struct device_node *leds;
> >  	int err;
> >  
> > +	/* Skip LED registration if we are Generic PHY */
> > +	if (phy_driver_is_genphy(phydev) ||
> > +	    phy_driver_is_genphy_10g(phydev))
> > +		return 0;
> 
> Why fix it link this, when what you propose for net-next, that the drv
> ops must also exist, would fix it.
> 
> I don't see any need to special case genphy.
>

While the patch in net-next fix a broken condition (PHY driver exist but
doesn't have LEDs OPs), this account a much possible scenario.

It's totally ok if the PHY driver is not loaded and we fallback to the
Generic PHY and there are LEDs node.

This is the case with something like
ip link set eth0 down
rmmod air_en8811h
ip link set eth0 up

On this up, the Generic PHY is loaded and LEDs will wrongly be
registered. We should not add the LED to the phydev LEDs list.

Do you think this logic is wrong and we should print a warning also in
this case? Or should we bite it and just return 0 with no warning at
all? (again my concern is the additional LEDs entry in sysfs that won't
be actually usable as everything will be rejected)

-- 
	Ansuel

Re: [net PATCH 2/2] net: phy: Skip PHY LEDs OF registration for Generic PHY driver

[Andrew Lunn]

> While the patch in net-next fix a broken condition (PHY driver exist but
> doesn't have LEDs OPs), this account a much possible scenario.
> 
> It's totally ok if the PHY driver is not loaded and we fallback to the
> Generic PHY and there are LEDs node.
> 
> This is the case with something like
> ip link set eth0 down
> rmmod air_en8811h
> ip link set eth0 up
> 
> On this up, the Generic PHY is loaded and LEDs will wrongly be
> registered. We should not add the LED to the phydev LEDs list.
> 
> Do you think this logic is wrong and we should print a warning also in
> this case? Or should we bite it and just return 0 with no warning at
> all? (again my concern is the additional LEDs entry in sysfs that won't
> be actually usable as everything will be rejected)

We should not add LEDs which we cannot drive. That much is clear to
me.

I would also agree that LEDs in DT which we cannot drive is not
fatal. So the return value should be 0.

The only really open point is phydev_err(), phydev_warn() or
phydev_dbg(). Since it is not fatal, phydev_err() is wrong. I would
probably go with phydev_dbg(), to aid somebody debugging why the LEDs
don't appear in some conditions.

	Andrew

Re: [net PATCH 2/2] net: phy: Skip PHY LEDs OF registration for Generic PHY driver

[Christian Marangi]

On Fri, Oct 04, 2024 at 03:44:33PM +0200, Andrew Lunn wrote:
> > While the patch in net-next fix a broken condition (PHY driver exist but
> > doesn't have LEDs OPs), this account a much possible scenario.
> > 
> > It's totally ok if the PHY driver is not loaded and we fallback to the
> > Generic PHY and there are LEDs node.
> > 
> > This is the case with something like
> > ip link set eth0 down
> > rmmod air_en8811h
> > ip link set eth0 up
> > 
> > On this up, the Generic PHY is loaded and LEDs will wrongly be
> > registered. We should not add the LED to the phydev LEDs list.
> > 
> > Do you think this logic is wrong and we should print a warning also in
> > this case? Or should we bite it and just return 0 with no warning at
> > all? (again my concern is the additional LEDs entry in sysfs that won't
> > be actually usable as everything will be rejected)
> 
> We should not add LEDs which we cannot drive. That much is clear to
> me.
> 
> I would also agree that LEDs in DT which we cannot drive is not
> fatal. So the return value should be 0.
> 
> The only really open point is phydev_err(), phydev_warn() or
> phydev_dbg(). Since it is not fatal, phydev_err() is wrong. I would
> probably go with phydev_dbg(), to aid somebody debugging why the LEDs
> don't appear in some conditions.
>

Ok I will squash this and the net-next patch and change to dbg.

Do you think it's still "net" content? I'm more tempted to post in
net-next since I have to drop the Generic PHY condition.

-- 
	Ansuel

Re: [net PATCH 2/2] net: phy: Skip PHY LEDs OF registration for Generic PHY driver

[Andrew Lunn]

> Ok I will squash this and the net-next patch and change to dbg.
> 
> Do you think it's still "net" content? I'm more tempted to post in
> net-next since I have to drop the Generic PHY condition.

Does it cause real problems for users? That is the requirement for
stable.

	Andrew

Re: [net PATCH 2/2] net: phy: Skip PHY LEDs OF registration for Generic PHY driver

[Christian Marangi]

On Fri, Oct 04, 2024 at 04:11:22PM +0200, Andrew Lunn wrote:
> > Ok I will squash this and the net-next patch and change to dbg.
> > 
> > Do you think it's still "net" content? I'm more tempted to post in
> > net-next since I have to drop the Generic PHY condition.
> 
> Does it cause real problems for users? That is the requirement for
> stable.
>

Not strictly bugs or kernel panic, just annoyance, ok will post to
net-next. 

-- 
	Ansuel