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

[Paolo Abeni]

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