linux-kernel - Re: [PATCH net-next 3/4] net: phy: smsc: fix and improve WoL support

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: <1fc9d75e-459d-454f-b8dc-1fb7f59d09b4@foss.st.com>
Date: Mon, 21 Jul 2025 14:23:28 +0200
From: Gatien CHEVALLIER <gatien.chevallier@...s.st.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
CC: Andrew Lunn <andrew+netdev@...n.ch>,
        "David S. Miller"
	<davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...nel.org>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre
 Torgue <alexandre.torgue@...s.st.com>,
        Christophe Roullier
	<christophe.roullier@...s.st.com>,
        Andrew Lunn <andrew@...n.ch>, Heiner
 Kallweit <hkallweit1@...il.com>,
        Simon Horman <horms@...nel.org>,
        Tristram Ha
	<Tristram.Ha@...rochip.com>,
        Florian Fainelli
	<florian.fainelli@...adcom.com>,
        <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 3/4] net: phy: smsc: fix and improve WoL support

Hello Russel,

On 7/21/25 13:28, Russell King (Oracle) wrote:
> On Mon, Jul 21, 2025 at 01:14:45PM +0200, Gatien Chevallier wrote:
>> +static int smsc_phy_suspend(struct phy_device *phydev)
>> +{
>> +	if (!phydev->wol_enabled)
>> +		return genphy_suspend(phydev);
> 
> This should not be necessary. Take a look at phy_suspend(). Notice:
> 
>          phydev->wol_enabled = phy_drv_wol_enabled(phydev) ||
>                                (netdev && netdev->ethtool->wol_enabled);
>          /* If the device has WOL enabled, we cannot suspend the PHY */
>          if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
>                  return -EBUSY;
> 
> PHY_ALWAYS_CALL_SUSPEND is not set for this PHY, therefore if
> phydev->wol_enabled is set by the above code, phydrv->suspend will
> not be called.
> 

Indeed, thank you for pointing this out. I will remove this callback for
v2.

>> +
>> +	return 0;
>> +}
>> +
>> +static int smsc_phy_resume(struct phy_device *phydev)
>> +{
>> +	int rc;
>> +
>> +	if (!phydev->wol_enabled)
>> +		return genphy_resume(phydev);
>> +
>> +	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	if (!(rc & MII_LAN874X_PHY_WOL_STATUS_MASK))
>> +		return 0;
>> +
>> +	dev_info(&phydev->mdio.dev, "Woke up from LAN event.\n");
>> +	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR,
>> +			   rc | MII_LAN874X_PHY_WOL_STATUS_MASK);
>> +
>> +	return rc;
> 
> Note that this will be called multiple times, e.g. during attachment of
> the PHY to the network device, when the device is opened, etc even
> without ->suspend having been called, and before ->wol_enabled has
> been set. Make sure your code is safe for this.
> 

If ->wol_enabled isn't set, then we should fallback to the previous
implementation so I expect it to be fine for that matter.
Then, I expect flags to be set only in case of WoL event received.
Nevertheless, I will double check the phy_* API used in this sequence
for V2, thank you.

Best regards,
Gatien