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

[Heiner Kallweit <hkallweit1@gmail.com>]

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:
>> On 05.01.2021 17:11, Marek Vasut wrote:
>>> @@ -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.