netdev - Re: [PATCH v4 net-next 0/5] net: phy: Fix SMSC LAN87xx external reset

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: <20210118200253.m2pahxgn7ui7vups@pengutronix.de>
Date:   Mon, 18 Jan 2021 21:02:53 +0100
From:   Marco Felsch <m.felsch@...gutronix.de>
To:     "Badel, Laurent" <LaurentBadel@...on.com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "fugang.duan@....com" <fugang.duan@....com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "richard.leitner@...data.com" <richard.leitner@...data.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "marex@...x.de" <marex@...x.de>
Subject: Re: [PATCH v4 net-next 0/5] net: phy: Fix SMSC LAN87xx external reset

Hi Laurent,

thanks for your patches :) Can you check your setup since we get 6
individual emails: 'git send-email --thread ...' ;)

On 21-01-18 16:57, Badel, Laurent wrote:
> Description:
> External PHY reset from the FEC driver was introduced in commit [1] to 
> mitigate an issue with iMX SoCs and LAN87xx PHYs. The issue occurs 
> because the FEC driver turns off the reference clock for power saving 
> reasons [2], which doesn't work out well with LAN87xx PHYs which require 
> a running REF_CLK during the power-up sequence.

Not only during the power-up sequence. The complete phy internal state
machine (the hardware state machine) gets confused if the clock is
turned off randomly.

> As a result, the PHYs 
> occasionally (and unpredictably) fail to establish a stable link and 
> require a hardware reset to work reliably.
> 
> As previously noted [3], the solution in [1] integrates poorly with the
> PHY abstraction layer, and it also performs many unnecessary resets. This
> patch series suggests a simpler solution to this problem, namely to hold
> the PHY in reset during the time between the PHY driver probe and the first
> opening of the FEC driver.

Holding the Phy within reset during the FEC is in reset seems wrong to
me because: The clock can be supplied by an external crystal/oszi. This
would add unnecessary delays. Also this is again a FEC/SMSC combination
fix again. The phy has the same problem on other hosts if they are the
clock provider and toggling the ref-clk.

> To illustrate why this is sufficient, below is a representation of the PHY
> RST and REF_CLK status at relevant time points (note that RST signal is
> active-low for LAN87xx):
> 
>  1. During system boot when the PHY is probed:
>  RST    111111111111111111111000001111111111111
>  CLK    000011111111111111111111111111111000000
>  REF_CLK is enabled during fec_probe(), and there is a short reset pulse
>  due to mdiobus_register_gpiod() which calls gpiod_get_optional() with

There is also a deprecated "phy-reset-gpios" did you test this as well?

>  the GPIOD_OUT_LOW flag, which sets the initial value to 0. The reset is
>  deasserted by phy_device_register() shortly after.  After that, the PHY
>  runs without clock until the FEC is opened, which causes the unstable 
>  link issue.

Nope that's not true, you can specify the clock within the device-tree
so the fec-ref-clk isn't disabled anymore.

>  2. At first opening of the FEC:

...

> Extensive testing with LAN8720 confirmed that the REF_CLK can be disabled
> without problems as long as the PHY is either in reset or in power-down 
> mode (which is relevant for suspend-to-ram as well).

You can't disable the clock. What you listing here means that the smsc
phy needs to be re-initialized after such an clock loss. If we can
disbale the clock randomly we wouldn't need to re-initialize the phy
again.

Regards,
  Marco