[PATCH net-next 0/4] net: bcmgenet: support for flow control

[PATCH net-next 0/4] net: bcmgenet: support for flow control

[Florian Fainelli]

This patch series adds support for flow control to the GENET driver, the
first 2 patches remove superfluous code, the 3rd one does re-organize
code a little bit and the 4th one ads the support for flow control
proper.

Doug Berger (4):
  net: bcmgenet: remove netif_carrier_off from adjust_link
  net: bcmgenet: remove old link state values
  net: bcmgenet: pull mac_config from adjust_link
  net: bcmgenet: add support for ethtool flow control

 .../net/ethernet/broadcom/genet/bcmgenet.c    |  56 ++++++-
 .../net/ethernet/broadcom/genet/bcmgenet.h    |   8 +-
 drivers/net/ethernet/broadcom/genet/bcmmii.c  | 155 +++++++++---------
 3 files changed, 130 insertions(+), 89 deletions(-)

-- 
2.25.1

[PATCH net-next 1/4] net: bcmgenet: remove netif_carrier_off from adjust_link

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

The bcmgenet_mii_setup() function is registered as the adjust_link
callback from the phylib for the GENET driver.

The phylib always sets the netif_carrier according to phydev->link
prior to invoking the adjust_link callback, so there is no need to
repeat that in the link down case within the network driver.

Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index ff1efd52ce16..8a9d8ceaa5bf 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -106,9 +106,6 @@ void bcmgenet_mii_setup(struct net_device *dev)
 		/* done if nothing has changed */
 		if (!status_changed)
 			return;
-
-		/* needed for MoCA fixed PHY to reflect correct link status */
-		netif_carrier_off(dev);
 	}
 
 	phy_print_status(phydev);
-- 
2.25.1

Re: [PATCH net-next 1/4] net: bcmgenet: remove netif_carrier_off from adjust_link

[Andrew Lunn]

On Sat, Sep 25, 2021 at 08:21:11PM -0700, Florian Fainelli wrote:
> From: Doug Berger <opendmb@gmail.com>
> 
> The bcmgenet_mii_setup() function is registered as the adjust_link
> callback from the phylib for the GENET driver.
> 
> The phylib always sets the netif_carrier according to phydev->link
> prior to invoking the adjust_link callback, so there is no need to
> repeat that in the link down case within the network driver.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 2/4] net: bcmgenet: remove old link state values

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

The PHY state machine has been fixed to only call the adjust_link
callback when the link state has changed. Therefore the old link
state variables are no longer needed to detect a change in link
state.

This commit effectively reverts
commit 5ad6e6c50899 ("net: bcmgenet: improve bcmgenet_mii_setup()")

Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../net/ethernet/broadcom/genet/bcmgenet.c    |  5 ---
 .../net/ethernet/broadcom/genet/bcmgenet.h    |  4 ---
 drivers/net/ethernet/broadcom/genet/bcmmii.c  | 36 -------------------
 3 files changed, 45 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 23c7595d2a1d..3427f9ed7eb9 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3408,11 +3408,6 @@ static void bcmgenet_netif_stop(struct net_device *dev)
 	 */
 	cancel_work_sync(&priv->bcmgenet_irq_work);
 
-	priv->old_link = -1;
-	priv->old_speed = -1;
-	priv->old_duplex = -1;
-	priv->old_pause = -1;
-
 	/* tx reclaim */
 	bcmgenet_tx_reclaim_all(dev);
 	bcmgenet_fini_dma(priv);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 0a6d91b0f0aa..406249bc9fe5 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -606,10 +606,6 @@ struct bcmgenet_priv {
 	bool clk_eee_enabled;
 
 	/* PHY device variables */
-	int old_link;
-	int old_speed;
-	int old_duplex;
-	int old_pause;
 	phy_interface_t phy_interface;
 	int phy_addr;
 	int ext_phy;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 8a9d8ceaa5bf..8fce5878a7d9 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -33,34 +33,8 @@ void bcmgenet_mii_setup(struct net_device *dev)
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct phy_device *phydev = dev->phydev;
 	u32 reg, cmd_bits = 0;
-	bool status_changed = false;
-
-	if (priv->old_link != phydev->link) {
-		status_changed = true;
-		priv->old_link = phydev->link;
-	}
 
 	if (phydev->link) {
-		/* check speed/duplex/pause changes */
-		if (priv->old_speed != phydev->speed) {
-			status_changed = true;
-			priv->old_speed = phydev->speed;
-		}
-
-		if (priv->old_duplex != phydev->duplex) {
-			status_changed = true;
-			priv->old_duplex = phydev->duplex;
-		}
-
-		if (priv->old_pause != phydev->pause) {
-			status_changed = true;
-			priv->old_pause = phydev->pause;
-		}
-
-		/* done if nothing has changed */
-		if (!status_changed)
-			return;
-
 		/* speed */
 		if (phydev->speed == SPEED_1000)
 			cmd_bits = CMD_SPEED_1000;
@@ -102,10 +76,6 @@ void bcmgenet_mii_setup(struct net_device *dev)
 			reg |= CMD_TX_EN | CMD_RX_EN;
 		}
 		bcmgenet_umac_writel(priv, reg, UMAC_CMD);
-	} else {
-		/* done if nothing has changed */
-		if (!status_changed)
-			return;
 	}
 
 	phy_print_status(phydev);
@@ -294,12 +264,6 @@ int bcmgenet_mii_probe(struct net_device *dev)
 	if (priv->internal_phy)
 		phy_flags = priv->gphy_rev;
 
-	/* Initialize link state variables that bcmgenet_mii_setup() uses */
-	priv->old_link = -1;
-	priv->old_speed = -1;
-	priv->old_duplex = -1;
-	priv->old_pause = -1;
-
 	/* This is an ugly quirk but we have not been correctly interpreting
 	 * the phy_interface values and we have done that across different
 	 * drivers, so at least we are consistent in our mistakes.
-- 
2.25.1

Re: [PATCH net-next 2/4] net: bcmgenet: remove old link state values

[Andrew Lunn]

On Sat, Sep 25, 2021 at 08:21:12PM -0700, Florian Fainelli wrote:
> From: Doug Berger <opendmb@gmail.com>
> 
> The PHY state machine has been fixed to only call the adjust_link
> callback when the link state has changed. Therefore the old link
> state variables are no longer needed to detect a change in link
> state.
> 
> This commit effectively reverts
> commit 5ad6e6c50899 ("net: bcmgenet: improve bcmgenet_mii_setup()")
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 3/4] net: bcmgenet: pull mac_config from adjust_link

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

This commit separates out the MAC configuration that occurs on a
PHY state change into a function named bcmgenet_mac_config().

This allows the function to be called directly elsewhere.

Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 94 ++++++++++----------
 1 file changed, 49 insertions(+), 45 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 8fce5878a7d9..789ca6212817 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -25,59 +25,63 @@
 
 #include "bcmgenet.h"
 
-/* setup netdev link state when PHY link status change and
- * update UMAC and RGMII block when link up
- */
-void bcmgenet_mii_setup(struct net_device *dev)
+static void bcmgenet_mac_config(struct net_device *dev)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct phy_device *phydev = dev->phydev;
 	u32 reg, cmd_bits = 0;
 
-	if (phydev->link) {
-		/* speed */
-		if (phydev->speed == SPEED_1000)
-			cmd_bits = CMD_SPEED_1000;
-		else if (phydev->speed == SPEED_100)
-			cmd_bits = CMD_SPEED_100;
-		else
-			cmd_bits = CMD_SPEED_10;
-		cmd_bits <<= CMD_SPEED_SHIFT;
-
-		/* duplex */
-		if (phydev->duplex != DUPLEX_FULL)
-			cmd_bits |= CMD_HD_EN;
-
-		/* pause capability */
-		if (!phydev->pause)
-			cmd_bits |= CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
-
-		/*
-		 * Program UMAC and RGMII block based on established
-		 * link speed, duplex, and pause. The speed set in
-		 * umac->cmd tell RGMII block which clock to use for
-		 * transmit -- 25MHz(100Mbps) or 125MHz(1Gbps).
-		 * Receive clock is provided by the PHY.
-		 */
-		reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL);
-		reg &= ~OOB_DISABLE;
-		reg |= RGMII_LINK;
-		bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL);
-
-		reg = bcmgenet_umac_readl(priv, UMAC_CMD);
-		reg &= ~((CMD_SPEED_MASK << CMD_SPEED_SHIFT) |
-			       CMD_HD_EN |
-			       CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE);
-		reg |= cmd_bits;
-		if (reg & CMD_SW_RESET) {
-			reg &= ~CMD_SW_RESET;
-			bcmgenet_umac_writel(priv, reg, UMAC_CMD);
-			udelay(2);
-			reg |= CMD_TX_EN | CMD_RX_EN;
-		}
+	/* speed */
+	if (phydev->speed == SPEED_1000)
+		cmd_bits = CMD_SPEED_1000;
+	else if (phydev->speed == SPEED_100)
+		cmd_bits = CMD_SPEED_100;
+	else
+		cmd_bits = CMD_SPEED_10;
+	cmd_bits <<= CMD_SPEED_SHIFT;
+
+	/* duplex */
+	if (phydev->duplex != DUPLEX_FULL)
+		cmd_bits |= CMD_HD_EN;
+
+	/* pause capability */
+	if (!phydev->pause)
+		cmd_bits |= CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
+
+	/* Program UMAC and RGMII block based on established
+	 * link speed, duplex, and pause. The speed set in
+	 * umac->cmd tell RGMII block which clock to use for
+	 * transmit -- 25MHz(100Mbps) or 125MHz(1Gbps).
+	 * Receive clock is provided by the PHY.
+	 */
+	reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL);
+	reg &= ~OOB_DISABLE;
+	reg |= RGMII_LINK;
+	bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL);
+
+	reg = bcmgenet_umac_readl(priv, UMAC_CMD);
+	reg &= ~((CMD_SPEED_MASK << CMD_SPEED_SHIFT) |
+		       CMD_HD_EN |
+		       CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE);
+	reg |= cmd_bits;
+	if (reg & CMD_SW_RESET) {
+		reg &= ~CMD_SW_RESET;
 		bcmgenet_umac_writel(priv, reg, UMAC_CMD);
+		udelay(2);
+		reg |= CMD_TX_EN | CMD_RX_EN;
 	}
+	bcmgenet_umac_writel(priv, reg, UMAC_CMD);
+}
+
+/* setup netdev link state when PHY link status change and
+ * update UMAC and RGMII block when link up
+ */
+void bcmgenet_mii_setup(struct net_device *dev)
+{
+	struct phy_device *phydev = dev->phydev;
 
+	if (phydev->link)
+		bcmgenet_mac_config(dev);
 	phy_print_status(phydev);
 }
 
-- 
2.25.1

Re: [PATCH net-next 3/4] net: bcmgenet: pull mac_config from adjust_link

[Andrew Lunn]

On Sat, Sep 25, 2021 at 08:21:13PM -0700, Florian Fainelli wrote:
> From: Doug Berger <opendmb@gmail.com>
> 
> This commit separates out the MAC configuration that occurs on a
> PHY state change into a function named bcmgenet_mac_config().
> 
> This allows the function to be called directly elsewhere.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 4/4] net: bcmgenet: add support for ethtool flow control

[Florian Fainelli]

From: Doug Berger <opendmb@gmail.com>

This commit extends the supported ethtool operations to allow MAC
level flow control to be configured for the bcmgenet driver.

The ethtool utility can be used to change the configuration of
auto-negotiated symmetric and asymmetric modes as well as manually
configuring support for RX and TX Pause frames individually.

Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../net/ethernet/broadcom/genet/bcmgenet.c    | 51 +++++++++++++++++++
 .../net/ethernet/broadcom/genet/bcmgenet.h    |  4 ++
 drivers/net/ethernet/broadcom/genet/bcmmii.c  | 44 +++++++++++++---
 3 files changed, 92 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 3427f9ed7eb9..6a8234bc9428 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -935,6 +935,48 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
 	return 0;
 }
 
+static void bcmgenet_get_pauseparam(struct net_device *dev,
+				    struct ethtool_pauseparam *epause)
+{
+	struct bcmgenet_priv *priv;
+	u32 umac_cmd;
+
+	priv = netdev_priv(dev);
+
+	epause->autoneg = priv->autoneg_pause;
+
+	if (netif_carrier_ok(dev)) {
+		/* report active state when link is up */
+		umac_cmd = bcmgenet_umac_readl(priv, UMAC_CMD);
+		epause->tx_pause = !(umac_cmd & CMD_TX_PAUSE_IGNORE);
+		epause->rx_pause = !(umac_cmd & CMD_RX_PAUSE_IGNORE);
+	} else {
+		/* otherwise report stored settings */
+		epause->tx_pause = priv->tx_pause;
+		epause->rx_pause = priv->rx_pause;
+	}
+}
+
+static int bcmgenet_set_pauseparam(struct net_device *dev,
+				   struct ethtool_pauseparam *epause)
+{
+	struct bcmgenet_priv *priv = netdev_priv(dev);
+
+	if (!dev->phydev)
+		return -ENODEV;
+
+	if (!phy_validate_pause(dev->phydev, epause))
+		return -EINVAL;
+
+	priv->autoneg_pause = !!epause->autoneg;
+	priv->tx_pause = !!epause->tx_pause;
+	priv->rx_pause = !!epause->rx_pause;
+
+	bcmgenet_phy_pause_set(dev, priv->rx_pause, priv->tx_pause);
+
+	return 0;
+}
+
 /* standard ethtool support functions. */
 enum bcmgenet_stat_type {
 	BCMGENET_STAT_NETDEV = -1,
@@ -1587,6 +1629,8 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
 	.get_ts_info		= ethtool_op_get_ts_info,
 	.get_rxnfc		= bcmgenet_get_rxnfc,
 	.set_rxnfc		= bcmgenet_set_rxnfc,
+	.get_pauseparam		= bcmgenet_get_pauseparam,
+	.set_pauseparam		= bcmgenet_set_pauseparam,
 };
 
 /* Power down the unimac, based on mode. */
@@ -3364,6 +3408,8 @@ static int bcmgenet_open(struct net_device *dev)
 		goto err_irq1;
 	}
 
+	bcmgenet_phy_pause_set(dev, priv->rx_pause, priv->tx_pause);
+
 	bcmgenet_netif_start(dev);
 
 	netif_tx_start_all_queues(dev);
@@ -3945,6 +3991,11 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	spin_lock_init(&priv->lock);
 
+	/* Set default pause parameters */
+	priv->autoneg_pause = 1;
+	priv->tx_pause = 1;
+	priv->rx_pause = 1;
+
 	SET_NETDEV_DEV(dev, &pdev->dev);
 	dev_set_drvdata(&pdev->dev, dev);
 	dev->watchdog_timeo = 2 * HZ;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 406249bc9fe5..1cc2838e52c6 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -594,6 +594,9 @@ struct bcmgenet_priv {
 
 	/* other misc variables */
 	struct bcmgenet_hw_params *hw_params;
+	unsigned autoneg_pause:1;
+	unsigned tx_pause:1;
+	unsigned rx_pause:1;
 
 	/* MDIO bus variables */
 	wait_queue_head_t wq;
@@ -686,6 +689,7 @@ int bcmgenet_mii_init(struct net_device *dev);
 int bcmgenet_mii_config(struct net_device *dev, bool init);
 int bcmgenet_mii_probe(struct net_device *dev);
 void bcmgenet_mii_exit(struct net_device *dev);
+void bcmgenet_phy_pause_set(struct net_device *dev, bool rx, bool tx);
 void bcmgenet_phy_power_set(struct net_device *dev, bool enable);
 void bcmgenet_mii_setup(struct net_device *dev);
 
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 789ca6212817..ad56f54eda0a 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -41,12 +41,29 @@ static void bcmgenet_mac_config(struct net_device *dev)
 	cmd_bits <<= CMD_SPEED_SHIFT;
 
 	/* duplex */
-	if (phydev->duplex != DUPLEX_FULL)
-		cmd_bits |= CMD_HD_EN;
+	if (phydev->duplex != DUPLEX_FULL) {
+		cmd_bits |= CMD_HD_EN |
+			CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
+	} else {
+		/* pause capability defaults to Symmetric */
+		if (priv->autoneg_pause) {
+			bool tx_pause = 0, rx_pause = 0;
+
+			if (phydev->autoneg)
+				phy_get_pause(phydev, &tx_pause, &rx_pause);
 
-	/* pause capability */
-	if (!phydev->pause)
-		cmd_bits |= CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
+			if (!tx_pause)
+				cmd_bits |= CMD_TX_PAUSE_IGNORE;
+			if (!rx_pause)
+				cmd_bits |= CMD_RX_PAUSE_IGNORE;
+		}
+
+		/* Manual override */
+		if (!priv->rx_pause)
+			cmd_bits |= CMD_RX_PAUSE_IGNORE;
+		if (!priv->tx_pause)
+			cmd_bits |= CMD_TX_PAUSE_IGNORE;
+	}
 
 	/* Program UMAC and RGMII block based on established
 	 * link speed, duplex, and pause. The speed set in
@@ -101,6 +118,21 @@ static int bcmgenet_fixed_phy_link_update(struct net_device *dev,
 	return 0;
 }
 
+void bcmgenet_phy_pause_set(struct net_device *dev, bool rx, bool tx)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->advertising, rx);
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->advertising,
+			 rx | tx);
+	phy_start_aneg(phydev);
+
+	mutex_lock(&phydev->lock);
+	if (phydev->link)
+		bcmgenet_mac_config(dev);
+	mutex_unlock(&phydev->lock);
+}
+
 void bcmgenet_phy_power_set(struct net_device *dev, bool enable)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
@@ -351,8 +383,6 @@ int bcmgenet_mii_probe(struct net_device *dev)
 		return ret;
 	}
 
-	linkmode_copy(phydev->advertising, phydev->supported);
-
 	/* The internal PHY has its link interrupts routed to the
 	 * Ethernet MAC ISRs. On GENETv5 there is a hardware issue
 	 * that prevents the signaling of link UP interrupts when
-- 
2.25.1

Re: [PATCH net-next 4/4] net: bcmgenet: add support for ethtool flow control

[Andrew Lunn]

On Sat, Sep 25, 2021 at 08:21:14PM -0700, Florian Fainelli wrote:
> From: Doug Berger <opendmb@gmail.com>
> 
> This commit extends the supported ethtool operations to allow MAC
> level flow control to be configured for the bcmgenet driver.
> 
> The ethtool utility can be used to change the configuration of
> auto-negotiated symmetric and asymmetric modes as well as manually
> configuring support for RX and TX Pause frames individually.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  .../net/ethernet/broadcom/genet/bcmgenet.c    | 51 +++++++++++++++++++
>  .../net/ethernet/broadcom/genet/bcmgenet.h    |  4 ++
>  drivers/net/ethernet/broadcom/genet/bcmmii.c  | 44 +++++++++++++---
>  3 files changed, 92 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 3427f9ed7eb9..6a8234bc9428 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -935,6 +935,48 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>  	return 0;
>  }
>  
> +static void bcmgenet_get_pauseparam(struct net_device *dev,
> +				    struct ethtool_pauseparam *epause)
> +{
> +	struct bcmgenet_priv *priv;
> +	u32 umac_cmd;
> +
> +	priv = netdev_priv(dev);
> +
> +	epause->autoneg = priv->autoneg_pause;
> +
> +	if (netif_carrier_ok(dev)) {
> +		/* report active state when link is up */
> +		umac_cmd = bcmgenet_umac_readl(priv, UMAC_CMD);
> +		epause->tx_pause = !(umac_cmd & CMD_TX_PAUSE_IGNORE);
> +		epause->rx_pause = !(umac_cmd & CMD_RX_PAUSE_IGNORE);
> +	} else {
> +		/* otherwise report stored settings */
> +		epause->tx_pause = priv->tx_pause;
> +		epause->rx_pause = priv->rx_pause;
> +	}
> +}
> +
> +static int bcmgenet_set_pauseparam(struct net_device *dev,
> +				   struct ethtool_pauseparam *epause)
> +{
> +	struct bcmgenet_priv *priv = netdev_priv(dev);
> +
> +	if (!dev->phydev)
> +		return -ENODEV;
> +
> +	if (!phy_validate_pause(dev->phydev, epause))
> +		return -EINVAL;
> +
> +	priv->autoneg_pause = !!epause->autoneg;
> +	priv->tx_pause = !!epause->tx_pause;
> +	priv->rx_pause = !!epause->rx_pause;
> +
> +	bcmgenet_phy_pause_set(dev, priv->rx_pause, priv->tx_pause);

I don't think this is correct. If epause->autoneg is false, you
probably want to pass false, false here, so that the PHY will not
announce any modes. And then call bcmgenet_mac_config() to set the
manual pause bits. But watch out that you don't hold the PHY lock, so
you should not access any phydev members.

> +	} else {
> +		/* pause capability defaults to Symmetric */
> +		if (priv->autoneg_pause) {
> +			bool tx_pause = 0, rx_pause = 0;
> +
> +			if (phydev->autoneg)
> +				phy_get_pause(phydev, &tx_pause, &rx_pause);
>  
> -	/* pause capability */
> -	if (!phydev->pause)
> -		cmd_bits |= CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
> +			if (!tx_pause)
> +				cmd_bits |= CMD_TX_PAUSE_IGNORE;
> +			if (!rx_pause)
> +				cmd_bits |= CMD_RX_PAUSE_IGNORE;
> +		}

Looks like there should be an else here?

> +
> +		/* Manual override */
> +		if (!priv->rx_pause)
> +			cmd_bits |= CMD_RX_PAUSE_IGNORE;
> +		if (!priv->tx_pause)
> +			cmd_bits |= CMD_TX_PAUSE_IGNORE;
> +	}
>  
>  	/* Program UMAC and RGMII block based on established
>  	 * link speed, duplex, and pause. The speed set in
> @@ -101,6 +118,21 @@ static int bcmgenet_fixed_phy_link_update(struct net_device *dev,
>  	return 0;
>  }
>  
> +void bcmgenet_phy_pause_set(struct net_device *dev, bool rx, bool tx)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->advertising, rx);
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->advertising,
> +			 rx | tx);
> +	phy_start_aneg(phydev);
> +
> +	mutex_lock(&phydev->lock);
> +	if (phydev->link)
> +		bcmgenet_mac_config(dev);
> +	mutex_unlock(&phydev->lock);

It is a bit oddly named, but phy_set_asym_pause() does this, minus the
lock. Please use that, rather than open coding this.

Locking is something i'm looking at now. I'm trying to go through all
the phylib calls the MAC use and checking if locks need to be added.

    Andrew

Re: [PATCH net-next 4/4] net: bcmgenet: add support for ethtool flow control

[Doug Berger]

Thank you for your review, and sorry for the delayed response (Florian
submitted this while I was on vacation).

You may remember that a while back I submitted a more general patch set
with the goal of improving the implementation of ethernet pause for all
network drivers while maintaining backward compatibility for network
drivers that preferred their current behavior:
https://www.lkml.org/lkml/2020/5/11/1408

I would summarize the previous discussion as follows:
Russell King has kindly documented the known deficiencies with the
current common implementation of ethernet pause support, and believes
that it is necessary to live with them to provide the consistency
necessary for his phylink implementation.

This leaves me in the position of having to choose between consistency
and IEEE standard compliance for the bcmgenet driver that I co-maintain
with Florian Fainelli. Having spent decades of my career focused on
producing IEEE 802 standard compliant implementations it is difficult
for me to accept submitting an implementation of ethernet pause frame
support that I believe does not comply with the IEEE 802.3 standard.

Consistency with other drivers interpretations of ethtool flow control
is not particularly relevant to the users of current systems that make
use of the bcmgenet driver. As a result we have chosen to implement
ethtool flow control for the bcmgenet driver in our downstream kernels
in the manner documented by this patch set, which favors correctness
over consistency.

Florian would like this implementation to be added to the upstream
kernel to benefit other potential users and to ease a minor maintenance
burden for us.

It would probably be useful to include a more complete description of
the behavior of this implementation in the commit message of this fourth
part of the patch, and I can do that in a resubmission if desired.

Here is the description I provided in the email discussion of the
previous submission:
"The Pause and AsymPause bits as defined by the IEEE 802.3 standard are
for the purpose of advertising a capability. While the Tx_Pause and
Rx_Pause parameters of ethtool allow a user to indicate whether the
feature should be used on a link that is capable of the feature.

When pause autonegotiation is enabled the local and peer Pause and
AsymPause bits should be used to negotiate the CAPABILITY of using the
pause feature for each direction. This is not the same as enabling pause
in those directions.

So for the problematic cases:

If you specify Tx_Pause = 0, Rx_Pause = 1 you advertise that the link is
capable of both Symmetric PAUSE and Asymmetric PAUSE toward local device
according to Table 37-2 in IEEE Std 802.3-2018. If the result of link
autonegotiation indicates that both directions are capable of supporting
pause control frames you choose not to send pause control frames because
the user asked you not to by setting Tx_Pause = 0.

If you specify Tx_Pause = 1, Rx_Pause = 1 you advertise that the link is
capable of both Symmetric PAUSE and Asymmetric PAUSE toward local device
according to Table 37-2 in IEEE Std 802.3-2018. If the far end supports
only AsymPause, then the link autonegotiation will indicate that only
the receive direction is capable of supporting the pause feature and you
should not send pause control frames to the peer even though the user
has set Tx_Pause = 1.

If link autonegotiation is disabled, then the capability of the link to
support pause frames cannot be negotiated and therefore pause control
frames should not be used.

When pause autonegotiation is disabled the local peer does not care what
its peer is capable of and it can choose to send and process pause
control frames based entirely, on the users requested Tx_Pause and
Rx_Pause parameters. However, if link autonegotiation is enabled it
might as well not be rude and should advertise its intended usage."

Responses to feedback below.

On 9/26/2021 7:26 AM, Andrew Lunn wrote:
> On Sat, Sep 25, 2021 at 08:21:14PM -0700, Florian Fainelli wrote:
>> From: Doug Berger <opendmb@gmail.com>
>>
>> This commit extends the supported ethtool operations to allow MAC
>> level flow control to be configured for the bcmgenet driver.
>>
>> The ethtool utility can be used to change the configuration of
>> auto-negotiated symmetric and asymmetric modes as well as manually
>> configuring support for RX and TX Pause frames individually.
>>
>> Signed-off-by: Doug Berger <opendmb@gmail.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  .../net/ethernet/broadcom/genet/bcmgenet.c    | 51 +++++++++++++++++++
>>  .../net/ethernet/broadcom/genet/bcmgenet.h    |  4 ++
>>  drivers/net/ethernet/broadcom/genet/bcmmii.c  | 44 +++++++++++++---
>>  3 files changed, 92 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index 3427f9ed7eb9..6a8234bc9428 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -935,6 +935,48 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>>  	return 0;
>>  }
>>  
>> +static void bcmgenet_get_pauseparam(struct net_device *dev,
>> +				    struct ethtool_pauseparam *epause)
>> +{
>> +	struct bcmgenet_priv *priv;
>> +	u32 umac_cmd;
>> +
>> +	priv = netdev_priv(dev);
>> +
>> +	epause->autoneg = priv->autoneg_pause;
>> +
>> +	if (netif_carrier_ok(dev)) {
>> +		/* report active state when link is up */
>> +		umac_cmd = bcmgenet_umac_readl(priv, UMAC_CMD);
>> +		epause->tx_pause = !(umac_cmd & CMD_TX_PAUSE_IGNORE);
>> +		epause->rx_pause = !(umac_cmd & CMD_RX_PAUSE_IGNORE);
>> +	} else {
>> +		/* otherwise report stored settings */
>> +		epause->tx_pause = priv->tx_pause;
>> +		epause->rx_pause = priv->rx_pause;
>> +	}
>> +}
>> +
>> +static int bcmgenet_set_pauseparam(struct net_device *dev,
>> +				   struct ethtool_pauseparam *epause)
>> +{
>> +	struct bcmgenet_priv *priv = netdev_priv(dev);
>> +
>> +	if (!dev->phydev)
>> +		return -ENODEV;
>> +
>> +	if (!phy_validate_pause(dev->phydev, epause))
>> +		return -EINVAL;
>> +
>> +	priv->autoneg_pause = !!epause->autoneg;
>> +	priv->tx_pause = !!epause->tx_pause;
>> +	priv->rx_pause = !!epause->rx_pause;
>> +
>> +	bcmgenet_phy_pause_set(dev, priv->rx_pause, priv->tx_pause);
> 
> I don't think this is correct. If epause->autoneg is false, you
> probably want to pass false, false here, so that the PHY will not
> announce any modes. And then call bcmgenet_mac_config() to set the
> manual pause bits. But watch out that you don't hold the PHY lock, so
> you should not access any phydev members.
As noted above, it is my belief that when epause->autoneg is false it is
more polite for the local node to advertise the mode it will be using
even if it doesn't respect its peer's advertised capability. This at
least gives the peer the opportunity to negotiate its pause behavior if
it happens to be running Linux and its epause->autoneg is true.

I also do hold the PHY lock within bcmgenet_phy_pause_set() below.

> 
>> +	} else {
>> +		/* pause capability defaults to Symmetric */
>> +		if (priv->autoneg_pause) {
>> +			bool tx_pause = 0, rx_pause = 0;
>> +
>> +			if (phydev->autoneg)
>> +				phy_get_pause(phydev, &tx_pause, &rx_pause);
>>  
>> -	/* pause capability */
>> -	if (!phydev->pause)
>> -		cmd_bits |= CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
>> +			if (!tx_pause)
>> +				cmd_bits |= CMD_TX_PAUSE_IGNORE;
>> +			if (!rx_pause)
>> +				cmd_bits |= CMD_RX_PAUSE_IGNORE;
>> +		}
> 
> Looks like there should be an else here?
It may look like that is the case, but it is not necessary. The cmd_bits
are initialized to enable tx and rx (as the comment is intended to
clarify). If autoneg_pause is true then the negotiation will disable
pause where the capability does not exist. Regardless of autoneg_pause
if the user does not want to use pause it should not be enabled.

> 
>> +
>> +		/* Manual override */
>> +		if (!priv->rx_pause)
>> +			cmd_bits |= CMD_RX_PAUSE_IGNORE;
>> +		if (!priv->tx_pause)
>> +			cmd_bits |= CMD_TX_PAUSE_IGNORE;
>> +	}
>>  
>>  	/* Program UMAC and RGMII block based on established
>>  	 * link speed, duplex, and pause. The speed set in
>> @@ -101,6 +118,21 @@ static int bcmgenet_fixed_phy_link_update(struct net_device *dev,
>>  	return 0;
>>  }
>>  
>> +void bcmgenet_phy_pause_set(struct net_device *dev, bool rx, bool tx)
>> +{
>> +	struct phy_device *phydev = dev->phydev;
>> +
>> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->advertising, rx);
>> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->advertising,
>> +			 rx | tx);
>> +	phy_start_aneg(phydev);
>> +
>> +	mutex_lock(&phydev->lock);
>> +	if (phydev->link)
>> +		bcmgenet_mac_config(dev);
>> +	mutex_unlock(&phydev->lock);
> 
> It is a bit oddly named, but phy_set_asym_pause() does this, minus the
> lock. Please use that, rather than open coding this.
This is, in fact, the crux of the matter. It is subtle, but
phy_set_asym_pause() does NOT do this. phy_set_asym_pause() uses an
EXCLUSIVE OR of rx and tx to set Asym_Pause which leads to incorrect
advertisement of capability. That is why this code needs to use an
INCLUSIVE OR of rx and tx to comply with the IEEE standard.

> 
> Locking is something i'm looking at now. I'm trying to go through all
> the phylib calls the MAC use and checking if locks need to be added.
> 
>     Andrew
> 

Thanks again for your time (and patience ;),
    Doug

Re: [PATCH net-next 4/4] net: bcmgenet: add support for ethtool flow control

[Florian Fainelli]

Hi Andrew, and Doug,

On 10/12/21 12:13 PM, Doug Berger wrote:
> Thank you for your review, and sorry for the delayed response (Florian
> submitted this while I was on vacation).
> 
> You may remember that a while back I submitted a more general patch set
> with the goal of improving the implementation of ethernet pause for all
> network drivers while maintaining backward compatibility for network
> drivers that preferred their current behavior:
> https://www.lkml.org/lkml/2020/5/11/1408
> 
> I would summarize the previous discussion as follows:
> Russell King has kindly documented the known deficiencies with the
> current common implementation of ethernet pause support, and believes
> that it is necessary to live with them to provide the consistency
> necessary for his phylink implementation.
> 
> This leaves me in the position of having to choose between consistency
> and IEEE standard compliance for the bcmgenet driver that I co-maintain
> with Florian Fainelli. Having spent decades of my career focused on
> producing IEEE 802 standard compliant implementations it is difficult
> for me to accept submitting an implementation of ethernet pause frame
> support that I believe does not comply with the IEEE 802.3 standard.
> 
> Consistency with other drivers interpretations of ethtool flow control
> is not particularly relevant to the users of current systems that make
> use of the bcmgenet driver. As a result we have chosen to implement
> ethtool flow control for the bcmgenet driver in our downstream kernels
> in the manner documented by this patch set, which favors correctness
> over consistency.
> 
> Florian would like this implementation to be added to the upstream
> kernel to benefit other potential users and to ease a minor maintenance
> burden for us.
> 
> It would probably be useful to include a more complete description of
> the behavior of this implementation in the commit message of this fourth
> part of the patch, and I can do that in a resubmission if desired.
> 
> Here is the description I provided in the email discussion of the
> previous submission:
> "The Pause and AsymPause bits as defined by the IEEE 802.3 standard are
> for the purpose of advertising a capability. While the Tx_Pause and
> Rx_Pause parameters of ethtool allow a user to indicate whether the
> feature should be used on a link that is capable of the feature.
> 
> When pause autonegotiation is enabled the local and peer Pause and
> AsymPause bits should be used to negotiate the CAPABILITY of using the
> pause feature for each direction. This is not the same as enabling pause
> in those directions.
> 
> So for the problematic cases:
> 
> If you specify Tx_Pause = 0, Rx_Pause = 1 you advertise that the link is
> capable of both Symmetric PAUSE and Asymmetric PAUSE toward local device
> according to Table 37-2 in IEEE Std 802.3-2018. If the result of link
> autonegotiation indicates that both directions are capable of supporting
> pause control frames you choose not to send pause control frames because
> the user asked you not to by setting Tx_Pause = 0.
> 
> If you specify Tx_Pause = 1, Rx_Pause = 1 you advertise that the link is
> capable of both Symmetric PAUSE and Asymmetric PAUSE toward local device
> according to Table 37-2 in IEEE Std 802.3-2018. If the far end supports
> only AsymPause, then the link autonegotiation will indicate that only
> the receive direction is capable of supporting the pause feature and you
> should not send pause control frames to the peer even though the user
> has set Tx_Pause = 1.
> 
> If link autonegotiation is disabled, then the capability of the link to
> support pause frames cannot be negotiated and therefore pause control
> frames should not be used.
> 
> When pause autonegotiation is disabled the local peer does not care what
> its peer is capable of and it can choose to send and process pause
> control frames based entirely, on the users requested Tx_Pause and
> Rx_Pause parameters. However, if link autonegotiation is enabled it
> might as well not be rude and should advertise its intended usage."
> 
> Responses to feedback below.
> 
> On 9/26/2021 7:26 AM, Andrew Lunn wrote:
>> On Sat, Sep 25, 2021 at 08:21:14PM -0700, Florian Fainelli wrote:
>>> From: Doug Berger <opendmb@gmail.com>
>>>
>>> This commit extends the supported ethtool operations to allow MAC
>>> level flow control to be configured for the bcmgenet driver.
>>>
>>> The ethtool utility can be used to change the configuration of
>>> auto-negotiated symmetric and asymmetric modes as well as manually
>>> configuring support for RX and TX Pause frames individually.
>>>
>>> Signed-off-by: Doug Berger <opendmb@gmail.com>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>>  .../net/ethernet/broadcom/genet/bcmgenet.c    | 51 +++++++++++++++++++
>>>  .../net/ethernet/broadcom/genet/bcmgenet.h    |  4 ++
>>>  drivers/net/ethernet/broadcom/genet/bcmmii.c  | 44 +++++++++++++---
>>>  3 files changed, 92 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> index 3427f9ed7eb9..6a8234bc9428 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> @@ -935,6 +935,48 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>>>  	return 0;
>>>  }
>>>  
>>> +static void bcmgenet_get_pauseparam(struct net_device *dev,
>>> +				    struct ethtool_pauseparam *epause)
>>> +{
>>> +	struct bcmgenet_priv *priv;
>>> +	u32 umac_cmd;
>>> +
>>> +	priv = netdev_priv(dev);
>>> +
>>> +	epause->autoneg = priv->autoneg_pause;
>>> +
>>> +	if (netif_carrier_ok(dev)) {
>>> +		/* report active state when link is up */
>>> +		umac_cmd = bcmgenet_umac_readl(priv, UMAC_CMD);
>>> +		epause->tx_pause = !(umac_cmd & CMD_TX_PAUSE_IGNORE);
>>> +		epause->rx_pause = !(umac_cmd & CMD_RX_PAUSE_IGNORE);
>>> +	} else {
>>> +		/* otherwise report stored settings */
>>> +		epause->tx_pause = priv->tx_pause;
>>> +		epause->rx_pause = priv->rx_pause;
>>> +	}
>>> +}
>>> +
>>> +static int bcmgenet_set_pauseparam(struct net_device *dev,
>>> +				   struct ethtool_pauseparam *epause)
>>> +{
>>> +	struct bcmgenet_priv *priv = netdev_priv(dev);
>>> +
>>> +	if (!dev->phydev)
>>> +		return -ENODEV;
>>> +
>>> +	if (!phy_validate_pause(dev->phydev, epause))
>>> +		return -EINVAL;
>>> +
>>> +	priv->autoneg_pause = !!epause->autoneg;
>>> +	priv->tx_pause = !!epause->tx_pause;
>>> +	priv->rx_pause = !!epause->rx_pause;
>>> +
>>> +	bcmgenet_phy_pause_set(dev, priv->rx_pause, priv->tx_pause);
>>
>> I don't think this is correct. If epause->autoneg is false, you
>> probably want to pass false, false here, so that the PHY will not
>> announce any modes. And then call bcmgenet_mac_config() to set the
>> manual pause bits. But watch out that you don't hold the PHY lock, so
>> you should not access any phydev members.
> As noted above, it is my belief that when epause->autoneg is false it is
> more polite for the local node to advertise the mode it will be using
> even if it doesn't respect its peer's advertised capability. This at
> least gives the peer the opportunity to negotiate its pause behavior if
> it happens to be running Linux and its epause->autoneg is true.
> 
> I also do hold the PHY lock within bcmgenet_phy_pause_set() below.
> 
>>
>>> +	} else {
>>> +		/* pause capability defaults to Symmetric */
>>> +		if (priv->autoneg_pause) {
>>> +			bool tx_pause = 0, rx_pause = 0;
>>> +
>>> +			if (phydev->autoneg)
>>> +				phy_get_pause(phydev, &tx_pause, &rx_pause);
>>>  
>>> -	/* pause capability */
>>> -	if (!phydev->pause)
>>> -		cmd_bits |= CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
>>> +			if (!tx_pause)
>>> +				cmd_bits |= CMD_TX_PAUSE_IGNORE;
>>> +			if (!rx_pause)
>>> +				cmd_bits |= CMD_RX_PAUSE_IGNORE;
>>> +		}
>>
>> Looks like there should be an else here?
> It may look like that is the case, but it is not necessary. The cmd_bits
> are initialized to enable tx and rx (as the comment is intended to
> clarify). If autoneg_pause is true then the negotiation will disable
> pause where the capability does not exist. Regardless of autoneg_pause
> if the user does not want to use pause it should not be enabled.

Maybe a comment should be in place to prevent a drive by reviewer from
thinking that there should be an else being placed here?

> 
>>
>>> +
>>> +		/* Manual override */
>>> +		if (!priv->rx_pause)
>>> +			cmd_bits |= CMD_RX_PAUSE_IGNORE;
>>> +		if (!priv->tx_pause)
>>> +			cmd_bits |= CMD_TX_PAUSE_IGNORE;
>>> +	}
>>>  
>>>  	/* Program UMAC and RGMII block based on established
>>>  	 * link speed, duplex, and pause. The speed set in
>>> @@ -101,6 +118,21 @@ static int bcmgenet_fixed_phy_link_update(struct net_device *dev,
>>>  	return 0;
>>>  }
>>>  
>>> +void bcmgenet_phy_pause_set(struct net_device *dev, bool rx, bool tx)
>>> +{
>>> +	struct phy_device *phydev = dev->phydev;
>>> +
>>> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->advertising, rx);
>>> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->advertising,
>>> +			 rx | tx);
>>> +	phy_start_aneg(phydev);
>>> +
>>> +	mutex_lock(&phydev->lock);
>>> +	if (phydev->link)
>>> +		bcmgenet_mac_config(dev);
>>> +	mutex_unlock(&phydev->lock);
>>
>> It is a bit oddly named, but phy_set_asym_pause() does this, minus the
>> lock. Please use that, rather than open coding this.
> This is, in fact, the crux of the matter. It is subtle, but
> phy_set_asym_pause() does NOT do this. phy_set_asym_pause() uses an
> EXCLUSIVE OR of rx and tx to set Asym_Pause which leads to incorrect
> advertisement of capability. That is why this code needs to use an
> INCLUSIVE OR of rx and tx to comply with the IEEE standard.

Would it be worthwhile introducing something like this then (not compile
tested, comments not updated):

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index f1db6699f81f..3465db9a5769 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -3114,7 +3114,7 @@ static void hclge_update_pause_advertising(struct
hclge_dev *hdev)
 		break;
 	}

-	linkmode_set_pause(mac->advertising, tx_en, rx_en);
+	linkmode_set_pause(mac->advertising, tx_en, rx_en, false);
 }

 static void hclge_update_advertising(struct hclge_dev *hdev)
diff --git a/drivers/net/phy/linkmode.c b/drivers/net/phy/linkmode.c
index f60560fe3499..96582eb65ca0 100644
--- a/drivers/net/phy/linkmode.c
+++ b/drivers/net/phy/linkmode.c
@@ -48,6 +48,7 @@ EXPORT_SYMBOL_GPL(linkmode_resolve_pause);
  * @advertisement: advertisement in ethtool format
  * @tx: boolean from ethtool struct ethtool_pauseparam tx_pause member
  * @rx: boolean from ethtool struct ethtool_pauseparam rx_pause member
+ * @ieee_compliant: Resolve according to IEEE 802.3-2018
  *
  * Configure the advertised Pause and Asym_Pause bits according to the
  * capabilities of provided in @tx and @rx.
@@ -86,10 +87,14 @@ EXPORT_SYMBOL_GPL(linkmode_resolve_pause);
  *  rx=1 tx=1 gives Pause only, which will only allow tx+rx pause
  *            if the other end also advertises Pause.
  */
-void linkmode_set_pause(unsigned long *advertisement, bool tx, bool rx)
+void linkmode_set_pause(unsigned long *advertisement, bool tx, bool rx,
+			bool ieee_compliant)
 {
+	if (!ieee_compliant)
+		mode = rx ^ tx;
+	else
+		mode = rx | tx;
 	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertisement, rx);
-	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertisement,
-			 rx ^ tx);
+	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertisement, mode);
 }
 EXPORT_SYMBOL_GPL(linkmode_set_pause);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 74d8e1dc125f..56265ec6a41b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2731,6 +2731,18 @@ void phy_set_sym_pause(struct phy_device *phydev,
bool rx, bool tx,
 }
 EXPORT_SYMBOL(phy_set_sym_pause);

+static void __phy_set_asym_pause(struct phy_device *phydev, bool rx,
bool tx,
+				 bool ieee_compliant)
+{
+
+	linkmode_copy(oldadv, phydev->advertising);
+	linkmode_set_pause(phydev->advertising, tx, rx, ieee_compliant);
+
+	if (!linkmode_equal(oldadv, phydev->advertising) &&
+	    phydev->autoneg)
+		phy_start_aneg(phydev);
+}
+
 /**
  * phy_set_asym_pause - Configure Pause and Asym Pause
  * @phydev: target phy_device struct
@@ -2744,17 +2756,27 @@ EXPORT_SYMBOL(phy_set_sym_pause);
  */
 void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(oldadv);
-
-	linkmode_copy(oldadv, phydev->advertising);
-	linkmode_set_pause(phydev->advertising, tx, rx);
-
-	if (!linkmode_equal(oldadv, phydev->advertising) &&
-	    phydev->autoneg)
-		phy_start_aneg(phydev);
+	__phy_set_asym_pause(phydev, rx, tx, false);
 }
 EXPORT_SYMBOL(phy_set_asym_pause);

+/**
+ * phy_set_asym_pause_ieee - Configure Pause and Asym Pause in IEEE
compliance mode
+ * @phydev: target phy_device struct
+ * @rx: Receiver Pause is supported
+ * @tx: Transmit Pause is supported
+ *
+ * Description: Configure advertised Pause support depending on if
+ * transmit and receiver pause is supported. If there has been a
+ * change in adverting, trigger a new autoneg. Generally called from
+ * the set_pauseparam .ndo.
+ */
+void phy_set_asym_pause_ieee(struct phy_device *phydev, bool rx, bool tx)
+{
+	__phy_set_asym_pause(phydev, rx, tx, true);
+}
+EXPORT_SYMBOL(phy_set_asym_pause_ieee);
+
 /**
  * phy_validate_pause - Test if the PHY/MAC support the pause configuration
  * @phydev: phy_device struct
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 14c7d73790b4..443f383d3589 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1770,7 +1770,7 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
 	 * rx/tx pause resolution.
 	 */
 	linkmode_set_pause(config->advertising, pause->tx_pause,
-			   pause->rx_pause);
+			   pause->rx_pause, true);

 	manual_changed = (config->pause ^ pause_state) & MLO_PAUSE_AN ||
 			 (!(pause_state & MLO_PAUSE_AN) &&
diff --git a/include/linux/linkmode.h b/include/linux/linkmode.h
index f8397f300fcd..2bce29cb2605 100644
--- a/include/linux/linkmode.h
+++ b/include/linux/linkmode.h
@@ -98,6 +98,7 @@ void linkmode_resolve_pause(const unsigned long
*local_adv,
 			    const unsigned long *partner_adv,
 			    bool *tx_pause, bool *rx_pause);

-void linkmode_set_pause(unsigned long *advertisement, bool tx, bool rx);
+void linkmode_set_pause(unsigned long *advertisement, bool tx, bool rx,
+			bool ieee_compliant);

 #endif /* __LINKMODE_H */



Andrew what do you think?
-- 
Florian