> > > Maybe we should think about this locking a bit. It is normal for the > > > lock to be held when using ops in the phy driver structure. The > > > exception is suspend/resume. Maybe we should also take the lock before > > > calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()? > > > > Yes, that certainly seems like a good approach to me, let me cook a > > patch doing that. > > Just for my understanding (such that I will not make the same mistake > again)... > > Why is it that phy functions such as get_wol needs to take the phy_lock and > others like get_tunable does not. > > I do understand the arguments on why the lock should be held by the caller of > get_tunable, but I do not understand why the same argument does not apply for > get_wol.
Hi Allan phy_ethtool_get_wol and friends probably should take the phy_lock. This inconsistency is probably leading to locking bugs. e.g. at803x_set_wol() does a read-modify-write, and does not take the lock. There is no comment in the patch adding phy_ethtool_set_wol() to say why the lock is not taken, and a quick look at the code does not suggest a reason why it could not be taken/released by phy_ethtool_set_wol(). I think it would be a good idea to change this. phy_suspend()/phy_resume() might have good reasons to avoid the lock, i've no idea how it is supposed to work. Is there a danger something else is holding the lock and has already been suspended? I guess not, otherwise there is little hope suspend would work at all. Andrew