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

Florian Fainelli
2021-10-25T17:16:03Z

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