netdev - Re: [PATCH net-next] net: phy: avoid kernel warning dump when stopping an errored 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]

Date: Tue, 5 Sep 2023 16:59:41 +0800
From: Jijie Shao <shaojijie@...wei.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
CC: <shaojijie@...wei.com>, <f.fainelli@...il.com>, Andrew Lunn
	<andrew@...n.ch>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<hkallweit1@...il.com>, <kuba@...nel.org>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>, "shenjian15@...wei.com" <shenjian15@...wei.com>,
	"liuyonglong@...wei.com" <liuyonglong@...wei.com>, <wangjie125@...wei.com>,
	<chenhao418@...wei.com>, Hao Lan <lanhao@...wei.com>,
	"wangpeiyang1@...wei.com" <wangpeiyang1@...wei.com>
Subject: Re: [PATCH net-next] net: phy: avoid kernel warning dump when
 stopping an errored PHY


on 2023/9/4 22:42, Russell King (Oracle) wrote:
> Are you suggesting that the sequence is:
>
> phy_stop();
> reset netdev
> phy_start();
>
> ?
>
> Is the reason for doing this because you've already detected an issue
> with the hardware, and you're trying to recover it - and before you've
> called phy_stop() the hardware is already dead?

In our case, hardware is already dead before we called phy_stop().

>
> If that is the case, I'm not really sure what you expect to happen
> here. You've identified a race where the state machine is running in

What we expect is that the phy state is PHY_HALTED after we called
phy_stop(). So that we can do a phy_start() successully after resetting
our netdev.

> unison with phy_stop(), but in this circumstance it is also possible
> that the state machine could complete executing and have called
> phy_error_precise() before phy_stop() has even been called. In that
> case, you'll still get a warning-splat on the console from
> phy_error_precise().
>
> The only difference is that phy_stop() won't warn.
>
> That all said, this is obviously buggy, because phy_stop() has set
> the phydev state to PHY_HALTED and the state machine has unexpectedly
> changed its state.
>
> I wonder whether we should be tracking the phy_start/stop state
> separately, since we've had issues with phy_stop() warning when an
> error has occurred (see commit 59088b5a946e).
>
> Maybe something like this (untested)?

Thanks for your patch and it works in our case. By the way, when would you
push this patch?


Jijie Shao