On 5/13/2020 2:52 AM, Russell King - ARM Linux admin wrote:
> On Mon, May 11, 2020 at 05:24:10PM -0700, Doug Berger wrote:
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> index 511d553a4d11..788da1ecea0c 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> @@ -25,6 +25,21 @@
>>
>> #include "bcmgenet.h"
>>
>> +static u32 _flow_control_autoneg(struct phy_device *phydev)
>> +{
>> + bool tx_pause, rx_pause;
>> + u32 cmd_bits = 0;
>> +
>> + phy_get_pause(phydev, &tx_pause, &rx_pause);
>> +
>> + if (!tx_pause)
>> + cmd_bits |= CMD_TX_PAUSE_IGNORE;
>> + if (!rx_pause)
>> + cmd_bits |= CMD_RX_PAUSE_IGNORE;
>> +
>> + return cmd_bits;
>> +}
>> +
>> /* setup netdev link state when PHY link status change and
>> * update UMAC and RGMII block when link up
>> */
>> @@ -71,12 +86,20 @@ void bcmgenet_mii_setup(struct net_device *dev)
>> 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;
>> + if (phydev->duplex != DUPLEX_FULL) {
>> + cmd_bits |= CMD_HD_EN |
>> + CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
>
> phy_get_pause() already takes account of whether the PHY is in half
> duplex mode. So:
>
> bool tx_pause, rx_pause;
>
> if (phydev->autoneg && priv->autoneg_pause) {
> phy_get_pause(phydev, &tx_pause, &rx_pause);
> } else if (phydev->duplex == DUPLEX_FULL) {
> tx_pause = priv->tx_pause;
> rx_pause = priv->rx_pause;
> } else {
> tx_pause = false;
> rx_pause = false;
> }
>
> if (!tx_pause)
> cmd_bits |= CMD_TX_PAUSE_IGNORE;
> if (!rx_pause)
> cmd_bits |= CMD_RX_PAUSE_IGNORE;
>
> would be entirely sufficient here.
I need to check the duplex to configure the HD bit in the same register
with the pause controls so I am covering both with one compare.
This also includes a bug here that I mentioned in one of my responses to
the first patch of the set. The call to phy_get_pause() should only be
conditioned on phydev->autoneg_pause here.
The idea is that both directions of pause default to on, but if
autoneg_pause is on then phy_get_pause() has an opportunity to disable
any direction if the capability can't be negotiated for that direction.
Finally, the pause feature can be explicitly disabled by the tx_pause
and rx_pause parameters.
> I wonder whether your implementation (which mine follows) is really
> correct though. Consider this:
>
> # ethtool -A eth0 autoneg on tx on rx on
> # ethtool -s eth0 autoneg off speed 1000 duplex full
>
> At this point, what do you expect the resulting pause state to be? It
> may not be what you actually think it should be - it will be tx and rx
> pause enabled (it's easier to see why that happens with my rewritten
> version of your implementation, which is functionally identical.)
>
> If we take the view that if link autoneg is disabled, and therefore the
> link partner's advertisement is zero, shouldn't it result in tx and rx
> pause being disabled?
So with the bug fixed as described above, after these commands
autoneg_pause would be on and autoneg would be off. The logic would call
phy_get_pause() which should return tx_pause = rx_pause = false turning
pause off in both directions.
