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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZZU3Ijo2TCIHJvJh@shell.armlinux.org.uk>
Date: Wed, 3 Jan 2024 10:29:54 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Marek Behún <kabel@...nel.org>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next] net: phy: extend
 genphy_c45_read_eee_abilities() to read capability 2 register

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!