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

Hi Andrew,

On Wednesday, 10 June 2026 22:39:12 CEST Andrew Lunn wrote:

quoted

quoted

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