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

[Andrew Lunn <andrew@lunn.ch>]

> +static void dp83869_module_remove(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +
> +	phydev_info(phydev, "SFP module removed\n");

I think this should probably be downgraded to phydev_dbg().

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

    Andrew

---
pw-bot: cr