From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Lunn Subject: Re: [PATCH RFC 0/4] Fixes for Marvell MII paged register access races Date: Fri, 8 Dec 2017 17:17:14 +0100 Message-ID: <20171208161714.GC30846@lunn.ch> References: <20171208154756.GF10595@n2100.armlinux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Florian Fainelli , netdev@vger.kernel.org To: Russell King - ARM Linux Return-path: Received: from vps0.lunn.ch ([185.16.172.187]:53566 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754466AbdLHQRT (ORCPT ); Fri, 8 Dec 2017 11:17:19 -0500 Content-Disposition: inline In-Reply-To: <20171208154756.GF10595@n2100.armlinux.org.uk> Sender: netdev-owner@vger.kernel.org List-ID: 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); We might even be able to do without phy_get_page(), if we assume page 0 should be selected by default, and all paged reads/write return to page 0 afterwards. > - The helpers become more expensive, and because they're in a separate > compilation unit, the compiler will be unable to optimise them by > inlining the static functions. The mdio bus is slow. Often there is a completion function triggered by an interrupt etc. Worst case it is bit-banging. I suspect the gains from inlining are just noise in the bigger picture. > - The helpers would be re-usable, saving replications of that code, and > making it more likely for phy authors to safely access the PHY. This is the key point for me. This has likely to of been broken for years. If it is obviously broken, driver writers are more likely to get it right. Here it is not obvious, so we should take it out of the driver writers hands and do it in the core. > Another potential question is whether using the mdiobus lock (which > excludes all other MII bus access) is best - while it has the advantage > of also ensuring atomicity with userspace accesses, it means that no one > else can access an independent PHY on the same bus while a paged access > is on-going. It feels like a big hammer, but I'm not convinced that we > will see a lot of contention on it. I think you are right about there being little contention on the lock. I suspect most paged accesses are performed during initial setup and configuration. I guess the once per second poll does not need to use paged registers. And the weight of that hammer can be reduced a lot by using interrupts instead of polling. Andrew