Messages in this thread | | | From | Sean Anderson <> | Subject | Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate adaptation | Date | Thu, 21 Jul 2022 12:55:16 -0400 |
| |
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
| |