Re: [PATCH net-next] net: phy: call phy_init_hw() in phy resume path

[Russell King (Oracle)]

On Fri, Apr 10, 2026 at 05:27:02PM +0000, Biju Das wrote:
> Hi Russell King/ Andrew,
> 
> > -----Original Message-----
> > From: Russell King <linux@xxxxxxxxxxxxxxx>
> > Sent: 10 April 2026 15:55
> > Subject: Re: [PATCH net-next] net: phy: call phy_init_hw() in phy resume path
> > 
> > On Fri, Apr 10, 2026 at 03:51:18PM +0100, Russell King (Oracle) wrote:
> > > On Fri, Apr 10, 2026 at 03:29:01PM +0100, Biju wrote:
> > > > From: Ovidiu Panait <ovidiu.panait.rb@xxxxxxxxxxx>
> > > >
> > > > When mac_managed_pm flag is set, mdio_bus_phy_resume() is skipped,
> > > > so phy_init_hw(), which performs soft_reset and config_init, is not
> > > > called during resume.
> > > >
> > > > This is inconsistent with the non-mac_managed_pm path, where
> > > > mdio_bus_phy_resume() calls phy_init_hw() before phy_resume() on
> > > > every resume.
> > > >
> > > > To align both paths, add a phy_init_hw() call at the top of
> > > > __phy_resume(), before invoking the driver's resume callback. This
> > > > guarantees the PHY undergoes soft reset and re-initialization
> > > > regardless of whether PM is managed by the MAC or the MDIO bus.
> > > >
> > > > Signed-off-by: Ovidiu Panait <ovidiu.panait.rb@xxxxxxxxxxx>
> > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/net/phy/phy_device.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/net/phy/phy_device.c
> > > > b/drivers/net/phy/phy_device.c index 0edff47478c2..8255f4208d66
> > > > 100644
> > > > --- a/drivers/net/phy/phy_device.c
> > > > +++ b/drivers/net/phy/phy_device.c
> > > > @@ -2008,6 +2008,10 @@ int __phy_resume(struct phy_device *phydev)
> > > >  	if (!phydrv || !phydrv->resume)
> > > >  		return 0;
> > > >
> > > > +	ret = phy_init_hw(phydev);
> > > > +	if (ret)
> > > > +		return ret;
> > >
> > > Do we want to do this even when phydrv->resume is NULL?
> > 
> > I should've also added (sorry, busy packing) - with it always being called even when phydrv->resume is
> > NULL, it means that the call sites to phy_resume() in phylib which are preceeded by a call to
> > phy_init_hw() should have that call removed, otherwise we're going to be calling phy_init_hw() twice.
> > 
> > As the patch currently stands, that's the case when phydrv->resume is populated, and I think we should
> > avoid that.
> > 
> > > Apart from that, looks fine to me - it seems some paths call
> > > phy_init_hw() can be called with or without phydev->lock held, and
> > > this one will call it with the lock held which seems to be okay.
> 
> 
> The new patch will be like this, after moving phy_init_hw() without
> phydev->lock held. Please let me know are you ok with this?
> 
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 0edff47478c2..4a2b19d39373 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -396,10 +396,6 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
>  	WARN_ON(phydev->state != PHY_HALTED && phydev->state != PHY_READY &&
>  		phydev->state != PHY_UP);
>  
> -	ret = phy_init_hw(phydev);
> -	if (ret < 0)
> -		return ret;
> -
>  	ret = phy_resume(phydev);
>  	if (ret < 0)
>  		return ret;
> @@ -1857,16 +1853,14 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	if (dev)
>  		netif_carrier_off(phydev->attached_dev);
>  
> -	/* Do initial configuration here, now that
> +	/* Do initial configuration inside phy_init_hw(), now that
>  	 * we have certain key parameters
>  	 * (dev_flags and interface)
>  	 */
> -	err = phy_init_hw(phydev);
> +	err = phy_resume(phydev);
>  	if (err)
>  		goto error;
>  
> -	phy_resume(phydev);
> -
>  	/**
>  	 * If the external phy used by current mac interface is managed by
>  	 * another mac interface, so we should create a device link between
> @@ -2020,6 +2014,10 @@ int phy_resume(struct phy_device *phydev)
>  {
>  	int ret;
>  
> +	ret = phy_init_hw(phydev);
> +	if (ret)
> +		return ret;
> +
>  	mutex_lock(&phydev->lock);
>  	ret = __phy_resume(phydev);
>  	mutex_unlock(&phydev->lock);

Looks good to me. Thanks!

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