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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ce0c0cd6-3532-be90-1307-5ccd8e3b02a4@gmail.com>
Date:   Wed, 13 May 2020 15:00:37 -0700
From:   Doug Berger <opendmb@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 4/4] net: bcmgenet: add support for ethtool flow
 control

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.