[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <66fff1c0.050a0220.f97fa.fec2@mx.google.com>
Date: Fri, 4 Oct 2024 15:46: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 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
Powered by blists - more mailing lists