netdev - Re: [PATCH net] net: phy: skip LED triggers on PHYs on SFP modules

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]

Date: Thu, 14 Dec 2023 16:52:07 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Daniel Golle <daniel@...rotopia.org>, Andrew Lunn <andrew@...n.ch>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: phy: skip LED triggers on PHYs on SFP modules

On Thu, Dec 14, 2023 at 10:48:10AM +0100, Paolo Abeni wrote:
> On Wed, 2023-12-13 at 19:01 +0000, Daniel Golle wrote:
> > On Wed, Dec 13, 2023 at 03:27:28PM +0000, Russell King (Oracle) wrote:
> > > On Wed, Dec 13, 2023 at 10:08:25AM +0100, Andrew Lunn wrote:
> > > > On Tue, Dec 12, 2023 at 03:35:12PM +0100, Maxime Chevallier wrote:
> > > > > Hi Daniel
> > > > > 
> > > > > On Tue, 12 Dec 2023 00:05:35 +0000
> > > > > Daniel Golle <daniel@...rotopia.org> wrote:
> > > > > 
> > > > > > Calling led_trigger_register() when attaching a PHY located on an SFP
> > > > > > module potentially (and practically) leads into a deadlock.
> > > > > > Fix this by not calling led_trigger_register() for PHYs localted on SFP
> > > > > > modules as such modules actually never got any LEDs.
> > > > > 
> > > > > While I don't have a fix for this issue, I think your justification
> > > > > isn't good. This isn't about having LEDs on the module or not, but
> > > > > rather the PHY triggering LED events for LEDS that can be located
> > > > > somewhere else on the system (like the front pannel of a switch).
> > > > 
> > > > SFP LEDs are very unlikely to be on the front panel, since there is no
> > > > such pins on the SFP cage.
> > > > 
> > > > Russell, in your collection of SFPs do you have any with LEDs?
> > > 
> > > No, and we should _not_ mess around with the "LED" configuration on
> > > PHYs on SFPs. It's possible that the LED output is wired to the LOS
> > > pin on the module, and messing around with the configuration of that
> > > would be asking for trouble.
> > > 
> > > In any case, I thought we didn't drive the LED configuration on PHYs
> > > where the LED configuration isn't described by firmware - and as the
> > > PHY on SFP modules would never be described by firmware, hooking
> > > such a PHY up to the LED framework sounds like a waste of resources
> > > to me.
> > 
> > This was exactly my line of thought when posting the patch, however,
> > Maxime correctly pointed out that the issue with locking and also
> > what the patch prevents is registration of LED *triggers* rather than
> > the PHY-controlled LEDs themselves. And having the triggers available
> > is desirable even beyond the hardware offloaded case (which is probably
> > the aspect we both were dealing with the most recently and hence had in
> > mind). It is common to control another system SoC GPIO driven LED(s)
> > representing the link status and rx/tx traffic, for example.
> > 
> > So better we get to the core of it and fix the locking issue
> > (for example by registering LED trigger asynchronously using
> > delayed_work)...
> 
> I understand you are looking for a different solution, so let me mark
> this patch accordingly.
> 
> --
> pw-bot: cr

I disagree with that - analysing the locking and coming up with a fix
is likely going to be a lengthy affair, meanwhile the mainline kernel
will deadlock on this. This patch prevents that deadlock at the
expense of removing the LED triggers for the PHY-on-SFP which I don't
think is a big deal considering the age of the PHY-based LED triggers.

So I personally would prefer this patch to be merged while a
different solution (that we have little idea at this point what it
would look like) is sought.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!