[PATCH net] net: phy: Don't register LEDs for genphy

[PATCH net] net: phy: Don't register LEDs for genphy

[Sean Anderson]

If a PHY has no driver, the genphy driver is probed/removed directly in
phy_attach/detach. If the PHY's ofnode has an "leds" subnode, then the
LEDs will be (un)registered when probing/removing the genphy driver.
This could occur if the leds are for a non-generic driver that isn't
loaded for whatever reason. Synchronously removing the PHY device in
phy_detach leads to the following deadlock:

rtnl_lock()
ndo_close()
    ...
    phy_detach()
        phy_remove()
            phy_leds_unregister()
                led_classdev_unregister()
                    led_trigger_set()
                        netdev_trigger_deactivate()
                            unregister_netdevice_notifier()
                                rtnl_lock()

There is a corresponding deadlock on the open/register side of things
(and that one is reported by lockdep), but it requires a race while this
one is deterministic.

Generic PHYs do not support LEDs anyway, so don't bother registering
them.

Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 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 73f9cb2e2844..f76ee8489504 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3416,7 +3416,8 @@ static int phy_probe(struct device *dev)
 	/* Get the LEDs from the device tree, and instantiate standard
 	 * LEDs for them.
 	 */
-	if (IS_ENABLED(CONFIG_PHYLIB_LEDS))
+	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev) &&
+	    !phy_driver_is_genphy_10g(phydev))
 		err = of_phy_leds(phydev);
 
 out:
@@ -3433,7 +3434,8 @@ static int phy_remove(struct device *dev)
 
 	cancel_delayed_work_sync(&phydev->state_queue);
 
-	if (IS_ENABLED(CONFIG_PHYLIB_LEDS))
+	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev) &&
+	    !phy_driver_is_genphy_10g(phydev))
 		phy_leds_unregister(phydev);
 
 	phydev->state = PHY_DOWN;
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH net] net: phy: Don't register LEDs for genphy

[Jakub Kicinski]

On Mon,  7 Jul 2025 15:58:03 -0400 Sean Anderson wrote:
> -	if (IS_ENABLED(CONFIG_PHYLIB_LEDS))
> +	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev) &&
> +	    !phy_driver_is_genphy_10g(phydev))

Breaks build for smaller configs:

drivers/net/phy/phy_device.c: In function ‘phy_probe’:
drivers/net/phy/phy_device.c:3506:14: error: implicit declaration of function ‘phy_driver_is_genphy_10g’; did you mean ‘phy_driver_is_genphy’? [-Werror=implicit-function-declaration]
 3506 |             !phy_driver_is_genphy_10g(phydev))
      |              ^~~~~~~~~~~~~~~~~~~~~~~~
      |              phy_driver_is_genphy
-- 
pw-bot: cr

Re: [PATCH net] net: phy: Don't register LEDs for genphy

[Russell King (Oracle)]

On Mon, Jul 07, 2025 at 03:58:03PM -0400, Sean Anderson wrote:
> If a PHY has no driver, the genphy driver is probed/removed directly in
> phy_attach/detach. If the PHY's ofnode has an "leds" subnode, then the
> LEDs will be (un)registered when probing/removing the genphy driver.

Maybe checking whether the PHY driver supports LEDs would be more
sensible than checking whether it's one of the genphy drivers?

> This could occur if the leds are for a non-generic driver that isn't
> loaded for whatever reason. Synchronously removing the PHY device in
> phy_detach leads to the following deadlock:
> 
> rtnl_lock()
> ndo_close()
>     ...
>     phy_detach()
>         phy_remove()
>             phy_leds_unregister()
>                 led_classdev_unregister()
>                     led_trigger_set()
>                         netdev_trigger_deactivate()
>                             unregister_netdevice_notifier()
>                                 rtnl_lock()
> 
> There is a corresponding deadlock on the open/register side of things
> (and that one is reported by lockdep), but it requires a race while this
> one is deterministic.

Doesn't this deadlock exist irrespective of whether the genphy driver(s)
are being used, and whether or not the PHY driver supports LEDs?

-- 
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: Don't register LEDs for genphy

[Sean Anderson]

On 7/8/25 04:58, Russell King (Oracle) wrote:
> On Mon, Jul 07, 2025 at 03:58:03PM -0400, Sean Anderson wrote:
>> If a PHY has no driver, the genphy driver is probed/removed directly in
>> phy_attach/detach. If the PHY's ofnode has an "leds" subnode, then the
>> LEDs will be (un)registered when probing/removing the genphy driver.
> 
> Maybe checking whether the PHY driver supports LEDs would be more
> sensible than checking whether it's one of the genphy drivers?

The genphy driver is special, since it is probed synchronously from
phy_attach. All other drivers are probed asynchronously and don't have
this problem.

>> This could occur if the leds are for a non-generic driver that isn't
>> loaded for whatever reason. Synchronously removing the PHY device in
>> phy_detach leads to the following deadlock:
>> 
>> rtnl_lock()
>> ndo_close()
>>     ...
>>     phy_detach()
>>         phy_remove()
>>             phy_leds_unregister()
>>                 led_classdev_unregister()
>>                     led_trigger_set()
>>                         netdev_trigger_deactivate()
>>                             unregister_netdevice_notifier()
>>                                 rtnl_lock()
>> 
>> There is a corresponding deadlock on the open/register side of things
>> (and that one is reported by lockdep), but it requires a race while this
>> one is deterministic.
> 
> Doesn't this deadlock exist irrespective of whether the genphy driver(s)
> are being used, and whether or not the PHY driver supports LEDs?

Nope.

--Sean

Re: [PATCH net] net: phy: Don't register LEDs for genphy

[Sean Anderson]

On 7/7/25 19:32, Jakub Kicinski wrote:
> On Mon,  7 Jul 2025 15:58:03 -0400 Sean Anderson wrote:
>> -	if (IS_ENABLED(CONFIG_PHYLIB_LEDS))
>> +	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev) &&
>> +	    !phy_driver_is_genphy_10g(phydev))
> 
> Breaks build for smaller configs:
> 
> drivers/net/phy/phy_device.c: In function ‘phy_probe’:
> drivers/net/phy/phy_device.c:3506:14: error: implicit declaration of function ‘phy_driver_is_genphy_10g’; did you mean ‘phy_driver_is_genphy’? [-Werror=implicit-function-declaration]
>  3506 |             !phy_driver_is_genphy_10g(phydev))
>       |              ^~~~~~~~~~~~~~~~~~~~~~~~
>       |              phy_driver_is_genphy

This is due to
https://github.com/linux-netdev/testing/commit/42ed7f7e94da01391d3519ffb5747698d2be0a67
which is not in net/main yet.

--Sean

Re: [PATCH net] net: phy: Don't register LEDs for genphy

[Sean Anderson]

Hi Jakub,

On 7/8/25 11:52, Sean Anderson wrote:
> On 7/7/25 19:32, Jakub Kicinski wrote:
>> On Mon,  7 Jul 2025 15:58:03 -0400 Sean Anderson wrote:
>>> -	if (IS_ENABLED(CONFIG_PHYLIB_LEDS))
>>> +	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev) &&
>>> +	    !phy_driver_is_genphy_10g(phydev))
>> 
>> Breaks build for smaller configs:
>> 
>> drivers/net/phy/phy_device.c: In function ‘phy_probe’:
>> drivers/net/phy/phy_device.c:3506:14: error: implicit declaration of function ‘phy_driver_is_genphy_10g’; did you mean ‘phy_driver_is_genphy’? [-Werror=implicit-function-declaration]
>>  3506 |             !phy_driver_is_genphy_10g(phydev))
>>       |              ^~~~~~~~~~~~~~~~~~~~~~~~
>>       |              phy_driver_is_genphy
> 
> This is due to
> https://github.com/linux-netdev/testing/commit/42ed7f7e94da01391d3519ffb5747698d2be0a67
> which is not in net/main yet.
> 
> --Sean

I see this is marked "Changes Requested" in patchwork. However, I don't
believe that I need to change anything until the above commit is merged
into net/main. Will you be merging that commit? Or should I just resend
without changes?

--Sean

Re: [PATCH net] net: phy: Don't register LEDs for genphy

[Jakub Kicinski]

On Thu, 10 Jul 2025 13:40:33 -0400 Sean Anderson wrote:
> I see this is marked "Changes Requested" in patchwork. However, I don't
> believe that I need to change anything until the above commit is merged
> into net/main. Will you be merging that commit? Or should I just resend
> without changes?

The patch must build when posted. If it didn't you need to repost.

Re: [PATCH net] net: phy: Don't register LEDs for genphy

[Sean Anderson]

On 7/10/25 13:52, Jakub Kicinski wrote:
> On Thu, 10 Jul 2025 13:40:33 -0400 Sean Anderson wrote:
>> I see this is marked "Changes Requested" in patchwork. However, I don't
>> believe that I need to change anything until the above commit is merged
>> into net/main. Will you be merging that commit? Or should I just resend
>> without changes?
> 
> The patch must build when posted. If it didn't you need to repost.

It builds on net/main. Which is what I posted for. The CI applied it to net-next/main.

--Sean

Re: [PATCH net] net: phy: Don't register LEDs for genphy

[Jakub Kicinski]

On Thu, 10 Jul 2025 14:17:18 -0400 Sean Anderson wrote:
> On 7/10/25 13:52, Jakub Kicinski wrote:
> > On Thu, 10 Jul 2025 13:40:33 -0400 Sean Anderson wrote:  
> >> I see this is marked "Changes Requested" in patchwork. However, I don't
> >> believe that I need to change anything until the above commit is merged
> >> into net/main. Will you be merging that commit? Or should I just resend
> >> without changes?  
> > 
> > The patch must build when posted. If it didn't you need to repost.  
> 
> It builds on net/main. Which is what I posted for. The CI applied it to net-next/main.

Damn, I see your point now, sorry :/
So in net-next we'll have to drop the phy_driver_is_genphy_10g() ?

I think it may be best if we turn this into an explicit merge
conflict, IOW if you could post both net and net-next version
I will merge them at the same time. Upstream trees like CI or
linux-next will have easier time handling a git conflict than
a build failure. Does that make sense? For the net-next version
describe it from the perspective of the net patch already being
merged and you're writing the "-next" version of the patch.
I'll edit in the git hash of the net commit when applying.

Re: [PATCH net] net: phy: Don't register LEDs for genphy

[Sean Anderson]

On 7/10/25 14:49, Jakub Kicinski wrote:
> On Thu, 10 Jul 2025 14:17:18 -0400 Sean Anderson wrote:
>> On 7/10/25 13:52, Jakub Kicinski wrote:
>> > On Thu, 10 Jul 2025 13:40:33 -0400 Sean Anderson wrote:  
>> >> I see this is marked "Changes Requested" in patchwork. However, I don't
>> >> believe that I need to change anything until the above commit is merged
>> >> into net/main. Will you be merging that commit? Or should I just resend
>> >> without changes?  
>> > 
>> > The patch must build when posted. If it didn't you need to repost.  
>> 
>> It builds on net/main. Which is what I posted for. The CI applied it to net-next/main.
> 
> Damn, I see your point now, sorry :/
> So in net-next we'll have to drop the phy_driver_is_genphy_10g() ?

Yes. I believe phy_driver_is_genphy() is sufficient in net-next.

> I think it may be best if we turn this into an explicit merge
> conflict, IOW if you could post both net and net-next version
> I will merge them at the same time. Upstream trees like CI or
> linux-next will have easier time handling a git conflict than
> a build failure. Does that make sense? For the net-next version
> describe it from the perspective of the net patch already being
> merged and you're writing the "-next" version of the patch.
> I'll edit in the git hash of the net commit when applying.

OK, so if A is this patch and B is the conflict above, you'd like me to
post an A' like:

     B---merge---A' net-next
    /   /
base---A            net

? Or did you have something in mind more like

     B---A' net-next
    /
base---A    net

--Sean

Re: [PATCH net] net: phy: Don't register LEDs for genphy

[Jakub Kicinski]

On Thu, 10 Jul 2025 14:57:48 -0400 Sean Anderson wrote:
> OK, so if A is this patch and B is the conflict above, you'd like me to
> post an A' like:
> 
>      B---merge---A' net-next
>     /   /
> base---A            net
> 
> ? Or did you have something in mind more like
> 
>      B---A' net-next
>     /
> base---A    net

The latter, so we will end up with:

     B---A' net-next --M
    /                 /
base---A    net -----

where M is a merge with a conflict, to resolve will basically pick 
the net-next version.