Re: [PATCH net-next v2 3/3] net: phy: dp83869: Support 1000Base-X SFP

[Andrew Lunn]

> > Why is it safe to call dp83869_configure_mode() without the lock, but
> > dp83869_config_aneg() does need the lock? And what are the
> > consequences of not being able to get the lock and so aneg is not
> > configured?
> > 
> > Some comments would be good here.
> 
> Since phy_port has been merged, I'm currently preparing a v3 for this but I'm 
> in need of some general guidance regarding this `mutex_trylock(&phydev->lock)` 
> which has (rightfully) sparked some concerns.
> 
> What data is supposed to be protected by the `phydev->lock` mutex? Is it every 
> field of the phydev struct + standard hardware registers + vendor registers? Or 
> only a subset of these?

The phydev lock should prevent two phylib operations happening at
once. It also protects the members of phylib which are accessed during
the callback when the link changes.

Phylib operations should include access to hwmon sensors and LEDs, if
there is a danger such access could upset things. Since LEDs come
through the phylib core, locking is done for that in the core. But
hwmon sensors are not part of the core.

Since PHYs are pretty simple, and MDIO operations take a lot of time,
it is better to hold the lock than not hold the lock.

	Andrew