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

[PATCH RFC 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).

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        | 102 +++++++++++++++++--------------------------
 drivers/net/phy/phy_device.c |  80 ++++++++++++++++++++-------------
 drivers/net/phy/phylink.c    |   1 -
 include/linux/phy.h          |  14 ++++--
 4 files changed, 100 insertions(+), 97 deletions(-)

-- 
2.16.2

[PATCH RFC 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>
---
 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 05c1e8ef1..0aef35efd 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 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>
---
 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 478405e54..a5691536f 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 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>
---
 drivers/net/phy/phy_device.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a5691536f..c6fd79758 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 start;
+
+	mutex_lock(&phydev->lock);
+	start = phydev->state == PHY_UP && phydev->adjust_link;
+	mutex_unlock(&phydev->lock);
+
+	return start;
+}
+
 static int mdio_bus_phy_suspend(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
@@ -142,25 +154,25 @@ 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)
 {
 	struct phy_device *phydev = to_phy_device(dev);
-	struct net_device *netdev = phydev->attached_dev;
 	int ret;
 
-	if (!netdev)
+	if (!phydev->attached_dev)
 		return 0;
 
 	ret = phy_init_hw(phydev);
@@ -171,7 +183,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 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>
---
 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 0aef35efd..0ca1672a5 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 c6fd79758..08a5ff14f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -768,7 +768,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 51a011a34..402d08899 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 68127b002..bc7aa93c6 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 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>
---
 drivers/net/phy/phy.c | 48 ++++++++++++++++++++++++++++++++----------------
 include/linux/phy.h   | 12 +++++++++++-
 2 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0ca1672a5..54144cd10 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -737,21 +737,49 @@ int phy_stop_interrupts(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_stop_interrupts);
 
+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_stop - Bring down the PHY link, and stop checking the status
+ * __phy_stop - Bring down the PHY link, and stop checking the status
  * @phydev: target phy_device struct
+ * @suspending: called from a suspend callback
  */
-void phy_stop(struct phy_device *phydev)
+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;
+		phy_link_down(phydev, true);
+	}
+
+	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,7 +789,7 @@ void phy_stop(struct phy_device *phydev)
 	 * will not reenable interrupts.
 	 */
 }
-EXPORT_SYMBOL(phy_stop);
+EXPORT_SYMBOL(__phy_stop);
 
 /**
  * phy_start - start or restart a PHY device
@@ -804,18 +832,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 bc7aa93c6..be43bd270 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -940,7 +940,7 @@ struct phy_device *phy_connect(struct net_device *dev, const char *bus_id,
 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(struct phy_device *phydev, bool suspending);
 int phy_start_aneg(struct phy_device *phydev);
 int phy_aneg_done(struct phy_device *phydev);
 
@@ -964,6 +964,16 @@ static inline const char *phydev_name(const struct phy_device *phydev)
 	return dev_name(&phydev->mdio.dev);
 }
 
+static inline void phy_stop(struct phy_device *phydev)
+{
+	__phy_stop(phydev, false);
+}
+
+static inline void phy_stop_suspending(struct phy_device *phydev)
+{
+	__phy_stop(phydev, true);
+}
+
 void phy_attached_print(struct phy_device *phydev, const char *fmt, ...)
 	__printf(2, 3);
 void phy_attached_info(struct phy_device *phydev);
-- 
2.16.2

[PATCH RFC 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>
---
 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 08a5ff14f..ed501fabe 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 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>
---
 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 54144cd10..4c80e5ecb 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 ed501fabe..75afefac5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -830,8 +830,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 be43bd270..f51c68515 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1032,7 +1032,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

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

[Florian Fainelli]

On 03/14/2018 01:16 PM, Heiner Kallweit wrote:
> 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>
> ---
>  drivers/net/phy/phy_device.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index a5691536f..c6fd79758 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 start;

How about needs_start? This is uber nitpicking but it seems to be more
in line with what is being tested for here.

> +
> +	mutex_lock(&phydev->lock);
> +	start = phydev->state == PHY_UP && phydev->adjust_link;

You probably need to add an || phydev->phy_link_change here because that
is what PHYLINK uses, it does not register an adjust_link callback, but
would likely expect similar semantics. Even better, introduce a helper
function that tests for both to avoid possible issues...

> +	mutex_unlock(&phydev->lock);
> +
> +	return start;
> +}
> +
>  static int mdio_bus_phy_suspend(struct device *dev)
>  {
>  	struct phy_device *phydev = to_phy_device(dev);
> @@ -142,25 +154,25 @@ 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);

Humm, under which conditions can you not have phydev->attached_dev and
also not phydev->adjust_link being set? As mentioned earlier, you would
likely need to test for phy_link_change too here.

>  
> -	return 0;
> +	return ret;
>  }
>  
>  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)
> +	if (!phydev->attached_dev)
>  		return 0;

That does not seem to be making any functional difference, so I would
just drop this for now.

>  
>  	ret = phy_init_hw(phydev);
> @@ -171,7 +183,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;
>  }
> 


-- 
Florian

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

[Florian Fainelli]

On 03/14/2018 01:10 PM, Heiner Kallweit wrote:
> 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).

Thanks for the patch series, I will give it a spin on a number of
devices using different PHYLIB integration and see if something breaks.

> 
> 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        | 102 +++++++++++++++++--------------------------
>  drivers/net/phy/phy_device.c |  80 ++++++++++++++++++++-------------
>  drivers/net/phy/phylink.c    |   1 -
>  include/linux/phy.h          |  14 ++++--
>  4 files changed, 100 insertions(+), 97 deletions(-)
> 


-- 
Florian

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

[Florian Fainelli]

On 03/14/2018 01:16 PM, Heiner Kallweit wrote:
> 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.

Definitively an improvement, thanks!

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy.c | 48 ++++++++++++++++++++++++++++++++----------------
>  include/linux/phy.h   | 12 +++++++++++-
>  2 files changed, 43 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 0ca1672a5..54144cd10 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -737,21 +737,49 @@ int phy_stop_interrupts(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(phy_stop_interrupts);
>  
> +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_stop - Bring down the PHY link, and stop checking the status
> + * __phy_stop - Bring down the PHY link, and stop checking the status
>   * @phydev: target phy_device struct
> + * @suspending: called from a suspend callback

Can you put the same comment as what phy_stop_machine() has such that we
understand why there is a check for phydev->state > PHY_UP and what
happens when suspend is set to false and explain what happens when
@suspending is set to false.

>   */
> -void phy_stop(struct phy_device *phydev)
> +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;
> +		phy_link_down(phydev, true);
> +	}
> +
> +	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,7 +789,7 @@ void phy_stop(struct phy_device *phydev)
>  	 * will not reenable interrupts.
>  	 */
>  }
> -EXPORT_SYMBOL(phy_stop);
> +EXPORT_SYMBOL(__phy_stop);
>  
>  /**
>   * phy_start - start or restart a PHY device
> @@ -804,18 +832,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 bc7aa93c6..be43bd270 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -940,7 +940,7 @@ struct phy_device *phy_connect(struct net_device *dev, const char *bus_id,
>  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(struct phy_device *phydev, bool suspending);
>  int phy_start_aneg(struct phy_device *phydev);
>  int phy_aneg_done(struct phy_device *phydev);
>  
> @@ -964,6 +964,16 @@ static inline const char *phydev_name(const struct phy_device *phydev)
>  	return dev_name(&phydev->mdio.dev);
>  }
>  
> +static inline void phy_stop(struct phy_device *phydev)
> +{
> +	__phy_stop(phydev, false);
> +}
> +
> +static inline void phy_stop_suspending(struct phy_device *phydev)
> +{
> +	__phy_stop(phydev, true);
> +}

I am not a huge fond of these inline  functions, I would just move thos
down to phy.c and actually export both of these symbols, this is just
personal preference though.
-- 
Florian

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

[Geert Uytterhoeven]

Hi Heiner,

On Wed, Mar 14, 2018 at 9:10 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 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).
>
> 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

Thanks for your series!

I've gave this a try on a few machines, incl. r8a73a4/ape6evm and
sh73a0/kzm9g, which have smsc911x Ethernet chips on a power-managed bus.

On both machines it crashes during system suspend, which means the smsc911c's
registers are accessed while the device is suspended:

PM: suspend entry (deep)
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.001 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
PM: suspend devices took 0.130 seconds
Disabling non-boot CPUs ...
Unhandled fault: imprecise external abort (0x1406) at 0x000ce408
pgd = f4465d7b
[000ce408] *pgd=00000000
Internal error: : 1406 [#1] SMP ARM
Modules linked in:
CPU: 1 PID: 20 Comm: kworker/1:1 Not tainted
4.16.0-rc5-kzm9g-00470-g319cfb3643965f46-dirty #1030
Hardware name: Generic SH73A0 (Flattened Device Tree)
Workqueue: events linkwatch_event
PC is at __smsc911x_reg_read+0x1c/0x60
LR is at smsc911x_tx_get_txstatus+0x2c/0x7c
pc : [<c03eefd4>]    lr : [<c03ef820>]    psr: 20010093
sp : df51bd38  ip : df51bce0  fp : 00000000
r10: 00000000  r9 : 00000000  r8 : c0909b58
r7 : a0010013  r6 : df636e08  r5 : df636dc0  r4 : df636800
r3 : e0903000  r2 : 00000001  r1 : e0903080  r0 : 00000000
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 5ee4004a  DAC: 00000051
Process kworker/1:1 (pid: 20, stack limit = 0x1e2af6bb)
Stack: (0xdf51bd38 to 0xdf51c000)
bd20:                                                       c03efb14 df636800
bd40: df636dc0 c063c198 df51bdb0 c03efa80 c03efb14 df636800 df636800 c03efb20
bd60: c03efb14 dec5e8f4 df636800 c063c198 df51bdb0 c04b4494 dec5e8f0 dec4ea80
bd80: df636800 c04d7c28 dec4ea80 df636800 dec5e800 c04d3d68 0000002a 00000000
bda0: c04d3990 c020af0c df400a80 00000000 00000000 00000000 00000000 00000000
bdc0: 00000000 00000000 00000050 00000000 df51be03 c04a5828 00000580 c04a5758
bde0: dec4ea80 000004db 014000c0 c0908448 00000001 c04a58a0 df51be03 c04d14e0
be00: 00000000 3cef0b86 c04d13bc dec4ea80 df636800 00000010 00000000 00000000
be20: df636800 00000000 c0931b44 c04d73c0 00000000 00000000 00000000 00000000
be40: 00000000 00000000 00000000 ffffffff 014000c0 df636800 c0931ad8 df51bed4
be60: c0931ad8 c04d7468 014000c0 00000000 00000000 c014404c c0908448 c0908448
be80: df636800 c04d7534 014000c0 00000000 00000000 014000c0 c0908448 c04b9d8c
bea0: df636800 00000000 00000000 3cef0b86 c0931b44 df636800 c0931b44 c04d8854
bec0: df636aac c04d8b10 df51bf2c c0908448 00000000 df51bed4 df51bed4 3cef0b86
bee0: df51bf2c df50dc80 c0931ad8 dfbdaac0 df51bf2c dfbddd00 00000000 00000001
bf00: 00000000 c04d8b98 c04d8b74 c013cc8c 00000001 00000000 c013cc14 c013d214
bf20: c0908448 00000000 00000004 c0931ad8 00000000 00000000 c075f7d9 3cef0b86
bf40: c0905900 df50dc80 dfbdaac0 dfbdaac0 df51a000 dfbdaaf4 c0905900 df50dc98
bf60: 00000008 c013d4b0 df518540 df50de80 df5110c0 00000000 df491eb0 df50dc80
bf80: c013d1e4 df50deb8 00000000 c014293c df5110c0 c014281c 00000000 00000000
bfa0: 00000000 00000000 00000000 c01010b4 00000000 00000000 00000000 00000000
bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 7fdfffff fff7fdff
[<c03eefd4>] (__smsc911x_reg_read) from [<c03ef820>]
(smsc911x_tx_get_txstatus+0x2c/0x7c)
[<c03ef820>] (smsc911x_tx_get_txstatus) from [<c03efa80>]
(smsc911x_tx_update_txcounters+0x14/0xa8)
[<c03efa80>] (smsc911x_tx_update_txcounters) from [<c03efb20>]
(smsc911x_get_stats+0xc/0x58)
[<c03efb20>] (smsc911x_get_stats) from [<c04b4494>] (dev_get_stats+0x58/0xa8)
[<c04b4494>] (dev_get_stats) from [<c04d7c28>] (rtnl_fill_stats+0x38/0x118)
[<c04d7c28>] (rtnl_fill_stats) from [<c04d3d68>]
(rtnl_fill_ifinfo.constprop.12+0x6a8/0x105c)
[<c04d3d68>] (rtnl_fill_ifinfo.constprop.12) from [<c04d73c0>]
(rtmsg_ifinfo_build_skb+0x7c/0xd0)
[<c04d73c0>] (rtmsg_ifinfo_build_skb) from [<c04d7468>]
(rtmsg_ifinfo_event.part.6+0x28/0x4c)
[<c04d7468>] (rtmsg_ifinfo_event.part.6) from [<c04d7534>]
(rtmsg_ifinfo+0x24/0x2c)
[<c04d7534>] (rtmsg_ifinfo) from [<c04b9d8c>] (netdev_state_change+0x5c/0x80)
[<c04b9d8c>] (netdev_state_change) from [<c04d8854>]
(linkwatch_do_dev+0x50/0x74)
[<c04d8854>] (linkwatch_do_dev) from [<c04d8b10>]
(__linkwatch_run_queue+0x138/0x19c)
[<c04d8b10>] (__linkwatch_run_queue) from [<c04d8b98>]
(linkwatch_event+0x24/0x34)
[<c04d8b98>] (linkwatch_event) from [<c013cc8c>] (process_one_work+0x250/0x41c)
[<c013cc8c>] (process_one_work) from [<c013d4b0>] (worker_thread+0x2cc/0x40c)
[<c013d4b0>] (worker_thread) from [<c014293c>] (kthread+0x120/0x140)
[<c014293c>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xdf51bfb0 to 0xdf51bff8)
bfa0:                                     00000000 00000000 00000000 00000000
bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
Code: e5903000 e0831001 e5910000 f57ff04f (e12fff1e)

I've bisected this to  "net: phy: use new function phy_stop_suspending in,
mdio_bus_phy_suspend".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Heiner Kallweit]

Am 15.03.2018 um 00:53 schrieb Florian Fainelli:
> On 03/14/2018 01:10 PM, Heiner Kallweit wrote:
>> 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).
> 
> Thanks for the patch series, I will give it a spin on a number of
> devices using different PHYLIB integration and see if something breaks.
> 
Great, and thanks for the immediate feedback.
I'll prepare a v2 based on it, also considerung Geert's feedback.

>>
>> 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        | 102 +++++++++++++++++--------------------------
>>  drivers/net/phy/phy_device.c |  80 ++++++++++++++++++++-------------
>>  drivers/net/phy/phylink.c    |   1 -
>>  include/linux/phy.h          |  14 ++++--
>>  4 files changed, 100 insertions(+), 97 deletions(-)
>>
> 
> 

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

[Heiner Kallweit]

Am 15.03.2018 um 00:50 schrieb Florian Fainelli:
> On 03/14/2018 01:16 PM, Heiner Kallweit wrote:
>> 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>
>> ---
>>  drivers/net/phy/phy_device.c | 33 +++++++++++++++++++++++----------
>>  1 file changed, 23 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index a5691536f..c6fd79758 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 start;
> 
> How about needs_start? This is uber nitpicking but it seems to be more
> in line with what is being tested for here.
> 
Agree ..

>> +
>> +	mutex_lock(&phydev->lock);
>> +	start = phydev->state == PHY_UP && phydev->adjust_link;
> 
> You probably need to add an || phydev->phy_link_change here because that
> is what PHYLINK uses, it does not register an adjust_link callback, but
> would likely expect similar semantics. Even better, introduce a helper
> function that tests for both to avoid possible issues...
> 

phydev->phy_link_change is set in phy_attach_direct(). Therefore it's
always set if the device is attached. And mdio_bus_phy_needs_start()
is only used after we have verified that the device is attached.
Having said that I don't see when phydev->phy_link_change could
be NULL.

When talking about phydev->phy_link_change, why does it exist at all?
I found no driver setting an own callback, replacing the default
phy_link_change(). So we could use the default directly.
Or in which use case would a driver set an own callback?

>> +	mutex_unlock(&phydev->lock);
>> +
>> +	return start;
>> +}
>> +
>>  static int mdio_bus_phy_suspend(struct device *dev)
>>  {
>>  	struct phy_device *phydev = to_phy_device(dev);
>> @@ -142,25 +154,25 @@ 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);
> 
> Humm, under which conditions can you not have phydev->attached_dev and
> also not phydev->adjust_link being set? As mentioned earlier, you would
> likely need to test for phy_link_change too here.
> 
We come here only if phydev->attached_dev is set. If this is the case
and phydev->adjust_link is not set this indicates that the driver
doesn't use the phylib state machine.
And in this case I'd prefer to just call phy_resume().

>>  
>> -	return 0;
>> +	return ret;
>>  }
>>  
>>  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)
>> +	if (!phydev->attached_dev)
>>  		return 0;
> 
> That does not seem to be making any functional difference, so I would
> just drop this for now.
> 
>>  
>>  	ret = phy_init_hw(phydev);
>> @@ -171,7 +183,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;
>>  }
>>
> 
> 

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

[Heiner Kallweit]

Am 15.03.2018 um 11:07 schrieb Geert Uytterhoeven:
> Hi Heiner,
> 
> On Wed, Mar 14, 2018 at 9:10 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> 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).
>>
>> 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
> 
> Thanks for your series!
> 
> I've gave this a try on a few machines, incl. r8a73a4/ape6evm and
> sh73a0/kzm9g, which have smsc911x Ethernet chips on a power-managed bus.
> 
> On both machines it crashes during system suspend, which means the smsc911c's
> registers are accessed while the device is suspended:
> 
> PM: suspend entry (deep)
> PM: Syncing filesystems ... done.
> Freezing user space processes ... (elapsed 0.001 seconds) done.
> OOM killer disabled.
> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> PM: suspend devices took 0.130 seconds
> Disabling non-boot CPUs ...
> Unhandled fault: imprecise external abort (0x1406) at 0x000ce408
> pgd = f4465d7b
> [000ce408] *pgd=00000000
> Internal error: : 1406 [#1] SMP ARM
> Modules linked in:
> CPU: 1 PID: 20 Comm: kworker/1:1 Not tainted
> 4.16.0-rc5-kzm9g-00470-g319cfb3643965f46-dirty #1030
> Hardware name: Generic SH73A0 (Flattened Device Tree)
> Workqueue: events linkwatch_event
> PC is at __smsc911x_reg_read+0x1c/0x60
> LR is at smsc911x_tx_get_txstatus+0x2c/0x7c
> pc : [<c03eefd4>]    lr : [<c03ef820>]    psr: 20010093
> sp : df51bd38  ip : df51bce0  fp : 00000000
> r10: 00000000  r9 : 00000000  r8 : c0909b58
> r7 : a0010013  r6 : df636e08  r5 : df636dc0  r4 : df636800
> r3 : e0903000  r2 : 00000001  r1 : e0903080  r0 : 00000000
> Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 5ee4004a  DAC: 00000051
> Process kworker/1:1 (pid: 20, stack limit = 0x1e2af6bb)
> Stack: (0xdf51bd38 to 0xdf51c000)
> bd20:                                                       c03efb14 df636800
> bd40: df636dc0 c063c198 df51bdb0 c03efa80 c03efb14 df636800 df636800 c03efb20
> bd60: c03efb14 dec5e8f4 df636800 c063c198 df51bdb0 c04b4494 dec5e8f0 dec4ea80
> bd80: df636800 c04d7c28 dec4ea80 df636800 dec5e800 c04d3d68 0000002a 00000000
> bda0: c04d3990 c020af0c df400a80 00000000 00000000 00000000 00000000 00000000
> bdc0: 00000000 00000000 00000050 00000000 df51be03 c04a5828 00000580 c04a5758
> bde0: dec4ea80 000004db 014000c0 c0908448 00000001 c04a58a0 df51be03 c04d14e0
> be00: 00000000 3cef0b86 c04d13bc dec4ea80 df636800 00000010 00000000 00000000
> be20: df636800 00000000 c0931b44 c04d73c0 00000000 00000000 00000000 00000000
> be40: 00000000 00000000 00000000 ffffffff 014000c0 df636800 c0931ad8 df51bed4
> be60: c0931ad8 c04d7468 014000c0 00000000 00000000 c014404c c0908448 c0908448
> be80: df636800 c04d7534 014000c0 00000000 00000000 014000c0 c0908448 c04b9d8c
> bea0: df636800 00000000 00000000 3cef0b86 c0931b44 df636800 c0931b44 c04d8854
> bec0: df636aac c04d8b10 df51bf2c c0908448 00000000 df51bed4 df51bed4 3cef0b86
> bee0: df51bf2c df50dc80 c0931ad8 dfbdaac0 df51bf2c dfbddd00 00000000 00000001
> bf00: 00000000 c04d8b98 c04d8b74 c013cc8c 00000001 00000000 c013cc14 c013d214
> bf20: c0908448 00000000 00000004 c0931ad8 00000000 00000000 c075f7d9 3cef0b86
> bf40: c0905900 df50dc80 dfbdaac0 dfbdaac0 df51a000 dfbdaaf4 c0905900 df50dc98
> bf60: 00000008 c013d4b0 df518540 df50de80 df5110c0 00000000 df491eb0 df50dc80
> bf80: c013d1e4 df50deb8 00000000 c014293c df5110c0 c014281c 00000000 00000000
> bfa0: 00000000 00000000 00000000 c01010b4 00000000 00000000 00000000 00000000
> bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 7fdfffff fff7fdff
> [<c03eefd4>] (__smsc911x_reg_read) from [<c03ef820>]
> (smsc911x_tx_get_txstatus+0x2c/0x7c)
> [<c03ef820>] (smsc911x_tx_get_txstatus) from [<c03efa80>]
> (smsc911x_tx_update_txcounters+0x14/0xa8)
> [<c03efa80>] (smsc911x_tx_update_txcounters) from [<c03efb20>]
> (smsc911x_get_stats+0xc/0x58)
> [<c03efb20>] (smsc911x_get_stats) from [<c04b4494>] (dev_get_stats+0x58/0xa8)
> [<c04b4494>] (dev_get_stats) from [<c04d7c28>] (rtnl_fill_stats+0x38/0x118)
> [<c04d7c28>] (rtnl_fill_stats) from [<c04d3d68>]
> (rtnl_fill_ifinfo.constprop.12+0x6a8/0x105c)
> [<c04d3d68>] (rtnl_fill_ifinfo.constprop.12) from [<c04d73c0>]
> (rtmsg_ifinfo_build_skb+0x7c/0xd0)
> [<c04d73c0>] (rtmsg_ifinfo_build_skb) from [<c04d7468>]
> (rtmsg_ifinfo_event.part.6+0x28/0x4c)
> [<c04d7468>] (rtmsg_ifinfo_event.part.6) from [<c04d7534>]
> (rtmsg_ifinfo+0x24/0x2c)
> [<c04d7534>] (rtmsg_ifinfo) from [<c04b9d8c>] (netdev_state_change+0x5c/0x80)
> [<c04b9d8c>] (netdev_state_change) from [<c04d8854>]
> (linkwatch_do_dev+0x50/0x74)
> [<c04d8854>] (linkwatch_do_dev) from [<c04d8b10>]
> (__linkwatch_run_queue+0x138/0x19c)
> [<c04d8b10>] (__linkwatch_run_queue) from [<c04d8b98>]
> (linkwatch_event+0x24/0x34)
> [<c04d8b98>] (linkwatch_event) from [<c013cc8c>] (process_one_work+0x250/0x41c)
> [<c013cc8c>] (process_one_work) from [<c013d4b0>] (worker_thread+0x2cc/0x40c)
> [<c013d4b0>] (worker_thread) from [<c014293c>] (kthread+0x120/0x140)
> [<c014293c>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xdf51bfb0 to 0xdf51bff8)
> bfa0:                                     00000000 00000000 00000000 00000000
> bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> Code: e5903000 e0831001 e5910000 f57ff04f (e12fff1e)
> 
> I've bisected this to  "net: phy: use new function phy_stop_suspending in,
> mdio_bus_phy_suspend".
> 
Thanks a lot for testing and the quick feedback!
Reason for the problem seems to be that mdio_bus_phy_suspend() now includes
a call to phy_link_down() which eventually fires an asynchronous linkwatch
event. And this asynchronous event (accessing the chip registers) is
processed only after the chip has been powered down.

Maybe it's sufficient if I set the do_carrier parameter of phy_link_down()
to false if suspending. Have to spend few more thoughts on this.

Regards, Heiner


> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>