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

[Russell King (Oracle)]

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@xxxxxxxxxxxxxx> 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!