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]

Date:   Fri, 25 Jan 2019 11:00:46 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Samu Nuutamo <samu.nuutamo@...cit.fi>,
        Russell King <rmk+kernel@...linux.org.uk>
Cc:     netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: Regression: mv88e6xxx packet loss after 4.18's PHYLINK merge

On 1/22/19 6:10 PM, Andrew Lunn wrote:
>> Hi Andrew,
>>
>> I tested the patch and it has an issue that prevents the carrier from turning
>> on. I think it's caused by the change from using link_state to phy->phy_state,
>> as later in the same function the link_state attributes are read when
>> deciding if the carrier needs to be turned on.
>  
> Hi Samu
> 
> This version has had better testing. So i'm a bit more confident about
> it.
> 
> 	Andrew
> 
> From d093492f9aba75cb4844d15148e0efd014f193bb 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 only reconfigure the MAC if the link state differs from the
> netdev carrier 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index e7becc7379d7..394b1eb5b55a 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -409,7 +409,8 @@ 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);
> +			if (link_state.link != netif_carrier_ok(ndev))
> +				phylink_mac_config(pl, &link_state);

With that change though, I don't think we would ever be able to disable
the MAC upon link down which could be sucking additional power, so maybe
we should be comparing link_state.link with the previous link_state
(prior to calling phylink_get_fixed_state(), so something like that:

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e7becc7379d7..88e0045ecf79 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -409,7 +409,8 @@ 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);
+                       if (pl->mac_link_dropped != link_state.link)
+                               phylink_mac_config(pl, &link_state);
                        break;

                case MLO_AN_INBAND:


It seems to me that the problem might very well be mv88e6xxx specific in
that the function mv88e6xxx_port_setup_mac() calls into multiple other
functions in order to configure the port's MAC and that is not atomic
from the HW's perspective, therefore there is a chance for the HW to
latch in some register writes (even if they are the same values) trigger
a MAC reconfiguration upon that write, which is responsible for causing
some packets to be dropped.
-- 
Florian