Messages in this thread | | | Date | Wed, 7 Sep 2022 19:04:51 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE |
| |
On Wed, Sep 07, 2022 at 12:52:59PM -0400, Sean Anderson wrote: > On 9/7/22 5:38 AM, Russell King (Oracle) wrote: > > On Tue, Sep 06, 2022 at 12:18:45PM -0400, Sean Anderson wrote: > > > 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 <sean.anderson@seco.com> > > > --- > > > > > > (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.
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.
> > > + * 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.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |