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:   Mon, 18 Jul 2022 17:12:23 +0100
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
Message-ID: <YtWGZ4ZJ6rmLmlWk@shell.armlinux.org.uk>
References: <20220715215954.1449214-1-sean.anderson@seco.com>
 <20220715215954.1449214-11-sean.anderson@seco.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220715215954.1449214-11-sean.anderson@seco.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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;
> +		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.

It's only the PCS and MAC that care about this, so this should be dealt
with when calling into the PCS and MAC's link_up() method.

The problem we have are the legacy drivers (of which mv88e6xxx and
mtk_eth_soc are the only two I'm aware of) that make use of the
state->speed and state->duplex when configuring stuff. We could've been
down to just mv88e6xxx had the DSA and mv88e6xxx patches been sorted
out, but sadly that's now going to be some time off due to reviewer
failure.

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