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

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

[Daniel Golle]

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.

======================================================
WARNING: possible circular locking dependency detected
6.7.0-rc4-next-20231208+ #0 Tainted: G           O
------------------------------------------------------
kworker/u8:2/43 is trying to acquire lock:
ffffffc08108c4e8 (triggers_list_lock){++++}-{3:3}, at: led_trigger_register+0x4c/0x1a8

but task is already holding lock:
ffffff80c5c6f318 (&sfp->sm_mutex){+.+.}-{3:3}, at: cleanup_module+0x2ba8/0x3120 [sfp]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #3 (&sfp->sm_mutex){+.+.}-{3:3}:
       __mutex_lock+0x88/0x7a0
       mutex_lock_nested+0x20/0x28
       cleanup_module+0x2ae0/0x3120 [sfp]
       sfp_register_bus+0x5c/0x9c
       sfp_register_socket+0x48/0xd4
       cleanup_module+0x271c/0x3120 [sfp]
       platform_probe+0x64/0xb8
       really_probe+0x17c/0x3c0
       __driver_probe_device+0x78/0x164
       driver_probe_device+0x3c/0xd4
       __driver_attach+0xec/0x1f0
       bus_for_each_dev+0x60/0xa0
       driver_attach+0x20/0x28
       bus_add_driver+0x108/0x208
       driver_register+0x5c/0x118
       __platform_driver_register+0x24/0x2c
       init_module+0x28/0xa7c [sfp]
       do_one_initcall+0x70/0x2ec
       do_init_module+0x54/0x1e4
       load_module+0x1b78/0x1c8c
       __do_sys_init_module+0x1bc/0x2cc
       __arm64_sys_init_module+0x18/0x20
       invoke_syscall.constprop.0+0x4c/0xdc
       do_el0_svc+0x3c/0xbc
       el0_svc+0x34/0x80
       el0t_64_sync_handler+0xf8/0x124
       el0t_64_sync+0x150/0x154

-> #2 (rtnl_mutex){+.+.}-{3:3}:
       __mutex_lock+0x88/0x7a0
       mutex_lock_nested+0x20/0x28
       rtnl_lock+0x18/0x20
       set_device_name+0x30/0x130
       netdev_trig_activate+0x13c/0x1ac
       led_trigger_set+0x118/0x234
       led_trigger_write+0x104/0x17c
       sysfs_kf_bin_write+0x64/0x80
       kernfs_fop_write_iter+0x128/0x1b4
       vfs_write+0x178/0x2a4
       ksys_write+0x58/0xd4
       __arm64_sys_write+0x18/0x20
       invoke_syscall.constprop.0+0x4c/0xdc
       do_el0_svc+0x3c/0xbc
       el0_svc+0x34/0x80
       el0t_64_sync_handler+0xf8/0x124
       el0t_64_sync+0x150/0x154

-> #1 (&led_cdev->trigger_lock){++++}-{3:3}:
       down_write+0x4c/0x13c
       led_trigger_write+0xf8/0x17c
       sysfs_kf_bin_write+0x64/0x80
       kernfs_fop_write_iter+0x128/0x1b4
       vfs_write+0x178/0x2a4
       ksys_write+0x58/0xd4
       __arm64_sys_write+0x18/0x20
       invoke_syscall.constprop.0+0x4c/0xdc
       do_el0_svc+0x3c/0xbc
       el0_svc+0x34/0x80
       el0t_64_sync_handler+0xf8/0x124
       el0t_64_sync+0x150/0x154

-> #0 (triggers_list_lock){++++}-{3:3}:
       __lock_acquire+0x12a0/0x2014
       lock_acquire+0x100/0x2ac
       down_write+0x4c/0x13c
       led_trigger_register+0x4c/0x1a8
       phy_led_triggers_register+0x9c/0x214
       phy_attach_direct+0x154/0x36c
       phylink_attach_phy+0x30/0x60
       phylink_sfp_connect_phy+0x140/0x510
       sfp_add_phy+0x34/0x50
       init_module+0x15c/0xa7c [sfp]
       cleanup_module+0x1d94/0x3120 [sfp]
       cleanup_module+0x2bb4/0x3120 [sfp]
       process_one_work+0x1f8/0x4ec
       worker_thread+0x1e8/0x3d8
       kthread+0x104/0x110
       ret_from_fork+0x10/0x20

other info that might help us debug this:

Chain exists of:
  triggers_list_lock --> rtnl_mutex --> &sfp->sm_mutex

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&sfp->sm_mutex);
                               lock(rtnl_mutex);
                               lock(&sfp->sm_mutex);
  lock(triggers_list_lock);

 *** DEADLOCK ***

4 locks held by kworker/u8:2/43:
 #0: ffffff80c000f938 ((wq_completion)events_power_efficient){+.+.}-{0:0}, at: process_one_work+0x150/0x4ec
 #1: ffffffc08214bde8 ((work_completion)(&(&sfp->timeout)->work)){+.+.}-{0:0}, at: process_one_work+0x150/0x4ec
 #2: ffffffc0810902f8 (rtnl_mutex){+.+.}-{3:3}, at: rtnl_lock+0x18/0x20
 #3: ffffff80c5c6f318 (&sfp->sm_mutex){+.+.}-{3:3}, at: cleanup_module+0x2ba8/0x3120 [sfp]

stack backtrace:
CPU: 0 PID: 43 Comm: kworker/u8:2 Tainted: G           O       6.7.0-rc4-next-20231208+ #0
Hardware name: Bananapi BPI-R4 (DT)
Workqueue: events_power_efficient cleanup_module [sfp]
Call trace:
 dump_backtrace+0xa8/0x10c
 show_stack+0x14/0x1c
 dump_stack_lvl+0x5c/0xa0
 dump_stack+0x14/0x1c
 print_circular_bug+0x328/0x430
 check_noncircular+0x124/0x134
 __lock_acquire+0x12a0/0x2014
 lock_acquire+0x100/0x2ac
 down_write+0x4c/0x13c
 led_trigger_register+0x4c/0x1a8
 phy_led_triggers_register+0x9c/0x214
 phy_attach_direct+0x154/0x36c
 phylink_attach_phy+0x30/0x60
 phylink_sfp_connect_phy+0x140/0x510
 sfp_add_phy+0x34/0x50
 init_module+0x15c/0xa7c [sfp]
 cleanup_module+0x1d94/0x3120 [sfp]
 cleanup_module+0x2bb4/0x3120 [sfp]
 process_one_work+0x1f8/0x4ec
 worker_thread+0x1e8/0x3d8
 kthread+0x104/0x110
 ret_from_fork+0x10/0x20

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/phy_device.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d2caa89bf1fb8..08b87d7396eb9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1558,7 +1558,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		goto error;
 
 	phy_resume(phydev);
-	phy_led_triggers_register(phydev);
+	if (!phydev->is_on_sfp_module)
+		phy_led_triggers_register(phydev);
 
 	/**
 	 * If the external phy used by current mac interface is managed by
@@ -1827,7 +1828,8 @@ void phy_detach(struct phy_device *phydev)
 	}
 	phydev->phylink = NULL;
 
-	phy_led_triggers_unregister(phydev);
+	if (!phydev->is_on_sfp_module)
+		phy_led_triggers_unregister(phydev);
 
 	if (phydev->mdio.dev.driver)
 		module_put(phydev->mdio.dev.driver->owner);
-- 
2.43.0

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

[Maxime Chevallier]

Hi Daniel

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

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

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

[Andrew Lunn]

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@makrotopia.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?

> 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

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

[Maxime Chevallier]

On Wed, 13 Dec 2023 10:08:25 +0100
Andrew Lunn <andrew@lunn.ch> 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@makrotopia.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?

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

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

[Andrew Lunn]

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

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

[Russell King (Oracle)]

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!

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

[Russell King (Oracle)]

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

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

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

[Maxime Chevallier]

Hi Andrew, Russell,

On Wed, 13 Dec 2023 15:27:28 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> 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@makrotopia.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.
> 

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

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

[Daniel Golle]

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@makrotopia.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)...

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

[Russell King (Oracle)]

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!

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

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@makrotopia.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!

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

[Andrew Lunn]

> 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

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

[Maxime Chevallier]

Hi Andrew,

On Fri, 15 Dec 2023 10:46:18 +0100
Andrew Lunn <andrew@lunn.ch> 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

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

[Maxime Chevallier]

Hi all,

On Fri, 15 Dec 2023 10:59:28 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Hi Andrew,
> 
> On Fri, 15 Dec 2023 10:46:18 +0100
> Andrew Lunn <andrew@lunn.ch> 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

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

[Jakub Kicinski]

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?

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

[Daniel Golle]

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.

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

[Andrew Lunn]

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?

How about:

Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")

       Andrew

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

[Russell King (Oracle)]

On Tue, Dec 12, 2023 at 12:05:35AM +0000, Daniel Golle wrote:
> -> #3 (&sfp->sm_mutex){+.+.}-{3:3}:
>        __mutex_lock+0x88/0x7a0
>        mutex_lock_nested+0x20/0x28
>        cleanup_module+0x2ae0/0x3120 [sfp]
>        sfp_register_bus+0x5c/0x9c
>        sfp_register_socket+0x48/0xd4
>        cleanup_module+0x271c/0x3120 [sfp]
>        platform_probe+0x64/0xb8
>        really_probe+0x17c/0x3c0
>        __driver_probe_device+0x78/0x164
>        driver_probe_device+0x3c/0xd4
>        __driver_attach+0xec/0x1f0
>        bus_for_each_dev+0x60/0xa0
>        driver_attach+0x20/0x28
>        bus_add_driver+0x108/0x208
>        driver_register+0x5c/0x118
>        __platform_driver_register+0x24/0x2c
>        init_module+0x28/0xa7c [sfp]
>        do_one_initcall+0x70/0x2ec
>        do_init_module+0x54/0x1e4
>        load_module+0x1b78/0x1c8c
>        __do_sys_init_module+0x1bc/0x2cc
>        __arm64_sys_init_module+0x18/0x20
>        invoke_syscall.constprop.0+0x4c/0xdc
>        do_el0_svc+0x3c/0xbc
>        el0_svc+0x34/0x80
>        el0t_64_sync_handler+0xf8/0x124
>        el0t_64_sync+0x150/0x154

I suspect these backtraces aren't all that reliable, and look like they
are generated by walking through the stack and logging anything that
seems to be pointing into the text segment, which is rubbish for ARM32
and probably ARM64 as well.

We can see that this backtrace is a pile of lies because there is _no_
_way_ that sfp's cleanup_module() could ever be called while its
init_module() function is running.

In any case, I think this path is irrelevant.

> -> #2 (rtnl_mutex){+.+.}-{3:3}:
>        __mutex_lock+0x88/0x7a0
>        mutex_lock_nested+0x20/0x28
>        rtnl_lock+0x18/0x20
>        set_device_name+0x30/0x130
>        netdev_trig_activate+0x13c/0x1ac
>        led_trigger_set+0x118/0x234
>        led_trigger_write+0x104/0x17c
>        sysfs_kf_bin_write+0x64/0x80
>        kernfs_fop_write_iter+0x128/0x1b4
>        vfs_write+0x178/0x2a4
>        ksys_write+0x58/0xd4
>        __arm64_sys_write+0x18/0x20
>        invoke_syscall.constprop.0+0x4c/0xdc
>        do_el0_svc+0x3c/0xbc
>        el0_svc+0x34/0x80
>        el0t_64_sync_handler+0xf8/0x124
>        el0t_64_sync+0x150/0x154

This is one of the paths that matters. A userspace write is occuring
to the netdev trigger module. This path takes the following locks
(most recent first):

	rtnl_lock()
	trigger_lock (write)
	triggers_list_lock (read)

> -> #0 (triggers_list_lock){++++}-{3:3}:
>        __lock_acquire+0x12a0/0x2014
>        lock_acquire+0x100/0x2ac
>        down_write+0x4c/0x13c
>        led_trigger_register+0x4c/0x1a8
>        phy_led_triggers_register+0x9c/0x214
>        phy_attach_direct+0x154/0x36c
>        phylink_attach_phy+0x30/0x60
>        phylink_sfp_connect_phy+0x140/0x510
>        sfp_add_phy+0x34/0x50
>        init_module+0x15c/0xa7c [sfp]
>        cleanup_module+0x1d94/0x3120 [sfp]
>        cleanup_module+0x2bb4/0x3120 [sfp]
>        process_one_work+0x1f8/0x4ec
>        worker_thread+0x1e8/0x3d8
>        kthread+0x104/0x110
>        ret_from_fork+0x10/0x20

This path I suspect (but hard to see, we've got that cleanup_module
and init_module crud there again which is utter trash, sfp_add_phy
is not called from any of the functions previously listed)...
Manually going through the code instead, the locking order will be:

	triggers_list_lock (write)
	sm_mutex
	rtnl

I'm not sure that the lockdep report is accurate, as it seems to be
blaming the deadlock via three locks (triggers_list_lock --> rtnl_mutex
--> &sfp->sm_mutex) but I can't find a path where sm_mutex would be
involved (except the immediate above.)

It looks to me like the problem is in part caused by calling
phy_led_triggers_register() while holding the rtnl lock. Holding the
rtnl lock is fundamental to being able to safely remove and add PHY
devices to netdevs while they are up and running.

The other part that causes the problem is a write to a netdev trigger
that causes it to activate takes the rtnl and triggers_list_lock in
the opposite order.

I don't currently see a solution to this.

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

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 12 Dec 2023 00:05:35 +0000 you 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.
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 6.7.0-rc4-next-20231208+ #0 Tainted: G           O
> 
> [...]

Here is the summary with links:
  - [net] net: phy: skip LED triggers on PHYs on SFP modules
    https://git.kernel.org/netdev/net/c/b1dfc0f76231

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html