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: 1697 bytes --]

Hi Andrew,

On Tuesday, 11 November 2025 02:08:05 CEST Andrew Lunn wrote:
...
> > +static int dp83869_module_insert(void *upstream, const struct
> > sfp_eeprom_id *id) +{
> > +	struct phy_device *phydev = upstream;
> > +	const struct sfp_module_caps *caps;
> > +	struct dp83869_private *dp83869;
> > +	int ret;
> > +
> > +	caps = sfp_get_module_caps(phydev->sfp_bus);
> > +
> > +	if (!linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> > +			       caps->link_modes)) {
> > +		phydev_err(phydev, "incompatible SFP module inserted\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	dp83869 = phydev->priv;
> > +
> > +	dp83869->mode = DP83869_RGMII_1000_BASE;
> > +	phydev->port = PORT_FIBRE;
> > +
> > +	ret = dp83869_configure_mode(phydev, dp83869);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Reconfigure advertisement */
> > +	if (mutex_trylock(&phydev->lock)) {
> > +		ret = dp83869_config_aneg(phydev);
> > +		mutex_unlock(&phydev->lock);
> 
> 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?

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