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

[Andrew Lunn <andrew@lunn.ch>]

> 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;
> }

I don't think you need a mutex here. dev_ioctl.c:

       case SIOCETHTOOL:
                dev_load(net, ifr.ifr_name);
                rtnl_lock();
                ret = dev_ethtool(net, &ifr);
                rtnl_unlock();
 
So all ethtool operations are protected by the rtnl. You cannot have
two tests running at once.

> 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?

You cannot ensure this. But it would be a pretty obvious bug.

    Andrew