[PATCH net v2 0/2] net: spacemit: Fix TX timeout work deadlock and RTNL in PM callbacks

[PATCH net v2 0/2] net: spacemit: Fix TX timeout work deadlock and RTNL in PM callbacks

[Ali Tariq]

This series fixes two locking problems in the SpacemiT K1 Ethernet
driver.

Patch 1 fixes the TX timeout work, which ends up waiting for itself
through cancel_work_sync() in emac_down(), so every TX timeout leaves
RTNL held forever.

Patch 2 takes RTNL in emac_suspend() and emac_resume(), which fixes the
"suspicious RCU usage" warning from phy_disconnect() on suspend. It
depends on patch 1: suspend disables the TX timeout work before taking
RTNL, so it never waits for the work while holding the lock.

Tested on Orange Pi RV2 (SpacemiT K1) with a lockdep kernel: 12
suspend/resume cycles without the warning. The board loses its network
after about a dozen cycles with or without this series; that is a
separate issue.

Changes in v2:
- New patch 1 to fix the TX timeout work deadlock (reported by Troy,
  approach suggested by Vivian)
- Patch 2: take RTNL before the netif_running() checks
- Patch 2: drop Breno's Reviewed-by, since the patch changed

v1: https://lore.kernel.org/netdev/20261105123441.2524171-1-alitariq45892@gmail.com/

Ali Tariq (2):
  net: spacemit: Fix TX timeout work deadlocking on itself
  net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

 drivers/net/ethernet/spacemit/k1_emac.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH net v2 1/2] net: spacemit: Fix TX timeout work deadlocking on itself

[Ali Tariq]

emac_tx_timeout_task() takes RTNL and calls dev_close(), which ends up
waiting for the work itself. The function call hierarchy is as follows

emac_tx_timeout_task()
    dev_close()
      emac_stop()
        emac_down()
          cancel_work_sync(&priv->tx_timeout_task)

The work never finishes and keeps RTNL held, so after a TX timeout
everything that needs RTNL hangs.

Remove cancel_work_sync() from emac_down(). Disable the work in
emac_suspend() instead, and enable it again in emac_resume(). emac_remove()
runs before the devm-managed unregister_netdev(), so use
disable_work_sync() there to stop the work from being queued again.

Found by code inspection. Not reproduced on hardware. Tested on Orange
Pi RV2 (SpacemiT K1) with suspend/resume cycles and kselftest, with
lockdep enabled.

Fixes: bfec6d7f2001 ("net: spacemit: Add K1 Ethernet MAC")
Reported-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
Closes: https://lore.kernel.org/all/DMO74K2MG3D7.15UUOU1O66K6N@linux.spacemit.com/
Suggested-by: Vivian Wang <wangruikang@iscas.ac.cn>
Signed-off-by: Ali Tariq <alitariq45892@gmail.com>
---
 drivers/net/ethernet/spacemit/k1_emac.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
index 71a80838bffd..51edc95f54c8 100644
--- a/drivers/net/ethernet/spacemit/k1_emac.c
+++ b/drivers/net/ethernet/spacemit/k1_emac.c
@@ -1732,7 +1732,6 @@ static int emac_down(struct emac_priv *priv)
 	napi_disable(&priv->napi);
 
 	timer_delete_sync(&priv->txtimer);
-	cancel_work_sync(&priv->tx_timeout_task);
 
 	timer_delete_sync(&priv->stats_timer);
 
@@ -2010,8 +2009,9 @@ static void emac_remove(struct platform_device *pdev)
 {
 	struct emac_priv *priv = platform_get_drvdata(pdev);
 
+	disable_work_sync(&priv->tx_timeout_task);
+
 	timer_shutdown_sync(&priv->txtimer);
-	cancel_work_sync(&priv->tx_timeout_task);
 
 	timer_shutdown_sync(&priv->stats_timer);
 
@@ -2024,6 +2024,8 @@ static int emac_resume(struct device *dev)
 	struct net_device *ndev = priv->ndev;
 	int ret;
 
+	enable_work(&priv->tx_timeout_task);
+
 	ret = clk_prepare_enable(priv->bus_clk);
 	if (ret < 0) {
 		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
@@ -2051,6 +2053,8 @@ static int emac_suspend(struct device *dev)
 	struct emac_priv *priv = dev_get_drvdata(dev);
 	struct net_device *ndev = priv->ndev;
 
+	disable_work_sync(&priv->tx_timeout_task);
+
 	if (!ndev || !netif_running(ndev)) {
 		clk_disable_unprepare(priv->bus_clk);
 		return 0;
-- 
2.34.1

[PATCH net v2 2/2] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[Ali Tariq]

The emac_resume() calls the emac_open() and emac_suspend() calls
emac_stop() functions directly without holding the lock through
rtnl_lock(). These functions rely on phy_disconnect() which reads pointer
with rtnl_dereference(). But since no lock is being held,
rtnl_dereference throws a suspicious RCU usage on
Orange Pi RV2 (SpacemiT K1) warning when lockdep is enabled.

WARNING: suspicious RCU usage
drivers/net/phy/phy_device.c:1831 suspicious rcu_dereference_protected() usage!
Call Trace:
phy_detach_internal+0x490/0x5f2
phy_disconnect+0x90/0xc0
emac_stop+0x114/0x626
emac_suspend+0xac/0x126
platform_pm_suspend+0x72/0xe4

Take RTNL in emac_suspend() and emac_resume() before the netif_running()
checks, so the interface can't be opened or closed between the check and
emac_stop() or emac_open().

Found by CI suspend tests on Orange Pi RV2 (SpacemiT K1). Tested on the
same board with lockdep enabled: 12 suspend/resume cycles without the
warning, and kselftest with no new failures.

Fixes: bfec6d7f2001 ("net: spacemit: Add K1 Ethernet MAC")
Signed-off-by: Ali Tariq <alitariq45892@gmail.com>
---
Changes in v2:
- Moved the rtnl_lock() before the 'running' flag check
Warning Log: https://kernelci.cloud-v.co/scheduler/job/549#L2168
Fixed Log: https://kernelci.cloud-v.co/scheduler/job/589
---
 drivers/net/ethernet/spacemit/k1_emac.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
index 51edc95f54c8..d07d8a1aa5ee 100644
--- a/drivers/net/ethernet/spacemit/k1_emac.c
+++ b/drivers/net/ethernet/spacemit/k1_emac.c
@@ -2031,11 +2031,16 @@ static int emac_resume(struct device *dev)
 		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
 		return ret;
 	}
+	rtnl_lock();
 
-	if (!netif_running(ndev))
+	if (!netif_running(ndev)) {
+		rtnl_unlock();
 		return 0;
+	}
 
 	ret = emac_open(ndev);
+	rtnl_unlock();
+
 	if (ret) {
 		clk_disable_unprepare(priv->bus_clk);
 		return ret;
@@ -2055,12 +2060,16 @@ static int emac_suspend(struct device *dev)
 
 	disable_work_sync(&priv->tx_timeout_task);
 
+	rtnl_lock();
+
 	if (!ndev || !netif_running(ndev)) {
+		rtnl_unlock();
 		clk_disable_unprepare(priv->bus_clk);
 		return 0;
 	}
 
 	emac_stop(ndev);
+	rtnl_unlock();
 
 	clk_disable_unprepare(priv->bus_clk);
 	netif_device_detach(ndev);
-- 
2.34.1