Messages in this thread Patch in this message | | | From | Florian Fainelli <> | Subject | Re: [PATCH net-next 4/4] net: bcmgenet: add support for ethtool flow control | Date | Mon, 25 Oct 2021 10:15:57 -0700 |
| |
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
| |