netdev - Re: [PATCH] net: phy: Trigger link_change_notify on PHY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ce5c40cf-ae20-85d9-e188-8b097a2c7c81@gmail.com>
Date:   Tue, 5 Jan 2021 22:06:55 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Marek Vasut <marex@...x.de>, netdev@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        "David S . Miller" <davem@...emloft.net>
Subject: 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:
>> 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.