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: <20190117022935.GJ24870@lunn.ch>
Date:   Thu, 17 Jan 2019 03:29:35 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Samu Nuutamo <samu.nuutamo@...cit.fi>
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 Mon, Jan 14, 2019 at 01:34:44PM +0200, Samu Nuutamo wrote:
> Hi,
> 
> We have an imx6q-b450v3 board that has one of the switch ports configured as a
> fixed link. After upgrading the kernel to version 4.18 the link has experienced
> a small amount of packet loss, around 0.15%.
> 
> The issue was bisected to a commit: aab9c4067d23 net: dsa: Plug in PHYLINK support
> 
> After enabling debug we could see that the new phylink code causes the link to
> reset once every second:
> 
> [  309.992368] mv88e6085 gpio-0:00 enembc: phylink_mac_config: mode=fixed//100Mbps/Full adv=00000,00000208 pause=10 link=1 an=1
> [  309.998451] mv88e6085 gpio-0:00: p5: Force link down
> [  309.998869] mv88e6085 gpio-0:00: p5: Speed set to 100 Mbps
> [  309.999280] mv88e6085 gpio-0:00: p5: Force full duplex
> [  309.999895] mv88e6085 gpio-0:00: p5: Force link up
> [  311.032400] mv88e6085 gpio-0:00 enembc: phylink_mac_config: mode=fixed//100Mbps/Full adv=00000,00000208 pause=10 link=1 an=1
> [  311.038248] mv88e6085 gpio-0:00: p5: Force link down
> [  311.038660] mv88e6085 gpio-0:00: p5: Speed set to 100 Mbps
> [  311.039069] mv88e6085 gpio-0:00: p5: Force full duplex
> [  311.039678] mv88e6085 gpio-0:00: p5: Force link up
> [  312.072328] mv88e6085 gpio-0:00 enembc: phylink_mac_config: mode=fixed//100Mbps/Full adv=00000,00000208 pause=10 link=1 an=1
> [  312.077603] mv88e6085 gpio-0:00: p5: Force link down
> [  312.078010] mv88e6085 gpio-0:00: p5: Speed set to 100 Mbps
> [  312.078417] mv88e6085 gpio-0:00: p5: Force full duplex
> [  312.079026] mv88e6085 gpio-0:00: p5: Force link up

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