netdev - Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

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: <CAOiHx=nwbs7030GKZHLc6Pc6LA6Hqq0NYfNSt=3zOgnj5zpAYQ@mail.gmail.com>
Date: Mon, 19 May 2025 21:44:16 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Florian Fainelli <florian.fainelli@...adcom.com>, Vladimir Oltean <olteanv@...il.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Vivien Didelot <vivien.didelot@...il.com>, Álvaro Fernández Rojas <noltari@...il.com>, 
	Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 2/3] net: dsa: b53: fix configuring RGMII delay on bcm63xx

On Mon, May 19, 2025 at 9:14 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Mon, May 19, 2025 at 07:45:49PM +0200, Jonas Gorski wrote:
> > The RGMII delay type of the PHY interface is intended for the PHY, not
> > the MAC, so we need to configure the opposite. Else we double the delay
> > or don't add one at all if the PHY also supports configuring delays.
> >
> > Additionally, we need to enable RGMII_CTRL_TIMING_SEL for the delay
> > actually being effective.
> >
> > Fixes e.g. BCM54612E connected on RGMII ports that also configures RGMII
> > delays in its driver.
>
> We have to be careful here not to cause regressions. It might be
> wrong, but are there systems using this which actually work? Does this
> change break them?

The only user (of bcm63xx and b53 dsa) I am aware of is OpenWrt, and
we are capable of updating our dts files in case they were using
broken configuration. Though having PHYs on the RGMII ports is a very
rare configuration, and usually there is switch connected with a fixed
link, so likely the issue was never detected.

> >
> > Fixes: ce3bf94871f7 ("net: dsa: b53: add support for BCM63xx RGMIIs")
> > Signed-off-by: Jonas Gorski <jonas.gorski@...il.com>
> > ---
> >  drivers/net/dsa/b53/b53_common.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> > index a316f8c01d0a..b00975189dab 100644
> > --- a/drivers/net/dsa/b53/b53_common.c
> > +++ b/drivers/net/dsa/b53/b53_common.c
> > @@ -1328,19 +1328,19 @@ static void b53_adjust_63xx_rgmii(struct dsa_switch *ds, int port,
> >
> >       switch (interface) {
> >       case PHY_INTERFACE_MODE_RGMII_ID:
> > -             rgmii_ctrl |= (RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> > +             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> >               break;
> >       case PHY_INTERFACE_MODE_RGMII_RXID:
> > -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_TXC);
> > -             rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
> > +             rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
> > +             rgmii_ctrl &= ~RGMII_CTRL_DLL_RXC;
> >               break;
> >       case PHY_INTERFACE_MODE_RGMII_TXID:
> > -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC);
> > -             rgmii_ctrl |= RGMII_CTRL_DLL_TXC;
> > +             rgmii_ctrl |= RGMII_CTRL_DLL_RXC;
> > +             rgmii_ctrl &= ~RGMII_CTRL_DLL_TXC;
> >               break;
> >       case PHY_INTERFACE_MODE_RGMII:
> >       default:
> > -             rgmii_ctrl &= ~(RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC);
> > +             rgmii_ctrl |= RGMII_CTRL_DLL_RXC | RGMII_CTRL_DLL_TXC;
> >               break;
>
> These changes look wrong. There is more background here:
>
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287

This is what makes it work for me (I tested all four modes, rgmii,
rgmii-id, rgmii-txid and rgmii-rxid). Without this change, b53 will
configure the same delays on the MAC layer as the PHY driver (bcm54xx,
https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/net/phy/broadcom.c#L73
), which breaks connectivity at least for me.

E.g. with a phy-mode of "rgmii-id", both b53 and the PHY driver would
enable rx and tx delays, causing the delays to be 4 ns instead of 2
ns. So I don't see how this could have ever worked.

Also note that b53_adjust_531x5_rgmii()
https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/net/dsa/b53/b53_common.c#L1360
already behaves that way, this just makes bcm63xx now work the same
(so these functions could now even be merged).

I did see the part where the document says the MAC driver is supposed
to change the phy mode, but I haven't really found out how I am
supposed to do that within phylink/dsa, since it never passes any phy
modes to any PHYs anywhere, just reports what it supports, then
phylink tells it what to configure AFAICT.

Best regards,
Jonas