Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test

[l00371289]

Hi, Andrew

On 2017/6/21 21:34, Andrew Lunn wrote:
>> drivers/net/ph/marvell.c
>> marvell_set_loopback(struct phy_device *dev, bool enable)
>> {
>> 	/* do some device specific setting */
>> 	........
>>
>> 	return genphy_loopback(dev, enable);
>> }
>>
>> I don't know if this makes sense or not?
> 
> Nope, you want something in phy.c like this. Not compiled, not
> tested....
> 
> int phy_loopback(struct phy_device *dev, bool enable)
> {
> 
> 	int err;
> 
> 	mutex_lock(dev->mutex);
> 
> 	if (enable && dev->loopback_enabled) {
> 	   err = -EBUSY;
> 	   goto out;
> 	}
> 
> 	if (!enable && !dev->loopback_enabled) {
> 	   err = -EINVAL;
> 	   goto out;
> 	}
> 
> 	if (dev->drv->loopback)
> 	   err = dev->drv->loopback(dev, enable);
> 
> 	if (!err)
> 	   dev->loopback_enabled = enable
> 
> 	mutex_unlock(dev->mutex);
> 
> out:
> 	return err;
> }
> EXPORT_SYMBOL(phy_loopback);
> 
> The interesting part is how to handle two attempts to enable loopback.
> Should the MAC driver be responsible for preventing that? Or do it in
> the PHY driver? And what will the MAC driver do if it gets EBUSY?
> Return it to user space?
I don't think returning to user space is an option, because it mean
ethtool phy self test would fail because if availability of some software
resoure, which is not some application would expect. here is what I can
think of:

driver/net/ethernet/hisilicon/hns/hns_ethtool.c
int phy_loopback_selftest(struct phy_device *dev)
{
	int err;

	mutex_lock(dev->mutex);

	if (dev->drv->loopback)
		err = dev->drv->loopback(dev, 1);

	if (err)
		goto out;
	
	/* mac driver do the test*/
	.................


	if (dev->drv->loopback)
		err = dev->drv->loopback(dev, 0);

	if (err)
		goto out;

out:
	mutex_unlock(dev->mutex);
	return err;
}

One question I can think of is that, how do we ensure mac driver enable and
disable phy loopback in pair, enable loopback after dev->mutex is taken,
disable loopback  before dev->mutex is released?
I hope I am not missing something obvious, any suggestion and idea?

Best Regards
Yunsheng Lin