linux-kernel - 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: <a463ca8c-ebd7-4fd4-98a9-bc869a92548c@lunn.ch>
Date: Fri, 4 Oct 2024 15:44:33 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Christian Marangi <ansuelsmth@...il.com>
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

> 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