[PATCH net-next v2 0/4] net: phy: prevent double suspend

[PATCH net-next v2 0/4] net: phy: prevent double suspend

[Florian Fainelli]

Hi David, Fugang,

This patch series addresses a problem that Fugang and I observed on different
platforms where a given PHY device might end-up being suspended twice.

Once as part of the call from ndo_open() all the way down to phy_detach() and
phy_suspend() and a second time when the generic platform device/driver
suspend/resume callbacks are called in drivers/net/phy/mdio_bus.c.

Thanks to Fugang for giving this a quick try on i.MX6/FEC and reporting
positive test results!

Florian Fainelli (4):
  net: phy: utilize phy_suspend and phy_resume
  net: phy: document has_fixups field
  net: phy: keep track of the PHY suspend state
  net: phy: avoid suspending twice a PHY

 drivers/net/phy/mdio_bus.c   | 14 ++++++++------
 drivers/net/phy/phy_device.c | 22 ++++++++++++++++++----
 include/linux/phy.h          |  3 +++
 3 files changed, 29 insertions(+), 10 deletions(-)

-- 
2.1.0

[PATCH net-next v2 1/4] net: phy: utilize phy_suspend and phy_resume

[Florian Fainelli]

phy_suspend and phy_resume are an abstraction on top of the PHY device
driver suspend and resume callbacks, utilize those since they are the
proper interface to suspending and resuming a PHY device.

Acked-by: Fugang Duan <B38611@freescale.com>
Tested-by: Fugang Duan <B38611@freescale.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:
- added Fugang's tags

 drivers/net/phy/mdio_bus.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 50051f271b10..20447741893a 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -465,7 +465,6 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 
 static int mdio_bus_suspend(struct device *dev)
 {
-	struct phy_driver *phydrv = to_phy_driver(dev->driver);
 	struct phy_device *phydev = to_phy_device(dev);
 
 	/* We must stop the state machine manually, otherwise it stops out of
@@ -479,19 +478,18 @@ static int mdio_bus_suspend(struct device *dev)
 	if (!mdio_bus_phy_may_suspend(phydev))
 		return 0;
 
-	return phydrv->suspend(phydev);
+	return phy_suspend(phydev);
 }
 
 static int mdio_bus_resume(struct device *dev)
 {
-	struct phy_driver *phydrv = to_phy_driver(dev->driver);
 	struct phy_device *phydev = to_phy_device(dev);
 	int ret;
 
 	if (!mdio_bus_phy_may_suspend(phydev))
 		goto no_resume;
 
-	ret = phydrv->resume(phydev);
+	ret = phy_resume(phydev);
 	if (ret < 0)
 		return ret;
 
-- 
2.1.0

[PATCH net-next v2 2/4] net: phy: document has_fixups field

[Florian Fainelli]

has_fixups was introduced to help keeping track of fixups/quirks running
on a PHY device, but we did not update the comment above struct
phy_device accordingly.

Fixes: b0ae009f3dc14 (net: phy: add "has_fixups" boolean property")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
No changes in v2

 include/linux/phy.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 9c189a1fa3a2..1b3690b597d5 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -327,6 +327,7 @@ struct phy_c45_device_ids {
  * c45_ids: 802.3-c45 Device Identifers if is_c45.
  * is_c45:  Set to true if this phy uses clause 45 addressing.
  * is_internal: Set to true if this phy is internal to a MAC.
+ * has_fixups: Set to true if this phy has fixups/quirks.
  * state: state of the PHY for management purposes
  * dev_flags: Device-specific flags used by the PHY driver.
  * addr: Bus address of PHY
-- 
2.1.0

[PATCH net-next v2 3/4] net: phy: keep track of the PHY suspend state

[Florian Fainelli]

In order to avoid double calls to phydev->drv->suspend and resume, keep
track of whether the PHY has already been suspended as a consequence of
a successful call to phy_suspend(). We will use this in our MDIO bus
suspend/resume hooks to avoid a double suspend call.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v3:
- added missing ret initialization reported by Fugang

 drivers/net/phy/phy_device.c | 22 ++++++++++++++++++----
 include/linux/phy.h          |  2 ++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3fc91e89f5a5..bdfe51fc3a65 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -699,6 +699,7 @@ int phy_suspend(struct phy_device *phydev)
 {
 	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
 	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
+	int ret = 0;
 
 	/* If the device has WOL enabled, we cannot suspend the PHY */
 	phy_ethtool_get_wol(phydev, &wol);
@@ -706,18 +707,31 @@ int phy_suspend(struct phy_device *phydev)
 		return -EBUSY;
 
 	if (phydrv->suspend)
-		return phydrv->suspend(phydev);
-	return 0;
+		ret = phydrv->suspend(phydev);
+
+	if (ret)
+		return ret;
+
+	phydev->suspended = true;
+
+	return ret;
 }
 EXPORT_SYMBOL(phy_suspend);
 
 int phy_resume(struct phy_device *phydev)
 {
 	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+	int ret = 0;
 
 	if (phydrv->resume)
-		return phydrv->resume(phydev);
-	return 0;
+		ret = phydrv->resume(phydev);
+
+	if (ret)
+		return ret;
+
+	phydev->suspended = false;
+
+	return ret;
 }
 EXPORT_SYMBOL(phy_resume);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1b3690b597d5..685809835b5c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -328,6 +328,7 @@ struct phy_c45_device_ids {
  * is_c45:  Set to true if this phy uses clause 45 addressing.
  * is_internal: Set to true if this phy is internal to a MAC.
  * has_fixups: Set to true if this phy has fixups/quirks.
+ * suspended: Set to true if this phy has been suspended successfully.
  * state: state of the PHY for management purposes
  * dev_flags: Device-specific flags used by the PHY driver.
  * addr: Bus address of PHY
@@ -365,6 +366,7 @@ struct phy_device {
 	bool is_c45;
 	bool is_internal;
 	bool has_fixups;
+	bool suspended;
 
 	enum phy_state state;
 
-- 
2.1.0

[PATCH net-next v2 4/4] net: phy: avoid suspending twice a PHY

[Florian Fainelli]

As part of a call to ndo_close() a netdevice driver may call
phy_disconnect() -> phy_detach() -> phy_suspend(), such that the PHY is
suspsended at this point and a netdevice driver may clock gate the
backing peripheral providing MDIO bus accessses as well.

Update mdio_bus_phy_may_suspend() to return whether a PHY is allowed to
be suspended and conversely resumed if and only if it was not previously
suspended before while it is currently in detached (netdev pointer is
NULL) state.

This fixes bus errors seen during S2/S3 suspend/resume cycles for
netdevice drivers such as GENET which clock gates the entire Ethernet
MAC, including the MDIO bus block.

Acked-by: Fugang Duan <B38611@freescale.com>
Tested-by: Fugang Duan <B38611@freescale.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changess in v2:
- added Fugang's tags

 drivers/net/phy/mdio_bus.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 20447741893a..095ef3fe369a 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -443,9 +443,13 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	if (!drv || !phydrv->suspend)
 		return false;
 
-	/* PHY not attached? May suspend. */
+	/* PHY not attached? May suspend if the PHY has not already been
+	 * suspended as part of a prior call to phy_disconnect() ->
+	 * phy_detach() -> phy_suspend() because the parent netdev might be the
+	 * MDIO bus driver and clock gated at this point.
+	 */
 	if (!netdev)
-		return true;
+		return !phydev->suspended;
 
 	/* Don't suspend PHY if the attched netdev parent may wakeup.
 	 * The parent may point to a PCI device, as in tg3 driver.
-- 
2.1.0

Re: [PATCH net-next v2 0/4] net: phy: prevent double suspend

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 26 Jan 2015 22:05:36 -0800

> This patch series addresses a problem that Fugang and I observed on different
> platforms where a given PHY device might end-up being suspended twice.
> 
> Once as part of the call from ndo_open() all the way down to phy_detach() and
> phy_suspend() and a second time when the generic platform device/driver
> suspend/resume callbacks are called in drivers/net/phy/mdio_bus.c.
> 
> Thanks to Fugang for giving this a quick try on i.MX6/FEC and reporting
> positive test results!

Series applied, thanks Florian.

Re: [PATCH net-next v2 0/4] net: phy: prevent double suspend

[Sergei Shtylyov]

Hello.

On 01/27/2015 09:05 AM, Florian Fainelli wrote:

> This patch series addresses a problem that Fugang and I observed on different
> platforms where a given PHY device might end-up being suspended twice.

> Once as part of the call from ndo_open() all the way down to phy_detach() and
> phy_suspend() and a second time when the generic platform device/driver
> suspend/resume callbacks are called in drivers/net/phy/mdio_bus.c.

> Thanks to Fugang for giving this a quick try on i.MX6/FEC and reporting
> positive test results!

    I can now confirm this patchset fixed our issue with an external abort 
during suspend on R-Car platform (with the 'sh_eth' driver)!

> Florian Fainelli (4):
>    net: phy: utilize phy_suspend and phy_resume
>    net: phy: document has_fixups field
>    net: phy: keep track of the PHY suspend state
>    net: phy: avoid suspending twice a PHY

WBR, Sergei

Re: [PATCH net-next v2 0/4] net: phy: prevent double suspend

[Florian Fainelli]

On 27/01/15 11:04, Sergei Shtylyov wrote:
> Hello.
> 
> On 01/27/2015 09:05 AM, Florian Fainelli wrote:
> 
>> This patch series addresses a problem that Fugang and I observed on
>> different
>> platforms where a given PHY device might end-up being suspended twice.
> 
>> Once as part of the call from ndo_open() all the way down to
>> phy_detach() and
>> phy_suspend() and a second time when the generic platform device/driver
>> suspend/resume callbacks are called in drivers/net/phy/mdio_bus.c.
> 
>> Thanks to Fugang for giving this a quick try on i.MX6/FEC and reporting
>> positive test results!
> 
>    I can now confirm this patchset fixed our issue with an external
> abort during suspend on R-Car platform (with the 'sh_eth' driver)!

Great, thanks Sergei!

> 
>> Florian Fainelli (4):
>>    net: phy: utilize phy_suspend and phy_resume
>>    net: phy: document has_fixups field
>>    net: phy: keep track of the PHY suspend state
>>    net: phy: avoid suspending twice a PHY
> 
> WBR, Sergei
> 


-- 
Florian