Re: [PATCH] net: phy: Trigger link_change_notify on PHY_HALTED

On 05.01.2021 18:05, Russell King - ARM Linux admin wrote:

On Tue, Jan 05, 2021 at 05:58:21PM +0100, Heiner Kallweit wrote:

quoted

On 05.01.2021 17:11, Marek Vasut wrote:

quoted

@@ -1021,8 +1022,17 @@ void phy_stop(struct phy_device *phydev)
 	if (phydev->sfp_bus)
 		sfp_upstream_stop(phydev->sfp_bus);
 
+	old_state = phydev->state;
 	phydev->state = PHY_HALTED;
 
+	if (old_state != phydev->state) {

This check shouldn't be needed because it shouldn't happen that
phy_stop() is called from status PHY_HALTED. In this case the
WARN() a few lines above would have fired already.

That is incorrect. If an error happens with the phy, phy_error() will
be called, which sets phydev->state = PHY_HALTED. If you then
subsequently take the interface down, phy_stop() will be called, but
phydev->state will be set to PHY_HALTED.

OK, so we have to fix the way phy_error() works. Still the check isn't
needed here. So far nobody is interested in transitions to PHY_HALTED,
so nothing is broken.

This is a long standing bug since you changed the code, and I think is
something I've reported previously, since I've definitely encountered
it.

IIRC there was a start of a discussion whether phy_error() is useful
at all regarding how it works as of today. A single failed MDIO access
(e.g. timeout) stops the state machine, when e.g. one missed PHY status
update in polling mode doesn't really cause any harm.
If we want to keep the functionality of phy_error(), then I'd say
a separate error phy state (e.g. PHY_ERROR) would make sense, as it
would make clear that the network was stopped due to an error.