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

Commit Message

Florian Fainelli Sept. 26, 2021, 3:21 a.m. UTC

From: Doug Berger <[e-mail]>

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 <[e-mail]>
Signed-off-by: Florian Fainelli <[e-mail]>
---
 .../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(-)

Comments

Andrew Lunn Sept. 26, 2021, 2:26 p.m. UTC | #1

On Sat, Sep 25, 2021 at 08:21:14PM -0700, Florian Fainelli wrote:
> From: Doug Berger <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Florian Fainelli <[e-mail]>
> ---
>  .../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

Doug Berger Oct. 12, 2021, 7:13 p.m. UTC | #2

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 <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Florian Fainelli <[e-mail]>
>> ---
>>  .../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

Florian Fainelli Oct. 25, 2021, 5:15 p.m. UTC | #3

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Florian Fainelli <[e-mail]>
>>> ---
>>>  .../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?

Patch

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