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

[Florian Fainelli <f.fainelli@gmail.com>]

On 12/08/2017 07:47 AM, Russell King - ARM Linux wrote:
> Hi,
> 
> While doing final testing of the mvneta changes for phylink, a very
> easy to trigger race condition was found with the Marvell PHY driver
> which manifested itself as the link going down when a hibernate cycle
> terminates.
> 
> The issue turned out to be a race between two threads accessing the
> PHY - one trying to do a status read and the other configuring the PHY.
> 
> The result is the configuration thread tries to read-modify-write a
> paged register in a non-copper page, but the status read thread
> switches the PHY back to the copper page half-way through.
> 
> Various solutions involving phy->lock were considered, but found to
> create more lock dependency issues than were nice to deal with.

I can certainly imagine that, because you would ideally want a
phy_device wide lock which serializes the page entry/exit, which needs
to be able to run within the PHY state machine (so with phydev->lock
held), but also from a context where phydev->lock is not held, wheee.

> 
> The solution proposed here uses the mdiobus lock to ensure that accesses
> to paged registers become atomic with respect to all other bus accesses,
> including those from userspace.
> 
> 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.
> 
> - 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 helpers would be re-usable, saving replications of that code, and
>   making it more likely for phy authors to safely access the PHY.
> 
> 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.

Regarding that last topic, this could become a fairly contended lock on
a switch with lots (e.g: > 5-6) of built-in PHYs, all being polled
(which is usually the case right now). One would expect that the polling
should be limited to 2 BMSR reads to minimize the bus utilization.

> 
> Comments?
> 
>  drivers/net/phy/marvell.c  | 365 +++++++++++++++++++++------------------------
>  drivers/net/phy/mdio_bus.c |  65 ++++++--
>  drivers/net/phy/phy-core.c |  11 +-
>  include/linux/mdio.h       |   3 +
>  include/linux/phy.h        |  26 ++++
>  5 files changed, 256 insertions(+), 214 deletions(-)
> 

-- 
Florian