netdev - Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy switch 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 17 Aug 2023 21:01:29 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Linus Walleij <linus.walleij@...aro.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy
 switch drivers

Hi Russell,

On Tue, Aug 15, 2023 at 11:13:07AM +0100, Russell King (Oracle) wrote:
> There is one case that I have missed, and it's totally screwed by
> this behaviour where a Marvell DSA switch is connected to a Marvell
> PHY via a RGMII connection:
> 
>    DSA <---------------------------------> PHY
>     v					    v
> dt-dsa-node {				phy: dt-phy-node {
>   phy-handle = <&phy>;			  ...
>   phy-mode = "rgmii-foo";		};
> };
> 
> parses phy mode
> configures for RGMII mode
> configures RGMII delays associated
>  with phy-mode
> calls phy_attach(..., mode);
> phylib sets phy_dev->interface
> 					PHY driver looks at
> 					phydev->interface and
> 					configures delays
> 
> In this case, we have *both* ends configuring the RGMII delays and it
> will not work - because having the DSA MAC end configure the delays
> breaks the phylib model where the MAC *shouldn't* be configuring the
> delays.
> 
> So, should mv88e6xxx_mac_config() also be forcing all RGMII modes
> in state->interface to be PHY_INTERFACE_MODE_RGMII when passing
> that into mv88e6xxx_port_config_interface() if, and only if the
> port is a user port? Or maybe if and only if the port is actually
> connected to a real PHY?

I'd tend to believe that you're right, this would be broken (and not
just with Marvell RGMII PHYs).

I was under the impression that mv88e6xxx_mac_config() will only
configure the RGMII delays associated with phy-mode if the port is in
fixed-link mode.

But looking at the actual condition upon which mv88e6xxx_mac_config()
decides to call mv88e6xxx_port_config_interface(), that is:

	if (mode != MLO_AN_PHY || !mv88e6xxx_phy_is_internal(chip, port))
		mv88e6xxx_port_config_interface()

and thus, an external PHY would return false for the first term of the
OR operation, but true for the second, and it would proceed to configure
the MAC-side RGMII delays, resulting in a double delay.

However, I am not fully confident in my analysis, since I don't have
mv88e6xxx hardware with an RGMII port to confirm.

It's interesting that we haven't seen reports?