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

[PATCH net-next 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.

Fugang, please test this as an alternative to your submission, since it did
break for me on GENET at least. Thanks!

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

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 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

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

[fugang.duan]

From: Florian Fainelli <f.fainelli@gmail.com> Sent: Tuesday, January 27, 2015 8:31 AM
> To: netdev@vger.kernel.org
> Cc: davem@davemloft.net; s.hauer@pengutronix.de; Duan Fugang-B38611;
> Florian Fainelli
> Subject: [PATCH net-next 1/4] net: phy: utilize phy_suspend and
> phy_resume
> 
> 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.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  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

Test on i.MX6q sabresd board, works fine.

Acked-by: Fugang Duan <B38611@freescale.com>
Tested-by: Fugang Duan <B38611@freescale.com>

[PATCH net-next 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>
---
 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 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>
---
 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..c90cecc0fe15 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;
 
 	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

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

[fugang.duan]

From: Florian Fainelli <f.fainelli@gmail.com> Sent: Tuesday, January 27, 2015 8:31 AM
> To: netdev@vger.kernel.org
> Cc: davem@davemloft.net; s.hauer@pengutronix.de; Duan Fugang-B38611;
> Florian Fainelli
> Subject: [PATCH net-next 3/4] net: phy: keep track of the PHY suspend
> state
> 
> 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>
> ---
>  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..c90cecc0fe15 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;

=> int ret = 0;
[...]

Regards,
Andy

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

[Florian Fainelli]

2015-01-26 18:43 GMT-08:00 fugang.duan@freescale.com
<fugang.duan@freescale.com>:
> From: Florian Fainelli <f.fainelli@gmail.com> Sent: Tuesday, January 27, 2015 8:31 AM
>> To: netdev@vger.kernel.org
>> Cc: davem@davemloft.net; s.hauer@pengutronix.de; Duan Fugang-B38611;
>> Florian Fainelli
>> Subject: [PATCH net-next 3/4] net: phy: keep track of the PHY suspend
>> state
>>
>> 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>
>> ---
>>  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..c90cecc0fe15 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;
>
> => int ret = 0;

Good catch, let me re-submit with your tags, thanks for testing!
-- 
Florian

[PATCH net-next 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.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 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 4/4] net: phy: avoid suspending twice a PHY

[fugang.duan]

From: Florian Fainelli <f.fainelli@gmail.com> Sent: Tuesday, January 27, 2015 8:31 AM
> To: netdev@vger.kernel.org
> Cc: davem@davemloft.net; s.hauer@pengutronix.de; Duan Fugang-B38611;
> Florian Fainelli
> Subject: [PATCH net-next 4/4] net: phy: avoid suspending twice a PHY
> 
> 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.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  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

Acked-by: Fugang Duan <B38611@freescale.com>
Tested-by: Fugang Duan <B38611@freescale.com>