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  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]
Date:   Wed, 13 May 2020 10:52:46 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Doug Berger <opendmb@...il.com>
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 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 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?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Powered by blists - more mailing lists