linux-kernel - Re: [RFC PATCH net-next 4/5] net: macb: Configure High Speed Mac for given speed.

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: <ZwZKumS3IEy54Jsk@shell.armlinux.org.uk>
Date: Wed, 9 Oct 2024 10:19:54 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vineeth Karumanchi <vineeth.karumanchi@....com>
Cc: nicolas.ferre@...rochip.com, claudiu.beznea@...on.dev,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, andrew@...n.ch, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	git@....com
Subject: Re: [RFC PATCH net-next 4/5] net: macb: Configure High Speed Mac for
 given speed.

On Wed, Oct 09, 2024 at 11:09:45AM +0530, Vineeth Karumanchi wrote:
> HS Mac configuration steps:
> - Configure speed and serdes rate bits of USX_CONTROL register from
>   user specified speed in the device-tree.
> - Enable HS Mac for 5G and 10G speeds.
> - Reset RX receive path to achieve USX block lock for the
>   configured serdes rate.
> - Wait for USX block lock synchronization.
> 
> Move the initialization instances to macb_usx_pcs_link_up().

It only partly moves stuff there, creating what I can only call a mess
which probably doesn't work correctly.

Please consider the MAC and PCS as two separate boxes - register
settings controlled in one box should not be touched by the other box.

For example, macb_mac_config() now does this:

        old_ncr = ncr = macb_or_gem_readl(bp, NCR);
...
        } else if (macb_is_gem(bp)) {
...
                ncr &= ~GEM_BIT(ENABLE_HS_MAC);
...
        if (old_ncr ^ ncr)
                macb_or_gem_writel(bp, NCR, ncr);

meanwhile:

> @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
>  				 int duplex)
>  {
>  	struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
...
> +	/* Enable HS MAC for high speeds */
> +	if (hs_mac) {
> +		config = macb_or_gem_readl(bp, NCR);
> +		config |= GEM_BIT(ENABLE_HS_MAC);
> +		macb_or_gem_writel(bp, NCR, config);
> +	}

Arguably, the time that this would happen is when the interface mode
changes which would cause a full reconfiguration and thus both of
these functions will be called, but it's not easy to follow that's
what is going on here.

It also looks like you're messing with MAC registers in the PCS code,
setting the MAC speed there. Are the PCS and MAC so integrated together
that abstracting the PCS into its own separate code block leads to
problems?

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