Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework

[Andrew Lunn <andrew@lunn.ch>]

> phy_resume call phydrv->resume without take mutex.
> if phy driver implement resume function, for example marvell_resume, then
> 
> static int marvell_resume(struct phy_device *phydev)
> {
> 	int err;
> 
> 	/* Resume the fiber mode first */
> 	if (!(phydev->supported & SUPPORTED_FIBRE)) {
> 		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> 		if (err < 0)
> 			goto error;
> 
> 		/* With the page set, use the generic resume */
> 		err = genphy_resume(phydev);
> 		if (err < 0)
> 			goto error;
> 
> 		/* Then, the copper link */
> 		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> 		if (err < 0)
> 			goto error;
> 	}
> 
> the code above executes without holding a mutex expect genphy_resume
> 
> 	/* With the page set, use the generic resume */
> 	return genphy_resume(phydev);
> 
> error:
> 	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> 	return err;
> }
> 
> So I think the correct way to hold a lock is in phy_* function, not in genphy_*,
> and current genphy_resume and phy_resume is broken in this way.

Suspend and resume are a bit odd. As you found out, everywhere else,
the core takes the mutex before calling into the driver function.

The general rule of thumb, is that many driver writers don't
understand locking. So it is better to do the locking in the core,
which is generally written by people who do understand locking. And
core code gets used more, so bugs tend to be found.

     Andrew