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

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Sean Anderson <sean.anderson@seco.com>
Cc: "David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Madalin Bucur <madalin.bucur@nxp.com>,
	netdev@vger.kernel.org, Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Alexandru Marginean <alexandru.marginean@nxp.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Vladimir Oltean <olteanv@gmail.com>
Subject: Re: [PATCH net-next v3 10/47] net: phylink: Adjust link settings based on rate adaptation
Date: Mon, 18 Jul 2022 18:58:08 +0100	[thread overview]
Message-ID: <YtWfMGh39sFPtHJ7@shell.armlinux.org.uk> (raw)
In-Reply-To: <9a968425-5621-09b9-febe-2086d5492c96@seco.com>

On Mon, Jul 18, 2022 at 12:45:01PM -0400, Sean Anderson wrote:
> 
> 
> On 7/18/22 12:12 PM, Russell King (Oracle) wrote:
> > On Fri, Jul 15, 2022 at 05:59:17PM -0400, Sean Anderson wrote:
> >> If the phy is configured to use pause-based rate adaptation, ensure that
> >> the link is full duplex with pause frame reception enabled. Note that these
> >> settings may be overridden by ethtool.
> >> 
> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >> ---
> >> 
> >> Changes in v3:
> >> - New
> >> 
> >>  drivers/net/phy/phylink.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >> 
> >> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> >> index 7fa21941878e..7f65413aa778 100644
> >> --- a/drivers/net/phy/phylink.c
> >> +++ b/drivers/net/phy/phylink.c
> >> @@ -1445,6 +1445,10 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
> >>  	pl->phy_state.speed = phy_interface_speed(phydev->interface,
> >>  						  phydev->speed);
> >>  	pl->phy_state.duplex = phydev->duplex;
> >> +	if (phydev->rate_adaptation == RATE_ADAPT_PAUSE) {
> >> +		pl->phy_state.duplex = DUPLEX_FULL;
> 
> Just form context, as discussed with Andrew, this should never change
> anything. That is, it could be replaced with
> 
> WARN_ON_ONCE(pl->phy_state.duplex != DUPLEX_FULL);
> 
> Since the phy should never report that it is using rate_adaptation
> unless it is using full duplex.

The "rate adaption" thing tends not to be a result of negotiation with
the link partner, but more a configuration issue. At least that is the
case with 88x3310 PHYs. There is no mention of any kind of restriction
on duplex when operating in rate adaption mode (whether it's the MACSEC
version that can generate pause frames, or the non-MACSEC that can't.)

> >> +		rx_pause = true;
> >> +	}
> > 
> > I really don't like this - as I've pointed out in my previous email, the
> > reporting in the kernel message log for "Link is Up" will be incorrect
> > if you force the phy_state here like this. If the media side link has
> > been negotiated to be half duplex, we should state that in the "Link is
> > Up" message.
> 
> So I guess the question here is whether there are phys which do duplex
> adaptation. There definitely are phys which support a half-duplex
> interface mode and a full duplex link mode (such as discussed in patch 08/47).
> If it's important to get this right, I can do the same treatment for duplex
> as I did for speed.

I guess it's something we don't know.

The sensible thing is not to add a WARN_ON() for the case, but to
restrict the PHY advertisement so the half-duplex case can't happen if
the host link is operating in a mode that requires rate adaption to
gain the other speeds.

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