Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework
> 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_M_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_M_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_M_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
Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework
> 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_M_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_M_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_M_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
Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework
On 2017/6/24 11:40, Yunsheng Lin wrote:
> Hi, Andrew
>
> On 2017/6/24 11:12, Andrew Lunn wrote:
>>> +int phy_loopback(struct phy_device *phydev, bool enable)
>>> +{
>>> + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
>>> + int ret = 0;
>>> +
>>> + if (enable && phydev->loopback_enabled)
>>> + return -EBUSY;
>>> +
>>> + if (!enable && !phydev->loopback_enabled)
>>> + return -EINVAL;
>>> +
>>> + if (phydev->drv && phydrv->set_loopback)
>>> + ret = phydrv->set_loopback(phydev, enable);
>>
>> else
>> ret = -EOPNOTSUPP;
>>
>>> +
>>> + if (ret)
>>> + return ret;
>>> +
>>> + phydev->loopback_enabled = enable;
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(phy_loopback);
>>
>> One of the comments we made of the PHY code in the hns driver is that
>> its locking is completely broken. You have made the same error
>> here. The core needs to hold the mutex while calling into the PHY
>> driver.
> Do you mean hns_nic_config_phy_loopback need to hold the mutex while
> calling phy_loopback? and other place that calling phy_* function?
I took some time looking into how to take mutex in phy core, here is what
I find:
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_M_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_M_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_M_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.
Please let me know if I misunderstand the mutex taking in phy core.
Best Regards
Yunsheng Lin
Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework
On 2017/6/24 11:40, Yunsheng Lin wrote:
> Hi, Andrew
>
> On 2017/6/24 11:12, Andrew Lunn wrote:
>>> +int phy_loopback(struct phy_device *phydev, bool enable)
>>> +{
>>> + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
>>> + int ret = 0;
>>> +
>>> + if (enable && phydev->loopback_enabled)
>>> + return -EBUSY;
>>> +
>>> + if (!enable && !phydev->loopback_enabled)
>>> + return -EINVAL;
>>> +
>>> + if (phydev->drv && phydrv->set_loopback)
>>> + ret = phydrv->set_loopback(phydev, enable);
>>
>> else
>> ret = -EOPNOTSUPP;
>>
>>> +
>>> + if (ret)
>>> + return ret;
>>> +
>>> + phydev->loopback_enabled = enable;
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(phy_loopback);
>>
>> One of the comments we made of the PHY code in the hns driver is that
>> its locking is completely broken. You have made the same error
>> here. The core needs to hold the mutex while calling into the PHY
>> driver.
> Do you mean hns_nic_config_phy_loopback need to hold the mutex while
> calling phy_loopback? and other place that calling phy_* function?
I took some time looking into how to take mutex in phy core, here is what
I find:
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_M_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_M_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_M_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.
Please let me know if I misunderstand the mutex taking in phy core.
Best Regards
Yunsheng Lin
Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework
Hi, Andrew
On 2017/6/24 11:12, Andrew Lunn wrote:
>> +int phy_loopback(struct phy_device *phydev, bool enable)
>> +{
>> +struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
>> +int ret = 0;
>> +
>> +if (enable && phydev->loopback_enabled)
>> +return -EBUSY;
>> +
>> +if (!enable && !phydev->loopback_enabled)
>> +return -EINVAL;
>> +
>> +if (phydev->drv && phydrv->set_loopback)
>> +ret = phydrv->set_loopback(phydev, enable);
>
> else
> ret = -EOPNOTSUPP;
>
>> +
>> +if (ret)
>> +return ret;
>> +
>> +phydev->loopback_enabled = enable;
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL(phy_loopback);
>
> One of the comments we made of the PHY code in the hns driver is that
> its locking is completely broken. You have made the same error
> here. The core needs to hold the mutex while calling into the PHY
> driver.
Do you mean hns_nic_config_phy_loopback need to hold the mutex while
calling phy_loopback? and other place that calling phy_* function?
Best Regards
Yunsheng Lin
Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework
Hi, Andrew
On 2017/6/24 11:12, Andrew Lunn wrote:
>> +int phy_loopback(struct phy_device *phydev, bool enable)
>> +{
>> +struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
>> +int ret = 0;
>> +
>> +if (enable && phydev->loopback_enabled)
>> +return -EBUSY;
>> +
>> +if (!enable && !phydev->loopback_enabled)
>> +return -EINVAL;
>> +
>> +if (phydev->drv && phydrv->set_loopback)
>> +ret = phydrv->set_loopback(phydev, enable);
>
> else
> ret = -EOPNOTSUPP;
>
>> +
>> +if (ret)
>> +return ret;
>> +
>> +phydev->loopback_enabled = enable;
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL(phy_loopback);
>
> One of the comments we made of the PHY code in the hns driver is that
> its locking is completely broken. You have made the same error
> here. The core needs to hold the mutex while calling into the PHY
> driver.
Do you mean hns_nic_config_phy_loopback need to hold the mutex while
calling phy_loopback? and other place that calling phy_* function?
Best Regards
Yunsheng Lin
Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework
> +int phy_loopback(struct phy_device *phydev, bool enable)
> +{
> + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
> + int ret = 0;
> +
> + if (enable && phydev->loopback_enabled)
> + return -EBUSY;
> +
> + if (!enable && !phydev->loopback_enabled)
> + return -EINVAL;
> +
> + if (phydev->drv && phydrv->set_loopback)
> + ret = phydrv->set_loopback(phydev, enable);
else
ret = -EOPNOTSUPP;
> +
> + if (ret)
> + return ret;
> +
> + phydev->loopback_enabled = enable;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(phy_loopback);
One of the comments we made of the PHY code in the hns driver is that
its locking is completely broken. You have made the same error
here. The core needs to hold the mutex while calling into the PHY
driver.
Andrew
Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework
> +int phy_loopback(struct phy_device *phydev, bool enable)
> +{
> + struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
> + int ret = 0;
> +
> + if (enable && phydev->loopback_enabled)
> + return -EBUSY;
> +
> + if (!enable && !phydev->loopback_enabled)
> + return -EINVAL;
> +
> + if (phydev->drv && phydrv->set_loopback)
> + ret = phydrv->set_loopback(phydev, enable);
else
ret = -EOPNOTSUPP;
> +
> + if (ret)
> + return ret;
> +
> + phydev->loopback_enabled = enable;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(phy_loopback);
One of the comments we made of the PHY code in the hns driver is that
its locking is completely broken. You have made the same error
here. The core needs to hold the mutex while calling into the PHY
driver.
Andrew
