Re: [PATCH net] net: phy: micrel: fix LAN8814 QSGMII soft reset

[Andrew Lunn]

> > @@ -4548,6 +4548,13 @@ static int lan8814_config_init(struct phy_device *phydev)
> >  	struct kszphy_priv *lan8814 = phydev->priv;
> >  	int ret;
> >  
> > +	if (phy_package_init_once(phydev))
> > +		/* Reset the PHY */
> > +		lanphy_modify_page_reg(phydev, LAN8814_PAGE_COMMON_REGS,
> > +				       LAN8814_QSGMII_SOFT_RESET,
> > +				       LAN8814_QSGMII_SOFT_RESET_BIT,
> > +				       LAN8814_QSGMII_SOFT_RESET_BIT)
> 
> Sashiko says:
> 
> ---
> Could this introduce a race condition if multiple ports are brought up
> concurrently?
> Because phy_package_init_once() does not provide a synchronization
> barrier for followers, they might proceed immediately to configure their
> registers while the leader is still performing the reset.
> ---
> 
> on top of my head IDK if such race is possible at all.

config_init() is called from phy_init_hw(). That is called from
mdio_bus_phy_resume() and phy_attach_direct().

It seems unlikely resumes of devices on one bus is done in parallel,
same as probing of devices on one bus is not performed in parallel.

phy_attach_direct() is either used in the MAC drivers probe() or
open(). Again, probe should not be running in parallel especially
since this PHY is likely connect to a switch, and the ports are
created sequentially by the DSA core. open() should be protected by
RTNL.

So it seems unlikely to me.

lanphy_modify_page_reg() also takes the MDIO bus lock. That will
prevent any other MDIO operations being performed in parallel. This
does however make the assumption the software reset can be performed
within one MDIO bus cycle.

So a race here seems pretty theoretical to me.

    Andrew