Re: [PATCH net v3 3/5] net: phy: set PHY_READY after LED setup

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Aug 20, 2026 at 06:06:41PM +0800, Xuanqiang Luo wrote:
> 
> 在 2026/8/19 14:02, Xuanqiang Luo 写道:
> > From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> > 
> > phy_probe() sets PHY_READY before calling of_phy_leds(). If LED setup
> > fails, the error path releases the initialized resources while the PHY
> > state remains READY even though probing failed.
> > 
> > Set PHY_READY only after LED setup succeeds.
> > 
> > Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
> > Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> > 
> I noticed that Sashiko found two new issues:
> 
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index 538e9ca44d093..8fd6fbf378f0a 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -3799,9 +3799,6 @@ static int phy_probe(struct device *dev)
> > >                   phydev->supported);
> > >      }
> > >  > -    /* Set the state to READY by default */
> > > -    phydev->state = PHY_READY;
> > > -
> > >      /* Register the PHY LED triggers */
> > >      if (!phydev->is_on_sfp_module)
> > >          phy_led_triggers_register(phydev);
> > [Severity: Critical]
> > This is a pre-existing issue, but by ignoring the return value of
> > phy_led_triggers_register() here, could we trigger a double-free later?
> > 
> > In drivers/net/phy/phy_led_triggers.c, if phy_led_triggers_register() fails,
> > it frees phy->phy_led_triggers but leaves it as a dangling pointer and sets
> > phy_num_led_triggers = 0. Since the return value is ignored here, phy_probe()
> > continues execution.
> > 	
> > If of_phy_leds() then fails, the code jumps to out_unreg_led_triggers and
> > calls phy_led_triggers_unregister(). Inside that function, the loop is
> > skipped because phy_num_led_triggers is 0, but it unconditionally calls
> > kfree(phy->phy_led_triggers). This results in a double-free of the dangling
> > pointer.	
> 
> Although this part was not introduced by the current patch, I think it
> still falls within the scope of probe failure handling, so I plan to add
> a separate patch in the next revision to fix it.
> 
> > > @@ -3815,6 +3812,9 @@ static int phy_probe(struct device *dev)
> > >  			goto out_unreg_led_triggers;
> > >  	}
> > >  > +	/* Set the state to READY by default */
> > > +	phydev->state = PHY_READY;
> > > +
> > 
> > [Severity: High]
> > Does moving this lockless state assignment to the end of phy_probe() create
> > a race condition with concurrent MAC driver attachments?

I'm not sure this is correct. If phy_probe() is still ongoing, can you
even call phy_attach()?

In the normal case the MAC driver registers the MDIO bus during its
probe. The PHYs will be scanned and probed synchronously. Once the
MDIO is registered, the MAC driver will then continue. Generally the
PHY is attached in open() but it could be later in probe. So think
this serialisation is performed by general code structure.

It gets more interesting with a shared MDIO bus, which has two or more
PHYs on it. The first MAC driver registers its MDIO bus as above, and
later attaches its PHY. However in parallel a second MAC driver can
probe, and use a phandle to point to the PHY on the MDIO bus, and try
to attach to it. But i _guess_ following the phandle does not work
until the PHY probe has completed?

> This appears to be a pre-existing issue, since phy_attach_direct() and
> phy_probe() are not serialized.
> 
> This patch does widen the race window, but phydev->state should still
> be set to PHY_READY before phy_probe() returns.

I would agree it is an old issues, if it is an issue. So i would say
it is out of scope of this patchset. You can look deeper if you want,
but it is not required.

    Andrew