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:   Wed, 20 Jul 2022 08:08:18 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Sean Anderson <sean.anderson@seco.com>
Cc:     netdev@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Alexandru Marginean <alexandru.marginean@nxp.com>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S . Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org, Vladimir Oltean <olteanv@gmail.com>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>
Subject: Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on
 rate adaptation
Message-ID: <Ytep4isHcwFM7Ctc@shell.armlinux.org.uk>
References: <20220719235002.1944800-1-sean.anderson@seco.com>
 <20220719235002.1944800-9-sean.anderson@seco.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220719235002.1944800-9-sean.anderson@seco.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Tue, Jul 19, 2022 at 07:49:58PM -0400, Sean Anderson wrote:
> +static int phylink_caps_to_speed(unsigned long caps)
> +{
> +	unsigned int max_cap = __fls(caps);
> +
> +	if (max_cap == __fls(MAC_10HD) || max_cap == __fls(MAC_10FD))
> +		return SPEED_10;
> +	if (max_cap == __fls(MAC_100HD) || max_cap == __fls(MAC_100FD))
> +		return SPEED_100;
> +	if (max_cap == __fls(MAC_1000HD) || max_cap == __fls(MAC_1000FD))
> +		return SPEED_1000;
> +	if (max_cap == __fls(MAC_2500FD))
> +		return SPEED_2500;
> +	if (max_cap == __fls(MAC_5000FD))
> +		return SPEED_5000;
> +	if (max_cap == __fls(MAC_10000FD))
> +		return SPEED_10000;
> +	if (max_cap == __fls(MAC_20000FD))
> +		return SPEED_20000;
> +	if (max_cap == __fls(MAC_25000FD))
> +		return SPEED_25000;
> +	if (max_cap == __fls(MAC_40000FD))
> +		return SPEED_40000;
> +	if (max_cap == __fls(MAC_50000FD))
> +		return SPEED_50000;
> +	if (max_cap == __fls(MAC_56000FD))
> +		return SPEED_56000;
> +	if (max_cap == __fls(MAC_100000FD))
> +		return SPEED_100000;
> +	if (max_cap == __fls(MAC_200000FD))
> +		return SPEED_200000;
> +	if (max_cap == __fls(MAC_400000FD))
> +		return SPEED_400000;
> +	return SPEED_UNKNOWN;
> +}

One of my recent patches introduced "phylink_caps_params" table into
the DSA code (which isn't merged) but it's about converting the caps
into the SPEED_* and DUPLEX_*. This is doing more or less the same
7thing but with a priority for speed rather than duplex. The question
about whether it should be this way for the DSA case or whether speed
should take priority was totally ignored by all reviewers of the code
despite being explicitly asked.

Maybe this could be reused here rather than having similar code.

> @@ -482,7 +529,39 @@ unsigned long phylink_get_capabilities(phy_interface_t interface,
>  		break;
>  	}
>  
> -	return caps & mac_capabilities;
> +	switch (rate_adaptation) {
> +	case RATE_ADAPT_NONE:
> +		break;
> +	case RATE_ADAPT_PAUSE: {
> +		/* The MAC must support asymmetric pause towards the local
> +		 * device for this. We could allow just symmetric pause, but
> +		 * then we might have to renegotiate if the link partner
> +		 * doesn't support pause.

Why do we need to renegotiate, and what would this achieve? The link
partner isn't going to say "oh yes I do support pause after all",
and in any case this function is working out what the capabilities
of the system is prior to bringing anything up.

All that we need to know here is whether the MAC supports receiving
pause frames from the PHY - if it doesn't, then the MAC is
incompatible with the PHY using rate adaption.

> +		 */
> +		if (!(mac_capabilities & MAC_SYM_PAUSE) ||
> +		    !(mac_capabilities & MAC_ASYM_PAUSE))
> +			break;
> +
> +		/* Can't adapt if the MAC doesn't support the interface's max
> +		 * speed
> +		 */
> +		if (state.speed != phylink_caps_to_speed(mac_capabilities))
> +			break;

I'm not sure this is the right way to check. If the MAC supports e.g.
10G, 1G, 100M and 10M, but we have a PHY operating in 1000base-X mode
to the PCS/MAC and is using rate adaption, then phylink_caps_to_speed()
will return 10G, but state.speed will be 1G.

Don't we instead want to check whether the MAC capabilities has the FD
bit corresponding to state.speed set?

> +
> +		adapted_caps = GENMASK(__fls(caps), __fls(MAC_10HD));
> +		/* We can't use pause frames in half-duplex mode */
> +		adapted_caps &= ~(MAC_1000HD | MAC_100HD | MAC_10HD);

Have you checked the PHY documentation to see what the behaviour is
in rate adaption mode with pause frames and it negotiates HD on the
media side? Does it handle the HD issue internally?

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