| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3844f2a6-90fb-354e-ce88-0e9ff0a10475@seco.com>
Date: Thu, 21 Jul 2022 12:55:16 -0400
From: Sean Anderson <sean.anderson@...o.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Alexandru Marginean <alexandru.marginean@....com>,
Paolo Abeni <pabeni@...hat.com>,
"David S . Miller" <davem@...emloft.net>,
linux-kernel@...r.kernel.org, Vladimir Oltean <olteanv@...il.com>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH v2 08/11] net: phylink: Adjust advertisement based on rate
adaptation
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
Powered by blists - more mailing lists