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] [day] [month] [year] [list]

Message-ID: <20190125191556.ihomwoa2hid3w4wi@e5254000004ec.dyn.armlinux.org.uk>
Date:   Fri, 25 Jan 2019 19:15:56 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Samu Nuutamo <samu.nuutamo@...cit.fi>, 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 Fri, Jan 25, 2019 at 11:00:46AM -0800, Florian Fainelli wrote:
> > +++ 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.

The mac_config is expected to _update_ the MAC, not reprogram it in
full each time it's called.

Consider a SGMII link with a PHY on a SFP module.  The SGMII link
must be placed in "negotiation" mode (so must use in-band negotiation)
but there is no mechanism on the link to convey the negotiated flow
control settings.  So, mac_config() must be called whenever the link
changes to update the MAC with the flow control settings read from
the PHY.  However, the MAC still needs negotiation enabled on the
link, and must not disturb that, because it could cause the PHY to
then report link-down.  That would throw phylink into a continual
loop of the link bouncing up and down.

This is how I've implemented mac_config() in both mvneta and the out
of tree mvpp2x driver - in mvneta, I read the current register
settings, modify them according to the requested parameters, then
compare them, and only write the new register settings back as
required taking account of hardware restrictions (eg, port needs to
be temporarily disabled if certain settings are changed.)

If mac_config() is implemented as per that, then the above patch is
unnecessary.

I know that detail is not documented, it's something that I need to
update in the header file and my .rst documentation (also something
else that needs submitting.)

I also have a patch for phylink that allows us to use the GPIO
interrupt rather than polling with a 1s timer to check the state of
the fixed-link GPIO.

-- 
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