netdev - Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

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:   Tue, 23 Oct 2018 11:58:29 +0100
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Jose Abreu <jose.abreu@...opsys.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Joao Pinto <joao.pinto@...opsys.com>
Subject: Re: [PATCH net-next 3/4] net: phy-c45: Implement
 reset/suspend/resume callbacks

On Tue, Oct 23, 2018 at 11:28:09AM +0100, Jose Abreu wrote:
> On 23-10-2018 11:20, Russell King - ARM Linux wrote:
> > I have no idea what you're proposing there - your patches weren't copied
> > to me.
> 
> They just set / unset  MDIO_CTRL1_LPOWER bit in PCS. I find that
> without this remote end doesn't detect link is down ...
> 
> If it's okay for Generic 10G driver I can submit only this and
> manually reset PHY in stmmac driver so that I don't need to
> implement custom PHY driver ...

> BTW, I just found out currently Generic 10G Driver is broken
> without patch 4/4 of this series [1]
> 
> [1] https://patchwork.ozlabs.org/patch/987570/

How is it broken - what are the symptoms?

The generic 10G driver is bound not via the normal bus matching and
phy_bus_match(), but via a manual bind in phy_attach_direct().  This
calls the probe function, which is phy_probe(), which initialises
the supported/advertising to the driver's features (which as you note
are zero.)

However, phy_attach_direct() goes on to call phy_init_hw(), which
calls the config_init() method.  The config_init() method initialises
the supported/advertising masks to 10GbaseT.  This is (partly) what
I refer to when I say that the generic 10G support is crippled - it
only supports this single speed and media.

So the supported/advertising masks should be forced to only 10GbaseT
at the completion of phy_attach_direct().

The "generic 10G" support doesn't do autonegotiation, configuration
or link mode forcing.  It only assumes 10GbaseT is supported, and
only checks for the "link up" bits.

It isn't like the non-10G generic PHY support due to history - it
was added in 2014 by Andy Fleming (see 124059fd53af).

BTW, your patch 1 is wrong as well (introducing phy_update_link()).
You don't take account that a 10G phy may have alternative ways of
reading the link (like 88x3310 does, because it has multiple
instances of AN/PCS/PHYXS at 1k offsets.)  All the gen10g_*
functions are legacy functions for the crippled "generic" 10G
support.

-- 
RMK's Patch system: http://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