netdev - Re: Regression: mv88e6xxx packet loss after 4.18's PHYLINK merge

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: <20190117154604.GA15435@samu-ThinkPad-T480s>
Date:   Thu, 17 Jan 2019 17:46:04 +0200
From:   Samu Nuutamo <samu.nuutamo@...cit.fi>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: Regression: mv88e6xxx packet loss after 4.18's PHYLINK merge

On Thu, Jan 17, 2019 at 03:29:35AM +0100, Andrew Lunn wrote:
> Hi Samu
> 
> Please could you try this patch. I've not had chance to properly test
> it, and i'm about to go away for a long weekend.
> 
> Thanks
> 	Andrew
> 
> From 02438712824d3c7a9dd1aab8bd5dfb4383e55bfd Mon Sep 17 00:00:00 2001
> From: Andrew Lunn <andrew@...n.ch>
> Date: Wed, 16 Jan 2019 20:22:04 -0600
> Subject: [PATCH] net: phy: phylink: Only change mac when fixed link changes
>  state
> 
> phylink polls the fixed-link once per second to see if the GPIO has
> changed state, or if the callback indicates if there has been a change
> in state. It then calls the MAC to reconfigure itself to the current
> state.
> 
> For some MACs, reconfiguration can result in packets being dropped.
> Hence keep track of the link state between polls and only reconfigure
> the MAC if there has been a change of state.
> 
> Reported-by: Samu Nuutamo <samu.nuutamo@...cit.fi>
> Fixes: 9cd00a8aa42e ("net: phy: phylink: Poll link GPIOs")
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> ---
>  drivers/net/phy/phylink.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index e7becc7379d7..6473af228842 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -407,11 +407,14 @@ static void phylink_resolve(struct work_struct *w)
>  			phylink_mac_config(pl, &link_state);
>  			break;
>  
> -		case MLO_AN_FIXED:
> -			phylink_get_fixed_state(pl, &link_state);
> -			phylink_mac_config(pl, &link_state);
> -			break;
> +		case MLO_AN_FIXED: {
> +			bool old_link = pl->phy_state.link;
>  
> +			phylink_get_fixed_state(pl, &pl->phy_state);
> +			if (pl->phy_state.link != old_link)
> +				phylink_mac_config(pl, &pl->phy_state);
> +			break;
> +		}
>  		case MLO_AN_INBAND:
>  			phylink_get_mac_state(pl, &link_state);
>  			if (pl->phydev) {
> -- 
> 2.20.1
> 

Hi Andrew,

I've debugged the issue further by dumping all the values inside
phylink_resolve to get a better understand how the link state behaves.

As this is a fixed link the link_state structure is populated by
phylink_get_fixed_state(), but in our case neither get_fixed_state callback
or GPIO is used. This means the link_state comes straight from the
link_config, meaning link_state.link will always be 1. On the other hand the
pl->phy_state seems to never change and the phy_state.link stays 0 even
when the link is up and functional.

This makes it impossible to use these variables for deciding if
phylink_mac_config needs to be run, and this got me thinking: do we even need
to reconfigure the link? The phylink_mac_config() is already called from
phylink_start and fixed links shouldn't change(?).

I created the following patch that simply removes the phylink_mac_config() call
and everything seems to be working. The patch was only tested with the board
we have on hand.

- Samu

>From 6786cc3b9fef40adb3f68c72236220fafaa26eee Mon Sep 17 00:00:00 2001
From: Samu Nuutamo <samu.nuutamo@...cit.fi>
Date: Thu, 17 Jan 2019 16:42:29 +0200
Subject: [PATCH] net: phy: phylink: Configure fixed links only once

This prevents unnecessary link restarts from causing packet loss.

Fixes: 9cd00a8aa42e ("net: phy: phylink: Poll link GPIOs")
Signed-off-by: Samu Nuutamo <samu.nuutamo@...cit.fi>
---
 drivers/net/phy/phylink.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 70f3f90c2ed6..1ef76382c593 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -437,7 +437,6 @@ static void phylink_resolve(struct work_struct *w)

        case MLO_AN_FIXED:
            phylink_get_fixed_state(pl, &link_state);
-           phylink_mac_config(pl, &link_state);
            break;

        case MLO_AN_INBAND:
--
2.17.1