netdev - Re: [PATCH net-next 1/2] net: dsa: felix: Handle PAUSE RX regardless of AN result

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: <20200120094304.GZ25745@shell.armlinux.org.uk>
Date:   Mon, 20 Jan 2020 09:43:04 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, andrew@...n.ch,
        f.fainelli@...il.com, vivien.didelot@...il.com,
        claudiu.manoil@....com,
        Alex Marginean <alexandru.marginean@....com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next 1/2] net: dsa: felix: Handle PAUSE RX regardless
 of AN result

On Thu, Jan 16, 2020 at 08:19:32PM +0200, Vladimir Oltean wrote:
> From: Alex Marginean <alexandru.marginean@....com>
> 
> Flow control is used with 2500Base-X and AQR PHYs to do rate adaptation
> between line side 100/1000 links and MAC running at 2.5G.
> 
> This is independent of the flow control configuration settled on line
> side though AN.
> 
> In general, allowing the MAC to handle flow control even if not
> negotiated with the link partner should not be a problem, so the patch
> just enables it in all cases.
> 
> Signed-off-by: Alex Marginean <alexandru.marginean@....com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

I think this is not the best approach - you're working around the
issue in your network driver, rather than recognising that it's a
larger problem than just your network driver.  Rate adaption is
present in other PHYs using exactly the same mechanism so why do we
want to hack around this in each network driver?  It is a property
of the PHY, not of the network driver.

Surely it not be better to address this in phylib/phylink - after
all, there are several aspects to this:

1) separation of the MAC configuration (reported to the MAC) from
   the negotiation results (reported to the user).
2) we need the MAC to be able to receive and act on flow control.
3) we need to report the correct speed setting to the MAC.

I already have patches to improve the current phylib method of
reporting the flow control information to MAC drivers with the resolved
flow state rather than just the current link partner advertisement
bits, which should make (2) fairly easy to achieve.  (1) and (3) will
require additional work.

> ---
>  drivers/net/dsa/ocelot/felix.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index d6ee089dbfe1..46334436a8fe 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -222,8 +222,12 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
>  	 * specification in incoming pause frames.
>  	 */
>  	mac_fc_cfg = SYS_MAC_FC_CFG_FC_LINK_SPEED(state->speed);
> -	if (state->pause & MLO_PAUSE_RX)
> -		mac_fc_cfg |= SYS_MAC_FC_CFG_RX_FC_ENA;
> +
> +	/* handle Rx pause in all cases, with 2500base-X this is used for rate
> +	 * adaptation.
> +	 */
> +	mac_fc_cfg |= SYS_MAC_FC_CFG_RX_FC_ENA;
> +
>  	if (state->pause & MLO_PAUSE_TX)
>  		mac_fc_cfg |= SYS_MAC_FC_CFG_TX_FC_ENA |
>  			      SYS_MAC_FC_CFG_PAUSE_VAL_CFG(0xffff) |
> -- 
> 2.17.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up