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

[Romain Gantois <romain.gantois@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

Hi Andrew,

On Wednesday, 10 June 2026 22:39:12 CEST Andrew Lunn wrote:
> > > 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.
> 

Okay, thanks for the explanation!

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

ACK, so presumably we should also hold the lock when doing 
dp83869_configure_mode() since it modifies PHY registers. This'll be a bit of a 
headache since configure_mii() may be called with the lock already held or not 
depending on the call path... I'll see what can be done about this.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]