Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation

[Sean Anderson <sean.anderson@seco.com>]

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