Message-ID: <Y7ccNSSnPxTR2AQs@shell.armlinux.org.uk>
Date:   Thu, 5 Jan 2023 18:51:33 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Sean Anderson <sean.anderson@...o.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
        "David S . Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Tim Harvey <tharvey@...eworks.com>
Subject: Re: [PATCH net-next v5 4/4] phy: aquantia: Determine rate adaptation
 support from registers

On Thu, Jan 05, 2023 at 07:43:42PM +0200, Vladimir Oltean wrote:
> On Thu, Jan 05, 2023 at 02:40:50PM +0000, Russell King (Oracle) wrote:
> > > If the PHY firmware uses a combination like this: 10GBASE-R/XFI for
> > > media speeds of 10G, 5G, 2.5G (rate adapted), and SGMII for 1G, 100M
> > > and 10M, a call to your implementation of
> > > aqr107_get_rate_matching(PHY_INTERFACE_MODE_10GBASER) would return
> > > RATE_MATCH_NONE, right? So only ETHTOOL_LINK_MODE_10000baseT_Full_BIT
> > > would be advertised on the media side?
> > 
> > No, beause of the special condition in phylink that if it's a clause 45
> > PHY and we use something like 10GBASE-R, we don't limit to just 10G
> > speed, but try all interface modes - on the assumption that the PHY
> > will switch its host interface.
> > 
> > RATE_MATCH_NONE doesn't state anything about whether the PHY operates
> > in a single interface mode or not - with 10G PHYs (and thus clause 45
> > PHYs) it seems very common from current observations for
> > implementations to do this kind of host-interface switching.
> 
> So you mention commits
> 7642cc28fd37 ("net: phylink: fix PHY validation with rate adaption") and
> df3f57ac9605 ("net: phylink: extend clause 45 PHY validation workaround").
> 
> IIUC, these allow the advertised capabilities to be more than 10G (based
> on supported_interfaces), on the premise that it's possible for the PHY
> to switch SERDES protocol to achieve lower speeds.

I didn't mention any commits, but yes, it's ever since the second commit
you list above, which was necessary to get PHYs which switch their
interface mode to work sanely. It essentially allows everything that
the combination of host and PHY supports, because we couldn't do much
better at the time that commit was written.

> This does partly correct the last part of my question, but I believe
> that the essence of it still remains. We won't make use of PAUSE rate
> adaptation to support the speeds which aren't directly covered by the
> supported_interfaces. Aren't we interpreting the PHY provisioning somewhat
> too conservatively in this case, or do you believe that this is just an
> academic concern?

Do you have a better idea how to come up with a list of link modes that
the PHY should advertise to its link partner and also report as
supported given the combination of:

- PHYs that switch their host interface
- PHYs that may support some kind of rate adaption
- PCS/MACs that may support half-duplex at some speeds
- PCS/MACs that might support pause modes, and might support them only
  with certain interface modes

Layered on top of that is being able to determine which interface a PHY/
PCS/MAC should be using when e.g. a 10G copper PHY is inserted (which
could be inserted into a host which only supports up to 1G.)

I've spent considerable time trying to work out a solution to this, and
even before we had rate adaption, it isn't easy to solve. I've
experimented with several different solutions, and it's from numerous
trials that led to this host_interfaces/mac_capabilities structure -
but that still doesn't let us solve the problems I mention above since
we have no idea what the PHY itself is capable of, or how it's going to
behave, or really which interface modes it might switch between if it's
a clause 45 PHY.

I've experimented with adding phy->supported_interfaces so a phylib
driver can advertise what interfaces it supports. I've also
experimented with phy->possible_interfaces which reports the interface
modes that the PHY _is_ going to switch between having selected its
operating mode. I've not submitted them because even with this, it all
still seems rather inadequate - and there is a huge amount of work to
update all the phylib drivers to provide even that basic information,
let alone have much confidence that it is correct.

You can find these experiments, as normal, in my net-queue branch in
my git tree. These date from before we had rate adaption, so they take
no account of the recent addition of this extra variable.

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

Powered by blists - more mailing lists