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: <b075514b-74b6-46bb-ba3b-da5a2490e5b3@engleder-embedded.com>
Date: Tue, 29 Oct 2024 06:49:48 +0100
From: Gerhard Engleder <gerhard@...leder-embedded.com>
To: Andrew Lunn <andrew@...n.ch>
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

On 29.10.24 03:45, Andrew Lunn wrote:
>> -	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?

Yes, the PHY and the MAC has to agree on the speed. In my opinion
adjust_link/mac_link_up shall be called like in normal operation without
loopback. The question is when? adjust_link/mac_link_up could be called
after phy_loopback() returns with success or before phy_loopback()
returns. I would prefer that a successful return of phy_loopback()
guarantees that the loopback mode is active and signaled. This would
eliminate the need to wait for the loopback, which would be again
error prone.

It is not guaranteed that the link goes down before its coming up at
another speed. That needs to be fixed. If the link is up and the speed
matches, then the link shall stay up. If the link is up and the speed
does not match, then the link shall first go down and then come up after
the loopback is established. If the link is down, then the link come up
after the loopback is established. Would that be ok? Or should the link
always go down before the loopback is switched on?

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

I agree that adjust_link/mac_link_up shall still be used to let the MAC
know the speed.

> 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.

I would like to try it. Would be great to hear your opinion about when
adjust_link/mac_link_up should be called in case of successful
set_loopback() and if the link shall always go down before loopback.

Thank you for your feedback!

Gerhard