Re: [PATCH] net: phy: phy_ethtool_ksettings_set: Allow any supported speed

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Dec 03, 2024 at 04:51:47PM +0100, Maxime Chevallier wrote:
> Hi Andrew,
> 
> On Tue, 3 Dec 2024 15:21:27 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Tue, Dec 03, 2024 at 03:45:09PM +0100, Andrew Lunn wrote:
> > > On Tue, Dec 03, 2024 at 02:05:07PM +0000, Dennis Ostermann wrote:  
> > > > Hi,
> > > > 
> > > > according to IEE 802.3-2022, ch. 125.2.4.3, Auto-Negotiation is optional for 2.5GBASE-T1
> > > >   
> > > > > 125.2.4.3 Auto-Negotiation, type single differential-pair media
> > > > > Auto-Negotiation (Clause 98) may be used by 2.5GBASE-T1 and 5GBASE-T1 devices to detect the
> > > > > abilities (modes of operation) supported by the device at the other end of a link segment, determine common
> > > > > abilities, and configure for joint operation. Auto-Negotiation is performed upon link startup through the use
> > > > > of half-duplex differential Manchester encoding.
> > > > > The use of Clause 98 Auto-Negotiation is optional for 2.5GBASE-T1 and 5GBASE-T1 PHYs  
> > > > 
> > > > So, purposed change could make sense for T1 PHYs.  
> > > 
> > > The proposed change it too liberal. We need the PHY to say it supports
> > > 2.5GBASE-T1, not 2.5GBASE-T. We can then allow 2.5GBASE-T1 to not use
> > > autoneg, but 2.5GBASE-T has to use autoneg.  
> > 
> > I'm wondering whether we should add:
> > 
> > 	__ETHTOOL_DECLARE_LINK_MODE_MASK(requires_an);
> > 
> > to struct phy_device, and have phylib populate that by default with all
> > base-T link modes > 1G (which would be the default case as it is now.)
> > Then, PHY drivers can change this bitmap as they need for their device.
> > After the PHY features have been discovered, this should then get
> > AND-ed with the supported bitmap.
> 
> If the standards says that BaseT4 >1G needs aneg, and that we can't
> have it for baseT1, couldn't we just have some lookup table for each
> mode indicating if they need or support aneg ?

When operating in !AN mode, all that the ethtool API passes is the
speed and duplex, with a guess at the advertising mask (which doesn't
take account of the PHY's supported ethtool link modes.)

If e.g. we have a PHY that supports 1000base-T and 1000base-X, and the
user attempts to disable AN specifying speed 1000 duplex full, we don't
know whether the user means 1000base-T (which actually requires AN, but
we work around that *) or 1000base-X without AN.

Specifying speed + duplex for !AN is nice for humans, but ambiguous
for computers.

* - the workaround adopted is to do what Marvell PHYs internally do but
in phylib code, which is to accept the request to disable AN and
operate at the specified speed, but actually program AN to be enabled
with only a single speed/duplex that can be negotiated. Without this,
we end up with hacks in MAC drivers because the PHY they're paired with
doesn't support AN being disabled at 1G speed. See
__genphy_config_aneg(). Note: this is probably going to interact badly
with the baseT1 case.

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