Add suspend()/resume() callbacks that do not shut down the PHY if the
WoL is supported and handle the WoL status flags.

If the WoL is supported by the PHY, indicate that the PHY device can
be a source of wake up for the platform. When setting the WoL
configuration, enable this capability.

Fixes: 8b305ee2a91c ("net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs")
Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
 drivers/net/phy/smsc.c  | 42 ++++++++++++++++++++++++++++++++++++++----
 include/linux/smscphy.h |  2 ++
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index b6489da5cfcdfb405457058dc88575c0d84d259d..cf4e763907aefd2d725c734d3e0f2926128f770e 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -537,14 +537,45 @@ static int lan874x_set_wol(struct phy_device *phydev,
 		}
 	}
 
-	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR,
-			   val_wucsr);
+	/* Enable wakeup on PHY device if at least one WoL feature is configured */
+	device_set_wakeup_enable(&phydev->mdio.dev, !!(val_wucsr & MII_LAN874X_PHY_WOL_MASK));
+
+	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR, val_wucsr);
 	if (rc < 0)
 		return rc;
 
 	return 0;
 }
 
+static int smsc_phy_suspend(struct phy_device *phydev)
+{
+	if (!phydev->wol_enabled)
+		return genphy_suspend(phydev);
+
+	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;
+}
+
 static int smsc_get_sset_count(struct phy_device *phydev)
 {
 	return ARRAY_SIZE(smsc_hw_stats);
@@ -673,6 +704,9 @@ int smsc_phy_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
+	if (phydev->drv->set_wol)
+		device_set_wakeup_capable(&phydev->mdio.dev, true);
+
 	/* Make clk optional to keep DTB backward compatibility. */
 	refclk = devm_clk_get_optional_enabled_with_rate(dev, NULL,
 							 50 * 1000 * 1000);
@@ -875,8 +909,8 @@ static struct phy_driver smsc_phy_driver[] = {
 	.set_wol	= lan874x_set_wol,
 	.get_wol	= lan874x_get_wol,
 
-	.suspend	= genphy_suspend,
-	.resume		= genphy_resume,
+	.suspend	= smsc_phy_suspend,
+	.resume		= smsc_phy_resume,
 } };
 
 module_phy_driver(smsc_phy_driver);
diff --git a/include/linux/smscphy.h b/include/linux/smscphy.h
index 1a6a851d2cf80d225bada7adeb79969e625964bd..cdf266960032609241afc8316da23f1c4834bfee 100644
--- a/include/linux/smscphy.h
+++ b/include/linux/smscphy.h
@@ -65,6 +65,8 @@ int smsc_phy_probe(struct phy_device *phydev);
 #define MII_LAN874X_PHY_WOL_WUEN		BIT(2)
 #define MII_LAN874X_PHY_WOL_MPEN		BIT(1)
 #define MII_LAN874X_PHY_WOL_BCSTEN		BIT(0)
+#define MII_LAN874X_PHY_WOL_MASK		GENMASK(4, 0)
+#define MII_LAN874X_PHY_WOL_STATUS_MASK		GENMASK(7, 4)
 
 #define MII_LAN874X_PHY_WOL_FILTER_EN		BIT(15)
 #define MII_LAN874X_PHY_WOL_FILTER_MCASTTEN	BIT(9)

-- 
2.35.3

> +static int smsc_phy_suspend(struct phy_device *phydev)
> +{
> +	if (!phydev->wol_enabled)
> +		return genphy_suspend(phydev);
> +
> +	return 0;
> +}

Suspend/resume is somewhat complex, and i don't know all the
details. But this looks odd. Why does the phylib core call suspend
when phydev->wol_enabled is true? That at least needs an explanation
in the commit message.

> +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");

Please don't spam the log. It is clear the system woke up, there are
messages in the log...

	Andrew

Hello Andrew,

On 7/21/25 15:26, Andrew Lunn wrote:
>> +static int smsc_phy_suspend(struct phy_device *phydev)
>> +{
>> +	if (!phydev->wol_enabled)
>> +		return genphy_suspend(phydev);
>> +
>> +	return 0;
>> +}
> 
> Suspend/resume is somewhat complex, and i don't know all the
> details. But this looks odd. Why does the phylib core call suspend
> when phydev->wol_enabled is true? That at least needs an explanation
> in the commit message.
> 

As stated by Russel, this callback is not needed because phy_suspend()
will not call this suspend() callback if phydev->wol_enabled is set.
Therefore, I'm removing it vor V2.

>> +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");
> 
> Please don't spam the log. It is clear the system woke up, there are
> messages in the log...

I wanted to state clearly that the wake up happended because of a WoL
event but sure, I understand that it's best if log isn't spammed. Do you
prefer it completely removed or dev_info()->dev_dbg() ?

Best regards,
Gatien

> 
> 	Andrew

> I wanted to state clearly that the wake up happended because of a WoL
> event but sure, I understand that it's best if log isn't spammed. Do you
> prefer it completely removed or dev_info()->dev_dbg() ?

phydev_dbg() is fine.

	Andrew

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.

> +
> +	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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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