On 9/7/22 2:04 PM, Russell King (Oracle) wrote:

On Wed, Sep 07, 2022 at 12:52:59PM -0400, Sean Anderson wrote:

quoted

On 9/7/22 5:38 AM, Russell King (Oracle) wrote:

quoted

On Tue, Sep 06, 2022 at 12:18:45PM -0400, Sean Anderson wrote:

quoted

This documents the possible MLO_PAUSE_* settings which can result from
different combinations of MLO_(A)SYM_PAUSE. These are more-or-less a
direct consequence of IEEE 802.3 Table 28B-2.

Signed-off-by: Sean Anderson <redacted>
---

(no changes since v3)

Changes in v3:
- New

   include/linux/phylink.h | 16 ++++++++++++++++
   1 file changed, 16 insertions(+)

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 6d06896fc20d..a431a0b0d217 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h

@@ -21,6 +21,22 @@ enum {
   	MLO_AN_FIXED,	/* Fixed-link mode */
   	MLO_AN_INBAND,	/* In-band protocol */
+	/* MAC_SYM_PAUSE and MAC_ASYM_PAUSE correspond to the PAUSE and
+	 * ASM_DIR bits used in autonegotiation, respectively. See IEEE 802.3

"used in our autonegotiation advertisement" would be more clear.

What else would it be (besides advertisement)? Regarding "our", these bits are
also set based on the link partner pause settings (e.g. by phylink_decode_c37_word).

No they aren't - MAC_(SYM|ASYM)_PAUSE are only the local side.
phylink_decode_c37_word() makes no use of these enums - it uses the
advertisement masks and decodes them to booleans, which are then used
to set MLO_PAUSE_TX and MLO_PAUSE_RX.

Sorry, I mistakenly conflated the two while reviewing things, but see below.

What I'm getting at is the comment is ambiguous.

MAC_(SYM|ASYM)_PAUSE are used to determine the values of PAUSE and
ASM_DIR bits in our local advertisement to the remote end.

I agree that this sentence is confusing. A more precise version of it might be

MAC_SYM_PAUSE and MAC_ASYM_PAUSE are used when configuring our autonegotiation
advertisement. They correspond to the PAUSE and ASM_DIR bits defined by 802.3,
respectively.

My intention here is to clarify the relationship between the terminology. Your
proposed modification has "our autonegotiation advertisement" apply to PAUSE/ASM_DIR
instead of MAC_*_PAUSE, which is also confusing, since those bits can apply to either
party's advertisement.

quoted

quoted

quoted

+	 * 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_TXRX,
+	 *                              MLO_PAUSE_RX

Any of none, tx, txrx and rx can occur with both bits set in the last
case, the tx-only case will be due to user configuration.

What flow did you have in mind? According to the comment on linkmode_set_pause,
if ethtool requests tx-only, we will use MAC_ASYM_PAUSE for the advertising,
which is the second row above.

I think you're missing some points on the ethtool interface. Let me
go through it:

First, let's go through the man page:

            autoneg on|off
                   Specifies whether pause autonegotiation should be enabled.

            rx on|off
                   Specifies whether RX pause should be enabled.

            tx on|off
                   Specifies whether TX pause should be enabled.

This is way too vague and doesn't convey very much inforamtion about
the function of these options. One can rightfully claim that it is
actually wrong and misleading, especially the first option, because
there is no way to control whether "pause autonegotiation should be
enabled." Either autonegotiation with the link partner is enabled
or it isn't.
  
Thankfully, the documentation of the field in struct
ethtool_pauseparam documents this more fully:

  * If @autoneg is non-zero, the MAC is configured to send and/or
  * receive pause frames according to the result of autonegotiation.
  * Otherwise, it is configured directly based on the @rx_pause and
  * @tx_pause flags.

So, autoneg controls whether the result of autonegotiation is used, or
we override the result of autonegotiation with the specified transmit
and receive settings.

The next issue with the man page is that it doesn't specify that tx
and rx control the advertisement of pause modes - and it doesn't
specify how. Again, the documentation of struct ethtool_pauseparam
helps somewhat:

  * If the link is autonegotiated, drivers should use
  * mii_advertise_flowctrl() or similar code to set the advertised
  * pause frame capabilities based on the @rx_pause and @tx_pause flags,
  * even if @autoneg is zero.  They should also allow the advertised
  * pause frame capabilities to be controlled directly through the
  * advertising field of &struct ethtool_cmd.

So:

1. in the case of autoneg=0:
1a. local end's enablement of tx and rx pause frames depends solely
     on the capabilities of the network adapter and the tx and rx
     parameters, ignoring the results of any autonegotiation
     resolution.
1b. the behaviour in mii_advertise_flowctrl() or similar code shall
     be used to derive the advertisement, which results in the
     tx=1 rx=0 case advertising ASYM_DIR only which does not tie up
     with what we actually end up configuring on the local end.

2. in the case of autoneg=1, the tx and rx parameters are used to
    derive the advertisement as in 1b and the results of
    autonegotiation resolution are used.

The full behaviour of mii_advertise_flowctrl() is:

ethtool  local advertisement	possible autoneg resolutions
  rx  tx  Pause AsymDir
  0   0   0     0		!tx !rx
  1   0   1     1		!tx !rx, !tx rx, tx rx
  0   1   0     1		!tx !rx, tx !rx
  1   1   1     0		!tx !rx, tx rx

but as I say, the "possible autoneg resolutions" and table 28B-3
is utterly meaningless when ethtool specifies "autoneg off" for
the pause settings.

So, "ethtool -A autoneg off tx on rx off" will result in an
advertisement with PAUSE=0 ASYM_DIR=1 and we force the local side
to enable transmit pause and disabel receive pause no matter what
the remote side's advertisement is.

I hope this clears the point up.

My intent here is to provide some help for driver authors when they
need to fill in their mac capabilities. The driver author probably
knows things like "My device supports MLO_PAUSE_TX and MLO_PAUSE_TXRX
but not MLO_PAUSE_RX." They have to translate that into the correct
values for MAC_*_PAUSE. When the user starts messing with this process,
it's no longer the driver author's problem whether the result is sane
or not.

How about

The following table lists the values of tx_pause and rx_pause which
might be requested in mac_link_up depending on the results of> autonegotiation (when MLO_PAUSE_AN is set):>
MAC_SYM_PAUSE MAC_ASYM_PAUSE tx_pause rx_pause
============= ============== ======== ========
            0              0        0        0
            0              1        0        0>                                     1        0
            1              0        0        0
                                    1        1>             1              1        0        0
                                    0        1
                                    1        1

When MLO_PAUSE_AN is not set, any combination of tx_pause and> rx_pause may be requested. This depends on user configuration,
without regard to the values of MAC_SYM_PAUSE and MAC_ASYM_PAUSE.

Perhaps there should be a note either here or in mac_link_up documenting
what to do if e.g. the user requests just MLO_PAUSE_TX but only symmetric
pause is supported. In mvneta_mac_link_up we enable symmetric pause if
either tx_pause or rx_pause is requested.

--Sean