Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed

[Andrew Lunn <andrew@lunn.ch>]

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