On 9/18/2022 1:55 PM, Lukas Wunner wrote:
On Sun, Sep 18, 2022 at 01:41:13PM -0700, Florian Fainelli wrote:
On 9/18/2022 12:13 PM, Lukas Wunner wrote:
On Mon, Aug 29, 2022 at 01:40:05PM +0200, Marek Szyprowski wrote:
I've finally traced what has happened. I've double checked and indeed
the 1758bde2e4aa commit fixed the issue on next-20220516 kernel and as
such it has been merged to linus tree. Then the commit 744d23c71af3
("net: phy: Warn about incorrect mdio_bus_phy_resume() state") has been
merged to linus tree, which triggers a new warning during the
suspend/resume cycle with smsc95xx driver. Please note, that the
smsc95xx still works fine regardless that warning. However it look that
the commit 1758bde2e4aa only hide a real problem, which the commit
744d23c71af3 warns about.
Probably a proper fix for smsc95xx driver is to call phy_stop/start
during suspend/resume cycle, like in similar patches for other drivers:
https://lore.kernel.org/all/20220825023951.3220-1-f.fainelli@xxxxxxxxx/
No, smsc95xx.c relies on mdio_bus_phy_{suspend,resume}() and there's
no need to call phy_{stop,start}() >
744d23c71af3 was flawed and 6dbe852c379f has already fixed a portion
of the fallout.
However the WARN() condition still seems too broad and causes false
positives such as in your case. In particular, mdio_bus_phy_suspend()
may leave the device in PHY_UP state, so that's a legal state that
needs to be exempted from the WARN().
How is that a legal state when the PHY should be suspended? Even if we are
interrupt driven, the state machine should be stopped, does not mean that
Wake-on-LAN or other activity interrupts should be disabled.
mdio_bus_phy_suspend()
phy_stop_machine()
phydev->state = PHY_UP # if (phydev->state >= PHY_UP)
So apparently PHY_UP is a legal state for a suspended PHY.
It is not clear to me why, however. Sure it does ensure that when we
resume we set needs_aneg = true but this feels like a hack in the sense
that we are setting the PHY in a provisional state in anticipation for
what might come next.
Does the issue still appear even after 6dbe852c379f?
If it does, could you test whether exempting PHY_UP silences the
gratuitous WARN splat? I.e.:
If you allow PHY_UP, then the warning becomes effectively useless, so I
don't believe this is quite what you want to do here.
Hm, maybe the WARN() should be dropped altogether?
And then be left with debugging similar problems that prompted me to
submit the patch in the first place, no thank you. I guess I would
rather accept that PHY_UP needs to be special cased then.
--
Florian
