Re: [PATCH net-next] net: phy: extend genphy_c45_read_eee_abilities() to read capability 2 register

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Dec 20, 2023 at 05:12:58PM +0100, Marek Behún wrote:
> +/**
> + * genphy_c45_read_eee_cap2 - read supported EEE link modes from register 3.21
> + * @phydev: target phy_device struct
> + */
> +static int genphy_c45_read_eee_cap2(struct phy_device *phydev)
> +{
> +	int val;
> +
> +	/* IEEE 802.3-2018 45.2.3.11 EEE control and capability 2
> +	 * (Register 3.21)
> +	 */
> +	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE2);
> +	if (val < 0)
> +		return val;
> +
> +	/* The 802.3 2018 standard says the top 6 bits are reserved and should
> +	 * read as 0.
> +	 * If MDIO_PCS_EEE_ABLE2 is 0xffff assume EEE is not supported.
> +	 */
> +	if (val == 0xffff)
> +		return 0;

802.3 also says that unimplemented registers should read as zeros.
Reserved bits should read as 0, but reserved typically means (as we've
seen several times) that bits get used in the future. Do you have a
good reason why this check is necessary?

> +
> +	mii_eee_cap2_mod_linkmode_t(phydev->supported_eee, val);
> +
> +	/* Some buggy devices may indicate EEE link modes in MDIO_PCS_EEE_ABLE2
> +	 * which they don't support as indicated by BMSR, ESTATUS etc.
> +	 */
> +	linkmode_and(phydev->supported_eee, phydev->supported_eee,
> +		     phydev->supported);

I wonder whether we should just do that as a general thing after
reading all the abilities in phy_probe() rather than burying it in
various capability reading functions?

Apart from those two, I don't see any other issues with the patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!