netdev - Re: [PATCH net] net: stmmac: Fix false "invalid port speed" warning

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <32bevr5jxmmm3ynnj3idpk3wdyaddoynyb7hv5tro3n7tsswwd@bbly52u3mzmn>
Date: Fri, 9 Aug 2024 15:41:04 -0500
From: Andrew Halaney <ahalaney@...hat.com>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Jose Abreu <joabreu@...opsys.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>, 
	Giuseppe CAVALLARO <peppe.cavallaro@...com>, Russell King <linux@...linux.org.uk>, 
	Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: stmmac: Fix false "invalid port speed" warning

On Fri, Aug 09, 2024 at 10:21:39PM GMT, Serge Semin wrote:
> If the internal SGMII/TBI/RTBI PCS is available in a DW GMAC or DW QoS Eth
> instance and there is no "snps,ps-speed" property specified (or the
> plat_stmmacenet_data::mac_port_sel_speed field is left zero), then the
> next warning will be printed to the system log:
> 
> > [  294.611899] stmmaceth 1f060000.ethernet: invalid port speed
> 
> By the original intention the "snps,ps-speed" property was supposed to be
> utilized on the platforms with the MAC2MAC link setup to fix the link
> speed with the specified value. But since it's possible to have a device
> with the DW *MAC with the SGMII/TBI/RTBI interface attached to a PHY, then
> the property is actually optional (which is also confirmed by the DW MAC
> DT-bindings). Thus it's absolutely normal to have the
> plat_stmmacenet_data::mac_port_sel_speed field zero initialized indicating
> that there is no need in the MAC-speed fixing and the denoted warning is
> false.

Can you help me understand what snps,ps-speed actually does? Its turned
into a bool and pushed down into srgmi_ral right now:

	/**
	 * dwmac_ctrl_ane - To program the AN Control Register.
	 * @ioaddr: IO registers pointer
	 * @reg: Base address of the AN Control Register.
	 * @ane: to enable the auto-negotiation
	 * @srgmi_ral: to manage MAC-2-MAC SGMII connections.
	 * @loopback: to cause the PHY to loopback tx data into rx path.
	 * Description: this is the main function to configure the AN control register
	 * and init the ANE, select loopback (usually for debugging purpose) and
	 * configure SGMII RAL.
	 */
	static inline void dwmac_ctrl_ane(void __iomem *ioaddr, u32 reg, bool ane,
					  bool srgmi_ral, bool loopback)
	{
		u32 value = readl(ioaddr + GMAC_AN_CTRL(reg));

		/* Enable and restart the Auto-Negotiation */
		if (ane)
			value |= GMAC_AN_CTRL_ANE | GMAC_AN_CTRL_RAN;
		else
			value &= ~GMAC_AN_CTRL_ANE;

		/* In case of MAC-2-MAC connection, block is configured to operate
		 * according to MAC conf register.
		 */
		if (srgmi_ral)
			value |= GMAC_AN_CTRL_SGMRAL;

		if (loopback)
			value |= GMAC_AN_CTRL_ELE;

		writel(value, ioaddr + GMAC_AN_CTRL(reg));
	}

What is that bit doing exactly? The only user upstream I see is
sa8775p-ride variants, but they're all using a phy right now (not
fixed-link aka MAC2MAC iiuc)... so I should probably remove it from
there too.

I feel like that property really (if I'm following right) should be just
taken from a proper fixed-link devicetree description? i.e. we already
specify a speed in that case. Maybe this predates that (or reinvents it)
and should be marked as deprecated in the dt-bindings.

But I'm struggling to understand what the bit is really doing based
on the original commit that added it, so I don't know if my logic is
solid. i.e., what's different in the phy case vs mac2mac with this
particular bit?

Thanks for your never ending patience about my questions wrt the
hardware and this driver.

- Andrew