lkml.org 
[lkml]   [2022]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation
Date


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

\
 
 \ /
  Last update: 2022-07-21 18:57    [W:0.224 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site