LinuxLists.cc - [PATCH net] net: phy: skip LED triggers on PHYs on SFP modules

[permalink] [raw]

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

Hi Daniel

On Tue, 12 Dec 2023 00:05:35 +0000
Daniel Golle <[email protected]> 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).

So I think it would be wiser to avoid the deadlock with a proper
analysis of what the locking scheme does. Maybe Andrew or Russell
have a better vision of what's going-on here, I tried to dive into
it but it doesn't look straightfoward to me :(

Maxime

2023-12-13 09:08:56

[permalink] [raw]

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

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 <[email protected]> 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?

> So I think it would be wiser to avoid the deadlock with a proper
> analysis of what the locking scheme does. Maybe Andrew or Russell
> have a better vision of what's going-on here, I tried to dive into
> it but it doesn't look straightfoward to me :(

I agree we should at least look at the deadlock, and see if we can
rearrange the locks to avoid it, even if there are no SFPs with LEDs.

Andrew

2023-12-13 10:06:40

[permalink] [raw]

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

On Wed, 13 Dec 2023 10:08:25 +0100
Andrew Lunn <[email protected]> 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 <[email protected]> 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?

I mean, aren't the led triggers generic so that it can trigger any
other LED to blink, and it's up to the user to decide ?

I do however see one good thing with this patch is that it makes the
behaviour coherent regarding triggers regardless if we have a
media-converter or not.

If we have a SFP bus with phylink as its upstream (SFP bus directly
connected to the MAC), then the phy is going to be attached through
phy_attach_direct(), and before this patch, led triggers will be
registered.

If we have an intermediate PHY acting as a media-converter and
connected to the SFP bus, then the phy isn't attached to the net_device,
and the triggers aren't registered.

So if in the end it doesn't make sense to register led triggers for
PHY in modules, it might be more coherent not registering them at all
as this patch does. What do you think ?

Thanks,

Maxime

2023-12-13 10:48:20

[permalink] [raw]

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

> > 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?
>
> I mean, aren't the led triggers generic so that it can trigger any
> other LED to blink, and it's up to the user to decide ?

Correct. However, generic LEDs won't be registered via this code path,
so the deadlock is not an issue. Only LED controllers in a PHY within
an SFP, inside an SFP cage are the issue here. I don't have any Copper
SFP modules, so i've no idea if they are physically big enough to have
LEDs?

> I do however see one good thing with this patch is that it makes the
> behaviour coherent regarding triggers regardless if we have a
> media-converter or not.
>
> If we have a SFP bus with phylink as its upstream (SFP bus directly
> connected to the MAC), then the phy is going to be attached through
> phy_attach_direct(), and before this patch, led triggers will be
> registered.
>
> If we have an intermediate PHY acting as a media-converter and
> connected to the SFP bus, then the phy isn't attached to the net_device,
> and the triggers aren't registered.
>
> So if in the end it doesn't make sense to register led triggers for
> PHY in modules, it might be more coherent not registering them at all
> as this patch does. What do you think ?

I think it is all messy. Say the media converter has its LEDs
connected to the front panel. You then get indications of activity on
the front panel. I've never seen a fibre SFP with LEDs, and its an
open question if Copper SFPs have LEDs. So you are limited to the MAC
LEDs or the media converter LEDs. Another scenario could be a PHY
which acts as a media switch, it can have an RJ-45 or an SFP cage,
first to get link wins. In such a situation, i would put the LEDs on
the front panel, since it would look odd for an empty RJ-45 socket
LEDs to blink for SFP activity.

So i think we should have the ability to describe all the LEDs in DT
and let it decided what gets registered.

Andrew

2023-12-13 15:31:35

[permalink] [raw]

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

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 <[email protected]> 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.

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

2023-12-13 15:39:01

[permalink] [raw]

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

On Wed, Dec 13, 2023 at 11:47:50AM +0100, Andrew Lunn wrote:
> > > 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?
> >
> > I mean, aren't the led triggers generic so that it can trigger any
> > other LED to blink, and it's up to the user to decide ?
>
> Correct. However, generic LEDs won't be registered via this code path,
> so the deadlock is not an issue. Only LED controllers in a PHY within
> an SFP, inside an SFP cage are the issue here. I don't have any Copper
> SFP modules, so i've no idea if they are physically big enough to have
> LEDs?

The ones I have, that is indeed the case - the RJ45 socket is the
absolute minimum size, with just enough metalwork around it to support
a RJ45 plug.

> I think it is all messy. Say the media converter has its LEDs
> connected to the front panel. You then get indications of activity on
> the front panel. I've never seen a fibre SFP with LEDs, and its an
> open question if Copper SFPs have LEDs.

A fibre module would only be able to repeat the information given via
RX_LOS and/or TX_FAULT if it had space to do so.

It's more normal in routers with SFP slots to see LEDs that are either
part of the SFP socket itself, or provided elsewhere.

> Another scenario could be a PHY
> which acts as a media switch, it can have an RJ-45 or an SFP cage,
> first to get link wins. In such a situation, i would put the LEDs on
> the front panel, since it would look odd for an empty RJ-45 socket
> LEDs to blink for SFP activity.

... and an example of this kind of setup would be the 88x3310 on
Macchiatobin - the LEDs are on the RJ45 socket, but they also
indicate for the status of the SFP connection if that is in use. I
don't remember off the top of my head what the LED configuration
register values allow one to select. We don't drive them because I
gave up on that idea - I don't believe that our LED framework is
"powerful" enough to be able to sensibly configure them... and I
personally use my own patches with register values in DT to
configure them to indicate sensibly.

However, from what I remember, configuring a LED to indicate for
1000M will mean that it will indicate whether the copper or fibre
interfaces are operating at 1000M.

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

2023-12-13 17:13:08

[permalink] [raw]

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

Hi Andrew, Russell,

On Wed, 13 Dec 2023 15:27:28 +0000
"Russell King (Oracle)" <[email protected]> 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 <[email protected]> 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.
>

So it looks to me that the Daniel's patch does make sense then, even
without considering the underlying locking issue ?

Sorry for my misunderstanding of the LED driving that started this
discussion :/

Maxime

2023-12-13 19:02:29

by Daniel Golle

[permalink] [raw]

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

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 <[email protected]> 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)...

2023-12-13 20:23:22

[permalink] [raw]

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

On Wed, Dec 13, 2023 at 07:01:29PM +0000, Daniel Golle wrote:
> On Wed, Dec 13, 2023 at 03:27:28PM +0000, Russell King (Oracle) wrote:
> > 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 don't think a delayed work solves anything. Well, it solves the
registration problem, but when the PHY is removed, you have to
_synchronously_ cancel the delayed work, which could result in it
waiting on the called work function to complete. If that called work
function is waiting for the lock which we're holding on the remove
path, then we're still in a deadlock-prone situation.

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

2023-12-14 09:48:42

by Paolo Abeni

[permalink] [raw]

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

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 <[email protected]> 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

2023-12-14 16:53:21

[permalink] [raw]

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 <[email protected]> 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!

2023-12-15 02:31:35

by Jakub Kicinski

[permalink] [raw]

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

On Tue, 12 Dec 2023 00:05:35 +0000 Daniel Golle 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.

Any suggestion of a Fixes tag?
Looks like the triggers were added a while back, are we only seeing it
now because we started exercising the code more?

2023-12-15 03:10:37

by Daniel Golle

[permalink] [raw]

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

On Thu, Dec 14, 2023 at 06:31:23PM -0800, Jakub Kicinski wrote:
> On Tue, 12 Dec 2023 00:05:35 +0000 Daniel Golle 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.
>
> Any suggestion of a Fixes tag?
> Looks like the triggers were added a while back, are we only seeing it
> now because we started exercising the code more?

I've noticed this as a consequence of commit 2f3ce7a56
"net: sfp: rework the RollBall PHY waiting code"
because (?) some PHYs on SFP+ now probe immediately as the previously
enforced minimum delay now became a timeout.

Blaming that commit would be wrong though, it's more that the problem
has probably always been there and was just previously augmented by
the delay.

2023-12-15 09:46:59

[permalink] [raw]

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

> 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.

So, if i'm reading this patch correctly, it only affects PHYs within
SFPs.

The discussion went off on a tangent and also talked about media
converter PHYs, but from my reading of this patch, they are unaffected
by this patch. Do they however also suffer from this deadlock? Anybody
tested that?

Andrew

2023-12-15 09:53:41

[permalink] [raw]

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

2023-12-15 09:59:49

[permalink] [raw]

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

Hi Andrew,

On Fri, 15 Dec 2023 10:46:18 +0100
Andrew Lunn <[email protected]> wrote:

> > 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.

I would agree, I feel bad about delaying it , although as Daniel
mentioned it's indeed the trigger registration that gets skipped.

> So, if i'm reading this patch correctly, it only affects PHYs within
> SFPs.
>
> The discussion went off on a tangent and also talked about media
> converter PHYs, but from my reading of this patch, they are unaffected
> by this patch. Do they however also suffer from this deadlock? Anybody
> tested that?

I can give it a try today (in a few hours probably, I'm experiencing
a power outage right now...) and make sure the issue doesn't occur with
media converter PHYs.

Maxime

> Andrew

2023-12-15 15:39:31

[permalink] [raw]

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

Hi all,

On Fri, 15 Dec 2023 10:59:28 +0100
Maxime Chevallier <[email protected]> wrote:

> Hi Andrew,
>
> On Fri, 15 Dec 2023 10:46:18 +0100
> Andrew Lunn <[email protected]> wrote:
> > So, if i'm reading this patch correctly, it only affects PHYs within
> > SFPs.
> >
> > The discussion went off on a tangent and also talked about media
> > converter PHYs, but from my reading of this patch, they are unaffected
> > by this patch. Do they however also suffer from this deadlock? Anybody
> > tested that?
>

I gave it a try with lockdep on a macchiatobin, but I couldn't even
reproduce the original issue (tested with the tip of net-next and net),
so I can't tell if the media-converter PHYs are affected as well. I got
no lockdep warnings regarding either paths.

Maxime

2023-12-15 16:46:49