netdev - Re: [PATCH v9 4/4] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes

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]

Date: Sat, 22 Jun 2024 21:12:23 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Kamil Horák (2N) <kamilh@...s.com>
Cc: florian.fainelli@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
	hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 4/4] net: phy: bcm-phy-lib: Implement BroadR-Reach
 link modes

On Fri, Jun 21, 2024 at 01:26:33PM +0200, Kamil Horák (2N) wrote:
> Implement single-pair BroadR-Reach modes on bcm5481x PHY by Broadcom.
> Create set of functions alternative to IEEE 802.3 to handle
> configuration of these modes on compatible Broadcom PHYs.

What i've not seen anywhere is a link between BroadR-Reach and LRE.
Maybe you could explain the relationship here in the commit message?
And maybe also how LDS fits in.

> +int bcm_setup_master_slave(struct phy_device *phydev)

This is missing the lre in the name.

> +static int bcm54811_read_abilities(struct phy_device *phydev)
> +{
> +	int i, val, err;
> +	u8 brr_mode;
> +
> +	for (i = 0; i < ARRAY_SIZE(bcm54811_linkmodes); i++)
> +		linkmode_clear_bit(bcm54811_linkmodes[i], phydev->supported);

I think that needs a comment since it is not clear what is going on
here. What set these bits in supported?

> +
> +	err = bcm5481x_get_brrmode(phydev, &brr_mode);
> +	if (err)
> +		return err;
> +
> +	if (brr_mode) {

I would expect the DT property to be here somewhere. If the DT
property is present, set phydev->supported to only the BRR modes,
otherwise set it to the standard baseT modes. That should then allow
the core to do most of the validation. This is based on my
understanding the coupling hardware makes the two modes mutually
exclusive?

> +	/* With BCM54811, BroadR-Reach implies no autoneg */
> +	if (brr)
> +		phydev->autoneg = 0;

So long as phydev->supported does not indicate autoneg, this should
not happen.

	Andrew