Hi, Andrew On 2017/6/22 11:35, Andrew Lunn wrote: >> 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. > Thanks for your reply, I will send a new patch based on the discussion above. Please let me know if there is anything else need changing.
Best Regards Yunsheng Lin