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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 14 Aug 2023 17:27:46 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>
Cc: Vladimir Oltean <olteanv@...il.com>,
	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

On Mon, Aug 14, 2023 at 05:46:21PM +0200, Andrew Lunn wrote:
> > > > +		__set_bit(PHY_INTERFACE_MODE_RGMII, interfaces);
> > > 
> > > also, I guess that this should allow all 4 variants of RGMII.
> > 
> > I'm not sure - looking at what's available, the RTL8366 datasheet (not
> > RB) says that there's pinstrapping for the RGMII delays. It also suggests
> > that there may be a register that can be modified for this, but the driver
> > doesn't appear to touch it - in fact, it does nothing with the interface
> > mode. Moreover, the only in-kernel DT for this has:
> > 
> >                         rtl8366rb_cpu_port: port@5 {
> >                                 reg = <5>;
> >                                 label = "cpu";
> >                                 ethernet = <&gmac0>;
> >                                 phy-mode = "rgmii";
> >                                 fixed-link {
> >                                         speed = <1000>;
> >                                         full-duplex;
> >                                         pause;
> >                                 };
> >                         };
> > 
> > Whether that can be changed in the RB version of the device or not, I
> > don't know, so whether it makes sense to allow the other RGMII modes,
> > again, I don't know.
> > 
> > Annoyingly, gmac0 doesn't exist in this file, it's defined in
> > gemini.dtsi, which this file references through a heirarchy of nodes
> > (makes it very much less readable), but it points at:
> > 
> > / {
> > ...
> >         soc {
> > ...
> >                 ethernet@...00000 {
> > ...
> >                         ethernet-port@0 {
> >                                 phy-mode = "rgmii";
> >                                 fixed-link {
> >                                         speed = <1000>;
> >                                         full-duplex;
> >                                         pause;
> >                                 };
> >                         };
> > 
> > So that also uses "rgmii".
> > 
> > I'm tempted not to allow the others as the driver doesn't make any
> > adjustments, and we only apparently have the one user.
> 
> RGMII on both ends is unlikely to work, so probably one is
> wrong. Probably the switch has strapping to set it to rgmii-id, but we
> don't actually know that. And i guess we have no ability to find out
> the truth?

"rgmii" on both ends _can_ work - from our own documentation:

* PHY_INTERFACE_MODE_RGMII: the PHY is not responsible for inserting any
  internal delay by itself, it assumes that either the Ethernet MAC (if capable)
  or the PCB traces insert the correct 1.5-2ns delay
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So, if the board is correctly laid out to include this delay, then RGMII
on both ends can work.

Next, we have no ability to find anything out - we have no hardware.
LinusW does, but I have no idea whether Linus can read the state of the
pin strapping. I can see from the RTL8366 info I found that there is
a register that the delay settings can be read from, but this is not
the RTL8366, it's RTL8366RB, which Linus already pointed out is
revision B and is different. So, I would defer to Linus for anything on
this, and without input from Linus, all we have to go on is what we
have in the kernel sources.

> So a narrow definition seems reasonable at the moment, to raise a red
> warning flag if somebody does try to use rgmii-id which is not
> actually implemented in the driver. And that user then gets to sort
> out the problem.

I think Vladimir will be having a party, because that's now two of us
who've made the mistake of infering that "phy-mode" changes the
configuration at the end that has that specified (I can hear the
baloons being inflated!)

Of course it shouldn't, as per our documentation on RGMII delays in
Documentation/networking/phy.rst that was added by Florian back in
November 2016.

That said, with DSA, we don't currently have a way for the MAC to
instruct the DSA switch what delay it should be using as unlike PHYLIB,
the MAC doesn't bind to the DSA switch in the same way (there's no
dsa_connect() or dsa_attach() call that MACs call which would pass
the phy interface mode to the DSA side.)

However, a DSA switch CPU node does have an "ethernet" property
which points at the CPU-side node, and a DSA switch _could_ read
that setting and use it to configure the RGMII delays in the same
way that PHYs would do - but no one does that.

This brings up the obvious question: does anyone deal with the RGMII
delays with DSA switches in software, or is it all done by hardware
pin strapping, hardware defaults, and/or a correctly laid out PCB?

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