Skip to contentSkip to conversation

net: bcmgenet: fix aborted suspend

PATCH In review Doug Berger 2 participants 4 messages Nov 19, 2018 – Nov 20, 2018

Subsystems (3)
Networking Network drivers Broadcom genet Ethernet driver
lore
Share & tools
lore

Doug Berger

author maintainer
#0 lore raw

v2 cover net: bcmgenet: fix aborted suspend

It is not enough to return an error code from the driver suspend routine. The driver must also restore the device functionality.

This commit corrects the issue introduced by commit 0db55093b566 ("net: bcmgenet: return correct value 'ret' from bcmgenet_power_down") by calling the driver resume function if the suspend function returns an error.

Changes in v2:
- Removed accidentally included write to HFB register

Doug Berger (2):
net: bcmgenet: code movement
net: bcmgenet: abort suspend on error

 drivers/net/ethernet/broadcom/genet/bcmgenet.c     | 63 +++++++++++-----------
 drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c |  2 +
 2 files changed, 35 insertions(+), 30 deletions(-)
Signature · 1 line
2.7.4
author maintainer
#1 lore raw

PATCH v2 1/2 net: bcmgenet: code movement

From: Doug Berger <[email protected]>

This commit switches the order of bcmgenet_suspend and bcmgenet_resume in the file to prevent the need for a forward declaration in the next commit and to make the review of that commit easier.

Signed-off-by Doug Berger <[email protected]>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 60 +++++++++++++-------------
 1 file changed, 30 insertions(+), 30 deletions(-)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +30 -30
index bf88749505a9..01c8499fd87f 100644
@@ -3612,36 +3612,6 @@ static int bcmgenet_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int bcmgenet_suspend(struct device *d)
-{
-	struct net_device *dev = dev_get_drvdata(d);
-	struct bcmgenet_priv *priv = netdev_priv(dev);
-	int ret = 0;
-
-	if (!netif_running(dev))
-		return 0;
-
-	netif_device_detach(dev);
-
-	bcmgenet_netif_stop(dev);
-
-	if (!device_may_wakeup(d))
-		phy_suspend(dev->phydev);
-
-	/* Prepare the device for Wake-on-LAN and switch to the slow clock */
-	if (device_may_wakeup(d) && priv->wolopts) {
-		ret = bcmgenet_power_down(priv, GENET_POWER_WOL_MAGIC);
-		clk_prepare_enable(priv->clk_wol);
-	} else if (priv->internal_phy) {
-		ret = bcmgenet_power_down(priv, GENET_POWER_PASSIVE);
-	}
-
-	/* Turn off the clocks */
-	clk_disable_unprepare(priv->clk);
-
-	return ret;
-}
-
 static int bcmgenet_resume(struct device *d)
 {
 	struct net_device *dev = dev_get_drvdata(d);

@@ -3719,6 +3689,36 @@ static int bcmgenet_resume(struct device *d)
 	clk_disable_unprepare(priv->clk);
 	return ret;
 }
+
+static int bcmgenet_suspend(struct device *d)
+{
+	struct net_device *dev = dev_get_drvdata(d);
+	struct bcmgenet_priv *priv = netdev_priv(dev);
+	int ret = 0;
+
+	if (!netif_running(dev))
+		return 0;
+
+	netif_device_detach(dev);
+
+	bcmgenet_netif_stop(dev);
+
+	if (!device_may_wakeup(d))
+		phy_suspend(dev->phydev);
+
+	/* Prepare the device for Wake-on-LAN and switch to the slow clock */
+	if (device_may_wakeup(d) && priv->wolopts) {
+		ret = bcmgenet_power_down(priv, GENET_POWER_WOL_MAGIC);
+		clk_prepare_enable(priv->clk_wol);
+	} else if (priv->internal_phy) {
+		ret = bcmgenet_power_down(priv, GENET_POWER_PASSIVE);
+	}
+
+	/* Turn off the clocks */
+	clk_disable_unprepare(priv->clk);
+
+	return ret;
+}
 #endif /* CONFIG_PM_SLEEP */
 
 static SIMPLE_DEV_PM_OPS(bcmgenet_pm_ops, bcmgenet_suspend, bcmgenet_resume);
Signature · 1 line
2.7.4
author maintainer
#2 lore raw

PATCH v2 2/2 net: bcmgenet: abort suspend on error

In reply to Doug Berger

From: Doug Berger <[email protected]>

If an error occurs during suspension of the driver the driver should restore the hardware configuration and return an error to force the system to resume.

Fixes 0db55093b566 ("net: bcmgenet: return correct value 'ret' from bcmgenet_power_down")
Signed-off-by Doug Berger <[email protected]>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c     | 3 +++
 drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 2 ++
 2 files changed, 5 insertions(+)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +3 -0
index 01c8499fd87f..983245c0867c 100644
@@ -3717,6 +3717,9 @@ static int bcmgenet_suspend(struct device *d)
 	/* Turn off the clocks */
 	clk_disable_unprepare(priv->clk);
 
+	if (ret)
+		bcmgenet_resume(d);
+
 	return ret;
 }
 #endif /* CONFIG_PM_SLEEP */
drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c +2 -0
index 2fbd027f0148..57582efa362d 100644
@@ -186,6 +186,8 @@ void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
 	}
 
 	reg = bcmgenet_umac_readl(priv, UMAC_MPD_CTRL);
+	if (!(reg & MPD_EN))
+		return;	/* already powered up so skip the rest */
 	reg &= ~MPD_EN;
 	bcmgenet_umac_writel(priv, reg, UMAC_MPD_CTRL);
Signature · 1 line
2.7.4
maintainer
#3 lore raw
In reply to Doug Berger

From: Doug Berger <[email protected]>
Date: Mon, 19 Nov 2018 09:56:00 -0800

Quoted 10 lines

It is not enough to return an error code from the driver suspend routine. The driver must also restore the device functionality.

This commit corrects the issue introduced by commit 0db55093b566 ("net: bcmgenet: return correct value 'ret' from bcmgenet_power_down") by calling the driver resume function if the suspend function returns an error.

Changes in v2:
- Removed accidentally included write to HFB register

Doug, this doesn't apply cleanly to net-next.