Re: [BUG] micrel phy suspend/resume

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

On Sun, Dec 10, 2017 at 03:50:28PM -0800, Florian Fainelli wrote:
> Hi Russell,
> 
> On 12/10/2017 08:47 AM, Russell King - ARM Linux wrote:
> > Guys,
> > 
> > I've just tripped over a bug with the Micrel PHY driver, but it
> > really isn't specific to the Micrel PHY driver.
> > 
> > When we suspend, we suspend the PHY and then the MAC driver (eg,
> > on the ZII board):
> > 
> > [  198.822751] 400d0000.ethernet-1:00: bus : mdio_bus_suspend+0x0/0x34
> > [  198.822859] __mdiobus_read: 117 400d0000.ethernet-1 00 00 => 3100
> > [  198.822878] __mdiobus_write: 117 400d0000.ethernet-1 00 00 <= 3900
> > ...
> > [  198.826235] 400d0000.ethernet: bus : platform_pm_freeze+0x0/0x5c
> > [  198.826354] __mdiobus_read: 117 400d0000.ethernet-1 00 1f => 9198
> > [  198.826374] __mdiobus_write: 117 400d0000.ethernet-1 00 1f <= 9198
> > [  198.826503] __mdiobus_write: 117 400d0000.ethernet-1 00 1b <= 0000
> > [  198.826699] __mdiobus_read: 117 400d0000.ethernet-1 00 1b => 0000
> > 
> > When we resume, the order is reversed:
> > 
> > [  198.848300] 400d0000.ethernet: bus : platform_pm_thaw+0x0/0x54
> > [  198.849024] __mdiobus_read: 117 400d0000.ethernet-1 00 1b => 0000
> > [  198.849120] __mdiobus_read: 117 400d0000.ethernet-1 00 1f => 9198
> > [  198.849141] __mdiobus_write: 117 400d0000.ethernet-1 00 1f <= 9198
> > [  198.849243] __mdiobus_write: 117 400d0000.ethernet-1 00 1b <= 0500
> > [  198.849401] __mdiobus_read: 117 400d0000.ethernet-1 00 00 => 3900
> > [  198.849419] __mdiobus_write: 117 400d0000.ethernet-1 00 00 <= 3100
> > [  198.849637] __mdiobus_read: 61 400d0000.ethernet-1 00 01 => 7849
> > ...
> > [  198.852677] 400d0000.ethernet-1:00: bus : mdio_bus_resume+0x0/0x34
> > [  198.852778] __mdiobus_read: 117 400d0000.ethernet-1 00 00 => 3100
> > [  198.852797] __mdiobus_write: 117 400d0000.ethernet-1 00 00 <= 3100
> > 
> > Now, the MAC driver calls phy_stop() and phy_start() from within its
> > own suspend/resume methods, and at this is done while the PHY is,
> > as far as the kernel PM code is concerned, suspended.
> > 
> > However, phylib works around that by resuming the PHY itself when
> > phy_start() is called in this situation.  That looks good, but it
> > really isn't in this case.  Given the above sequence, we will be in
> > PHY_HALTED state.
> > 
> > So, when phy_start() is called, the first thing it does is check the
> > state, and finds PHY_HALTED.  It then tries to enable the PHY
> > interrupts.  This cause the Micrel driver to write to the PHY to
> > enable interrupts - it reads 0x1b to ack any pre-existing interrupt,
> > modifies 0x1f to set the interrupt pin level, and then supposedly
> > writes 0x1b to enable interrupts.
> > 
> > However, at this point, the PHY is still powered down, as we can see
> > in the following read of the BMCR - containing 0x3900.  The write to
> > enable interrupts is ignored, and so interrupts remain disabled.
> > 
> > The resume process continues, and the system resumes, but interrupts
> > on the PHY remain disabled, and the phylib state machine never
> > advances.  You can do anything you like with cables etc, as far as
> > phylib is concerned, the link steadfastly remains "down".
> > 
> > That's a bit of a problem if your platform is running root-NFS through
> > that network interface.
> > 
> > It looks like some variants of the Micrel phy code work around this by
> > including in their resume path code to enable PHY interrupts
> > independently of phylib.
> > 
> > phy_resume() can't be called inside the locked region in phy_start(),
> > where we enable interrupts, because genphy_resume() also wants to take
> > phydev->lock - and it wants to do that to safely read-modify-write the
> > BMCR register.  This, I feel, comes back to an abuse of the phy state
> > machine lock to also protect atomic bus operations.
> > 
> > If we move over to using the bus lock to protect bus atomic operations
> > (as I believe we should) then phy_resume() can be called while holding
> > phydev->lock from within bits of the code that are protecting themselves
> > from concurrency with the phylib state machine.  It also means that we
> > can get rid of some of these boolean variables, and most importantly
> > in this particular case, call phy_resume() before we enable interrupts.
> > 
> > With that arrangement, things look a lot better:
> > 
> > [   74.545584] 400d0000.ethernet: bus : platform_pm_thaw+0x0/0x54
> > [   74.546010] __mdiobus_read: 117 400d0000.ethernet-1 00 00 => 3900
> > [   74.546029] __mdiobus_write: 117 400d0000.ethernet-1 00 00 <= 3100
> > [   74.546264] __mdiobus_read: 117 400d0000.ethernet-1 00 1b => 0000
> > [   74.546354] __mdiobus_read: 117 400d0000.ethernet-1 00 1f => 8100
> > [   74.546373] __mdiobus_write: 117 400d0000.ethernet-1 00 1f <= 8100
> > [   74.546651] __mdiobus_write: 117 400d0000.ethernet-1 00 1b <= 0500
> > 
> > The patch for this is slightly larger than it needs to be because I've
> > converted more places that do read-modify-write to the new xxx_modify()
> > accessor, and it also ensures that phy_resume() is consistently called
> > with the phy state machine lock held.  It also means we can get rid of
> > the interrupt enable hack for some micrel PHYs, which I suspect comes
> > from this same root cause.
> 
> Since this is a bug fix, I would really rather have the ability to
> target the "net" tree with a simpler fix that is contained within
> drivers/net/phy/phy.c and which does not require the use of phy_modify().

Not really that easy.

The simple solution would be to open-code the phy_modify() inside
genphy_resume() in phy_device.c, and move the phy_resume() call
in phy.c.

However, that leaves at803x taking phydev->lock in its resume path,
which will immediately deadlock.  We couldn't just delete that lock
without ensuring that phy_resume() is always called with phydev->lock
held to preserve the existing locking.  I wouldn't want to simply
delete that lock without some testing of that driver in case there's
a good reason for it being there.

So, to preserve that would mean making sure the two calls to
phy_resume() in phy_device.c are also under the lock, replacing
the at803x.c read-modify-write sequences with open-coded versions,
or convincing ourselves that the locks in at803x_suspend() and
at803x_resume() are not required.

My feeling is that the safest and minimal change for -rc is to add
the phy_modify() helper, switch both genphy_resume() and
at803x_resume() to it and delete the locking there, and ensure that
phy_resume() is always called with phydev->lock taken for a
consistent locking strategy there.

We can prove the phy_modify() helper works elsewhere, which means
the risk is reduced to the at803x_resume() conversion.

The unfortunate issue that gives us is that the calling convention
of "suspend" vs "resume" is different - resume ends up with
phydev->lock always held but suspend doesn't, and that will be
confusing for phy driver developers, and really doesn't help from
the review point of view.

-- 
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