[PATCH net-next v5 0/8] net: phy: Add support for rate adaptation

[PATCH net-next v5 0/8] net: phy: Add support for rate adaptation

[Sean Anderson]

This adds support for phy rate adaptation: when a phy adapts between
differing phy interface and link speeds. It was originally submitted as
part of [1], which is considered "v1" of this series.

Several past discussions [2-4] around adding rate adaptation provide
some context.

Although in earlier versions of this series, userspace could disable
rate adaptation, now it is only possible to determine the current rate
adaptation type. Disabling or otherwise configuring rate adaptation has
been left for future work. However, because currently only
RATE_ADAPT_PAUSE is implemented, it is possible to disable rate
adaptation by modifying the advertisement appropriately.

[1] https://lore.kernel.org/netdev/20220715215954.1449214-1-sean.anderson@seco.com/T/#t
[2] https://lore.kernel.org/netdev/1579701573-6609-1-git-send-email-madalin.bucur@oss.nxp.com/
[3] https://lore.kernel.org/netdev/1580137671-22081-1-git-send-email-madalin.bucur@oss.nxp.com/
[4] https://lore.kernel.org/netdev/20200116181933.32765-1-olteanv@gmail.com/

Changes in v5:
- Document phy_rate_adaptation_to_str
- Remove unnecessary comma
- Move phylink_cap_from_speed_duplex to this commit
- Drop patch "Add some helpers for working with mac caps"; it has been
  incorperated into the autonegotiation patch.
- Break off patch "net: phy: Add 1000BASE-KX interface mode" for
  separate submission.
- Rebase onto net-next/master

Changes in v4:
- Export phy_rate_adaptation_to_str
- Remove phylink_interface_max_speed, which was accidentally added
- Split off the LS1046ARDB 1G fix

Changes in v3:
- Document MAC_(A)SYM_PAUSE
- Modify link settings directly in phylink_link_up, instead of doing
  things more indirectly via link_*.
- Add phylink_cap_from_speed_duplex to look up the mac capability
  corresponding to the interface's speed.
- Include RATE_ADAPT_CRS; it's a few lines and it doesn't hurt.
- Move unused defines to next commit (where they will be used)
- Remove "Support differing link/interface speed/duplex". It has been
  rendered unnecessary due to simplification of the rate adaptation
  patches. Thanks Russell!
- Rewrite cover letter to better reflect the opinions of the developers
  involved

Changes in v2:
- Use int/defines instead of enum to allow for use in ioctls/netlink
- Add locking to phy_get_rate_adaptation
- Add (read-only) ethtool support for rate adaptation
- Move part of commit message to cover letter, as it gives a good
  overview of the whole series, and allows this patch to focus more on
  the specifics.
- Use the phy's rate adaptation setting to determine whether to use its
  link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND.
- Always use the rate adaptation setting to determine the interface
  speed/duplex (instead of sometimes using the interface mode).
- Determine the interface speed and max mac speed directly instead of
  guessing based on the caps.
- Add comments clarifying the register defines
- Reorder variables in aqr107_read_rate

Sean Anderson (8):
  net: phylink: Document MAC_(A)SYM_PAUSE
  net: phylink: Export phylink_caps_to_linkmodes
  net: phylink: Generate caps and convert to linkmodes separately
  net: phy: Add support for rate adaptation
  net: phylink: Adjust link settings based on rate adaptation
  net: phylink: Adjust advertisement based on rate adaptation
  net: phy: aquantia: Add some additional phy interfaces
  net: phy: aquantia: Add support for rate adaptation

 Documentation/networking/ethtool-netlink.rst |   2 +
 drivers/net/phy/aquantia_main.c              |  68 ++++-
 drivers/net/phy/phy-core.c                   |  21 ++
 drivers/net/phy/phy.c                        |  28 ++
 drivers/net/phy/phylink.c                    | 270 +++++++++++++++++--
 include/linux/phy.h                          |  22 +-
 include/linux/phylink.h                      |  27 +-
 include/uapi/linux/ethtool.h                 |  18 +-
 include/uapi/linux/ethtool_netlink.h         |   1 +
 net/ethtool/ioctl.c                          |   1 +
 net/ethtool/linkmodes.c                      |   5 +
 11 files changed, 429 insertions(+), 34 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Sean Anderson]

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
+	 * Annex 28B for more information.
+	 *
+	 * The following table lists the values of MLO_PAUSE_* (aside from
+	 * MLO_PAUSE_AN) which might be requested depending on the results of
+	 * autonegotiation or user configuration:
+	 *
+	 * 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
+	 */
 	MAC_SYM_PAUSE	= BIT(0),
 	MAC_ASYM_PAUSE	= BIT(1),
 	MAC_10HD	= BIT(2),
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next v5 2/8] net: phylink: Export phylink_caps_to_linkmodes

[Sean Anderson]

This function is convenient for MAC drivers. They can use it to add or
remove particular link modes based on capabilities (such as if half
duplex is not supported for a particular interface mode).

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

(no changes since v1)

 drivers/net/phy/phylink.c | 12 ++++++++++--
 include/linux/phylink.h   |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e9d62f9598f9..c5c3f0b62d7f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -155,8 +155,15 @@ static const char *phylink_an_mode_str(unsigned int mode)
 	return mode < ARRAY_SIZE(modestr) ? modestr[mode] : "unknown";
 }
 
-static void phylink_caps_to_linkmodes(unsigned long *linkmodes,
-				      unsigned long caps)
+/**
+ * phylink_caps_to_linkmodes() - Convert capabilities to ethtool link modes
+ * @linkmodes: ethtool linkmode mask (must be already initialised)
+ * @caps: bitmask of MAC capabilities
+ *
+ * Set all possible pause, speed and duplex linkmodes in @linkmodes that are
+ * supported by the @caps. @linkmodes must have been initialised previously.
+ */
+void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps)
 {
 	if (caps & MAC_SYM_PAUSE)
 		__set_bit(ETHTOOL_LINK_MODE_Pause_BIT, linkmodes);
@@ -295,6 +302,7 @@ static void phylink_caps_to_linkmodes(unsigned long *linkmodes,
 		__set_bit(ETHTOOL_LINK_MODE_400000baseCR4_Full_BIT, linkmodes);
 	}
 }
+EXPORT_SYMBOL_GPL(phylink_caps_to_linkmodes);
 
 /**
  * phylink_get_linkmodes() - get acceptable link modes
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index a431a0b0d217..9bb088e0ef3e 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -534,6 +534,7 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 		 phy_interface_t interface, int speed, int duplex);
 #endif
 
+void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps);
 void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
 			   unsigned long mac_capabilities);
 void phylink_generic_validate(struct phylink_config *config,
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next v5 3/8] net: phylink: Generate caps and convert to linkmodes separately

[Sean Anderson]

If we call phylink_caps_to_linkmodes directly from
phylink_get_linkmodes, it is difficult to re-use this functionality in
MAC drivers. This is because MAC drivers must then work with an ethtool
linkmode bitmap, instead of with mac capabilities. Instead, let the
caller of phylink_get_linkmodes do the conversion. To reflect this
change, rename the function to phylink_get_capabilities.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

(no changes since v1)

 drivers/net/phy/phylink.c | 21 +++++++++++----------
 include/linux/phylink.h   |  4 ++--
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index c5c3f0b62d7f..1f022e5d01ba 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -305,17 +305,15 @@ void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps)
 EXPORT_SYMBOL_GPL(phylink_caps_to_linkmodes);
 
 /**
- * phylink_get_linkmodes() - get acceptable link modes
- * @linkmodes: ethtool linkmode mask (must be already initialised)
+ * phylink_get_capabilities() - get capabilities for a given MAC
  * @interface: phy interface mode defined by &typedef phy_interface_t
  * @mac_capabilities: bitmask of MAC capabilities
  *
- * Set all possible pause, speed and duplex linkmodes in @linkmodes that
- * are supported by the @interface mode and @mac_capabilities. @linkmodes
- * must have been initialised previously.
+ * Get the MAC capabilities that are supported by the @interface mode and
+ * @mac_capabilities.
  */
-void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
-			   unsigned long mac_capabilities)
+unsigned long phylink_get_capabilities(phy_interface_t interface,
+				       unsigned long mac_capabilities)
 {
 	unsigned long caps = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
 
@@ -391,9 +389,9 @@ void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
 		break;
 	}
 
-	phylink_caps_to_linkmodes(linkmodes, caps & mac_capabilities);
+	return caps & mac_capabilities;
 }
-EXPORT_SYMBOL_GPL(phylink_get_linkmodes);
+EXPORT_SYMBOL_GPL(phylink_get_capabilities);
 
 /**
  * phylink_generic_validate() - generic validate() callback implementation
@@ -409,11 +407,14 @@ void phylink_generic_validate(struct phylink_config *config,
 			      unsigned long *supported,
 			      struct phylink_link_state *state)
 {
+	unsigned long caps;
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 
 	phylink_set_port_modes(mask);
 	phylink_set(mask, Autoneg);
-	phylink_get_linkmodes(mask, state->interface, config->mac_capabilities);
+	caps = phylink_get_capabilities(state->interface,
+					config->mac_capabilities);
+	phylink_caps_to_linkmodes(mask, caps);
 
 	linkmode_and(supported, supported, mask);
 	linkmode_and(state->advertising, state->advertising, mask);
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 9bb088e0ef3e..c2aa49c692a0 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -535,8 +535,8 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 #endif
 
 void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps);
-void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
-			   unsigned long mac_capabilities);
+unsigned long phylink_get_capabilities(phy_interface_t interface,
+				       unsigned long mac_capabilities);
 void phylink_generic_validate(struct phylink_config *config,
 			      unsigned long *supported,
 			      struct phylink_link_state *state);
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next v5 4/8] net: phy: Add support for rate adaptation

[Sean Anderson]

This adds support for rate adaptation to the phy subsystem. The general
idea is that the phy interface runs at one speed, and the MAC throttles
the rate at which it sends packets to the link speed. There's a good
overview of several techniques for achieving this at [1]. This patch
adds support for three: pause-frame based (such as in Aquantia phys),
CRS-based (such as in 10PASS-TS and 2BASE-TL), and open-loop-based (such
as in 10GBASE-W).

This patch makes a few assumptions and a few non assumptions about the
types of rate adaptation available. First, it assumes that different phys
may use different forms of rate adaptation. Second, it assumes that phys
can use rate adaptation for any of their supported link speeds (e.g. if a
phy supports 10BASE-T and XGMII, then it can adapt XGMII to 10BASE-T).
Third, it does not assume that all interface modes will use the same form
of rate adaptation. Fourth, it does not assume that all phy devices will
support rate adaptation (even some do). Relaxing or strengthening these
(non-)assumptions could result in a different API. For example, if all
interface modes were assumed to use the same form of rate adaptation, then
a bitmask of interface modes supportting rate adaptation would suffice.

For some better visibility into the process, the current rate adaptation
mode is exposed as part of the ethtool ksettings. For the moment, only
read access is supported. I'm not sure what userspace might want to
configure yet (disable it altogether, disable just one mode, specify the
mode to use, etc.). For the moment, since only pause-based rate
adaptation support is added in the next few commits, rate adaptation can
be disabled altogether by adjusting the advertisement.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
Should the unimplemented adaptation modes be kept in?

Changes in v5:
- Document phy_rate_adaptation_to_str
- Remove unnecessary comma

Changes in v4:
- Export phy_rate_adaptation_to_str

Changes in v2:
- Use int/defines instead of enum to allow for use in ioctls/netlink
- Add locking to phy_get_rate_adaptation
- Add (read-only) ethtool support for rate adaptation
- Move part of commit message to cover letter, as it gives a good
  overview of the whole series, and allows this patch to focus more on
  the specifics.

 Documentation/networking/ethtool-netlink.rst |  2 ++
 drivers/net/phy/phy-core.c                   | 21 +++++++++++++++
 drivers/net/phy/phy.c                        | 28 ++++++++++++++++++++
 include/linux/phy.h                          | 22 ++++++++++++++-
 include/uapi/linux/ethtool.h                 | 18 +++++++++++--
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/ioctl.c                          |  1 +
 net/ethtool/linkmodes.c                      |  5 ++++
 8 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index dbca3e9ec782..65ed29e78499 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -426,6 +426,7 @@ Kernel response contents:
   ``ETHTOOL_A_LINKMODES_DUPLEX``              u8      duplex mode
   ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG``    u8      Master/slave port mode
   ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE``  u8      Master/slave port state
+  ``ETHTOOL_A_LINKMODES_RATE_ADAPTATION``     u8      PHY rate adaptation
   ==========================================  ======  ==========================
 
 For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask
@@ -449,6 +450,7 @@ Request contents:
   ``ETHTOOL_A_LINKMODES_SPEED``               u32     link speed (Mb/s)
   ``ETHTOOL_A_LINKMODES_DUPLEX``              u8      duplex mode
   ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG``    u8      Master/slave port mode
+  ``ETHTOOL_A_LINKMODES_RATE_ADAPTATION``     u8      PHY rate adaptation
   ``ETHTOOL_A_LINKMODES_LANES``               u32     lanes
   ==========================================  ======  ==========================
 
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 2a2924bc8f76..b1c9c40fe378 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -74,6 +74,27 @@ const char *phy_duplex_to_str(unsigned int duplex)
 }
 EXPORT_SYMBOL_GPL(phy_duplex_to_str);
 
+/**
+ * phy_rate_adaptation_to_str - Return a string describing the rate adaptation
+ *
+ * @rate_adaptation: Type of rate adaptation to describe
+ */
+const char *phy_rate_adaptation_to_str(int rate_adaptation)
+{
+	switch (rate_adaptation) {
+	case RATE_ADAPT_NONE:
+		return "none";
+	case RATE_ADAPT_PAUSE:
+		return "pause";
+	case RATE_ADAPT_CRS:
+		return "crs";
+	case RATE_ADAPT_OPEN_LOOP:
+		return "open-loop";
+	}
+	return "Unsupported (update phy-core.c)";
+}
+EXPORT_SYMBOL_GPL(phy_rate_adaptation_to_str);
+
 /**
  * phy_interface_num_ports - Return the number of links that can be carried by
  *			     a given MAC-PHY physical link. Returns 0 if this is
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8d3ee3a6495b..77cbf07852e6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -114,6 +114,33 @@ void phy_print_status(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_print_status);
 
+/**
+ * phy_get_rate_adaptation - determine if rate adaptation is supported
+ * @phydev: The phy device to return rate adaptation for
+ * @iface: The interface mode to use
+ *
+ * This determines the type of rate adaptation (if any) that @phy supports
+ * using @iface. @iface may be %PHY_INTERFACE_MODE_NA to determine if any
+ * interface supports rate adaptation.
+ *
+ * Return: The type of rate adaptation @phy supports for @iface, or
+ *         %RATE_ADAPT_NONE.
+ */
+int phy_get_rate_adaptation(struct phy_device *phydev,
+			    phy_interface_t iface)
+{
+	int ret = RATE_ADAPT_NONE;
+
+	if (phydev->drv->get_rate_adaptation) {
+		mutex_lock(&phydev->lock);
+		ret = phydev->drv->get_rate_adaptation(phydev, iface);
+		mutex_unlock(&phydev->lock);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_get_rate_adaptation);
+
 /**
  * phy_config_interrupt - configure the PHY device for the requested interrupts
  * @phydev: the phy_device struct
@@ -256,6 +283,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
 	cmd->base.duplex = phydev->duplex;
 	cmd->base.master_slave_cfg = phydev->master_slave_get;
 	cmd->base.master_slave_state = phydev->master_slave_state;
+	cmd->base.rate_adaptation = phydev->rate_adaptation;
 	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
 		cmd->base.port = PORT_BNC;
 	else
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 337230c135f7..3e784137b2f3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -280,7 +280,6 @@ static inline const char *phy_modes(phy_interface_t interface)
 	}
 }
 
-
 #define PHY_INIT_TIMEOUT	100000
 #define PHY_FORCE_TIMEOUT	10
 
@@ -574,6 +573,7 @@ struct macsec_ops;
  * @lp_advertising: Current link partner advertised linkmodes
  * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
  * @autoneg: Flag autoneg being used
+ * @rate_adaptation: Current rate adaptation mode
  * @link: Current link state
  * @autoneg_complete: Flag auto negotiation of the link has completed
  * @mdix: Current crossover
@@ -641,6 +641,8 @@ struct phy_device {
 	unsigned irq_suspended:1;
 	unsigned irq_rerun:1;
 
+	int rate_adaptation;
+
 	enum phy_state state;
 
 	u32 dev_flags;
@@ -805,6 +807,21 @@ struct phy_driver {
 	 */
 	int (*get_features)(struct phy_device *phydev);
 
+	/**
+	 * @get_rate_adaptation: Get the supported type of rate adaptation for a
+	 * particular phy interface. This is used by phy consumers to determine
+	 * whether to advertise lower-speed modes for that interface. It is
+	 * assumed that if a rate adaptation mode is supported on an interface,
+	 * then that interface's rate can be adapted to all slower link speeds
+	 * supported by the phy. If iface is %PHY_INTERFACE_MODE_NA, and the phy
+	 * supports any kind of rate adaptation for any interface, then it must
+	 * return that rate adaptation mode (preferring %RATE_ADAPT_PAUSE to
+	 * %RATE_ADAPT_CRS). If the interface is not supported, this should
+	 * return %RATE_ADAPT_NONE.
+	 */
+	int (*get_rate_adaptation)(struct phy_device *phydev,
+				   phy_interface_t iface);
+
 	/* PHY Power Management */
 	/** @suspend: Suspend the hardware, saving state if needed */
 	int (*suspend)(struct phy_device *phydev);
@@ -971,6 +988,7 @@ struct phy_fixup {
 
 const char *phy_speed_to_str(int speed);
 const char *phy_duplex_to_str(unsigned int duplex);
+const char *phy_rate_adaptation_to_str(int rate_adaptation);
 
 int phy_interface_num_ports(phy_interface_t interface);
 
@@ -1687,6 +1705,8 @@ int phy_disable_interrupts(struct phy_device *phydev);
 void phy_request_interrupt(struct phy_device *phydev);
 void phy_free_interrupt(struct phy_device *phydev);
 void phy_print_status(struct phy_device *phydev);
+int phy_get_rate_adaptation(struct phy_device *phydev,
+			    phy_interface_t iface);
 void phy_set_max_speed(struct phy_device *phydev, u32 max_speed);
 void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode);
 void phy_advertise_supported(struct phy_device *phydev);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 2d5741fd44bb..49496acbeac9 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1840,6 +1840,20 @@ static inline int ethtool_validate_duplex(__u8 duplex)
 #define MASTER_SLAVE_STATE_SLAVE		3
 #define MASTER_SLAVE_STATE_ERR			4
 
+/* These are used to throttle the rate of data on the phy interface when the
+ * native speed of the interface is higher than the link speed. These should
+ * not be used for phy interfaces which natively support multiple speeds (e.g.
+ * MII or SGMII).
+ */
+/* No rate adaptation performed. */
+#define RATE_ADAPT_NONE		0
+/* The phy sends pause frames to throttle the MAC. */
+#define RATE_ADAPT_PAUSE	1
+/* The phy asserts CRS to prevent the MAC from transmitting. */
+#define RATE_ADAPT_CRS		2
+/* The MAC is programmed with a sufficiently-large IPG. */
+#define RATE_ADAPT_OPEN_LOOP	3
+
 /* Which connector port. */
 #define PORT_TP			0x00
 #define PORT_AUI		0x01
@@ -2033,8 +2047,8 @@ enum ethtool_reset_flags {
  *	reported consistently by PHYLIB.  Read-only.
  * @master_slave_cfg: Master/slave port mode.
  * @master_slave_state: Master/slave port state.
+ * @rate_adaptation: Rate adaptation performed by the PHY
  * @reserved: Reserved for future use; see the note on reserved space.
- * @reserved1: Reserved for future use; see the note on reserved space.
  * @link_mode_masks: Variable length bitmaps.
  *
  * If autonegotiation is disabled, the speed and @duplex represent the
@@ -2085,7 +2099,7 @@ struct ethtool_link_settings {
 	__u8	transceiver;
 	__u8	master_slave_cfg;
 	__u8	master_slave_state;
-	__u8	reserved1[1];
+	__u8	rate_adaptation;
 	__u32	reserved[7];
 	__u32	link_mode_masks[];
 	/* layout of link_mode_masks fields:
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index d2fb4f7be61b..3a5d81769ff4 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -242,6 +242,7 @@ enum {
 	ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,	/* u8 */
 	ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,	/* u8 */
 	ETHTOOL_A_LINKMODES_LANES,		/* u32 */
+	ETHTOOL_A_LINKMODES_RATE_ADAPTATION,	/* u8 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_LINKMODES_CNT,
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 9298eb3251cb..6166f3e6b8d5 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -571,6 +571,7 @@ static int ethtool_get_link_ksettings(struct net_device *dev,
 		= __ETHTOOL_LINK_MODE_MASK_NU32;
 	link_ksettings.base.master_slave_cfg = MASTER_SLAVE_CFG_UNSUPPORTED;
 	link_ksettings.base.master_slave_state = MASTER_SLAVE_STATE_UNSUPPORTED;
+	link_ksettings.base.rate_adaptation = RATE_ADAPT_NONE;
 
 	return store_link_ksettings_for_user(useraddr, &link_ksettings);
 }
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index 99b29b4fe947..7905bd985c7f 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -70,6 +70,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
 		+ nla_total_size(sizeof(u32)) /* LINKMODES_SPEED */
 		+ nla_total_size(sizeof(u32)) /* LINKMODES_LANES */
 		+ nla_total_size(sizeof(u8)) /* LINKMODES_DUPLEX */
+		+ nla_total_size(sizeof(u8)) /* LINKMODES_RATE_ADAPTATION */
 		+ 0;
 	ret = ethnl_bitset_size(ksettings->link_modes.advertising,
 				ksettings->link_modes.supported,
@@ -143,6 +144,10 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
 		       lsettings->master_slave_state))
 		return -EMSGSIZE;
 
+	if (nla_put_u8(skb, ETHTOOL_A_LINKMODES_RATE_ADAPTATION,
+		       lsettings->rate_adaptation))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next v5 5/8] net: phylink: Adjust link settings based on rate adaptation

[Sean Anderson]

If the phy is configured to use pause-based rate adaptation, ensure that
the link is full duplex with pause frame reception enabled. As
suggested, if pause-based rate adaptation is enabled by the phy, then
pause reception is unconditionally enabled.

The interface duplex is determined based on the rate adaptation type.
When rate adaptation is enabled, so is the speed. We assume the maximum
interface speed is used. This is only relevant for MLO_AN_PHY. For
MLO_AN_INBAND, the MAC/PCS's view of the interface speed will be used.

Although there are no RATE_ADAPT_CRS phys in-tree, it has been added for
comparison (and the implementation is quite simple).

Co-developed-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
Russell, I need your SoB as well as RB, since you wrote some of this.

(no changes since v4)

Changes in v4:
- Remove phylink_interface_max_speed, which was accidentally added

Changes in v3:
- Modify link settings directly in phylink_link_up, instead of doing
  things more indirectly via link_*.

Changes in v2:
- Use the phy's rate adaptation setting to determine whether to use its
  link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND.
- Always use the rate adaptation setting to determine the interface
  speed/duplex (instead of sometimes using the interface mode).

 drivers/net/phy/phylink.c | 137 ++++++++++++++++++++++++++++++++++----
 include/linux/phylink.h   |   5 ++
 2 files changed, 130 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 1f022e5d01ba..8ce110c7be5c 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -155,6 +155,75 @@ static const char *phylink_an_mode_str(unsigned int mode)
 	return mode < ARRAY_SIZE(modestr) ? modestr[mode] : "unknown";
 }
 
+/**
+ * phylink_interface_max_speed() - get the maximum speed of a phy interface
+ * @interface: phy interface mode defined by &typedef phy_interface_t
+ *
+ * Determine the maximum speed of a phy interface. This is intended to help
+ * determine the correct speed to pass to the MAC when the phy is performing
+ * rate adaptation.
+ *
+ * Return: The maximum speed of @interface
+ */
+static int phylink_interface_max_speed(phy_interface_t interface)
+{
+	switch (interface) {
+	case PHY_INTERFACE_MODE_100BASEX:
+	case PHY_INTERFACE_MODE_REVRMII:
+	case PHY_INTERFACE_MODE_RMII:
+	case PHY_INTERFACE_MODE_SMII:
+	case PHY_INTERFACE_MODE_REVMII:
+	case PHY_INTERFACE_MODE_MII:
+		return SPEED_100;
+
+	case PHY_INTERFACE_MODE_TBI:
+	case PHY_INTERFACE_MODE_MOCA:
+	case PHY_INTERFACE_MODE_RTBI:
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_1000BASEKX:
+	case PHY_INTERFACE_MODE_TRGMII:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_GMII:
+		return SPEED_1000;
+
+	case PHY_INTERFACE_MODE_2500BASEX:
+		return SPEED_2500;
+
+	case PHY_INTERFACE_MODE_5GBASER:
+		return SPEED_5000;
+
+	case PHY_INTERFACE_MODE_XGMII:
+	case PHY_INTERFACE_MODE_RXAUI:
+	case PHY_INTERFACE_MODE_XAUI:
+	case PHY_INTERFACE_MODE_10GBASER:
+	case PHY_INTERFACE_MODE_10GKR:
+	case PHY_INTERFACE_MODE_USXGMII:
+	case PHY_INTERFACE_MODE_QUSGMII:
+		return SPEED_10000;
+
+	case PHY_INTERFACE_MODE_25GBASER:
+		return SPEED_25000;
+
+	case PHY_INTERFACE_MODE_XLGMII:
+		return SPEED_40000;
+
+	case PHY_INTERFACE_MODE_INTERNAL:
+	case PHY_INTERFACE_MODE_NA:
+	case PHY_INTERFACE_MODE_MAX:
+		/* No idea! Garbage in, unknown out */
+		return SPEED_UNKNOWN;
+	}
+
+	/* If we get here, someone forgot to add an interface mode above */
+	WARN_ON_ONCE(1);
+	return SPEED_UNKNOWN;
+}
+
 /**
  * phylink_caps_to_linkmodes() - Convert capabilities to ethtool link modes
  * @linkmodes: ethtool linkmode mask (must be already initialised)
@@ -791,11 +860,12 @@ static void phylink_mac_config(struct phylink *pl,
 			       const struct phylink_link_state *state)
 {
 	phylink_dbg(pl,
-		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
+		    "%s: mode=%s/%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
 		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
 		    phy_modes(state->interface),
 		    phy_speed_to_str(state->speed),
 		    phy_duplex_to_str(state->duplex),
+		    phy_rate_adaptation_to_str(state->rate_adaptation),
 		    __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
 		    state->pause, state->link, state->an_enabled);
 
@@ -932,7 +1002,8 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 	linkmode_zero(state->lp_advertising);
 	state->interface = pl->link_config.interface;
 	state->an_enabled = pl->link_config.an_enabled;
-	if  (state->an_enabled) {
+	state->rate_adaptation = pl->link_config.rate_adaptation;
+	if (state->an_enabled) {
 		state->speed = SPEED_UNKNOWN;
 		state->duplex = DUPLEX_UNKNOWN;
 		state->pause = MLO_PAUSE_NONE;
@@ -1015,19 +1086,45 @@ static void phylink_link_up(struct phylink *pl,
 			    struct phylink_link_state link_state)
 {
 	struct net_device *ndev = pl->netdev;
+	int speed, duplex;
+	bool rx_pause;
+
+	speed = link_state.speed;
+	duplex = link_state.duplex;
+	rx_pause = !!(link_state.pause & MLO_PAUSE_RX);
+
+	switch (link_state.rate_adaptation) {
+	case RATE_ADAPT_PAUSE:
+		/* The PHY is doing rate adaption from the media rate (in
+		 * the link_state) to the interface speed, and will send
+		 * pause frames to the MAC to limit its transmission speed.
+		 */
+		speed = phylink_interface_max_speed(link_state.interface);
+		duplex = DUPLEX_FULL;
+		rx_pause = true;
+		break;
+
+	case RATE_ADAPT_CRS:
+		/* The PHY is doing rate adaption from the media rate (in
+		 * the link_state) to the interface speed, and will cause
+		 * collisions to the MAC to limit its transmission speed.
+		 */
+		speed = phylink_interface_max_speed(link_state.interface);
+		duplex = DUPLEX_HALF;
+		break;
+	}
 
 	pl->cur_interface = link_state.interface;
+	if (link_state.rate_adaptation == RATE_ADAPT_PAUSE)
+		link_state.pause |= MLO_PAUSE_RX;
 
 	if (pl->pcs && pl->pcs->ops->pcs_link_up)
 		pl->pcs->ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
-					 pl->cur_interface,
-					 link_state.speed, link_state.duplex);
+					  pl->cur_interface, speed, duplex);
 
-	pl->mac_ops->mac_link_up(pl->config, pl->phydev,
-				 pl->cur_link_an_mode, pl->cur_interface,
-				 link_state.speed, link_state.duplex,
-				 !!(link_state.pause & MLO_PAUSE_TX),
-				 !!(link_state.pause & MLO_PAUSE_RX));
+	pl->mac_ops->mac_link_up(pl->config, pl->phydev, pl->cur_link_an_mode,
+				 pl->cur_interface, speed, duplex,
+				 !!(link_state.pause & MLO_PAUSE_TX), rx_pause);
 
 	if (ndev)
 		netif_carrier_on(ndev);
@@ -1119,6 +1216,17 @@ static void phylink_resolve(struct work_struct *w)
 				}
 				link_state.interface = pl->phy_state.interface;
 
+				/* If we are doing rate adaptation, then the
+				 * link speed/duplex comes from the PHY
+				 */
+				if (pl->phy_state.rate_adaptation) {
+					link_state.rate_adaptation =
+						pl->phy_state.rate_adaptation;
+					link_state.speed = pl->phy_state.speed;
+					link_state.duplex =
+						pl->phy_state.duplex;
+				}
+
 				/* If we have a PHY, we need to update with
 				 * the PHY flow control bits.
 				 */
@@ -1353,6 +1461,7 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 	mutex_lock(&pl->state_mutex);
 	pl->phy_state.speed = phydev->speed;
 	pl->phy_state.duplex = phydev->duplex;
+	pl->phy_state.rate_adaptation = phydev->rate_adaptation;
 	pl->phy_state.pause = MLO_PAUSE_NONE;
 	if (tx_pause)
 		pl->phy_state.pause |= MLO_PAUSE_TX;
@@ -1364,10 +1473,11 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 
 	phylink_run_resolve(pl);
 
-	phylink_dbg(pl, "phy link %s %s/%s/%s/%s\n", up ? "up" : "down",
+	phylink_dbg(pl, "phy link %s %s/%s/%s/%s/%s\n", up ? "up" : "down",
 		    phy_modes(phydev->interface),
 		    phy_speed_to_str(phydev->speed),
 		    phy_duplex_to_str(phydev->duplex),
+		    phy_rate_adaptation_to_str(phydev->rate_adaptation),
 		    phylink_pause_to_str(pl->phy_state.pause));
 }
 
@@ -1431,6 +1541,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	pl->phy_state.pause = MLO_PAUSE_NONE;
 	pl->phy_state.speed = SPEED_UNKNOWN;
 	pl->phy_state.duplex = DUPLEX_UNKNOWN;
+	pl->phy_state.rate_adaptation = RATE_ADAPT_NONE;
 	linkmode_copy(pl->supported, supported);
 	linkmode_copy(pl->link_config.advertising, config.advertising);
 
@@ -1873,8 +1984,10 @@ static void phylink_get_ksettings(const struct phylink_link_state *state,
 {
 	phylink_merge_link_mode(kset->link_modes.advertising, state->advertising);
 	linkmode_copy(kset->link_modes.lp_advertising, state->lp_advertising);
-	kset->base.speed = state->speed;
-	kset->base.duplex = state->duplex;
+	if (kset->base.rate_adaptation == RATE_ADAPT_NONE) {
+		kset->base.speed = state->speed;
+		kset->base.duplex = state->duplex;
+	}
 	kset->base.autoneg = state->an_enabled ? AUTONEG_ENABLE :
 				AUTONEG_DISABLE;
 }
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c2aa49c692a0..1524846e01b4 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -75,6 +75,10 @@ static inline bool phylink_autoneg_inband(unsigned int mode)
  * @speed: link speed, one of the SPEED_* constants.
  * @duplex: link duplex mode, one of DUPLEX_* constants.
  * @pause: link pause state, described by MLO_PAUSE_* constants.
+ * @rate_adaptation: rate adaptation being performed, one of the RATE_ADAPT_*
+ *   constants. If rate adaptation is taking place, then the speed/duplex of
+ *   the medium link mode (@speed and @duplex) and the speed/duplex of the phy
+ *   interface mode (@interface) are different.
  * @link: true if the link is up.
  * @an_enabled: true if autonegotiation is enabled/desired.
  * @an_complete: true if autonegotiation has completed.
@@ -86,6 +90,7 @@ struct phylink_link_state {
 	int speed;
 	int duplex;
 	int pause;
+	int rate_adaptation;
 	unsigned int link:1;
 	unsigned int an_enabled:1;
 	unsigned int an_complete:1;
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next v5 6/8] net: phylink: Adjust advertisement based on rate adaptation

[Sean Anderson]

This adds support for adjusting the advertisement for pause-based rate
adaptation. This may result in a lossy link, since the final link settings
are not adjusted. Asymmetric pause support is necessary. It would be
possible for a MAC supporting only symmetric pause to use pause-based rate
adaptation, but only if pause reception was enabled as well.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Move phylink_cap_from_speed_duplex to this commit

Changes in v3:
- Add phylink_cap_from_speed_duplex to look up the mac capability
  corresponding to the interface's speed.
- Include RATE_ADAPT_CRS; it's a few lines and it doesn't hurt.

Changes in v2:
- Determine the interface speed and max mac speed directly instead of
  guessing based on the caps.

 drivers/net/phy/phylink.c | 106 ++++++++++++++++++++++++++++++++++++--
 include/linux/phylink.h   |   3 +-
 2 files changed, 105 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 8ce110c7be5c..4d73a58f4854 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -373,18 +373,70 @@ void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps)
 }
 EXPORT_SYMBOL_GPL(phylink_caps_to_linkmodes);
 
+static struct {
+	unsigned long mask;
+	int speed;
+	unsigned int duplex;
+} phylink_caps_params[] = {
+	{ MAC_400000FD, SPEED_400000, DUPLEX_FULL },
+	{ MAC_200000FD, SPEED_200000, DUPLEX_FULL },
+	{ MAC_100000FD, SPEED_100000, DUPLEX_FULL },
+	{ MAC_56000FD,  SPEED_56000,  DUPLEX_FULL },
+	{ MAC_50000FD,  SPEED_50000,  DUPLEX_FULL },
+	{ MAC_40000FD,  SPEED_40000,  DUPLEX_FULL },
+	{ MAC_25000FD,  SPEED_25000,  DUPLEX_FULL },
+	{ MAC_20000FD,  SPEED_20000,  DUPLEX_FULL },
+	{ MAC_10000FD,  SPEED_10000,  DUPLEX_FULL },
+	{ MAC_5000FD,   SPEED_5000,   DUPLEX_FULL },
+	{ MAC_2500FD,   SPEED_2500,   DUPLEX_FULL },
+	{ MAC_1000FD,   SPEED_1000,   DUPLEX_FULL },
+	{ MAC_1000HD,   SPEED_1000,   DUPLEX_HALF },
+	{ MAC_100FD,    SPEED_100,    DUPLEX_FULL },
+	{ MAC_100HD,    SPEED_100,    DUPLEX_HALF },
+	{ MAC_10FD,     SPEED_10,     DUPLEX_FULL },
+	{ MAC_10HD,     SPEED_10,     DUPLEX_HALF },
+};
+
+/**
+ * phylink_cap_from_speed_duplex - Get mac capability from speed/duplex
+ * @speed: the speed to search for
+ * @duplex: the duplex to search for
+ *
+ * Find the mac capability for a given speed and duplex.
+ *
+ * Return: A mask with the mac capability patching @speed and @duplex, or 0 if
+ *         there were no matches.
+ */
+static unsigned long phylink_cap_from_speed_duplex(int speed,
+						   unsigned int duplex)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(phylink_caps_params); i++) {
+		if (speed == phylink_caps_params[i].speed &&
+		    duplex == phylink_caps_params[i].duplex)
+			return phylink_caps_params[i].mask;
+	}
+
+	return 0;
+}
+
 /**
  * phylink_get_capabilities() - get capabilities for a given MAC
  * @interface: phy interface mode defined by &typedef phy_interface_t
  * @mac_capabilities: bitmask of MAC capabilities
+ * @rate_adaptation: type of rate adaptation being performed
  *
  * Get the MAC capabilities that are supported by the @interface mode and
  * @mac_capabilities.
  */
 unsigned long phylink_get_capabilities(phy_interface_t interface,
-				       unsigned long mac_capabilities)
+				       unsigned long mac_capabilities,
+				       int rate_adaptation)
 {
+	int max_speed = phylink_interface_max_speed(interface);
 	unsigned long caps = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
+	unsigned long adapted_caps = 0;
 
 	switch (interface) {
 	case PHY_INTERFACE_MODE_USXGMII:
@@ -458,7 +510,53 @@ unsigned long phylink_get_capabilities(phy_interface_t interface,
 		break;
 	}
 
-	return caps & mac_capabilities;
+	switch (rate_adaptation) {
+	case RATE_ADAPT_OPEN_LOOP:
+		/* TODO */
+		fallthrough;
+	case RATE_ADAPT_NONE:
+		adapted_caps = 0;
+		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. This is because there's no way to
+		 * accept pause frames without transmitting them if we only
+		 * support symmetric pause.
+		 */
+		if (!(mac_capabilities & MAC_SYM_PAUSE) ||
+		    !(mac_capabilities & MAC_ASYM_PAUSE))
+			break;
+
+		/* We can't adapt if the MAC doesn't support the interface's
+		 * max speed at full duplex.
+		 */
+		if (mac_capabilities &
+		    phylink_cap_from_speed_duplex(max_speed, DUPLEX_FULL)) {
+			/* Although a duplex-adapting phy might exist, we
+			 * conservatively remove these modes because the MAC
+			 * will not be aware of the half-duplex nature of the
+			 * link.
+			 */
+			adapted_caps = GENMASK(__fls(caps), __fls(MAC_10HD));
+			adapted_caps &= ~(MAC_1000HD | MAC_100HD | MAC_10HD);
+		}
+		break;
+	}
+	case RATE_ADAPT_CRS:
+		/* The MAC must support half duplex at the interface's max
+		 * speed.
+		 */
+		if (mac_capabilities &
+		    phylink_cap_from_speed_duplex(max_speed, DUPLEX_HALF)) {
+			adapted_caps = GENMASK(__fls(caps), __fls(MAC_10HD));
+			adapted_caps &= mac_capabilities;
+		}
+		break;
+	}
+
+	return (caps & mac_capabilities) | adapted_caps;
 }
 EXPORT_SYMBOL_GPL(phylink_get_capabilities);
 
@@ -482,7 +580,8 @@ void phylink_generic_validate(struct phylink_config *config,
 	phylink_set_port_modes(mask);
 	phylink_set(mask, Autoneg);
 	caps = phylink_get_capabilities(state->interface,
-					config->mac_capabilities);
+					config->mac_capabilities,
+					state->rate_adaptation);
 	phylink_caps_to_linkmodes(mask, caps);
 
 	linkmode_and(supported, supported, mask);
@@ -1514,6 +1613,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 		config.interface = PHY_INTERFACE_MODE_NA;
 	else
 		config.interface = interface;
+	config.rate_adaptation = phy_get_rate_adaptation(phy, config.interface);
 
 	ret = phylink_validate(pl, supported, &config);
 	if (ret) {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 1524846e01b4..c67a67620c8e 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -541,7 +541,8 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 
 void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps);
 unsigned long phylink_get_capabilities(phy_interface_t interface,
-				       unsigned long mac_capabilities);
+				       unsigned long mac_capabilities,
+				       int rate_adaptation);
 void phylink_generic_validate(struct phylink_config *config,
 			      unsigned long *supported,
 			      struct phylink_link_state *state);
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next v5 7/8] net: phy: aquantia: Add some additional phy interfaces

[Sean Anderson]

These are documented in the AQR115 register reference. I haven't tested
them, but perhaps they'll be useful to someone.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---

(no changes since v3)

Changes in v3:
- Move unused defines to next commit (where they will be used)

 drivers/net/phy/aquantia_main.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 8b7a46db30e0..b3a5db487e52 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -27,9 +27,12 @@
 #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK	GENMASK(7, 3)
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR	0
+#define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KX	1
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_XFI	2
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_USXGMII	3
+#define MDIO_PHYXS_VEND_IF_STATUS_TYPE_XAUI	4
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_SGMII	6
+#define MDIO_PHYXS_VEND_IF_STATUS_TYPE_RXAUI	7
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_OCSGMII	10
 
 #define MDIO_AN_VEND_PROV			0xc400
@@ -392,15 +395,24 @@ static int aqr107_read_status(struct phy_device *phydev)
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR:
 		phydev->interface = PHY_INTERFACE_MODE_10GKR;
 		break;
+	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_KX:
+		phydev->interface = PHY_INTERFACE_MODE_1000BASEKX;
+		break;
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_XFI:
 		phydev->interface = PHY_INTERFACE_MODE_10GBASER;
 		break;
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_USXGMII:
 		phydev->interface = PHY_INTERFACE_MODE_USXGMII;
 		break;
+	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_XAUI:
+		phydev->interface = PHY_INTERFACE_MODE_XAUI;
+		break;
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_SGMII:
 		phydev->interface = PHY_INTERFACE_MODE_SGMII;
 		break;
+	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_RXAUI:
+		phydev->interface = PHY_INTERFACE_MODE_RXAUI;
+		break;
 	case MDIO_PHYXS_VEND_IF_STATUS_TYPE_OCSGMII:
 		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
 		break;
@@ -513,11 +525,14 @@ static int aqr107_config_init(struct phy_device *phydev)
 
 	/* Check that the PHY interface type is compatible */
 	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
+	    phydev->interface != PHY_INTERFACE_MODE_1000BASEKX &&
 	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
 	    phydev->interface != PHY_INTERFACE_MODE_XGMII &&
 	    phydev->interface != PHY_INTERFACE_MODE_USXGMII &&
 	    phydev->interface != PHY_INTERFACE_MODE_10GKR &&
-	    phydev->interface != PHY_INTERFACE_MODE_10GBASER)
+	    phydev->interface != PHY_INTERFACE_MODE_10GBASER &&
+	    phydev->interface != PHY_INTERFACE_MODE_XAUI &&
+	    phydev->interface != PHY_INTERFACE_MODE_RXAUI)
 		return -ENODEV;
 
 	WARN(phydev->interface == PHY_INTERFACE_MODE_XGMII,
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net-next v5 8/8] net: phy: aquantia: Add support for rate adaptation

[Sean Anderson]

This adds support for rate adaptation for phys similar to the AQR107. We
assume that all phys using aqr107_read_status support rate adaptation.
However, it could be possible to determine support based on the firmware
revision if there are phys discovered which do not support rate adaptation.
However, as rate adaptation is advertised in the datasheets for these phys,
I suspect it is supported most boards.

Despite the name, the "config" registers are updated with the current rate
adaptation method (if any). Because they appear to be updated
automatically, I don't know if these registers can be used to disable rate
adaptation.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

(no changes since v2)

Changes in v2:
- Add comments clarifying the register defines
- Reorder variables in aqr107_read_rate

 drivers/net/phy/aquantia_main.c | 51 ++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index b3a5db487e52..dd73891cf68a 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -94,6 +94,19 @@
 #define VEND1_GLOBAL_FW_ID_MAJOR		GENMASK(15, 8)
 #define VEND1_GLOBAL_FW_ID_MINOR		GENMASK(7, 0)
 
+/* The following registers all have similar layouts; first the registers... */
+#define VEND1_GLOBAL_CFG_10M			0x0310
+#define VEND1_GLOBAL_CFG_100M			0x031b
+#define VEND1_GLOBAL_CFG_1G			0x031c
+#define VEND1_GLOBAL_CFG_2_5G			0x031d
+#define VEND1_GLOBAL_CFG_5G			0x031e
+#define VEND1_GLOBAL_CFG_10G			0x031f
+/* ...and now the fields */
+#define VEND1_GLOBAL_CFG_RATE_ADAPT		GENMASK(8, 7)
+#define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE	0
+#define VEND1_GLOBAL_CFG_RATE_ADAPT_USX		1
+#define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE	2
+
 #define VEND1_GLOBAL_RSVD_STAT1			0xc885
 #define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID	GENMASK(7, 4)
 #define VEND1_GLOBAL_RSVD_STAT1_PROV_ID		GENMASK(3, 0)
@@ -338,40 +351,57 @@ static int aqr_read_status(struct phy_device *phydev)
 
 static int aqr107_read_rate(struct phy_device *phydev)
 {
+	u32 config_reg;
 	int val;
 
 	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1);
 	if (val < 0)
 		return val;
 
+	if (val & MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX)
+		phydev->duplex = DUPLEX_FULL;
+	else
+		phydev->duplex = DUPLEX_HALF;
+
 	switch (FIELD_GET(MDIO_AN_TX_VEND_STATUS1_RATE_MASK, val)) {
 	case MDIO_AN_TX_VEND_STATUS1_10BASET:
 		phydev->speed = SPEED_10;
+		config_reg = VEND1_GLOBAL_CFG_10M;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_100BASETX:
 		phydev->speed = SPEED_100;
+		config_reg = VEND1_GLOBAL_CFG_100M;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_1000BASET:
 		phydev->speed = SPEED_1000;
+		config_reg = VEND1_GLOBAL_CFG_1G;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_2500BASET:
 		phydev->speed = SPEED_2500;
+		config_reg = VEND1_GLOBAL_CFG_2_5G;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_5000BASET:
 		phydev->speed = SPEED_5000;
+		config_reg = VEND1_GLOBAL_CFG_5G;
 		break;
 	case MDIO_AN_TX_VEND_STATUS1_10GBASET:
 		phydev->speed = SPEED_10000;
+		config_reg = VEND1_GLOBAL_CFG_10G;
 		break;
 	default:
 		phydev->speed = SPEED_UNKNOWN;
-		break;
+		return 0;
 	}
 
-	if (val & MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX)
-		phydev->duplex = DUPLEX_FULL;
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, config_reg);
+	if (val < 0)
+		return val;
+
+	if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) ==
+	    VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
+		phydev->rate_adaptation = RATE_ADAPT_PAUSE;
 	else
-		phydev->duplex = DUPLEX_HALF;
+		phydev->rate_adaptation = RATE_ADAPT_NONE;
 
 	return 0;
 }
@@ -612,6 +642,16 @@ static void aqr107_link_change_notify(struct phy_device *phydev)
 		phydev_info(phydev, "Aquantia 1000Base-T2 mode active\n");
 }
 
+static int aqr107_get_rate_adaptation(struct phy_device *phydev,
+				      phy_interface_t iface)
+{
+	if (iface == PHY_INTERFACE_MODE_10GBASER ||
+	    iface == PHY_INTERFACE_MODE_2500BASEX ||
+	    iface == PHY_INTERFACE_MODE_NA)
+		return RATE_ADAPT_PAUSE;
+	return RATE_ADAPT_NONE;
+}
+
 static int aqr107_suspend(struct phy_device *phydev)
 {
 	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, MDIO_CTRL1,
@@ -673,6 +713,7 @@ static struct phy_driver aqr_driver[] = {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQR107),
 	.name		= "Aquantia AQR107",
 	.probe		= aqr107_probe,
+	.get_rate_adaptation = aqr107_get_rate_adaptation,
 	.config_init	= aqr107_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
@@ -691,6 +732,7 @@ static struct phy_driver aqr_driver[] = {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQCS109),
 	.name		= "Aquantia AQCS109",
 	.probe		= aqr107_probe,
+	.get_rate_adaptation = aqr107_get_rate_adaptation,
 	.config_init	= aqcs109_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
@@ -717,6 +759,7 @@ static struct phy_driver aqr_driver[] = {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQR113C),
 	.name           = "Aquantia AQR113C",
 	.probe          = aqr107_probe,
+	.get_rate_adaptation = aqr107_get_rate_adaptation,
 	.config_init    = aqr107_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr    = aqr_config_intr,
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Russell King (Oracle)]

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.

> +	 * Annex 28B for more information.
> +	 *
> +	 * The following table lists the values of MLO_PAUSE_* (aside from
> +	 * MLO_PAUSE_AN) which might be requested depending on the results of
> +	 * autonegotiation or user configuration:
> +	 *
> +	 * 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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v5 3/8] net: phylink: Generate caps and convert to linkmodes separately

[Russell King (Oracle)]

On Tue, Sep 06, 2022 at 12:18:47PM -0400, Sean Anderson wrote:
> @@ -409,11 +407,14 @@ void phylink_generic_validate(struct phylink_config *config,
>  			      unsigned long *supported,
>  			      struct phylink_link_state *state)
>  {
> +	unsigned long caps;
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };

net code requires reverse christmas tree for variable declarations.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v5 5/8] net: phylink: Adjust link settings based on rate adaptation

[Russell King (Oracle)]

On Tue, Sep 06, 2022 at 12:18:49PM -0400, Sean Anderson wrote:
> @@ -1015,19 +1086,45 @@ static void phylink_link_up(struct phylink *pl,
>  			    struct phylink_link_state link_state)
>  {
>  	struct net_device *ndev = pl->netdev;
> +	int speed, duplex;
> +	bool rx_pause;
> +
> +	speed = link_state.speed;
> +	duplex = link_state.duplex;
> +	rx_pause = !!(link_state.pause & MLO_PAUSE_RX);
> +
> +	switch (link_state.rate_adaptation) {
> +	case RATE_ADAPT_PAUSE:
> +		/* The PHY is doing rate adaption from the media rate (in
> +		 * the link_state) to the interface speed, and will send
> +		 * pause frames to the MAC to limit its transmission speed.
> +		 */
> +		speed = phylink_interface_max_speed(link_state.interface);
> +		duplex = DUPLEX_FULL;
> +		rx_pause = true;
> +		break;
> +
> +	case RATE_ADAPT_CRS:
> +		/* The PHY is doing rate adaption from the media rate (in
> +		 * the link_state) to the interface speed, and will cause
> +		 * collisions to the MAC to limit its transmission speed.
> +		 */
> +		speed = phylink_interface_max_speed(link_state.interface);
> +		duplex = DUPLEX_HALF;
> +		break;
> +	}
>  
>  	pl->cur_interface = link_state.interface;
> +	if (link_state.rate_adaptation == RATE_ADAPT_PAUSE)
> +		link_state.pause |= MLO_PAUSE_RX;

I specifically omitted this from my patch because I don't think we
should tell the user that "Link is Up - ... - flow control rx" if
we have rate adaption, but the media link is not using flow control.

The "Link is Up" message tells the user what was negotiated on the
media, not what is going on inside their network device, so the
fact we're using rate adaption which has turned on RX pause on the
MAC is neither here nor there.

>  
>  	if (pl->pcs && pl->pcs->ops->pcs_link_up)
>  		pl->pcs->ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
> -					 pl->cur_interface,
> -					 link_state.speed, link_state.duplex);
> +					  pl->cur_interface, speed, duplex);

It seems you have one extra unnecessary space here - not sure how
that occurred as it isn't in my original patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Sean Anderson]

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

>> +	 * Annex 28B for more information.
>> +	 *
>> +	 * The following table lists the values of MLO_PAUSE_* (aside from
>> +	 * MLO_PAUSE_AN) which might be requested depending on the results of
>> +	 * autonegotiation or user configuration:
>> +	 *
>> +	 * 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 don't see the path where we set both
MAC_SYM_PAUSE and MAC_ASYM_PAUSE and then we resolve to MLO_PAUSE_TX. Would this
be due to manual user configuration of both the advertising and the pause? By
that logic, couldn't we get any pause mode from any combination of MLO_PAUSE_*?

--Sean

Re: [PATCH net-next v5 3/8] net: phylink: Generate caps and convert to linkmodes separately

[Sean Anderson]

On 9/7/22 5:42 AM, Russell King (Oracle) wrote:
> On Tue, Sep 06, 2022 at 12:18:47PM -0400, Sean Anderson wrote:
>> @@ -409,11 +407,14 @@ void phylink_generic_validate(struct phylink_config *config,
>>   			      unsigned long *supported,
>>   			      struct phylink_link_state *state)
>>   {
>> +	unsigned long caps;
>>   	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> 
> net code requires reverse christmas tree for variable declarations.
> 

OK

Re: [PATCH net-next v5 5/8] net: phylink: Adjust link settings based on rate adaptation

[Sean Anderson]

On 9/7/22 6:10 AM, Russell King (Oracle) wrote:
> On Tue, Sep 06, 2022 at 12:18:49PM -0400, Sean Anderson wrote:
>> @@ -1015,19 +1086,45 @@ static void phylink_link_up(struct phylink *pl,
>>   			    struct phylink_link_state link_state)
>>   {
>>   	struct net_device *ndev = pl->netdev;
>> +	int speed, duplex;
>> +	bool rx_pause;
>> +
>> +	speed = link_state.speed;
>> +	duplex = link_state.duplex;
>> +	rx_pause = !!(link_state.pause & MLO_PAUSE_RX);
>> +
>> +	switch (link_state.rate_adaptation) {
>> +	case RATE_ADAPT_PAUSE:
>> +		/* The PHY is doing rate adaption from the media rate (in
>> +		 * the link_state) to the interface speed, and will send
>> +		 * pause frames to the MAC to limit its transmission speed.
>> +		 */
>> +		speed = phylink_interface_max_speed(link_state.interface);
>> +		duplex = DUPLEX_FULL;
>> +		rx_pause = true;
>> +		break;
>> +
>> +	case RATE_ADAPT_CRS:
>> +		/* The PHY is doing rate adaption from the media rate (in
>> +		 * the link_state) to the interface speed, and will cause
>> +		 * collisions to the MAC to limit its transmission speed.
>> +		 */
>> +		speed = phylink_interface_max_speed(link_state.interface);
>> +		duplex = DUPLEX_HALF;
>> +		break;
>> +	}
>>   
>>   	pl->cur_interface = link_state.interface;
>> +	if (link_state.rate_adaptation == RATE_ADAPT_PAUSE)
>> +		link_state.pause |= MLO_PAUSE_RX;
> 
> I specifically omitted this from my patch because I don't think we
> should tell the user that "Link is Up - ... - flow control rx" if
> we have rate adaption, but the media link is not using flow control.
> 
> The "Link is Up" message tells the user what was negotiated on the
> media, not what is going on inside their network device, so the
> fact we're using rate adaption which has turned on RX pause on the
> MAC is neither here nor there.

OK, I will leave this out then.

>>   
>>   	if (pl->pcs && pl->pcs->ops->pcs_link_up)
>>   		pl->pcs->ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
>> -					 pl->cur_interface,
>> -					 link_state.speed, link_state.duplex);
>> +					  pl->cur_interface, speed, duplex);
> 
> It seems you have one extra unnecessary space here - not sure how
> that occurred as it isn't in my original patch.
> 

This corrects a spacing issue in the existing code.

--Sean

Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Russell King (Oracle)]

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!

Re: [PATCH net-next v5 5/8] net: phylink: Adjust link settings based on rate adaptation

[Russell King (Oracle)]

On Wed, Sep 07, 2022 at 01:01:49PM -0400, Sean Anderson wrote:
> > >   	if (pl->pcs && pl->pcs->ops->pcs_link_up)
> > >   		pl->pcs->ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode,
> > > -					 pl->cur_interface,
> > > -					 link_state.speed, link_state.duplex);
> > > +					  pl->cur_interface, speed, duplex);
> > 
> > It seems you have one extra unnecessary space here - not sure how
> > that occurred as it isn't in my original patch.
> > 
> 
> This corrects a spacing issue in the existing code.

Okay, I agree.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Sean Anderson]

On 9/7/22 2:04 PM, Russell King (Oracle) wrote:
> 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.

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.

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

Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Russell King (Oracle)]

On Wed, Sep 07, 2022 at 04:11:14PM -0400, Sean Anderson wrote:
> On 9/7/22 2:04 PM, Russell King (Oracle) wrote:
> > 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.

Please amend to make it clearer.

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

Given that going from tx/rx back to pause/asym_dir bits is not trivial
(because the translation depends on the remote advertisement) it is
highly unlikely that the description would frame the support in terms
of whether the hardware can transmit and/or receive pause frames.

Note from the table above, it is not possible to advertise that you
do not support transmission of pause frames.

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

The above is how I'm viewing this, and because of the broken formatting,
it's impossible to make sense of, sorry.

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

If the MAC only supports symmetric pause, the logic in phylink ensures
that the MAC will always be called with tx_pause == rx_pause:
- it will fail attempts by ethtool to set autoneg off with different rx
  and tx settings.
- we will only advertise support for symmetric pause, for which there
  are only two autonegotiation outcomes, both of which satisfy the
  requirement that tx_pause == rx_pause.

So, if a MAC only supports symmetric pause, it can key off either of
these two flags as it is guaranteed that they will be identical in
for a MAC that only supports symmetric pause.

Adding in the issue of rate adaption (sorry, I use "adaption" not
"adaptation" which I find rather irksome as in my - and apparently
a subsection of English speakers, the two have slightly different
meanings) brings with it the problem that when using pause frames,
we need RX pause enabled, but on a MAC which only supports symmetric
pause, we can't enable RX pause without also transmitting pause frames.
So I would say such a setup was fundamentally mis-designed, and there's
little we can do to correct such a stupidity. Should we detect such
stupidities? Maybe, but what then? Refuse to function?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Sean Anderson]

On 9/7/22 17:01, Russell King (Oracle) wrote:
> On Wed, Sep 07, 2022 at 04:11:14PM -0400, Sean Anderson wrote:
>> On 9/7/22 2:04 PM, Russell King (Oracle) wrote:
>>> 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.
> 
> Please amend to make it clearer.

Does what I proposed work?

>>>>>> +	 * 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.
> 
> Given that going from tx/rx back to pause/asym_dir bits is not trivial
> (because the translation depends on the remote advertisement) it is
> highly unlikely that the description would frame the support in terms
> of whether the hardware can transmit and/or receive pause frames.

I think it is? Usually if both symmetric and asymmetric pause is
possible then there are two PAUSE_TX and PAUSE_RX fields in a register
somewhere. Similarly, if there is only symmetric pause, then there is a
PAUSE_EN bit in a register. And if only one of TX and RX is possible,
then there will only be one field. There are a few drivers where you
program the advertisement and let the hardware do the rest, but even
then there's usually a manual mode (which should be enabled by the
poorly-documented permit_pause_to_mac parameter).

However, it is not obvious (at least it wasn't to me)

- That MAC_SYM_PAUSE/MAC_ASYM_PAUSE control to the PAUSE and ASYM_DIR
   bits (when MLO_PAUSE_AN is set).
- How MAC_*_PAUSE related to the resolved pause mode in mac_link_up.

> Note from the table above, it is not possible to advertise that you
> do not support transmission of pause frames.

Just don't set either of MAC_*_PAUSE :)

Of course, hardware manufacturers are hopefully aware that only half of
the possible combinations are supported and don't produce hardware with
capabilities that can't be advertised.

>>
>> 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.
> 
> The above is how I'm viewing this, and because of the broken formatting,
> it's impossible to make sense of, sorry.

Sorry, my mail client mangled it. Second attempt:

> 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

>> 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.
> 
> If the MAC only supports symmetric pause, the logic in phylink ensures
> that the MAC will always be called with tx_pause == rx_pause:
> - it will fail attempts by ethtool to set autoneg off with different rx
>    and tx settings.
> - we will only advertise support for symmetric pause, for which there
>    are only two autonegotiation outcomes, both of which satisfy the
>    requirement that tx_pause == rx_pause.
> 
> So, if a MAC only supports symmetric pause, it can key off either of
> these two flags as it is guaranteed that they will be identical in
> for a MAC that only supports symmetric pause.

OK, so taking that into account then perhaps the post-table explanation
should be reworded to

> When MLO_PAUSE_AN is not set and MAC_ASYM_PAUSE is set, any
> combination of tx_pause and rx_pause may be requested. This depends on
> user configuration, without regard to the value of MAC_SYM_PAUSE. When
> When MLO_PAUSE_AN is not set and MAC_ASYM_PAUSE is also unset, then 
> tx_pause and rx_pause will still depend on user configuration, but
> will always equal each other.

Or maybe the above table should be extended to be

> MLO_PAUSE_AN MAC_SYM_PAUSE MAC_ASYM_PAUSE  tx_pause rx_pause
> ============ ============= ==============  ======== ========
>            0             0              0         0        0
>            0             0              1         0        0
>                                                   1        0
>            0             1              0         0        0
>                                                   1        1
>            0             1              1         0        0
>                                                   0        1
>                                                   1        1
>            1             0              0         0        0
>            1             X              1         X        X
>            1             1              0         0        0
>                                                   1        1

With a note like

> When MLO_PAUSE_AN is not set, the values of tx_pause and rx_pause
> depend on user configuration. When MAC_ASYM_PAUSE is not set, tx_pause
> and rx_pause will be restricted to be either both enabled or both
> disabled. Otherwise, no restrictions are placed on their values,
> allowing configurations which would not be attainable as a result of
> autonegotiation.

IMO we should really switch to something like MAX_RX_PAUSE,
MAC_TX_PAUSE, MAC_RXTX_PAUSE and let phylink handle all the details of
turning that into sane advertisement. This would also let us return
-EINVAL in phylink_ethtool_set_pauseparam when the user requests e.g.
TX-only pause when the MAC only supports RX and RXTX.

> Adding in the issue of rate adaption (sorry, I use "adaption" not
> "adaptation" which I find rather irksome as in my - and apparently
> a subsection of English speakers, the two have slightly different
> meanings)

802.3 calls it "rate adaptation" in clause 49 (10GBASE-R) and "rate
matching" in clause 61 (10PASS-TL and 2BASE-TS). If you are opposed to
the former, then I think the latter could also work. It's also shorter,
which is definitely a plus.

Interestingly, wiktionary (with which I attempted to determine what that
slightly-different meaning was) labels "adaption" as "rare" :)

> brings with it the problem that when using pause frames,
> we need RX pause enabled, but on a MAC which only supports symmetric
> pause, we can't enable RX pause without also transmitting pause frames.
> So I would say such a setup was fundamentally mis-designed, and there's
> little we can do to correct such a stupidity. Should we detect such
> stupidities? Maybe, but what then? Refuse to function?

Previous discussion [1]. Right now we refuse to turn on rate adaptation
if the MAC only supports symmetric pause. The maximally-robust solution
would be to first try and autonegotiate with rate adaptation enabled and
using symmetric pause, and then renegotiate without either enabled. I
think that's significantly more complex, so I propose deferring such an
implementation to whoever first complains about their link not being
rate-adapted.

--Sean

[1] https://lore.kernel.org/netdev/4172fd87-8e51-e67d-bf86-fdc6829fa9b3@seco.com/

Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Russell King (Oracle)]

On Wed, Sep 07, 2022 at 06:39:34PM -0400, Sean Anderson wrote:
> On 9/7/22 17:01, Russell King (Oracle) wrote:
> > On Wed, Sep 07, 2022 at 04:11:14PM -0400, Sean Anderson wrote:
> > > On 9/7/22 2:04 PM, Russell King (Oracle) wrote:
> > > > 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.
> > 
> > Please amend to make it clearer.
> 
> Does what I proposed work?

If you mean:
| 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.

Then yes, and I completely missed that because it looked like a quoted
part of my reply (you quoted it using "> " which is the standard thing
for quoted parts of replies. Note that I've quoted it using "| " to
distinguish it as different above.)

> > Given that going from tx/rx back to pause/asym_dir bits is not trivial
> > (because the translation depends on the remote advertisement) it is
> > highly unlikely that the description would frame the support in terms
> > of whether the hardware can transmit and/or receive pause frames.
> 
> I think it is? Usually if both symmetric and asymmetric pause is
> possible then there are two PAUSE_TX and PAUSE_RX fields in a register
> somewhere. Similarly, if there is only symmetric pause, then there is a
> PAUSE_EN bit in a register. And if only one of TX and RX is possible,
> then there will only be one field. There are a few drivers where you
> program the advertisement and let the hardware do the rest, but even
> then there's usually a manual mode (which should be enabled by the
> poorly-documented permit_pause_to_mac parameter).

The problem with "if there is only symmetric pause, then there is a
PAUSE_EN bit in a register" is that for a device that only supports
the ability to transmit pause, it would have a bit to enable the
advertisement of the ASM_DIR bit, and possibly also have a PAUSE_EN
bit in a register to enable the transmission of pause frames.

So if you look just at what bits there are to enable, you might
mistake a single pause bit to mean symmetric pause when it doesn't
actually support that mode.

Let's take this a step further. Let's say that a device only has the
capability to receive pause frames. How does that correspond with
the SYM (PAUSE) and ASYM (ASM_DIR) bits? The only state that provides
for receive-only mode is if both of these bits are set, but wait a
moment, for a device that supports independent control of transmit
and receive, it's exactly the same encoding!

Fundamentally, a device can not really be "only capable of receiving
pause frames" because there is no way to set the local advertisement
to indicate to the remote end that the local end can not send pause
frames.

The next issue is... how do you determine that a MAC that supports
transmission and reception of pause frames has independent or common
control of those two functions? That determines whether ASM_DIR can
be set along with PAUSE.

So, trying to work back from whether tx and rx are supported to which
of PAUSE and ASM_DIR should be set is quite a non-starter.

> However, it is not obvious (at least it wasn't to me)
> 
> - That MAC_SYM_PAUSE/MAC_ASYM_PAUSE control to the PAUSE and ASYM_DIR
>   bits (when MLO_PAUSE_AN is set).

I'm not sure why, because the linkmodes that the MAC deals with in
its validate() callback determine what is supported and what is
advertised, and phylink_caps_to_linkmodes() which is used in the
implementation of this method does:

        if (caps & MAC_SYM_PAUSE)
                __set_bit(ETHTOOL_LINK_MODE_Pause_BIT, linkmodes);

        if (caps & MAC_ASYM_PAUSE)
                __set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, linkmodes);

Were you not aware that these two ethtool link mode bits control
the advertisement?

> - How MAC_*_PAUSE related to the resolved pause mode in mac_link_up.
> 
> > Note from the table above, it is not possible to advertise that you
> > do not support transmission of pause frames.
> 
> Just don't set either of MAC_*_PAUSE :)
> 
> Of course, hardware manufacturers are hopefully aware that only half of
> the possible combinations are supported and don't produce hardware with
> capabilities that can't be advertised.

Well, having read a few (although limited) number of documents on
ethernet MACs, they tend to frame the support in terms of whether
symmetric pause being supported or just the whole lot. Given that
IEEE 802.3's starting point for pause frames is the advertisement
rather than whether the hardware supports transmission or
reception, I think it would be rather silly to specify it in terms
of the tx/rx support.

If one's reverse engineering, then I think it's reasonable that if
you determine what the capabilities of the hardware is, it's then
up to the reverse engineer to do the next step and consult 802.3
table 28B-3 and work out what the advertisement should be.

> > > > 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.
> > 
> > The above is how I'm viewing this, and because of the broken formatting,
> > it's impossible to make sense of, sorry.
> 
> Sorry, my mail client mangled it. Second attempt:
> 
> > 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

That's fine for the autonegotiation resolution, but you originally stated
that your table was also for user-settings as well - and that's where I
originally took issue and still do.

As I've tried to explain, for a MAC that supports the MAC_SYM_PAUSE=1
MAC_ASYM_PAUSE=1 case, the full set of four states of tx_pause and
rx_pause are possible to configure when autoneg is disabled _even_
when there is no way to properly advertise it.

The point of forcing the pause state is to override autonegotiation,
because maybe the autonegotiation state is wrong and you explicitly
want a particular configuration for the link.

> > So, if a MAC only supports symmetric pause, it can key off either of
> > these two flags as it is guaranteed that they will be identical in
> > for a MAC that only supports symmetric pause.
> 
> OK, so taking that into account then perhaps the post-table explanation
> should be reworded to
> 
> > When MLO_PAUSE_AN is not set and MAC_ASYM_PAUSE is set, any
> > combination of tx_pause and rx_pause may be requested. This depends on
> > user configuration, without regard to the value of MAC_SYM_PAUSE. When
> > When MLO_PAUSE_AN is not set and MAC_ASYM_PAUSE is also unset, then
> > tx_pause and rx_pause will still depend on user configuration, but
> > will always equal each other.
> 
> Or maybe the above table should be extended to be
> 
> > MLO_PAUSE_AN MAC_SYM_PAUSE MAC_ASYM_PAUSE  tx_pause rx_pause
> > ============ ============= ==============  ======== ========
> >            0             0              0         0        0
> >            0             0              1         0        0
> >                                                   1        0
> >            0             1              0         0        0
> >                                                   1        1
> >            0             1              1         0        0
> >                                                   0        1
> >                                                   1        1
> >            1             0              0         0        0
> >            1             X              1         X        X
> >            1             1              0         0        0
> >                                                   1        1
> 
> With a note like
> 
> > When MLO_PAUSE_AN is not set, the values of tx_pause and rx_pause
> > depend on user configuration. When MAC_ASYM_PAUSE is not set, tx_pause
> > and rx_pause will be restricted to be either both enabled or both
> > disabled. Otherwise, no restrictions are placed on their values,
> > allowing configurations which would not be attainable as a result of
> > autonegotiation.
> 
> IMO we should really switch to something like MAX_RX_PAUSE,
> MAC_TX_PAUSE, MAC_RXTX_PAUSE and let phylink handle all the details of
> turning that into sane advertisement.

I completely disagree for the technical example I gave above, where it
is impossible to advertise "hey, I support *only* receive pause". Also
it brings with it the issue that - does "MAC_RXTX_PAUSE" mean that the
MAC has independent control of transmit and receive pause frames, or
is it common.

I'm really sorry, but I think there are fundamental issues with trying
to frame the support in terms of "do we support transmission of pause
frames" and "do we support reception of pause frames" and working from
that back to an advertisement. The translation function from
capabilities to tx/rx enablement is a one-way translation - there is
no "good" reverse translation that doesn't involve ambiguity.

> This would also let us return
> -EINVAL in phylink_ethtool_set_pauseparam when the user requests e.g.
> TX-only pause when the MAC only supports RX and RXTX.

As I've said, there is no way to advertise to the link partner that
RX-only is the only pause setting allowed, so it would be pretty
darn stupid for a manufacturer to design hardware with just that
capability..

> > Adding in the issue of rate adaption (sorry, I use "adaption" not
> > "adaptation" which I find rather irksome as in my - and apparently
> > a subsection of English speakers, the two have slightly different
> > meanings)
> 
> 802.3 calls it "rate adaptation" in clause 49 (10GBASE-R) and "rate
> matching" in clause 61 (10PASS-TL and 2BASE-TS). If you are opposed to
> the former, then I think the latter could also work. It's also shorter,
> which is definitely a plus.
> 
> Interestingly, wiktionary (with which I attempted to determine what that
> slightly-different meaning was) labels "adaption" as "rare" :)

I'm aware of that, but to me (and others) adaption is something that is
on-going. Adaptation is what animals _have_ done to cope with a changing
environment.

For this feature, I much prefer "rate matching" which avoids this whole
issue of "adaption" vs "adaptation" - you may notice that when we were
originally discussing this, I was using "rate matching" terminology!

> > brings with it the problem that when using pause frames,
> > we need RX pause enabled, but on a MAC which only supports symmetric
> > pause, we can't enable RX pause without also transmitting pause frames.
> > So I would say such a setup was fundamentally mis-designed, and there's
> > little we can do to correct such a stupidity. Should we detect such
> > stupidities? Maybe, but what then? Refuse to function?
> 
> Previous discussion [1]. Right now we refuse to turn on rate adaptation
> if the MAC only supports symmetric pause. The maximally-robust solution
> would be to first try and autonegotiate with rate adaptation enabled and
> using symmetric pause, and then renegotiate without either enabled. I
> think that's significantly more complex, so I propose deferring such an
> implementation to whoever first complains about their link not being
> rate-adapted.

We can not get away from the fact that the only capabilities that a
MAC could advertise to say that it supports Rx-only pause mode is
one where it has both the PAUSE and ASM_DIR bits set. If it doesn't,
then, if you look at table 28B-3, there are no possible resolutions
to any other local advertisement state that result in Rx pause only
being enabled.

Therefore, a MAC that only supports Rx pause would be incapable
of properly advertising that fact to the remote link partner and
is probably not conformant with 802.3.

I'll also point you to table 28B-2 "Pause encoding":

|   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
|
| The PAUSE bit indicates that the device is capable of providing the
| symmetric PAUSE functions as defined# in Annex 31B. The ASM_DIR bit
| indicates that asymmetric PAUSE is supported. The value of the PAUSE
| bit when the ASM_DIR bit is set indicates the direction the PAUSE
| frames are supported for flow across the link. Asymmetric PAUSE
| configuration results in independent enabling of the PAUSE receive
| and PAUSE transmit functions as defined by Annex 31B. See 28B.3
| regarding PAUSE configuration resolution.

So here, the capabilities of the local device are couched in terms of
support for "symmetric pause" and "asymmetric pause" and not whether
they support transmission of pause frames and reception of pause frames.

I put it that the use of "is symmetric pause supported" and "is
asymmetric pause supported" by phylink is the right set of capabilities
that the MAC should be supplying, and not whether transmission and or
reception of pause frames is supported.

As I've pointed out, one can not go from tx and rx pause support to an
advertisement without ambiguity. That is why we can't advertise a
correct setting of PAUSE and ASM_DIR bits when using ethtool to
force a particular state of enables at the local end. To move to
using "is transmit pause supported" and "is receive pause supported"
will only _add_ ambiguity, and then we really do need documentation
to describe the behaviour we implement - because we then fall outside
of 802.3.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Sean Anderson]

On 9/8/22 10:21 AM, Russell King (Oracle) wrote:
> On Wed, Sep 07, 2022 at 06:39:34PM -0400, Sean Anderson wrote:
>> On 9/7/22 17:01, Russell King (Oracle) wrote:
>> > On Wed, Sep 07, 2022 at 04:11:14PM -0400, Sean Anderson wrote:
>> > > On 9/7/22 2:04 PM, Russell King (Oracle) wrote:
>> > Given that going from tx/rx back to pause/asym_dir bits is not trivial
>> > (because the translation depends on the remote advertisement) it is
>> > highly unlikely that the description would frame the support in terms
>> > of whether the hardware can transmit and/or receive pause frames.
>> 
>> I think it is? Usually if both symmetric and asymmetric pause is
>> possible then there are two PAUSE_TX and PAUSE_RX fields in a register
>> somewhere. Similarly, if there is only symmetric pause, then there is a
>> PAUSE_EN bit in a register. And if only one of TX and RX is possible,
>> then there will only be one field. There are a few drivers where you
>> program the advertisement and let the hardware do the rest, but even
>> then there's usually a manual mode (which should be enabled by the
>> poorly-documented permit_pause_to_mac parameter).
> 
> The problem with "if there is only symmetric pause, then there is a
> PAUSE_EN bit in a register" is that for a device that only supports
> the ability to transmit pause, it would have a bit to enable the
> advertisement of the ASM_DIR bit, and possibly also have a PAUSE_EN
> bit in a register to enable the transmission of pause frames.
> 
> So if you look just at what bits there are to enable, you might
> mistake a single pause bit to mean symmetric pause when it doesn't
> actually support that mode.

Sure, but usually that is noted in the documentation.

> Let's take this a step further. Let's say that a device only has the
> capability to receive pause frames. How does that correspond with
> the SYM (PAUSE) and ASYM (ASM_DIR) bits? The only state that provides
> for receive-only mode is if both of these bits are set, but wait a
> moment, for a device that supports independent control of transmit
> and receive, it's exactly the same encoding!
> 
> Fundamentally, a device can not really be "only capable of receiving
> pause frames" because there is no way to set the local advertisement
> to indicate to the remote end that the local end can not send pause
> frames.

Yup. Only half of the combinations can be expressed.

> The next issue is... how do you determine that a MAC that supports
> transmission and reception of pause frames has independent or common
> control of those two functions? That determines whether ASM_DIR can
> be set along with PAUSE.

This is why I suggested down below that we encode exactly that in the
mac caps.

> So, trying to work back from whether tx and rx are supported to which
> of PAUSE and ASM_DIR should be set is quite a non-starter.
> 
>> However, it is not obvious (at least it wasn't to me)
>> 
>> - That MAC_SYM_PAUSE/MAC_ASYM_PAUSE control to the PAUSE and ASYM_DIR
>>   bits (when MLO_PAUSE_AN is set).
> 
> I'm not sure why, because the linkmodes that the MAC deals with in
> its validate() callback determine what is supported and what is
> advertised, and phylink_caps_to_linkmodes() which is used in the
> implementation of this method does:
> 
>         if (caps & MAC_SYM_PAUSE)
>                 __set_bit(ETHTOOL_LINK_MODE_Pause_BIT, linkmodes);
> 
>         if (caps & MAC_ASYM_PAUSE)
>                 __set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, linkmodes);
> 
> Were you not aware that these two ethtool link mode bits control
> the advertisement?

Yes. I had to dig into the code to determine what these bits were for.
Since there is no documentation (which what this patch aims to address),
that really is the only option. Additionally, the terminology is
different from what IEEE uses (although IMO it better describes the
function of the bits).

>> - How MAC_*_PAUSE related to the resolved pause mode in mac_link_up.
>> 
>> > Note from the table above, it is not possible to advertise that you
>> > do not support transmission of pause frames.
>> 
>> Just don't set either of MAC_*_PAUSE :)
>> 
>> Of course, hardware manufacturers are hopefully aware that only half of
>> the possible combinations are supported and don't produce hardware with
>> capabilities that can't be advertised.
> 
> Well, having read a few (although limited) number of documents on
> ethernet MACs, they tend to frame the support in terms of whether
> symmetric pause being supported or just the whole lot. Given that
> IEEE 802.3's starting point for pause frames is the advertisement
> rather than whether the hardware supports transmission or
> reception, I think it would be rather silly to specify it in terms
> of the tx/rx support.
> 
> If one's reverse engineering, then I think it's reasonable that if
> you determine what the capabilities of the hardware is, it's then
> up to the reverse engineer to do the next step and consult 802.3
> table 28B-3 and work out what the advertisement should be.
> 
>> > > > 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.
>> > 
>> > The above is how I'm viewing this, and because of the broken formatting,
>> > it's impossible to make sense of, sorry.
>> 
>> Sorry, my mail client mangled it. Second attempt:
>> 
>> > 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
> 
> That's fine for the autonegotiation resolution, but you originally stated
> that your table was also for user-settings as well - and that's where I
> originally took issue and still do.
>
> As I've tried to explain, for a MAC that supports the MAC_SYM_PAUSE=1
> MAC_ASYM_PAUSE=1 case, the full set of four states of tx_pause and
> rx_pause are possible to configure when autoneg is disabled _even_
> when there is no way to properly advertise it.

I assume you wrote this before reading the below.

> The point of forcing the pause state is to override autonegotiation,
> because maybe the autonegotiation state is wrong and you explicitly
> want a particular configuration for the link.
> 
>> > So, if a MAC only supports symmetric pause, it can key off either of
>> > these two flags as it is guaranteed that they will be identical in
>> > for a MAC that only supports symmetric pause.
>> 
>> OK, so taking that into account then perhaps the post-table explanation
>> should be reworded to
>> 
>> > When MLO_PAUSE_AN is not set and MAC_ASYM_PAUSE is set, any
>> > combination of tx_pause and rx_pause may be requested. This depends on
>> > user configuration, without regard to the value of MAC_SYM_PAUSE. When
>> > When MLO_PAUSE_AN is not set and MAC_ASYM_PAUSE is also unset, then
>> > tx_pause and rx_pause will still depend on user configuration, but
>> > will always equal each other.
>> 
>> Or maybe the above table should be extended to be
>> 
>> > MLO_PAUSE_AN MAC_SYM_PAUSE MAC_ASYM_PAUSE  tx_pause rx_pause
>> > ============ ============= ==============  ======== ========
>> >            0             0              0         0        0
>> >            0             0              1         0        0
>> >                                                   1        0
>> >            0             1              0         0        0
>> >                                                   1        1
>> >            0             1              1         0        0
>> >                                                   0        1
>> >                                                   1        1
>> >            1             0              0         0        0
>> >            1             X              1         X        X
>> >            1             1              0         0        0
>> >                                                   1        1
>> 
>> With a note like
>> 
>> > When MLO_PAUSE_AN is not set, the values of tx_pause and rx_pause
>> > depend on user configuration. When MAC_ASYM_PAUSE is not set, tx_pause
>> > and rx_pause will be restricted to be either both enabled or both
>> > disabled. Otherwise, no restrictions are placed on their values,
>> > allowing configurations which would not be attainable as a result of
>> > autonegotiation.
>> 

These options are what I propose to do with the table. I think these address
your concern that user-specified behavior was not documented properly. Upon
review, I think using the first table with the second note would be best.

>> IMO we should really switch to something like MAX_RX_PAUSE,
>> MAC_TX_PAUSE, MAC_RXTX_PAUSE and let phylink handle all the details of
>> turning that into sane advertisement.
> 
> I completely disagree for the technical example I gave above, where it
> is impossible to advertise "hey, I support *only* receive pause". Also
> it brings with it the issue that - does "MAC_RXTX_PAUSE" mean that the
> MAC has independent control of transmit and receive pause frames, or
> is it common.
> 
> I'm really sorry, but I think there are fundamental issues with trying
> to frame the support in terms of "do we support transmission of pause
> frames" and "do we support reception of pause frames" and working from
> that back to an advertisement. The translation function from
> capabilities to tx/rx enablement is a one-way translation - there is
> no "good" reverse translation that doesn't involve ambiguity.

Of course. But this reflects what the hardware actually can do.

>> This would also let us return
>> -EINVAL in phylink_ethtool_set_pauseparam when the user requests e.g.
>> TX-only pause when the MAC only supports RX and RXTX.
> 
> As I've said, there is no way to advertise to the link partner that
> RX-only is the only pause setting allowed, so it would be pretty
> darn stupid for a manufacturer to design hardware with just that
> capability..

Well, when the user specifies things we ignore the results of
autonegotiation. So a user could specify tx only on one end of a link
and rx only on the other end and have a working result which couldn't
be the result of autonegotiation. By specifying what the hardware
actually supports, phylink can determine whether what the user requests
is supported, without regard to whether it could be autonegotiated. At
the moment we allow the user to specify configurations which might not
be supported at all. There is no error message when this happens, so a
user can only discover this issue by reading the driver/datasheet or by
sniffing the link traffic.

>> > Adding in the issue of rate adaption (sorry, I use "adaption" not
>> > "adaptation" which I find rather irksome as in my - and apparently
>> > a subsection of English speakers, the two have slightly different
>> > meanings)
>> 
>> 802.3 calls it "rate adaptation" in clause 49 (10GBASE-R) and "rate
>> matching" in clause 61 (10PASS-TL and 2BASE-TS). If you are opposed to
>> the former, then I think the latter could also work. It's also shorter,
>> which is definitely a plus.
>> 
>> Interestingly, wiktionary (with which I attempted to determine what that
>> slightly-different meaning was) labels "adaption" as "rare" :)
> 
> I'm aware of that, but to me (and others) adaption is something that is
> on-going. Adaptation is what animals _have_ done to cope with a changing
> environment.
> 
> For this feature, I much prefer "rate matching" which avoids this whole
> issue of "adaption" vs "adaptation" - you may notice that when we were
> originally discussing this, I was using "rate matching" terminology!

OK, I'll rename this in the next spin.

>> > brings with it the problem that when using pause frames,
>> > we need RX pause enabled, but on a MAC which only supports symmetric
>> > pause, we can't enable RX pause without also transmitting pause frames.
>> > So I would say such a setup was fundamentally mis-designed, and there's
>> > little we can do to correct such a stupidity. Should we detect such
>> > stupidities? Maybe, but what then? Refuse to function?
>> 
>> Previous discussion [1]. Right now we refuse to turn on rate adaptation
>> if the MAC only supports symmetric pause. The maximally-robust solution
>> would be to first try and autonegotiate with rate adaptation enabled and
>> using symmetric pause, and then renegotiate without either enabled. I
>> think that's significantly more complex, so I propose deferring such an
>> implementation to whoever first complains about their link not being
>> rate-adapted.
> 
> We can not get away from the fact that the only capabilities that a
> MAC could advertise to say that it supports Rx-only pause mode is
> one where it has both the PAUSE and ASM_DIR bits set. If it doesn't,
> then, if you look at table 28B-3, there are no possible resolutions
> to any other local advertisement state that result in Rx pause only
> being enabled.

Well, what we really want to advertise is MLO_PAUSE_TXRX *without*
MLO_PAUSE_NONE. This is of course not possible to advertise, hence
the retry approach I suggested above.

> Therefore, a MAC that only supports Rx pause would be incapable
> of properly advertising that fact to the remote link partner and
> is probably not conformant with 802.3.

Autonegotiation is optional for pause support. I agree that such an
implementation would be unusual.

> I'll also point you to table 28B-2 "Pause encoding":
> 
> |   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
> |
> | The PAUSE bit indicates that the device is capable of providing the
> | symmetric PAUSE functions as defined# in Annex 31B. The ASM_DIR bit
> | indicates that asymmetric PAUSE is supported. The value of the PAUSE
> | bit when the ASM_DIR bit is set indicates the direction the PAUSE
> | frames are supported for flow across the link. Asymmetric PAUSE
> | configuration results in independent enabling of the PAUSE receive
> | and PAUSE transmit functions as defined by Annex 31B. See 28B.3
> | regarding PAUSE configuration resolution.
> 
> So here, the capabilities of the local device are couched in terms of
> support for "symmetric pause" and "asymmetric pause" and not whether
> they support transmission of pause frames and reception of pause frames.
> 
> I put it that the use of "is symmetric pause supported" and "is
> asymmetric pause supported" by phylink is the right set of capabilities
> that the MAC should be supplying, and not whether transmission and or
> reception of pause frames is supported.

Well the funky bit is that one can say "I support *only* asymmetric pause"
which is pretty strange. By the above logic, devices supporting asymmetric
pause should be a strict subset of those supporting symmetric pause. And
yet it is not the case. IEEE has decided that this means tx-only devices.
We have some devices like this in Linux already (ksz8795, macb). IMO this
hijacking of meaning is precisely what needs to be documented, and also
makes the symmetric/asymmetric pause distinction less useful.

> As I've pointed out, one can not go from tx and rx pause support to an
> advertisement without ambiguity. That is why we can't advertise a
> correct setting of PAUSE and ASM_DIR bits when using ethtool to
> force a particular state of enables at the local end. To move to
> using "is transmit pause supported" and "is receive pause supported"
> will only _add_ ambiguity, and then we really do need documentation
> to describe the behaviour we implement - because we then fall outside
> of 802.3.

It removes ambiguity from the driver author's perspective. The ambiguity
then shifts to phylink_caps_to_linkmodes, which can handle the translation.

In any case, since you prefer the underspecified representation then go
ahead and keep using it.

--Sean

Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Russell King (Oracle)]

On Thu, Sep 08, 2022 at 05:03:57PM -0400, Sean Anderson wrote:
> On 9/8/22 10:21 AM, Russell King (Oracle) wrote:
> > On Wed, Sep 07, 2022 at 06:39:34PM -0400, Sean Anderson wrote:
> >> On 9/7/22 17:01, Russell King (Oracle) wrote:
> >> > On Wed, Sep 07, 2022 at 04:11:14PM -0400, Sean Anderson wrote:
> >> > > On 9/7/22 2:04 PM, Russell King (Oracle) wrote:
> >> > Given that going from tx/rx back to pause/asym_dir bits is not trivial
> >> > (because the translation depends on the remote advertisement) it is
> >> > highly unlikely that the description would frame the support in terms
> >> > of whether the hardware can transmit and/or receive pause frames.
> >> 
> >> I think it is? Usually if both symmetric and asymmetric pause is
> >> possible then there are two PAUSE_TX and PAUSE_RX fields in a register
> >> somewhere. Similarly, if there is only symmetric pause, then there is a
> >> PAUSE_EN bit in a register. And if only one of TX and RX is possible,
> >> then there will only be one field. There are a few drivers where you
> >> program the advertisement and let the hardware do the rest, but even
> >> then there's usually a manual mode (which should be enabled by the
> >> poorly-documented permit_pause_to_mac parameter).
> > 
> > The problem with "if there is only symmetric pause, then there is a
> > PAUSE_EN bit in a register" is that for a device that only supports
> > the ability to transmit pause, it would have a bit to enable the
> > advertisement of the ASM_DIR bit, and possibly also have a PAUSE_EN
> > bit in a register to enable the transmission of pause frames.
> > 
> > So if you look just at what bits there are to enable, you might
> > mistake a single pause bit to mean symmetric pause when it doesn't
> > actually support that mode.
> 
> Sure, but usually that is noted in the documentation.
> 
> > Let's take this a step further. Let's say that a device only has the
> > capability to receive pause frames. How does that correspond with
> > the SYM (PAUSE) and ASYM (ASM_DIR) bits? The only state that provides
> > for receive-only mode is if both of these bits are set, but wait a
> > moment, for a device that supports independent control of transmit
> > and receive, it's exactly the same encoding!
> > 
> > Fundamentally, a device can not really be "only capable of receiving
> > pause frames" because there is no way to set the local advertisement
> > to indicate to the remote end that the local end can not send pause
> > frames.
> 
> Yup. Only half of the combinations can be expressed.
> 
> > The next issue is... how do you determine that a MAC that supports
> > transmission and reception of pause frames has independent or common
> > control of those two functions? That determines whether ASM_DIR can
> > be set along with PAUSE.
> 
> This is why I suggested down below that we encode exactly that in the
> mac caps.
> 
> > So, trying to work back from whether tx and rx are supported to which
> > of PAUSE and ASM_DIR should be set is quite a non-starter.
> > 
> >> However, it is not obvious (at least it wasn't to me)
> >> 
> >> - That MAC_SYM_PAUSE/MAC_ASYM_PAUSE control to the PAUSE and ASYM_DIR
> >>   bits (when MLO_PAUSE_AN is set).
> > 
> > I'm not sure why, because the linkmodes that the MAC deals with in
> > its validate() callback determine what is supported and what is
> > advertised, and phylink_caps_to_linkmodes() which is used in the
> > implementation of this method does:
> > 
> >         if (caps & MAC_SYM_PAUSE)
> >                 __set_bit(ETHTOOL_LINK_MODE_Pause_BIT, linkmodes);
> > 
> >         if (caps & MAC_ASYM_PAUSE)
> >                 __set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, linkmodes);
> > 
> > Were you not aware that these two ethtool link mode bits control
> > the advertisement?
> 
> Yes. I had to dig into the code to determine what these bits were for.
> Since there is no documentation (which what this patch aims to address),
> that really is the only option. Additionally, the terminology is
> different from what IEEE uses (although IMO it better describes the
> function of the bits).
> 
> >> - How MAC_*_PAUSE related to the resolved pause mode in mac_link_up.
> >> 
> >> > Note from the table above, it is not possible to advertise that you
> >> > do not support transmission of pause frames.
> >> 
> >> Just don't set either of MAC_*_PAUSE :)
> >> 
> >> Of course, hardware manufacturers are hopefully aware that only half of
> >> the possible combinations are supported and don't produce hardware with
> >> capabilities that can't be advertised.
> > 
> > Well, having read a few (although limited) number of documents on
> > ethernet MACs, they tend to frame the support in terms of whether
> > symmetric pause being supported or just the whole lot. Given that
> > IEEE 802.3's starting point for pause frames is the advertisement
> > rather than whether the hardware supports transmission or
> > reception, I think it would be rather silly to specify it in terms
> > of the tx/rx support.
> > 
> > If one's reverse engineering, then I think it's reasonable that if
> > you determine what the capabilities of the hardware is, it's then
> > up to the reverse engineer to do the next step and consult 802.3
> > table 28B-3 and work out what the advertisement should be.
> > 
> >> > > > 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.
> >> > 
> >> > The above is how I'm viewing this, and because of the broken formatting,
> >> > it's impossible to make sense of, sorry.
> >> 
> >> Sorry, my mail client mangled it. Second attempt:
> >> 
> >> > 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
> > 
> > That's fine for the autonegotiation resolution, but you originally stated
> > that your table was also for user-settings as well - and that's where I
> > originally took issue and still do.
> >
> > As I've tried to explain, for a MAC that supports the MAC_SYM_PAUSE=1
> > MAC_ASYM_PAUSE=1 case, the full set of four states of tx_pause and
> > rx_pause are possible to configure when autoneg is disabled _even_
> > when there is no way to properly advertise it.
> 
> I assume you wrote this before reading the below.
> 
> > The point of forcing the pause state is to override autonegotiation,
> > because maybe the autonegotiation state is wrong and you explicitly
> > want a particular configuration for the link.
> > 
> >> > So, if a MAC only supports symmetric pause, it can key off either of
> >> > these two flags as it is guaranteed that they will be identical in
> >> > for a MAC that only supports symmetric pause.
> >> 
> >> OK, so taking that into account then perhaps the post-table explanation
> >> should be reworded to
> >> 
> >> > When MLO_PAUSE_AN is not set and MAC_ASYM_PAUSE is set, any
> >> > combination of tx_pause and rx_pause may be requested. This depends on
> >> > user configuration, without regard to the value of MAC_SYM_PAUSE. When
> >> > When MLO_PAUSE_AN is not set and MAC_ASYM_PAUSE is also unset, then
> >> > tx_pause and rx_pause will still depend on user configuration, but
> >> > will always equal each other.
> >> 
> >> Or maybe the above table should be extended to be
> >> 
> >> > MLO_PAUSE_AN MAC_SYM_PAUSE MAC_ASYM_PAUSE  tx_pause rx_pause
> >> > ============ ============= ==============  ======== ========
> >> >            0             0              0         0        0
> >> >            0             0              1         0        0
> >> >                                                   1        0
> >> >            0             1              0         0        0
> >> >                                                   1        1
> >> >            0             1              1         0        0
> >> >                                                   0        1
> >> >                                                   1        1
> >> >            1             0              0         0        0
> >> >            1             X              1         X        X
> >> >            1             1              0         0        0
> >> >                                                   1        1
> >> 
> >> With a note like
> >> 
> >> > When MLO_PAUSE_AN is not set, the values of tx_pause and rx_pause
> >> > depend on user configuration. When MAC_ASYM_PAUSE is not set, tx_pause
> >> > and rx_pause will be restricted to be either both enabled or both
> >> > disabled. Otherwise, no restrictions are placed on their values,
> >> > allowing configurations which would not be attainable as a result of
> >> > autonegotiation.
> >> 
> 
> These options are what I propose to do with the table. I think these address
> your concern that user-specified behavior was not documented properly. Upon
> review, I think using the first table with the second note would be best.
> 
> >> IMO we should really switch to something like MAX_RX_PAUSE,
> >> MAC_TX_PAUSE, MAC_RXTX_PAUSE and let phylink handle all the details of
> >> turning that into sane advertisement.
> > 
> > I completely disagree for the technical example I gave above, where it
> > is impossible to advertise "hey, I support *only* receive pause". Also
> > it brings with it the issue that - does "MAC_RXTX_PAUSE" mean that the
> > MAC has independent control of transmit and receive pause frames, or
> > is it common.
> > 
> > I'm really sorry, but I think there are fundamental issues with trying
> > to frame the support in terms of "do we support transmission of pause
> > frames" and "do we support reception of pause frames" and working from
> > that back to an advertisement. The translation function from
> > capabilities to tx/rx enablement is a one-way translation - there is
> > no "good" reverse translation that doesn't involve ambiguity.
> 
> Of course. But this reflects what the hardware actually can do.
> 
> >> This would also let us return
> >> -EINVAL in phylink_ethtool_set_pauseparam when the user requests e.g.
> >> TX-only pause when the MAC only supports RX and RXTX.
> > 
> > As I've said, there is no way to advertise to the link partner that
> > RX-only is the only pause setting allowed, so it would be pretty
> > darn stupid for a manufacturer to design hardware with just that
> > capability..
> 
> Well, when the user specifies things we ignore the results of
> autonegotiation. So a user could specify tx only on one end of a link
> and rx only on the other end and have a working result which couldn't
> be the result of autonegotiation. By specifying what the hardware
> actually supports, phylink can determine whether what the user requests
> is supported, without regard to whether it could be autonegotiated. At
> the moment we allow the user to specify configurations which might not
> be supported at all. There is no error message when this happens, so a
> user can only discover this issue by reading the driver/datasheet or by
> sniffing the link traffic.
> 
> >> > Adding in the issue of rate adaption (sorry, I use "adaption" not
> >> > "adaptation" which I find rather irksome as in my - and apparently
> >> > a subsection of English speakers, the two have slightly different
> >> > meanings)
> >> 
> >> 802.3 calls it "rate adaptation" in clause 49 (10GBASE-R) and "rate
> >> matching" in clause 61 (10PASS-TL and 2BASE-TS). If you are opposed to
> >> the former, then I think the latter could also work. It's also shorter,
> >> which is definitely a plus.
> >> 
> >> Interestingly, wiktionary (with which I attempted to determine what that
> >> slightly-different meaning was) labels "adaption" as "rare" :)
> > 
> > I'm aware of that, but to me (and others) adaption is something that is
> > on-going. Adaptation is what animals _have_ done to cope with a changing
> > environment.
> > 
> > For this feature, I much prefer "rate matching" which avoids this whole
> > issue of "adaption" vs "adaptation" - you may notice that when we were
> > originally discussing this, I was using "rate matching" terminology!
> 
> OK, I'll rename this in the next spin.
> 
> >> > brings with it the problem that when using pause frames,
> >> > we need RX pause enabled, but on a MAC which only supports symmetric
> >> > pause, we can't enable RX pause without also transmitting pause frames.
> >> > So I would say such a setup was fundamentally mis-designed, and there's
> >> > little we can do to correct such a stupidity. Should we detect such
> >> > stupidities? Maybe, but what then? Refuse to function?
> >> 
> >> Previous discussion [1]. Right now we refuse to turn on rate adaptation
> >> if the MAC only supports symmetric pause. The maximally-robust solution
> >> would be to first try and autonegotiate with rate adaptation enabled and
> >> using symmetric pause, and then renegotiate without either enabled. I
> >> think that's significantly more complex, so I propose deferring such an
> >> implementation to whoever first complains about their link not being
> >> rate-adapted.
> > 
> > We can not get away from the fact that the only capabilities that a
> > MAC could advertise to say that it supports Rx-only pause mode is
> > one where it has both the PAUSE and ASM_DIR bits set. If it doesn't,
> > then, if you look at table 28B-3, there are no possible resolutions
> > to any other local advertisement state that result in Rx pause only
> > being enabled.
> 
> Well, what we really want to advertise is MLO_PAUSE_TXRX *without*
> MLO_PAUSE_NONE. This is of course not possible to advertise, hence
> the retry approach I suggested above.
> 
> > Therefore, a MAC that only supports Rx pause would be incapable
> > of properly advertising that fact to the remote link partner and
> > is probably not conformant with 802.3.
> 
> Autonegotiation is optional for pause support. I agree that such an
> implementation would be unusual.
> 
> > I'll also point you to table 28B-2 "Pause encoding":
> > 
> > |   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
> > |
> > | The PAUSE bit indicates that the device is capable of providing the
> > | symmetric PAUSE functions as defined# in Annex 31B. The ASM_DIR bit
> > | indicates that asymmetric PAUSE is supported. The value of the PAUSE
> > | bit when the ASM_DIR bit is set indicates the direction the PAUSE
> > | frames are supported for flow across the link. Asymmetric PAUSE
> > | configuration results in independent enabling of the PAUSE receive
> > | and PAUSE transmit functions as defined by Annex 31B. See 28B.3
> > | regarding PAUSE configuration resolution.
> > 
> > So here, the capabilities of the local device are couched in terms of
> > support for "symmetric pause" and "asymmetric pause" and not whether
> > they support transmission of pause frames and reception of pause frames.
> > 
> > I put it that the use of "is symmetric pause supported" and "is
> > asymmetric pause supported" by phylink is the right set of capabilities
> > that the MAC should be supplying, and not whether transmission and or
> > reception of pause frames is supported.
> 
> Well the funky bit is that one can say "I support *only* asymmetric pause"
> which is pretty strange. By the above logic, devices supporting asymmetric
> pause should be a strict subset of those supporting symmetric pause. And
> yet it is not the case. IEEE has decided that this means tx-only devices.
> We have some devices like this in Linux already (ksz8795, macb). IMO this
> hijacking of meaning is precisely what needs to be documented, and also
> makes the symmetric/asymmetric pause distinction less useful.
> 
> > As I've pointed out, one can not go from tx and rx pause support to an
> > advertisement without ambiguity. That is why we can't advertise a
> > correct setting of PAUSE and ASM_DIR bits when using ethtool to
> > force a particular state of enables at the local end. To move to
> > using "is transmit pause supported" and "is receive pause supported"
> > will only _add_ ambiguity, and then we really do need documentation
> > to describe the behaviour we implement - because we then fall outside
> > of 802.3.
> 
> It removes ambiguity from the driver author's perspective. The ambiguity
> then shifts to phylink_caps_to_linkmodes, which can handle the translation.
> 
> In any case, since you prefer the underspecified representation then go
> ahead and keep using it.

tl;dr (sorry, I don't have time, I've had one hell of a day what with
needing to visit the eye hospital, and the passing of our Monarch
which needs me to organise stuff.)

As I've pointed out, I regard specifying whether something supports
tx pause, rx pause, or both to be an underspecification on the
grounds that there is no way to do the translation to the advertisement
in a way that is unambiguous. Moreover, I've pointed out that IEEE
802.3 talks about the capabilities in terms of symmetric and asymmetric
pause.

The fact is, IEEE 802.3 makes no provision for a MAC that only supports
RX pause. It's pretty obvious to me that the implementation choices are:

1) No pause capability (PAUSE=0 ASM_DIR=0)
2) TX pause capability (PAUSE=0 ASM_DIR=1)
3) TX and RX pause capability with a single control (PAUSE=1 ASM_DIR=0)
4) TX and RX pause capability with independent control (PAUSE=1 ASM_DIR=1)

Anything that goes outside of that is just trash, because the result of
autonegotiation can result in something that the MAC doesn't support
and there's sweet f.a. that the local side can do about it.

Anyway, I'm probably not going to respond on this anymore before next
week, I'm way too busy given today's events. Sorry.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!