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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <66ffb1c2.df0a0220.1b4c87.ce13@mx.google.com>
Date: Fri, 4 Oct 2024 11:13:32 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, Daniel Golle <daniel@...rotopia.org>,
	stable@...r.kernel.org
Subject: Re: [net PATCH 2/2] net: phy: Skip PHY LEDs OF registration for
 Generic PHY driver

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@...rotopia.org>
> > Cc: stable@...r.kernel.org
> > Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
> > Signed-off-by: Christian Marangi <ansuelsmth@...il.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