Re: [PATCH net-next 02/10] net: add helpers to resolve negotiated flow control

[Andrew Lunn <andrew@lunn.ch>]

On Sat, Feb 15, 2020 at 03:49:27PM +0000, Russell King wrote:
> Add a couple of helpers to resolve negotiated flow control. Two helpers
> are provided:
> 
> - linkmode_resolve_pause() which takes the link partner and local
>   advertisements, and decodes whether we should enable TX or RX pause
>   at the MAC. This is useful outside of phylib, e.g. in phylink.
> - phy_get_pause(), which returns the TX/RX enablement status for the
>   current negotiation results of the PHY.
> 
> This allows us to centralise the flow control resolution, rather than
> spreading it around.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/Makefile     |  3 ++-
>  drivers/net/phy/linkmode.c   | 44 ++++++++++++++++++++++++++++++++++++
>  drivers/net/phy/phy_device.c | 26 +++++++++++++++++++++
>  include/linux/linkmode.h     |  4 ++++
>  include/linux/phy.h          |  3 +++
>  5 files changed, 79 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/phy/linkmode.c
> 
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index fe5badf13b65..d523fd5670e4 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -1,7 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Makefile for Linux PHY drivers and MDIO bus drivers
>  
> -libphy-y			:= phy.o phy-c45.o phy-core.o phy_device.o
> +libphy-y			:= phy.o phy-c45.o phy-core.o phy_device.o \
> +				   linkmode.o
>  mdio-bus-y			+= mdio_bus.o mdio_device.o
>  
>  ifdef CONFIG_MDIO_DEVICE
> diff --git a/drivers/net/phy/linkmode.c b/drivers/net/phy/linkmode.c
> new file mode 100644
> index 000000000000..969918795228
> --- /dev/null
> +++ b/drivers/net/phy/linkmode.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <linux/linkmode.h>
> +
> +/**
> + * linkmode_resolve_pause - resolve the allowable pause modes
> + * @local_adv: local advertisement in ethtool format
> + * @partner_adv: partner advertisement in ethtool format
> + * @tx_pause: pointer to bool to indicate whether transmit pause should be
> + * enabled.
> + * @rx_pause: pointer to bool to indicate whether receive pause should be
> + * enabled.
> + *
> + * Flow control is resolved according to our and the link partners
> + * advertisements using the following drawn from the 802.3 specs:
> + *  Local device  Link partner
> + *  Pause AsymDir Pause AsymDir Result
> + *    0     X       0     X     Disabled
> + *    0     1       1     0     Disabled
> + *    0     1       1     1     TX
> + *    1     0       0     X     Disabled
> + *    1     X       1     X     TX+RX
> + *    1     1       0     1     RX
> + */
> +void linkmode_resolve_pause(const unsigned long *local_adv,
> +			    const unsigned long *partner_adv,
> +			    bool *tx_pause, bool *rx_pause)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(m);
> +
> +	linkmode_and(m, local_adv, partner_adv);
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, m)) {
> +		*tx_pause = true;
> +		*rx_pause = true;
> +	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, m)) {
> +		*tx_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +					      partner_adv);
> +		*rx_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +					      local_adv);
> +	} else {
> +		*tx_pause = false;
> +		*rx_pause = false;
> +	}

Hi Russell

It took me a while to check this. Maybe it is the way my brain works,
but to me, it is not obviously correct. I had to expand the table, and
they try all 16 combinations.

Maybe a lookup table would be more obvious?

However, now i spent the time:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew