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
| ||
|
Message-ID: <20220923042037.GA10101@wunner.de> Date: Fri, 23 Sep 2022 06:20:37 +0200 From: Lukas Wunner <lukas@...ner.de> To: Florian Fainelli <f.fainelli@...il.com> Cc: Marek Szyprowski <m.szyprowski@...sung.com>, "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org, linux-usb@...r.kernel.org, Steve Glendinning <steve.glendinning@...well.net>, UNGLinuxDriver@...rochip.com, Oliver Neukum <oneukum@...e.com>, Andre Edich <andre.edich@...rochip.com>, Oleksij Rempel <linux@...pel-privat.de>, Martyn Welch <martyn.welch@...labora.com>, Gabriel Hojda <ghojda@...urs.ro>, Christoph Fritz <chf.fritz@...glemail.com>, Lino Sanfilippo <LinoSanfilippo@....de>, Philipp Rosenberger <p.rosenberger@...bus.com>, Heiner Kallweit <hkallweit1@...il.com>, Andrew Lunn <andrew@...n.ch>, Russell King <linux@...linux.org.uk>, Ferry Toth <fntoth@...il.com>, Krzysztof Kozlowski <krzk@...nel.org>, 'Linux Samsung SOC' <linux-samsung-soc@...r.kernel.org> Subject: Re: [PATCH net-next v3 5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling On Sun, Sep 18, 2022 at 03:11:47PM -0700, Florian Fainelli wrote: > 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@gmail.com/ > > > > > > > > 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. I've just submitted a fix so that at least v6.0 doesn't get released with a false-positive WARN splat on resume: https://lore.kernel.org/netdev/8128fdb51eeebc9efbf3776a4097363a1317aaf1.1663905575.git.lukas@wunner.de/ I guess we can look into making the state setting more logical in a separate step. > > > 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. I've interpreted that as an Acked-by for exempting PHY_UP. If that was not what you wanted, please speak up. Thanks, Lukas
Powered by blists - more mailing lists