linux-kernel - Re: [PATCH] ARM: dts: imx6dl: SolidRun: add phy node with 100Mb/s max-speed

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: <20190920103622.GL25745@shell.armlinux.org.uk>
Date:   Fri, 20 Sep 2019 11:36:22 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Mark Rutland <mark.rutland@....com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Baruch Siach <baruch@...s.co.il>,
        Fabio Estevam <festevam@...il.com>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        tinywrkb <tinywrkb@...il.com>,
        open list <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        NXP Linux Team <linux-imx@....com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] ARM: dts: imx6dl: SolidRun: add phy node with 100Mb/s
 max-speed

On Tue, Sep 17, 2019 at 08:39:34PM +0200, Andrew Lunn wrote:
> > > Well, the _correct_ driver needs to be used for the PHY specific
> > > features to be properly controlled.  Using the generic driver
> > > in this situation will not be guaranteed to work.
> 
> I fully agree about the PHY driver. I'm expect this device is
> violating c22 when it modifies the advertisement register itself. So
> all bets are off for the genphy.
> 
> > Well, this hasn't worked, but not for the obvious reason.  Register 0x14
> > is documented as read/write.  Bits 15:6 are reserved, bit 5 is the
> > smart speed enable, 4:2 configures the attempts, bit 1 sets the link
> > stable condition, bit 0 is reserved.
> > 
> > Writing 0x80c results in the register reading back 0x82c.  Writing
> > 0x800 results in the same.  Writing 0 reads back 0x2c.  Writing 0xffff
> > seems to prevent packets being passed - and at that point I lost
> > control so I couldn't see what the result was.
> > 
> > There is nothing in the data sheet which suggests that there is any
> > gating of this register.  So it looks like we're stuck with smartspeed
> > enabled.
> > 
> > So, I think there's only two remaining ways forward - to revert commit
> > 5502b218e001 to restore the old behaviour, read back the advertisement
> > from the PHY along with the rest of the status, as I've previously
> > stated.  It means that phylib will modify phydev->advertising at
> > random points, just as it modifies phydev->lp_advertising, so locking
> > may become an issue.  The revert approach is probably best until we
> > have something working along those lines.
> 
> We have a couple of other PHYs which support downshift. We should see
> if we can follow what they do. What is i think important is that
> read_status return the correct speed. So we probably cannot use
> genphy_read_status() as is. Maybe we should split genphy_read_status()
> into two, so the register reading bit can be done unconditionally by
> phy drivers for hardware which don't report link down when they
> should?

I think we need to check how the downshift feature works on other PHYs
and whether it is enabled there.

Looking at the Marvell 88e151x PHYs, they have the feature, but do not
enable it by default.  If firmware has enabled the feature, phylib will
incorrectly resolve the link speed based on just the advertisements.

I think the safest way in the case of both PHYs to ascertain the real
link speed is to read the Specific Status register - register 17 in
both cases.  The top two bits indicate the negotiated speed resolution
and bit 13 indicates the duplex.  Bit 11 indicates whether the
resolution is valid.  This register layout seems to apply to both
88e151x and AR8035.

The register also contains the pause mode resolution in terms of
receive or transmit pause enabled, but this is not useful to phylib
as that is not what phylib wants to know.  However, it probably makes
sense for phylib to resolve the pause mode negotiation itself rather
than having that logic in the MAC drivers.

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