Correct way to access MDIO bus - phy.c seems buggy

Correct way to access MDIO bus - phy.c seems buggy

[Russell King - ARM Linux]

Hi,

While trying to track down instability in the FEC driver, I've come
across this question: what is the correct way to access the MDIO bus?

Is it via:

	bus->write()

where 'bus' is a struct mii_bus, or should it be via mdiobus_write()?

What I'm seeing in the FEC driver is two thread trying to access the
MDIO bus simultaneously - one thread trying to do a read, and another
trying to do a write.  The result is far from pretty with the current
mainline code, because we can end up re-initialising a spinlock while
it's held by the fec interrupt handler.

I think the correct answer is that mdiobus_write() should be used,
which makes drivers/net/phy/phy.c horribly buggy, as it bypasses the
locking at the mdiobus level by doing this:

mmd_phy_indirect()
{
	bus->write(bus, addr, MII_MMD_CTRL, devad);
	bus->write(bus, addr, MII_MMD_DATA, prtad);
	bus->write(bus, addr, MII_MMD_CTRL, (devad | MII_MMD_CTRL_NOINCR));
}

However, it's not as simple as that, because the whole set of writes
need to be done atomically.  The mdio bus lock needs to be taken around
the internals of phy_read_mmd_indirect() and phy_write_mmd_indirect().

This bug can be provoked by running an ethtool command which accesses
the phy in a tight loop on a SMP platform.  For example:

	while :; do ethtool --show-eee eth0; done

Patch will follow tomorrow.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: Correct way to access MDIO bus - phy.c seems buggy

[Florian Fainelli]

On 24/08/15 17:09, Russell King - ARM Linux wrote:
> Hi,
> 
> While trying to track down instability in the FEC driver, I've come
> across this question: what is the correct way to access the MDIO bus?
> 
> Is it via:
> 
> 	bus->write()
> 
> where 'bus' is a struct mii_bus, or should it be via mdiobus_write()?
> 
> What I'm seeing in the FEC driver is two thread trying to access the
> MDIO bus simultaneously - one thread trying to do a read, and another
> trying to do a write.  The result is far from pretty with the current
> mainline code, because we can end up re-initialising a spinlock while
> it's held by the fec interrupt handler.
> 
> I think the correct answer is that mdiobus_write() should be used,
> which makes drivers/net/phy/phy.c horribly buggy, as it bypasses the
> locking at the mdiobus level by doing this:

Right the correct way is to use mdiobus_write() which takes the bus mutex.

> 
> mmd_phy_indirect()
> {
> 	bus->write(bus, addr, MII_MMD_CTRL, devad);
> 	bus->write(bus, addr, MII_MMD_DATA, prtad);
> 	bus->write(bus, addr, MII_MMD_CTRL, (devad | MII_MMD_CTRL_NOINCR));
> }
> 
> However, it's not as simple as that, because the whole set of writes
> need to be done atomically.  The mdio bus lock needs to be taken around
> the internals of phy_read_mmd_indirect() and phy_write_mmd_indirect().

Well, yes, the bus lock should be grabbed at the beginning and released
at the end of this function at the very least, good catch.

> 
> This bug can be provoked by running an ethtool command which accesses
> the phy in a tight loop on a SMP platform.  For example:
> 
> 	while :; do ethtool --show-eee eth0; done
> 
> Patch will follow tomorrow.

Good thing is it looks like you have isolated the only cases where we do
not grab the MDIO bus mutexm the rest of the code, except
phy_mmd_{read,write}_indirect() looks correct.
-- 
Florian