[PATCH RFC v2 0/7] net: phy: patch series aiming to improve few aspects of phylib

[PATCH RFC v2 0/7] net: phy: patch series aiming to improve few aspects of phylib

[Heiner Kallweit]

This patch series aims to tackle few issues with phylib:
 
- address issues with patch series [1] (smsc911x + phylib changes)
- make phy_stop synchronous
- get rid of phy_start/stop_machine and handle it in phy_start/phy_stop
- in mdio_suspend consider runtime pm state of mdio bus parent
- consider more WOL conditions when deciding whether PHY is allowed to
  suspend
- only resume phy after system suspend if needed

[1] https://www.mail-archive.com/netdev@vger.kernel.org/msg196061.html

It works fine here but other NIC drivers may use phylib differently. 
Therefore I'd appreciate feedback and more testing.

I could think of some subsequent patches, e.g. phy_error() could be
reduced to calling phy_stop() and printing an error message
(today it silently sets the PHY state to PHY_HALTED).

Changes in v2:
- Incorporate review comments
- Address error reported by Geert by changing phy_stop() to not trigger
  a linkwatch event.

Heiner Kallweit (7):
  net: phy: unconditionally resume and re-enable interrupts in phy_start
  net: phy: improve checking for when PHY is allowed to suspend
  net: phy: resume PHY only if needed in mdio_bus_phy_suspend
  net: phy: remove phy_start_machine
  net: phy: make phy_stop synchronous
  net: phy: use new function phy_stop_suspending in mdio_bus_phy_suspend
  net: phy: remove phy_stop_machine

 drivers/net/phy/phy.c        | 127 +++++++++++++++++++++----------------------
 drivers/net/phy/phy_device.c |  77 ++++++++++++++++----------
 drivers/net/phy/phylink.c    |   1 -
 include/linux/phy.h          |   3 +-
 4 files changed, 112 insertions(+), 96 deletions(-)

-- 
2.16.2

[PATCH RFC v2 1/7] net: phy: unconditionally resume and re-enable interrupts, in phy_start

[Heiner Kallweit]

In subsequent patches of this series interrupts will be disabled during
system suspend, also we will have to resume the PHY in states other than
PHY_HALTED. To prepare for this unconditionally resume and re-enable
interrupts in phy_start().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- no changes
---
 drivers/net/phy/phy.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 05c1e8ef..0aef35ef 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -791,10 +791,16 @@ EXPORT_SYMBOL(phy_stop);
  */
 void phy_start(struct phy_device *phydev)
 {
-	int err = 0;
+	/* if phy was suspended, bring the physical link up again */
+	phy_resume(phydev);
 
-	mutex_lock(&phydev->lock);
+	/* make sure interrupts are re-enabled for the PHY */
+	if (phy_interrupt_is_valid(phydev) && phy_enable_interrupts(phydev)) {
+		phy_error(phydev);
+		return;
+	}
 
+	mutex_lock(&phydev->lock);
 	switch (phydev->state) {
 	case PHY_STARTING:
 		phydev->state = PHY_PENDING;
@@ -803,16 +809,6 @@ void phy_start(struct phy_device *phydev)
 		phydev->state = PHY_UP;
 		break;
 	case PHY_HALTED:
-		/* if phy was suspended, bring the physical link up again */
-		__phy_resume(phydev);
-
-		/* make sure interrupts are re-enabled for the PHY */
-		if (phy_interrupt_is_valid(phydev)) {
-			err = phy_enable_interrupts(phydev);
-			if (err < 0)
-				break;
-		}
-
 		phydev->state = PHY_RESUMING;
 		break;
 	default:
-- 
2.16.2

[PATCH RFC v2 2/7] net: phy: improve checking for when PHY is allowed to, suspend

[Heiner Kallweit]

This patch improves and unifies checking for when PHY is allowed to
suspend. New is a check for the parent of the MDIO bus being
runtime-suspended. In this case the MDIO bus may not be accessible and
therefore we don't try to suspend the PHY. Instead we rely on the
parent to suspend all devices on the MDIO bus when it's suspended.

In addition change the behavior to return 0 instead of -EBUSY from
phy_suspend() if we detect a situation where the PHY shouldn't be
suspended. Returning -EBUSY from a suspend callback is supported
for runtime pm, however in case of system suspend it prevents the
system from suspending.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- no changes
---
 drivers/net/phy/phy_device.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b2853233..85ebb969 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -35,6 +35,7 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 
 #include <asm/irq.h>
 
@@ -75,14 +76,27 @@ extern struct phy_driver genphy_10g_driver;
 static LIST_HEAD(phy_fixup_list);
 static DEFINE_MUTEX(phy_fixup_lock);
 
-#ifdef CONFIG_PM
-static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
+static bool phy_may_suspend(struct phy_device *phydev)
 {
 	struct device_driver *drv = phydev->mdio.dev.driver;
 	struct phy_driver *phydrv = to_phy_driver(drv);
 	struct net_device *netdev = phydev->attached_dev;
+	struct device *mdio_parent = phydev->mdio.bus->parent;
+	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
+
+	if (!drv || !phydrv->suspend || phydev->suspended)
+		return false;
+
+	/* If the device has WOL enabled, we cannot suspend the PHY */
+	phy_ethtool_get_wol(phydev, &wol);
+	if (wol.wolopts)
+		return false;
 
-	if (!drv || !phydrv->suspend)
+	/* If the parent of the MDIO bus is runtime-suspended, the MDIO bus may
+	 * not be accessible and we expect the parent to suspend all devices
+	 * on the MDIO bus when it suspends.
+	 */
+	if (mdio_parent && pm_runtime_suspended(mdio_parent))
 		return false;
 
 	/* PHY not attached? May suspend if the PHY has not already been
@@ -91,7 +105,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	 * MDIO bus driver and clock gated at this point.
 	 */
 	if (!netdev)
-		return !phydev->suspended;
+		return true;
 
 	/* Don't suspend PHY if the attached netdev parent may wakeup.
 	 * The parent may point to a PCI device, as in tg3 driver.
@@ -109,6 +123,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	return true;
 }
 
+#ifdef CONFIG_PM
 static int mdio_bus_phy_suspend(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
@@ -121,9 +136,6 @@ static int mdio_bus_phy_suspend(struct device *dev)
 	if (phydev->attached_dev && phydev->adjust_link)
 		phy_stop_machine(phydev);
 
-	if (!mdio_bus_phy_may_suspend(phydev))
-		return 0;
-
 	return phy_suspend(phydev);
 }
 
@@ -132,14 +144,10 @@ static int mdio_bus_phy_resume(struct device *dev)
 	struct phy_device *phydev = to_phy_device(dev);
 	int ret;
 
-	if (!mdio_bus_phy_may_suspend(phydev))
-		goto no_resume;
-
 	ret = phy_resume(phydev);
 	if (ret < 0)
 		return ret;
 
-no_resume:
 	if (phydev->attached_dev && phydev->adjust_link)
 		phy_start_machine(phydev);
 
@@ -1148,13 +1156,10 @@ EXPORT_SYMBOL(phy_detach);
 int phy_suspend(struct phy_device *phydev)
 {
 	struct phy_driver *phydrv = to_phy_driver(phydev->mdio.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);
-	if (wol.wolopts)
-		return -EBUSY;
+	if (!phy_may_suspend(phydev))
+		return 0;
 
 	if (phydev->drv && phydrv->suspend)
 		ret = phydrv->suspend(phydev);
-- 
2.16.2

[PATCH RFC v2 3/7] net: phy: resume PHY only if needed in mdio_bus_phy_suspend

[Heiner Kallweit]

Currently the PHY is unconditionally resumed in mdio_bus_phy_suspend().
In cases where the PHY was sleepinh before suspending or if somebody else
takes care of resuming later, this is not needed and wastes energy.

Also start the state machine only if it's used by the driver (indicated
by the adjust_link callback being defined).

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rename variable in mdio_bus_phy_needs_start
---
 drivers/net/phy/phy_device.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 85ebb969..c934725b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -124,6 +124,18 @@ static bool phy_may_suspend(struct phy_device *phydev)
 }
 
 #ifdef CONFIG_PM
+
+static bool mdio_bus_phy_needs_start(struct phy_device *phydev)
+{
+	bool needs_start;
+
+	mutex_lock(&phydev->lock);
+	needs_start = phydev->state == PHY_UP && phydev->adjust_link;
+	mutex_unlock(&phydev->lock);
+
+	return needs_start;
+}
+
 static int mdio_bus_phy_suspend(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
@@ -142,16 +154,17 @@ static int mdio_bus_phy_suspend(struct device *dev)
 static int mdio_bus_phy_resume(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
-	int ret;
+	int ret = 0;
 
-	ret = phy_resume(phydev);
-	if (ret < 0)
-		return ret;
+	if (!phydev->attached_dev)
+		return 0;
 
-	if (phydev->attached_dev && phydev->adjust_link)
-		phy_start_machine(phydev);
+	if (mdio_bus_phy_needs_start(phydev))
+		phy_start(phydev);
+	else if (!phydev->adjust_link)
+		ret = phy_resume(phydev);
 
-	return 0;
+	return ret;
 }
 
 static int mdio_bus_phy_restore(struct device *dev)
@@ -171,7 +184,8 @@ static int mdio_bus_phy_restore(struct device *dev)
 	phydev->link = 0;
 	phydev->state = PHY_UP;
 
-	phy_start_machine(phydev);
+	if (mdio_bus_phy_needs_start(phydev))
+		phy_start(phydev);
 
 	return 0;
 }
-- 
2.16.2

[PATCH RFC v2 4/7] net: phy: remove phy_start_machine

[Heiner Kallweit]

Now that phy_start() integrated the functionality of phy_start_machine()
we can remove it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- no changes
---
 drivers/net/phy/phy.c        | 16 ----------------
 drivers/net/phy/phy_device.c |  1 -
 drivers/net/phy/phylink.c    |  1 -
 include/linux/phy.h          |  1 -
 4 files changed, 19 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0aef35ef..0ca1672a 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -546,22 +546,6 @@ int phy_start_aneg(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_start_aneg);
 
-/**
- * phy_start_machine - start PHY state machine tracking
- * @phydev: the phy_device struct
- *
- * Description: The PHY infrastructure can run a state machine
- *   which tracks whether the PHY is starting up, negotiating,
- *   etc.  This function starts the delayed workqueue which tracks
- *   the state of the PHY. If you want to maintain your own state machine,
- *   do not call this function.
- */
-void phy_start_machine(struct phy_device *phydev)
-{
-	queue_delayed_work(system_power_efficient_wq, &phydev->state_queue, HZ);
-}
-EXPORT_SYMBOL_GPL(phy_start_machine);
-
 /**
  * phy_trigger_machine - trigger the state machine to run
  *
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c934725b..ed97f152 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -769,7 +769,6 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,
 		return rc;
 
 	phy_prepare_link(phydev, handler);
-	phy_start_machine(phydev);
 	if (phydev->irq > 0)
 		phy_start_interrupts(phydev);
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 51a011a3..402d0889 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -694,7 +694,6 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy)
 		   __ETHTOOL_LINK_MODE_MASK_NBITS, pl->supported,
 		   phy->advertising);
 
-	phy_start_machine(phy);
 	if (phy->irq > 0)
 		phy_start_interrupts(phy);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 68127b00..bc7aa93c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1022,7 +1022,6 @@ int phy_drivers_register(struct phy_driver *new_driver, int n,
 void phy_state_machine(struct work_struct *work);
 void phy_change_work(struct work_struct *work);
 void phy_mac_interrupt(struct phy_device *phydev);
-void phy_start_machine(struct phy_device *phydev);
 void phy_stop_machine(struct phy_device *phydev);
 void phy_trigger_machine(struct phy_device *phydev, bool sync);
 int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd);
-- 
2.16.2

[PATCH RFC v2 5/7] net: phy: make phy_stop synchronous

[Heiner Kallweit]

Currently phy_stop() just sets the state to PHY_HALTED and relies on the
state machine to do the remaining work. It can take up to 1s until the
state machine runs again what causes issues in situations where e.g.
driver / device is brought down directly after executing phy_stop().

Fix this by executing all phy_stop() activities synchronously.

Add a function phy_stop_suspending() which does basically the same as
phy_stop() and just adopts the state adjustment logic from
phy_stop_machine() to inform the resume callback about the status of
the PHY before suspending.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
Changes in v2:
- Extend documenting comment for phy_stop_suspend
- Address error reported by Geert by changing call to phy_link_down()
  to not trigger a linkwatch event
---
 drivers/net/phy/phy.c | 70 ++++++++++++++++++++++++++++++++++++++-------------
 include/linux/phy.h   |  1 +
 2 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0ca1672a..6ffb9952 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -737,21 +737,45 @@ int phy_stop_interrupts(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_stop_interrupts);
 
-/**
- * phy_stop - Bring down the PHY link, and stop checking the status
- * @phydev: target phy_device struct
- */
-void phy_stop(struct phy_device *phydev)
+static void phy_link_up(struct phy_device *phydev)
+{
+	phydev->phy_link_change(phydev, true, true);
+	phy_led_trigger_change_speed(phydev);
+}
+
+static void phy_link_down(struct phy_device *phydev, bool do_carrier)
+{
+	phydev->phy_link_change(phydev, false, do_carrier);
+	phy_led_trigger_change_speed(phydev);
+}
+
+static void __phy_stop(struct phy_device *phydev, bool suspending)
 {
 	mutex_lock(&phydev->lock);
 
 	if (PHY_HALTED == phydev->state)
 		goto out_unlock;
 
+	/* stop state machine */
+	cancel_delayed_work_sync(&phydev->state_queue);
+
 	if (phy_interrupt_is_valid(phydev))
 		phy_disable_interrupts(phydev);
 
-	phydev->state = PHY_HALTED;
+	if (phydev->link) {
+		phydev->link = 0;
+		if (phydev->adjust_link)
+			phy_link_down(phydev, false);
+	}
+
+	phy_suspend(phydev);
+
+	if (suspending) {
+		if (phydev->state > PHY_UP && phydev->state != PHY_HALTED)
+			phydev->state = PHY_UP;
+	} else {
+		phydev->state = PHY_HALTED;
+	}
 
 out_unlock:
 	mutex_unlock(&phydev->lock);
@@ -761,8 +785,30 @@ void phy_stop(struct phy_device *phydev)
 	 * will not reenable interrupts.
 	 */
 }
+
+/**
+ * phy_stop - Bring down the PHY link, and stop checking the status
+ * @phydev: target phy_device struct
+ */
+void phy_stop(struct phy_device *phydev)
+{
+	__phy_stop(phydev, false);
+}
 EXPORT_SYMBOL(phy_stop);
 
+/**
+ * phy_stop_suspending - Bring down the PHY link, preparing for system suspend
+ * @phydev: target phy_device struct
+ *
+ * Description: Basically the same as phy_stop(), just sets the state to UP
+ * (unless it wasn't up yet)
+ */
+void phy_stop_suspending(struct phy_device *phydev)
+{
+	__phy_stop(phydev, true);
+}
+EXPORT_SYMBOL(phy_stop_suspending);
+
 /**
  * phy_start - start or restart a PHY device
  * @phydev: target phy_device struct
@@ -804,18 +850,6 @@ void phy_start(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_start);
 
-static void phy_link_up(struct phy_device *phydev)
-{
-	phydev->phy_link_change(phydev, true, true);
-	phy_led_trigger_change_speed(phydev);
-}
-
-static void phy_link_down(struct phy_device *phydev, bool do_carrier)
-{
-	phydev->phy_link_change(phydev, false, do_carrier);
-	phy_led_trigger_change_speed(phydev);
-}
-
 /**
  * phy_state_machine - Handle the state machine
  * @work: work_struct that describes the work to be done
diff --git a/include/linux/phy.h b/include/linux/phy.h
index bc7aa93c..780c2690 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -941,6 +941,7 @@ void phy_disconnect(struct phy_device *phydev);
 void phy_detach(struct phy_device *phydev);
 void phy_start(struct phy_device *phydev);
 void phy_stop(struct phy_device *phydev);
+void phy_stop_suspending(struct phy_device *phydev);
 int phy_start_aneg(struct phy_device *phydev);
 int phy_aneg_done(struct phy_device *phydev);
 
-- 
2.16.2

[PATCH RFC v2 6/7] net: phy: use new function phy_stop_suspending in mdio_bus_phy_suspend

[Heiner Kallweit]

Use new function phy_stop_suspending() in mdio_bus_phy_suspend() to also
disable interrupts and set link state to down.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- no changes
---
 drivers/net/phy/phy_device.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ed97f152..a33dec37 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -139,6 +139,7 @@ static bool mdio_bus_phy_needs_start(struct phy_device *phydev)
 static int mdio_bus_phy_suspend(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
+	int ret = 0;
 
 	/* We must stop the state machine manually, otherwise it stops out of
 	 * control, possibly with the phydev->lock held. Upon resume, netdev
@@ -146,9 +147,11 @@ static int mdio_bus_phy_suspend(struct device *dev)
 	 * lead to a deadlock.
 	 */
 	if (phydev->attached_dev && phydev->adjust_link)
-		phy_stop_machine(phydev);
+		phy_stop_suspending(phydev);
+	else
+		ret = phy_suspend(phydev);
 
-	return phy_suspend(phydev);
+	return ret;
 }
 
 static int mdio_bus_phy_resume(struct device *dev)
-- 
2.16.2

[PATCH RFC v2 7/7] net: phy: remove phy_stop_machine

[Heiner Kallweit]

Now that the functionality of phy_stop() was integrated to __phy_stop()
we can remove phy_stop_machine().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- no changes
---
 drivers/net/phy/phy.c        | 18 ------------------
 drivers/net/phy/phy_device.c |  2 --
 include/linux/phy.h          |  1 -
 3 files changed, 21 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 6ffb9952..b6a24ab8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -565,24 +565,6 @@ void phy_trigger_machine(struct phy_device *phydev, bool sync)
 	queue_delayed_work(system_power_efficient_wq, &phydev->state_queue, 0);
 }
 
-/**
- * phy_stop_machine - stop the PHY state machine tracking
- * @phydev: target phy_device struct
- *
- * Description: Stops the state machine delayed workqueue, sets the
- *   state to UP (unless it wasn't up yet). This function must be
- *   called BEFORE phy_detach.
- */
-void phy_stop_machine(struct phy_device *phydev)
-{
-	cancel_delayed_work_sync(&phydev->state_queue);
-
-	mutex_lock(&phydev->lock);
-	if (phydev->state > PHY_UP && phydev->state != PHY_HALTED)
-		phydev->state = PHY_UP;
-	mutex_unlock(&phydev->lock);
-}
-
 /**
  * phy_error - enter HALTED state for this PHY device
  * @phydev: target phy_device struct
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a33dec37..a0c39b4d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -831,8 +831,6 @@ void phy_disconnect(struct phy_device *phydev)
 	if (phydev->irq > 0)
 		phy_stop_interrupts(phydev);
 
-	phy_stop_machine(phydev);
-
 	phydev->adjust_link = NULL;
 
 	phy_detach(phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 780c2690..5c953bd3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1023,7 +1023,6 @@ int phy_drivers_register(struct phy_driver *new_driver, int n,
 void phy_state_machine(struct work_struct *work);
 void phy_change_work(struct work_struct *work);
 void phy_mac_interrupt(struct phy_device *phydev);
-void phy_stop_machine(struct phy_device *phydev);
 void phy_trigger_machine(struct phy_device *phydev, bool sync);
 int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd);
 void phy_ethtool_ksettings_get(struct phy_device *phydev,
-- 
2.16.2

- Address error reported by Geert by changing call to phy_link_down()