[net] net: phy: consider that suspend2ram may cut off PHY power

Commit Message

Heiner Kallweit Feb. 11, 2021, 9:32 p.m. UTC

Claudiu reported that on his system S2R cuts off power to the PHY and
after resuming certain PHY settings are lost. The PM folks confirmed
that cutting off power to selected components in S2R is a valid case.
Therefore resuming from S2R, same as from hibernation, has to assume
that the PHY has power-on defaults. As a consequence use the restore
callback also as resume callback.
In addition make sure that the interrupt configuration is restored.
Let's do this in phy_init_hw() and ensure that after this call
actual interrupt configuration is in sync with phydev->interrupts.
Currently, if interrupt was enabled before hibernation, we would
resume with interrupt disabled because that's the power-on default.

This fix applies cleanly only after the commit marked as fixed.

I don't have an affected system, therefore change is compile-tested
only.

[0] https://lore.kernel.org/netdev/[e-mail]/

Fixes: 611d779af7ca ("net: phy: fix MDIO bus PM PHY resuming")
Reported-by: Claudiu Beznea <[e-mail]>
Signed-off-by: Heiner Kallweit <[e-mail]>
---
 drivers/net/phy/phy_device.c | 53 ++++++++++++------------------------
 1 file changed, 17 insertions(+), 36 deletions(-)

Comments

[e-mail] Feb. 12, 2021, 2:30 a.m. UTC | #1

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Thu, 11 Feb 2021 22:32:52 +0100 you wrote:
> Claudiu reported that on his system S2R cuts off power to the PHY and
> after resuming certain PHY settings are lost. The PM folks confirmed
> that cutting off power to selected components in S2R is a valid case.
> Therefore resuming from S2R, same as from hibernation, has to assume
> that the PHY has power-on defaults. As a consequence use the restore
> callback also as resume callback.
> In addition make sure that the interrupt configuration is restored.
> Let's do this in phy_init_hw() and ensure that after this call
> actual interrupt configuration is in sync with phydev->interrupts.
> Currently, if interrupt was enabled before hibernation, we would
> resume with interrupt disabled because that's the power-on default.
> 
> [...]

Here is the summary with links:
  - [net] net: phy: consider that suspend2ram may cut off PHY power
    https://git.kernel.org/netdev/net/c/4c0d2e96ba05

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Claudiu Beznea Feb. 12, 2021, 11:43 a.m. UTC | #2

On 11.02.2021 23:32, Heiner Kallweit wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Claudiu reported that on his system S2R cuts off power to the PHY and
> after resuming certain PHY settings are lost. The PM folks confirmed
> that cutting off power to selected components in S2R is a valid case.
> Therefore resuming from S2R, same as from hibernation, has to assume
> that the PHY has power-on defaults. As a consequence use the restore
> callback also as resume callback.
> In addition make sure that the interrupt configuration is restored.
> Let's do this in phy_init_hw() and ensure that after this call
> actual interrupt configuration is in sync with phydev->interrupts.
> Currently, if interrupt was enabled before hibernation, we would
> resume with interrupt disabled because that's the power-on default.
> 
> This fix applies cleanly only after the commit marked as fixed.
> 
> I don't have an affected system, therefore change is compile-tested
> only.
> 
> [0] https://lore.kernel.org/netdev/[e-mail]/
> 
> Fixes: 611d779af7ca ("net: phy: fix MDIO bus PM PHY resuming")
> Reported-by: Claudiu Beznea <[e-mail]>
> Signed-off-by: Heiner Kallweit <[e-mail]>

Hi Heiner,

I tested it on a system w/ SAMA7G5 and KSZ9131 PHY, with a S2R mode that
cuts the PHY's power and connectivity was good after resume. Thank you for
your patch!

Tested-by: Claudiu Beznea <[e-mail]>

Patch

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8447e56ba..d48bdcb91 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -300,50 +300,22 @@  static int mdio_bus_phy_resume(struct device *dev)
 
 	phydev->suspended_by_mdio_bus = 0;
 
-	ret = phy_resume(phydev);
+	ret = phy_init_hw(phydev);
 	if (ret < 0)
 		return ret;
 
-no_resume:
-	if (phydev->attached_dev && phydev->adjust_link)
-		phy_start_machine(phydev);
-
-	return 0;
-}
-
-static int mdio_bus_phy_restore(struct device *dev)
-{
-	struct phy_device *phydev = to_phy_device(dev);
-	struct net_device *netdev = phydev->attached_dev;
-	int ret;
-
-	if (!netdev)
-		return 0;
-
-	ret = phy_init_hw(phydev);
+	ret = phy_resume(phydev);
 	if (ret < 0)
 		return ret;
-
+no_resume:
 	if (phydev->attached_dev && phydev->adjust_link)
 		phy_start_machine(phydev);
 
 	return 0;
 }
 
-static const struct dev_pm_ops mdio_bus_phy_pm_ops = {
-	.suspend = mdio_bus_phy_suspend,
-	.resume = mdio_bus_phy_resume,
-	.freeze = mdio_bus_phy_suspend,
-	.thaw = mdio_bus_phy_resume,
-	.restore = mdio_bus_phy_restore,
-};
-
-#define MDIO_BUS_PHY_PM_OPS (&mdio_bus_phy_pm_ops)
-
-#else
-
-#define MDIO_BUS_PHY_PM_OPS NULL
-
+static SIMPLE_DEV_PM_OPS(mdio_bus_phy_pm_ops, mdio_bus_phy_suspend,
+			 mdio_bus_phy_resume);
 #endif /* CONFIG_PM */
 
 /**
@@ -554,7 +526,7 @@  static const struct device_type mdio_bus_phy_type = {
 	.name = "PHY",
 	.groups = phy_dev_groups,
 	.release = phy_device_release,
-	.pm = MDIO_BUS_PHY_PM_OPS,
+	.pm = pm_ptr(&mdio_bus_phy_pm_ops),
 };
 
 static int phy_request_driver_module(struct phy_device *dev, u32 phy_id)
@@ -1143,10 +1115,19 @@  int phy_init_hw(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	if (phydev->drv->config_init)
+	if (phydev->drv->config_init) {
 		ret = phydev->drv->config_init(phydev);
+		if (ret < 0)
+			return ret;
+	}
 
-	return ret;
+	if (phydev->drv->config_intr) {
+		ret = phydev->drv->config_intr(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(phy_init_hw);