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;
From:   Sean Anderson <sean.anderson@seco.com>
Subject: Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate
 adaptation
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
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>
References: <20220719235002.1944800-1-sean.anderson@seco.com>
 <20220719235002.1944800-9-sean.anderson@seco.com>
 <Ytep4isHcwFM7Ctc@shell.armlinux.org.uk>
Message-ID: <3844f2a6-90fb-354e-ce88-0e9ff0a10475@seco.com>
Date:   Thu, 21 Jul 2022 12:55:16 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
In-Reply-To: <Ytep4isHcwFM7Ctc@shell.armlinux.org.uk>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OWtGMjgzTXd4aG9pL0dXZ2xqK3FjZzhmUG9vR2ViTm0xOXdmOFBseGdZNVIx?=
 =?utf-8?B?MjVVenRPSThTRVNsUmRaMWdwekN4RHJqWVBUdmEvMmhPRWx6NG5sV3B6SC9z?=
 =?utf-8?B?Tk13cDZMUnNHYTZmamlhbG03VjVuNzFXNXBlWXhKMGlyZHk3aklxUlpocjhs?=
 =?utf-8?B?Y2d1VnNQa2VHOUFLVVEwdjFwaDROKytEL00zQ0s0eTB3QkFWNzZiWVl4Vk0y?=
 =?utf-8?B?bEszMlY0dm5LVVJmM0k5UmZ3WmtuRXBldUlKSUJQanZoTTc0WlprSm54ZWFn?=
 =?utf-8?B?TFlQTzIrT2kyNWovT2NUeFEydHNaT3JQK25BZ1RDbVNtYW9pbDZlUGpnYUJ3?=
 =?utf-8?B?c2pqTGlqNkdCR2FaTVNITXRWRWNIMU4rQ3FDZnR2WTRZeTBweWFrd0gwbUNk?=
 =?utf-8?B?RStrdzd4c3Y3SEptak8vVGFmc29FOEYzbFFJUkVtWm90MmYwZ001bC9iOUNy?=
 =?utf-8?B?NnpZM0JTVkhjaUhFckdQNTlDR0RpN3NXN2hkMXlDcFBhWUphMlY5OEVyam52?=
 =?utf-8?B?OHUxZlZmMTdDeWhzZ0lwNVpsY3JJL2FrSy8xNU1aUjNEZFVMRmhZdjF6MDJ2?=
 =?utf-8?B?Qmd2cnBsSmdEOFdsaVloY1p5cUpyVDQrKzdjVTRXYXhUd0dLd1RIRUlRcWJz?=
 =?utf-8?B?TzNTbjBxOEU5UVdMOFRjd1RxMHpoOXkzYzA2dVdBajE5WVg4N1htM0p2SmU1?=
 =?utf-8?B?TUpJZTR2SkN0TnFpcjAvMDdLSGZEUFRoVWQxQStWOTdQRXZTY29EcFNOOXNm?=
 =?utf-8?B?VG5Ec2FtUnVENzFoWUVQQkxjQ1h5VldWRDEzZjVTOFdqNU5KOG55enJvSm9U?=
 =?utf-8?B?c20zTEdseTQ3cVc2cXpDUEtUK2p6aE1YZGErSmJhVlZOK3VxejljZjFKVmcx?=
 =?utf-8?B?T3ZvWlByUmxEM21tL2tyRS9neDlYK2ZRZ09iOExzalc3Z0djYWdGYkIyTzdk?=
 =?utf-8?B?Kzh2SklwMmZ2ZWZ6RWhQRHNIdCt2Z2JRcHcxNCsxUHh0L3NUdVRnRGtSc0hr?=
 =?utf-8?B?dUU1emViZlloZk5tbmFtSGRISW9ETG1UVktDSkczeEZjUm56RUw0M0ZiWjNy?=
 =?utf-8?B?RXpobmYrYjllWWp0cUtrMFRDK1hJdnJMRDd2bVE3V2YwT1FrdGF5ZkhWQThO?=
 =?utf-8?B?dzkwT2lYWFNXQnRsUjZjVGxUV01obzNUWVFmbVpFYVhwN01UUkk4WHMwMTYx?=
 =?utf-8?B?TVpuM2ZlN2NrU3J0ZlI0cldRV1RCcW1kNVBsRjV3NzRlNnNsaFNNaVp2anJP?=
 =?utf-8?B?b1c3ZWpiaGhBVE5HbDdoYXc5OTZkK2JHRjJOVVE5VUplTUFiQU9PV2lzdWZF?=
 =?utf-8?B?Zm1BbTExMGszNGFVbmpxeHFhY1hIMy9QMmJLL2tLQXRHR2lqZXhVZXdkanNT?=
 =?utf-8?B?VjQ2c2QrdUloUUZ5cFFwd0tKVEZhVnNTUWxPME9QU3NLbWlhMmlFd0YxbDg1?=
 =?utf-8?B?Q0FKdHpWdjZjSkdUbnpSbWt2MWZCZmx5VG1uTCtlcENCOEwrRXVOZ1JwbkZx?=
 =?utf-8?B?Z3lQVmVOWDhIWE8ySWQ2K2dvTGxCTWR1UFNNVVpvdmRtaEF5ajA0UHFsSVdo?=
 =?utf-8?B?bkwvZlJzaFMzK1hYME9JdUprTDlKQkw3eVl3K2RGb01EYWtXWEZ6QXZGYVo1?=
 =?utf-8?B?cFFMU0J0dFlHWiszZTUwRm1PUUxXWVVTWktnbUxrWTFXMzNINVZhOEo2dWpr?=
 =?utf-8?B?bWNCZWx3aktoTzBXMWNiai9jS0VJNm9VTHBBTGZsbjdOeXFWWnQrRFJ2MEk2?=
 =?utf-8?B?cE5CQlJmSFF5RFExcCt4bDF4TkNtK1ZHOG9EU21UNEIwZXhLRmtQTGhMd3J4?=
 =?utf-8?B?R1lsWTIyblAwVEEzazVKQ3l5YjE3cjBoZjhoY01uTHlzSnp3NU9IaVpIbkZ6?=
 =?utf-8?B?cTV1VWFnU1I4NzlrbVVFVllxb0Y2RmhzL0xQeFUwUlFKZHN6RDJITVRQRWRN?=
 =?utf-8?B?b3VQNmhGbjdIdFltKzZqdGlFaStrN3pKc3hXS1BXV1dBL09lblZDdFloNTVj?=
 =?utf-8?B?OXQ0N3lLZWZMaktDNjdDeE9hS1VUTDk3UWtRNE5DOGszVHo2d2FmN2tCQ3Uv?=
 =?utf-8?B?TmdsMnJTQkJTSmE1bzE4NEtVOUlTSTE0RWZseFRYcm9hQjFoRkxLUnhvcGhB?=
 =?utf-8?B?UElCTkdpcTFka2ovekU5TDAyUk5PRVNQNCt1Qk1PUGFWcGxYWmd6ZjI4LzFt?=
 =?utf-8?B?MGc9PQ==?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4fb27086-8960-49a5-f659-08da6b39cc6d
X-MS-Exchange-CrossTenant-AuthSource: DB7PR03MB4972.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jul 2022 16:55:21.5444
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WcpMQrGM5kjCzozWDxbUh/T/VGXaIGtr0QCbNPg6VHKiBTSPEmlby5C+Si+l7utblmpDeEzfT5pFF96r/AMHug==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR03MB3030
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 7/20/22 3:08 AM, Russell King (Oracle) wrote:
> 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.

I'm in favor of that.

>> @@ -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.

AIUI, MAC_SYM_PAUSE and MAC_ASYM_PAUSE correspond to the PAUSE and
ASM_DIR bits used in autonegotiation. For reference, Table 28B-2 from
802.3 is:

PAUSE (A5) ASM_DIR (A6) Capability
========== ============ ================================================
         0            0 No PAUSE
         0            1 Asymmetric PAUSE toward link partner
         1            0 Symmetric PAUSE
	 1            1 Both Symmetric PAUSE and Asymmetric PAUSE toward
                        local device

These correspond to the following valid values for MLO_PAUSE:

MAC_SYM_PAUSE MAC_ASYM_PAUSE Valid pause modes
============= ============== ==============================
            0              0 MLO_PAUSE_NONE
            0              1 MLO_PAUSE_NONE, MLO_PAUSE_TX
            1              0 MLO_PAUSE_NONE, MLO_PAUSE_TXRX
	    1              1 MLO_PAUSE_NONE, MLO_PAUSE_RX,
                             MLO_PAUSE_TXRX

In order to support pause-based rate adaptation, we need MLO_PAUSE_RX to
be valid. This rules out the top two rows. In the bottom mode, we can
enable MLO_PAUSE_RX without MLO_PAUSE_TX. Whatever our link partner
supports, we can still enable it. For the third row, however, we can
only enable MLO_PAUSE_RX if we also enable MLO_PAUSE_TX. This can be a
problem if the link partner does not support pause frames (or the user
has disabled MLO_PAUSE_AN and MLO_PAUSE_TX). So if we were to enable
advertisement of pause-based, rate-adapted modes when only MAC_SYM_PAUSE
was present, then we might end up in a situation where we'd have to
renegotiate without those modes in order to get a valid link state. I
don't want to have to implement that, so for now we only advertise
pause-based, rate-adapted modes if we support MLO_PAUSE_RX without
MLO_PAUSE_TX.

>> +		 */
>> +		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?

Yes, that seems correct.

>> +
>> +		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?

It's not documented. This is just conservative. Presumably, there exists
(or could exist) a duplex-adapting phy, but I don't know if I have one.

--Sean