linux-kernel - Re: [PATCH net-next v3 10/47] net: phylink: Adjust link settings based on rate adaptation

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: <YtWIy2C/5Xf1CseG@shell.armlinux.org.uk>
Date:   Mon, 18 Jul 2022 17:22:35 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Sean Anderson <sean.anderson@...o.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....com>, netdev@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Alexandru Marginean <alexandru.marginean@....com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH net-next v3 10/47] net: phylink: Adjust link settings
 based on rate adaptation

On Sun, Jul 17, 2022 at 03:39:39AM +0200, Andrew Lunn wrote:
> > > I would not do this. If the requirements for rate adaptation are not
> > > fulfilled, you should turn off rate adaptation.
> > > 
> > > A MAC which knows rate adaptation is going on can help out, by not
> > > advertising 10Half, 100Half etc. Autoneg will then fail for modes
> > > where rate adaptation does not work.
> > 
> > OK, so maybe it is better to phylink_warn here. Something along the
> > lines of "phy using pause-based rate adaptation, but duplex is %s".
> 
> You say 1/2 duplex simply does not work with rate adaptation. So i
> would actually return -EINVAL at the point the MAC indicates what
> modes it supports if there is a 1/2 duplex mode in the list.

If we have a PHY that supports rate adaption using pause frames, which
implies a full duplex link between the PHY and MAC, one would hope that
someone isn't silly enough to integrate it with a half-duplex only MAC.

This ought to be handled while bringing up the PHY. If the PHY uses
pause frames but the MAC doesn't support full-duplex at the PHY
interface speed, then we should not allow the PHY to do rate adaption.
The easiest way to achieve that is to not allow the PHY to advertise
anything except the PHY interface speed on its media. If that means
there's nothing to advertise, then we fail.

> Right, so we need a table somewhere in the documentation listing the
> different combinations and what should happen.
> 
> If the MAC does not support rx_pause, rate adaptation is turned off.
> If the negotiation results in no rx_pause, force it on anyway with
> Pause based adaptation. If ethtool turns pause off, turn off rate
> adaptation.

That last bit is really awkward - what if the link partner is doing 100M
on the media because that's the fastest it's capable of, but our local
PHY is doing rate adaption to 1G, and we turn pause off, causing rate
adaption in the PHY to be turned off. We need to reconfigure the
advertisement to drop anything except the 1G speed and renegotiate the
link, which will cause the link to go down.

That's going to be really odd behaviour for a user to get their head
around.

> Does 802.3 say anything about this?

I think rate adaption is out of scope of 802.3.

> We might also want to add an additional state to the ethtool get for
> pause, to indicate rx_pause is enabled because of rate adaptation, not
> because of autoneg.

That may well be a much better approach; it lets the user see what is
going on and it becomes more understandable to the user IMHO.

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