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: Fri, 18 Aug 2023 14:40:55 +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

On Fri, Aug 18, 2023 at 12:11:15PM +0100, Russell King (Oracle) wrote:
> On Thu, Aug 17, 2023 at 10:17:54PM +0300, Vladimir Oltean wrote:
> > On Thu, Aug 17, 2023 at 08:52:12PM +0200, Andrew Lunn wrote:
> > > > Andrew, I'd argue that the MAC-PHY relationship between the DSA master
> > > > and the CPU port is equally clear as between 2 arbitrary cascade ports.
> > > > Which is: not clear at all. The RGMII standard does not talk about the
> > > > existence of a MAC role and a PHY role, to my knowledge.
> > > 
> > > The standard does talk about an optional in band status, placed onto
> > > the RXD pins during the inter packet gap. For that to work, there
> > > needs to be some notion of MAC and PHY side.
> > 
> > Well, opening the RGMII standard, it was quite stupid of myself to say
> > that. It says that the purpose of RGMII is to interconnect the MAC and
> > the PHY right from the first phrase.
> > 
> > You're also completely right in pointing out that the optional in-band
> > status is provided by the PHY on RXD[3:0].
> > 
> > Actually, MAC-to-MAC is not explicitly supported anywhere in the standard
> > (RGMII 2.0, 4/1/2002) that I can find. It simply seems to be a case of:
> > "whatever the PHY is required by the standard to do is specified in such
> > a way that when another MAC is put in its place (with RX and TX signals
> > inverted), the protocol still makes sense".
> > 
> > But, with that stretching of the standard considered, I'm still not
> > necessarily seeing which side is the MAC and which side is the PHY in a
> > MAC-to-MAC scenario.
> > 
> > With a bit of imagination, I could actually see 2 back-to-back MAC IPs
> > which both have logic to provide the optional in-band status (with
> > hardcoded information) to the link partner's RXD[3:0]. No theory seems
> > to be broken by this (though I can't point to any real implementation).
> > 
> > So a MAC role would be the side that expects the in-band status to be
> > present on its RXD[3:0], and a PHY role would be the side that provides
> > it, and being in the MAC role does not preclude being in the PHY role?
> 
> ... trying to find an appropriate place in this thread to put this
> as I would like to post the realtek patch adding the phylink_get_caps
> method.
> 
> So, given the discussion so far, we have two patches to consider.
> 
> One patch from Linus which changes one of the users of the Realtek
> DSA driver to use "rgmii-id" instead of "rgmii". Do we still think
> that this a correct change given that we seem to be agreeing that
> the only thing that matters on the DSA end of this is that it's
> "rgmii" and the delays for the DSA end should be specified using
  ~~~~~~~
  I'd say not necessarily specifically "rgmii", but rather "rgmii*"

> the [tr]x-internal-delay-ps properties?

As long as it is understood that changing "rgmii" to "rgmii-id" is
expected to be inconsequential (placebo) for a fixed-link, I don't have
an objection (in principle) to that patch.

Though, to have more confidence in the validity of the change, I'd need
the phy-mode of the &gmac0 node from arch/arm/boot/dts/gemini/gemini-dlink-dir-685.dts,
and I'm not seeing it.

Looking at its driver (drivers/net/ethernet/cortina/gemini.c), I don't
see any handling of RGMII delays, and it accepts any RGMII phy-mode.

So, if neither the Ethernet controller nor the RTL8366RB switch are
adding RGMII delays, it becomes plausible that there are skews added
through PCB traces. In that case, the current phy-mode = "rgmii" would
be the correct choice, and changing that would be invalid. Some more
documentary work would be needed.

In any case, I wouldn't rush a change to arch/arm/boot/dts/gemini/gemini-dlink-dir-685.dts,
and I'm not seeing any dependency between that and your phylink_get_caps
conversion for the rtl8366rb.

> The second patch is my patch adding a phylink_get_caps method for
> Realtek drivers - should that allow all "rgmii" interface types,
> or do we want to just allow "rgmii" to encourage the use of the
> [tr]x-internal-delay-ps properties?

Same opinion as above. As long as it's understood that the RTL8366RB
MAC, like any other MAC, shouldn't be acting upon the phy-mode when
adding delays, let's just accept all 4 variants, with future support to
be added for [rt]x-internal-delay-ps if there turn out to be
configurable MAC-side delays present.