netdev - Re: [PATCH net-next 1/4] net: phy: Allow loopback speed selection for PHY drivers

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: <04baac62-ace2-4f44-a32e-640f30b24d96@lunn.ch>
Date: Tue, 29 Oct 2024 03:45:24 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Gerhard Engleder <gerhard@...leder-embedded.com>
Cc: hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/4] net: phy: Allow loopback speed selection
 for PHY drivers

> -	int (*set_loopback)(struct phy_device *dev, bool enable);
> +	/**
> +	 * @set_loopback: Set the loopback mode of the PHY
> +	 * enable selects if the loopback mode is enabled or disabled. If the
> +	 * loopback mode is enabled, then the speed of the loopback mode can be
> +	 * requested with the speed argument. If the speed argument is zero,
> +	 * then any speed can be selected. If the speed argument is > 0, then
> +	 * this speed shall be selected for the loopback mode or EOPNOTSUPP
> +	 * shall be returned if speed selection is not supported.
> +	 */

I think we need to take a step back here and think about how this
currently works.

The MAC and the PHY probably need to agree on the speed. Does an RGMII
MAC sending at 10Mbps work against a PHY expecting 1000Mbps? The MAC
is repeating the symbols 100 times to fill the channel, so it might?
But does a PHY expecting 100 repeats work when actually passed a
signal instance of a symbol?  What about an SGMII link, 10Mbps one
side, 1G the other? What about 1000BaseX vs 2500BaseX?

I've never thought about how this actually works today. My _guess_
would be, you first either have the link perform auto-neg, or you
force a speed using ethtool. In both cases, the adjust_link callback
of phylib is called, which for phylink will result in the mac_link_up
callback being called with the current link mode, etc. So the PHY and
the MAC know the properties of the link between themselves.

> +	int (*set_loopback)(struct phy_device *dev, bool enable, int speed);

You say:

> +                                    If the speed argument is zero,
> +	 * then any speed can be selected.

How do the PHY and the MAC agree on the speed? Are you assuming the
adjust_link/mac_link_up will be called? Phylink has the hard
assumption the link will go down before coming up at another speed. Is
that guaranteed here?

What happens when you pass a specific speed? How does the MAC get to
know?

I think we need to keep as close as possible to the current
scheme. The problem is >= 1G, where the core will now enable autoneg
even for forced speeds. If there is a link partner, auto-neg should
succeed and adjust_link/mac_link_up will be called, and so the PHY and
MAC will already be in sync when loopback is enabled. If there is no
link partner, auto-neg will fail. In this case, we need set_loopback
to trigger link up so that the PHY and MAC get in sync. And then it is
disabled, the link needs to go down again.

	Andrew