Re: [PATCH RFC 0/4] Fixes for Marvell MII paged register access races

[Russell King - ARM Linux <linux@armlinux.org.uk>]

On Sat, Dec 09, 2017 at 10:22:58AM -0800, Florian Fainelli wrote:
> On 12/08/2017 08:44 AM, Russell King - ARM Linux wrote:
> > On Fri, Dec 08, 2017 at 05:17:14PM +0100, Andrew Lunn wrote:
> >> Hi Russell
> >>
> >>> There is an open question whether there should be generic helpers for
> >>> this.  Generic helpers would mean:
> >>>
> >>> - Additional couple of function pointers in phy_driver to read/write the
> >>>   paging register.  This has the restriction that there must only be one
> >>>   paging register.
> >>
> >> I must be missing something. I don't see why there is this
> >> restriction. Don't we just need
> >>
> >> int phy_get_page(phydev);
> >> int phy_set_page(phydev, page);
> > 
> > The restriction occurs because a PHY may have several different
> > registers, and knowing which of the registers need touching becomes an
> > issue.  We wouldn't want these accessors to needlessly access several
> > registers each and every time we requested an access to the page
> > register.
> > 
> > There's also the issue of whether an "int" or whatever type we choose to
> > pass the "page" around is enough bits.  I haven't surveyed all the PHY
> > drivers yet to know the answer to that.
> 
> I have not come across a PHY yet that required writing a page across two
> 16-bit quantities, in general, the page fits within less than 16-bit
> actually to fit within one MDIO write. That does not mean it cannot
> exist obviously, but having about 32-bit x pages of address space within
> a PHY sounds a bit extreme.

True, and phylib at the moment contains nothing beyond a single register.
I was thinking more of paging bits across several registers - such a case
would not lend itself well to this implementation as you'd have to read
every paging-capable register and write every paging capable register in
the phy_driver page accessor methods.

The good news is, having read through several drivers that contain the
caseless "page" string, there are no drivers that need anything but a
simple paging case, so it's not a concern.  Those which seem to use
page accesses are:

at803x: this only uses a single bit in a register for one access.

dp83640: looks like it implements its own locking and banks registers
	0x10-0x1e.  Multiple accesses throughout the driver.

marvell: we know about this one which is the problem case.

microchip: looks like it banks the registers 0x10-0x1e, and uses this
	for mdix control.

mscc: looks like it banks the registers 0x10-0x1e.  Several accesses
	throughout the driver, some under the phydev lock but others
	unclear whether they are locked.  Could be a problem.

realtek: looks like it banks the registers 0x10-0x1e.  Probably racy -
	interrupt handling uses paged accesses which may run in a
	threaded interrupt handler.

vitesse: "/* map extended registers set 0x10 - 0x1e */" in one place
	for mdix control via config_aneg.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up