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

From	Andrew Lunn <andrew@lunn.ch>
Newsgroups	linux.kernel
Subject	Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework
Date	2017-06-24 16:00 +0200
Message-ID	<tVTLs-8uO-9@gated-at.bofh.it> (permalink)
References	<tVsUW-hI-11@gated-at.bofh.it> <tVsUW-hI-17@gated-at.bofh.it> <tVJM5-2rK-5@gated-at.bofh.it> <tVKf8-2ER-3@gated-at.bofh.it> <tVMAh-4f7-3@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

> 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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH NET v3 0/2] Add loopback support in phy_driver and hns ethtool fix Lin Yun Sheng <linyunsheng@huawei.com> - 2017-06-23 11:20 +0200
  [PATCH NET v3 2/2] net: hns: Use phy_driver to setup Phy loopback Lin Yun Sheng <linyunsheng@huawei.com> - 2017-06-23 11:20 +0200
    Re: [PATCH NET v3 2/2] net: hns: Use phy_driver to setup Phy loopback Andrew Lunn <andrew@lunn.ch> - 2017-06-24 05:20 +0200
  [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework Lin Yun Sheng <linyunsheng@huawei.com> - 2017-06-23 11:20 +0200
    Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy  framework Andrew Lunn <andrew@lunn.ch> - 2017-06-24 05:20 +0200
      Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy  framework Yunsheng Lin <linyunsheng@huawei.com> - 2017-06-24 05:50 +0200
        Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy  framework Yunsheng Lin <linyunsheng@huawei.com> - 2017-06-24 08:20 +0200
          Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy  framework Andrew Lunn <andrew@lunn.ch> - 2017-06-24 16:00 +0200

csiph-web