From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [PATCH RFC 0/4] Fixes for Marvell MII paged register access races Date: Sat, 9 Dec 2017 23:49:06 +0000 Message-ID: <20171209234905.GL10595@n2100.armlinux.org.uk> References: <20171208154756.GF10595@n2100.armlinux.org.uk> <20171208161714.GC30846@lunn.ch> <20171208164446.GH10595@n2100.armlinux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Andrew Lunn , netdev@vger.kernel.org To: Florian Fainelli Return-path: Received: from pandora.armlinux.org.uk ([78.32.30.218]:48610 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751257AbdLIXtT (ORCPT ); Sat, 9 Dec 2017 18:49:19 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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