On 7/20/22 3:08 AM, Russell King (Oracle) wrote:

On Tue, Jul 19, 2022 at 07:49:58PM -0400, Sean Anderson wrote:

quoted

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

quoted

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

quoted

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

quoted

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