Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 6 Jan 2023 16:18:09 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     Sean Anderson <sean.anderson@seco.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>, netdev@vger.kernel.org,
        "David S . Miller" <davem@davemloft.net>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Tim Harvey <tharvey@gateworks.com>
Subject: Re: [PATCH net-next v5 4/4] phy: aquantia: Determine rate adaptation
 support from registers
Message-ID: <20230106141809.ohfbmptdnbtn4kfv@skbuf>
References: <20230103220511.3378316-1-sean.anderson@seco.com>
 <20230103220511.3378316-5-sean.anderson@seco.com>
 <20230105140421.bqd2aed6du5mtxn4@skbuf>
 <Y7bhctPZoyNnw1ay@shell.armlinux.org.uk>
 <20230105174342.jldjjisgzs6dmcpd@skbuf>
 <Y7ccNSSnPxTR2AQs@shell.armlinux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y7ccNSSnPxTR2AQs@shell.armlinux.org.uk>
Precedence: bulk

On Thu, Jan 05, 2023 at 06:51:33PM +0000, Russell King (Oracle) wrote:
> 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.

Don't we actually need an API for the PHY resembling the following?

struct phy_host_cfg {
	phy_interface_t interface;
	int rate_matching;
};

/* Caller must kfree() @host_cfg */
int phy_get_host_cfg_for_linkmode(struct phy_device *phydev,
				  enum ethtool_link_mode_bit_indices linkmode,
				  struct phy_host_cfg **host_cfg,
				  int *num_host_cfg)
{
	if (!phydev->drv->get_host_cfg_for_linkmode) {
		/* Assume that PHYs can't change host interface and don't
		 * support rate matching
		 */
		*host_cfg = kcalloc(sizeof(*host_cfg), GFP_KERNEL);
		*num_host_cfg = 1;
		*host_cfg[0].interface = phydev->interface;
		*host_cfg[0].rate_matching = RATE_MATCH_NONE;

		return 0;
	}

	return phydev->drv->get_host_cfg_for_linkmode(phydev, linkmode,
						      host_cfg, num_host_cfg);
}

/* Calling this is only necessary if @num_host_cfg returned by
 * phy_get_host_cfg_for_linkmode() is larger than 1.
 */
int phy_set_host_cfg_for_linkmode(struct phy_device *phydev,
				  enum ethtool_link_mode_bit_indices linkmode,
				  const struct phy_host_cfg *host_cfg)
{
	if (!phydev->drv->set_host_cfg_for_linkmode)
		return -EOPNOTSUPP;

	return phydev->drv->set_host_cfg_for_linkmode(phydev, linkmode,
						      host_cfg);
}

Based on the host_cfg array returned by the PHY for each link mode,
phylink could figure out (by intersecting with the MAC/PCS's
host_interfaces/mac_capabilities) what should be advertised and what
shouldn't.