[PATCH net v6 0/4] net: wwan: t7xx: fix DPMAIF data path vs system PM suspend

[PATCH net v6 0/4] net: wwan: t7xx: fix DPMAIF data path vs system PM suspend

[Tim JH Chen]

This series fixes a race between the t7xx DPMAIF data path and system PM
suspend, plus three pre-existing bugs uncovered while fixing it.

With ASPM L1 enabled and repeated suspend/resume cycles, several DPMAIF
data-plane contexts take a runtime PM reference and then access device
registers. System suspend ignores that reference, so these contexts can
touch the hardware while the suspend callback is tearing it down. The
observable symptom is a CPU soft lockup in the TX push kthread:

  watchdog: BUG: soft lockup - CPU#N stuck for 26s! [dpmaif_tx_hw_pu]
    __pm_runtime_resume+0x5b/0x80
    t7xx_dpmaif_tx_hw_push_thread+0xc4 [mtk_t7xx]

Patch 3 is the actual fix: it quiesces the DPMAIF data-plane contexts
across system suspend (a freezable TX push kthread plus an explicit drain
of the TX-done workers and the in-flight NAPI RX poll in the suspend
callback), rather than marking the data-plane workqueues freezable, which
the v5 review showed can deadlock suspend.

Patches 1, 2 and 4 are pre-existing bugs found while developing the fix,
each with its own Fixes: tag and independent of the main race:

  1 - runtime PM usage-count underflow on the -EACCES path
  2 - use-after-free from the TX push kthread exiting on resume failure
  4 - a NAPI that is never completed on the "RX queue not started" early
      return; a later napi_synchronize() under rtnl_lock then hangs the
      network stack. Patch 4 is argued from the NAPI contract (a poll
      returning < budget must call napi_complete_done()); it is not tied
      to a specific reproducer.

Only the DPMAIF data path is touched; the CLDMA control path uses a
different mechanism and is out of scope (t7xx_hif_cldma.c is unchanged).

The data-path fix (patch 3) was tested with 500+ suspend/resume cycles
with a SIM registered and ASPM L1 enabled.

v5 -> v6:
  - Drop the "freezer as a global quiesce" direction: remove every
    WQ_FREEZABLE annotation added in v4/v5. Marking the data-plane
    workqueues freezable can deadlock the suspend, because a
    flush_work()/cancel_work_sync() issued from a context the freezer
    does not freeze (the FSM kthread, or an unbind holding device_lock)
    blocks until thaw_workqueues(). (Reported on v5 review.)
  - Instead quiesce the DPMAIF data plane explicitly in the system
    suspend callback: mask interrupts, cancel_work_sync() the TX-done
    workers, call t7xx_dpmaif_rx_stop() to drain the in-flight NAPI RX
    poll (the freezer cannot park a softirq), then cancel
    bat_release_work and stop the hardware last.
  - Keep the PM freezer only for the lone TX push kthread, and use
    kthread_freezable_should_stop() instead of try_to_freeze(), so a
    concurrent kthread_stop() is honoured while the thread is frozen.
  - Fold in the pre-existing fixes previously deferred to a separate
    series: the -EACCES runtime PM usage-count underflow (patch 1), the
    TX push kthread self-exit use-after-free (patch 2), and a NAPI that
    is never completed on the not-started RX poll early return, which
    hangs a later napi_synchronize() under rtnl_lock (patch 4). This
    revision is therefore a 4-patch series.
  - Do not touch t7xx_hif_cldma.c. Restrict the commit messages to the
    DPMAIF data path; the CLDMA control path uses a different mechanism
    and is called out as out of scope.
v4 -> v5:
  - Fix freeze deadlock in t7xx_do_tx_hw_push(): when the TX-done
    workqueue (WQ_FREEZABLE) is frozen first it stops draining the DRB
    ring; the kthread then loops indefinitely in the ring-full retry
    branch and never reaches try_to_freeze(), causing a freezer timeout
    and suspend abort. (Simon Horman)
  - Extend WQ_FREEZABLE to the BAT-release and CLDMA TX/RX workqueues.
    (Simon Horman) [reverted in v6, see above]
  - Note the -EACCES underflow and the stale kthread pointer as
    pre-existing issues to be addressed separately. [done in v6, patches 1-2]
v3 -> v4:
  - Drop the tx_pm_lock / state-snapshot approach entirely and use the PM
    freezer instead. The previous approach deadlocked through the runtime
    PM wait queue and opened ISR windows by writing dpmaif_ctrl->state in
    suspend/resume.
v2 -> v3: process fixes (Fixes tag, changelog placement).
v1 -> v2: save/restore pre-suspend state; wrap pm_runtime with a mutex.

Tim JH Chen (4):
  net: wwan: t7xx: fix runtime PM usage count underflow on -EACCES
  net: wwan: t7xx: do not exit the TX push kthread on resume failure
  net: wwan: t7xx: fix race between TX/RX data path and system PM
    suspend
  net: wwan: t7xx: complete NAPI on the not-started RX poll early return

 drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c    | 24 +++++++++-
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c |  9 +++-
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 55 ++++++++++++++++++----
 3 files changed, 77 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH net v6 1/4] net: wwan: t7xx: fix runtime PM usage count underflow on -EACCES

[Tim JH Chen]

t7xx_dpmaif_tx_hw_push_thread(), t7xx_dpmaif_tx_done() and
t7xx_dpmaif_bat_release_work() treat -EACCES from
pm_runtime_resume_and_get() as success and proceed to access the
hardware. That is intentional, but pm_runtime_resume_and_get() has
already dropped the usage count it took before returning -EACCES, so the
unconditional pm_runtime_put_autosuspend() at the end of each context
drops a reference that was never held and drives the usage count
negative.

Only balance the reference when it was actually taken.

Fixes: 46e8f49ed7b3 ("net: wwan: t7xx: Introduce power management")
Signed-off-by: Tim JH Chen <tim770802@gmail.com>
---
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c |  3 ++-
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 10 ++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
index 5af90ca6e063..0e1174ee611d 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
@@ -1082,7 +1082,8 @@ static void t7xx_dpmaif_bat_release_work(struct work_struct *work)
 	}
 
 	t7xx_pci_enable_sleep(dpmaif_ctrl->t7xx_dev);
-	pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
+	if (ret != -EACCES)
+		pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
 }
 
 int t7xx_dpmaif_bat_rel_wq_alloc(struct dpmaif_ctrl *dpmaif_ctrl)
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
index 236d632cf591..bd6116a8c541 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
@@ -160,12 +160,16 @@ static void t7xx_dpmaif_tx_done(struct work_struct *work)
 	struct dpmaif_tx_queue *txq = container_of(work, struct dpmaif_tx_queue, dpmaif_tx_work);
 	struct dpmaif_ctrl *dpmaif_ctrl = txq->dpmaif_ctrl;
 	struct dpmaif_hw_info *hw_info;
+	bool pm_ref;
 	int ret;
 
 	ret = pm_runtime_resume_and_get(dpmaif_ctrl->dev);
 	if (ret < 0 && ret != -EACCES)
 		return;
 
+	/* -EACCES means no reference was taken; only balance a real one. */
+	pm_ref = !ret;
+
 	/* The device may be in low power state. Disable sleep if needed */
 	t7xx_pci_disable_sleep(dpmaif_ctrl->t7xx_dev);
 	if (t7xx_pci_sleep_disable_complete(dpmaif_ctrl->t7xx_dev)) {
@@ -185,7 +189,8 @@ static void t7xx_dpmaif_tx_done(struct work_struct *work)
 	}
 
 	t7xx_pci_enable_sleep(dpmaif_ctrl->t7xx_dev);
-	pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
+	if (pm_ref)
+		pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
 }
 
 static void t7xx_setup_msg_drb(struct dpmaif_ctrl *dpmaif_ctrl, unsigned int q_num,
@@ -467,7 +472,8 @@ static int t7xx_dpmaif_tx_hw_push_thread(void *arg)
 		t7xx_pci_disable_sleep(dpmaif_ctrl->t7xx_dev);
 		t7xx_do_tx_hw_push(dpmaif_ctrl);
 		t7xx_pci_enable_sleep(dpmaif_ctrl->t7xx_dev);
-		pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
+		if (ret != -EACCES)
+			pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
 	}
 
 	return 0;
-- 
2.43.0

[PATCH net v6 2/4] net: wwan: t7xx: do not exit the TX push kthread on resume failure

[Tim JH Chen]

t7xx_dpmaif_tx_hw_push_thread() returns, ending the kthread, when
pm_runtime_resume_and_get() fails with anything other than -EACCES.
kthread_run() keeps no extra reference to the task, so the task_struct
can be reaped while dpmaif_ctrl->tx_thread still points at it. A later
t7xx_dpmaif_tx_thread_rel() then calls kthread_stop() on the stale
pointer, which does get_task_struct() on freed memory: a use-after-free.
It also stops TX permanently and silently.

Log the failure and retry after a short back-off instead of exiting, so
the thread stays alive until kthread_stop() tears it down.

Fixes: 46e8f49ed7b3 ("net: wwan: t7xx: Introduce power management")
Signed-off-by: Tim JH Chen <tim770802@gmail.com>
---
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
index bd6116a8c541..2a405bc74312 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
@@ -447,6 +447,11 @@ static void t7xx_do_tx_hw_push(struct dpmaif_ctrl *dpmaif_ctrl)
 		 (dpmaif_ctrl->state == DPMAIF_STATE_PWRON));
 }
 
+/* Back-off before retrying a failed runtime PM resume in the TX push
+ * kthread, so a persistent error does not busy-loop.
+ */
+#define DPMAIF_TX_RESUME_RETRY_MS	20
+
 static int t7xx_dpmaif_tx_hw_push_thread(void *arg)
 {
 	struct dpmaif_ctrl *dpmaif_ctrl = arg;
@@ -466,8 +471,16 @@ static int t7xx_dpmaif_tx_hw_push_thread(void *arg)
 		}
 
 		ret = pm_runtime_resume_and_get(dpmaif_ctrl->dev);
-		if (ret < 0 && ret != -EACCES)
-			return ret;
+		if (ret < 0 && ret != -EACCES) {
+			/* Do not exit the thread: dpmaif_ctrl->tx_thread still
+			 * points at this task and t7xx_dpmaif_tx_thread_rel()
+			 * will call kthread_stop() on it. Back off and retry.
+			 */
+			dev_err_ratelimited(dpmaif_ctrl->dev,
+					    "Failed to resume for TX push: %d\n", ret);
+			msleep_interruptible(DPMAIF_TX_RESUME_RETRY_MS);
+			continue;
+		}
 
 		t7xx_pci_disable_sleep(dpmaif_ctrl->t7xx_dev);
 		t7xx_do_tx_hw_push(dpmaif_ctrl);
-- 
2.43.0

[PATCH net v6 3/4] net: wwan: t7xx: fix race between TX/RX data path and system PM suspend

[Tim JH Chen]

Several DPMAIF data-plane contexts call pm_runtime_resume_and_get() and
then access hardware registers. System suspend ignores the runtime PM
reference they hold, so with ASPM L1 enabled and repeated suspend/resume
cycles they can touch the device while the suspend callback tears it
down, ending in a CPU soft lockup:

  watchdog: BUG: soft lockup - CPU#N stuck for 26s! [dpmaif_tx_hw_pu]
    __pm_runtime_resume+0x5b/0x80
    t7xx_dpmaif_tx_hw_push_thread+0xc4 [mtk_t7xx]

Runtime suspend is already safe: while any of these contexts holds its PM
reference the runtime suspend callback cannot run. Only system suspend,
which ignores that reference, is exposed.

Quiesce the DPMAIF data-plane contexts across system suspend:

 - Make the TX push kthread freezable (set_freezable(),
   wait_event_freezable(), kthread_freezable_should_stop()) so the PM
   freezer parks it before dpm_suspend() runs the device suspend
   callbacks. kthread_freezable_should_stop() also lets a concurrent
   kthread_stop() proceed while the thread is frozen, and a
   freezing(current) bail-out in the DRB-ring-full retry loop keeps the
   thread from looping there under sustained TX.

 - The suspend callback masks interrupts and drains the TX-done workers
   (cancel_work_sync(); their producer irq_tx_done is masked and
   cancel_work_sync() also blocks a self-requeue). It then calls
   t7xx_dpmaif_rx_stop(), which clears que_started and waits for the
   in-flight NAPI RX poll to finish, so that poll -- which writes
   registers via t7xx_dpmaif_clr_ip_busy_sts() /
   t7xx_dpmaif_dlq_unmask_rx_done() -- cannot run after the hardware is
   torn down. bat_release_work is cancelled only after rx_stop(), since
   its sole producer is that NAPI poll.

The data-plane workqueues are intentionally left non-freezable: marking
them WQ_FREEZABLE would let a flush_work()/cancel_work_sync() from a
context the freezer does not freeze (the FSM kthread, or an unbind
holding device_lock) block until thaw_workqueues(), which can hang the
suspend. Draining them from the suspend callback avoids that.

This covers the DPMAIF data path that produces the observed soft lockup;
the CLDMA control path uses a different mechanism and is out of scope.

Tested with 500+ suspend/resume cycles, SIM registered and ASPM L1
enabled.

Fixes: 46e8f49ed7b3 ("net: wwan: t7xx: Introduce power management")
Signed-off-by: Tim JH Chen <tim770802@gmail.com>
---
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c    | 24 +++++++++++++++--
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 30 ++++++++++++++++++----
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c
index 7ff33c1d6ac7..b5a857e940b7 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c
@@ -410,12 +410,32 @@ static int t7xx_dpmaif_stop(struct dpmaif_ctrl *dpmaif_ctrl)
 static int t7xx_dpmaif_suspend(struct t7xx_pci_dev *t7xx_dev, void *param)
 {
 	struct dpmaif_ctrl *dpmaif_ctrl = param;
+	unsigned int i;
 
+	/* Stop new TX and mask interrupts first, so nothing re-arms the
+	 * contexts drained below.
+	 */
 	t7xx_dpmaif_tx_stop(dpmaif_ctrl);
-	t7xx_dpmaif_hw_stop_all_txq(&dpmaif_ctrl->hw_info);
-	t7xx_dpmaif_hw_stop_all_rxq(&dpmaif_ctrl->hw_info);
 	t7xx_dpmaif_disable_irq(dpmaif_ctrl);
+
+	/* irq_tx_done is masked now and cancel_work_sync() also blocks a
+	 * self-requeue, so the TX-done workers can be drained here.
+	 */
+	for (i = 0; i < DPMAIF_TXQ_NUM; i++)
+		cancel_work_sync(&dpmaif_ctrl->txq[i].dpmaif_tx_work);
+
+	/* t7xx_dpmaif_rx_stop() clears que_started and waits for the
+	 * in-flight NAPI poll (rx_processing) to finish, so no poll issues
+	 * MMIO after this point. It is also the sole producer of
+	 * bat_release_work, so cancel that work only after rx_stop();
+	 * otherwise a residual poll re-queues it and it runs against
+	 * torn-down hardware.
+	 */
 	t7xx_dpmaif_rx_stop(dpmaif_ctrl);
+	cancel_work_sync(&dpmaif_ctrl->bat_release_work);
+
+	t7xx_dpmaif_hw_stop_all_txq(&dpmaif_ctrl->hw_info);
+	t7xx_dpmaif_hw_stop_all_rxq(&dpmaif_ctrl->hw_info);
 	return 0;
 }
 
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
index 2a405bc74312..450e030fc696 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
@@ -22,6 +22,7 @@
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
+#include <linux/freezer.h>
 #include <linux/gfp.h>
 #include <linux/kernel.h>
 #include <linux/kthread.h>
@@ -426,6 +427,12 @@ static void t7xx_do_tx_hw_push(struct dpmaif_ctrl *dpmaif_ctrl)
 
 		drb_send_cnt = t7xx_txq_burst_send_skb(txq);
 		if (drb_send_cnt <= 0) {
+			/* Bail out promptly on a pending freeze so the caller can
+			 * drop its runtime-PM reference and this thread can reach
+			 * the freeze point instead of looping here under load.
+			 */
+			if (freezing(current))
+				return;
 			usleep_range(10, 20);
 			cond_resched();
 			continue;
@@ -457,19 +464,30 @@ static int t7xx_dpmaif_tx_hw_push_thread(void *arg)
 	struct dpmaif_ctrl *dpmaif_ctrl = arg;
 	int ret;
 
+	set_freezable();
+
 	while (!kthread_should_stop()) {
 		if (t7xx_tx_lists_are_all_empty(dpmaif_ctrl) ||
 		    dpmaif_ctrl->state != DPMAIF_STATE_PWRON) {
-			if (wait_event_interruptible(dpmaif_ctrl->tx_wq,
-						     (!t7xx_tx_lists_are_all_empty(dpmaif_ctrl) &&
-						     dpmaif_ctrl->state == DPMAIF_STATE_PWRON) ||
-						     kthread_should_stop()))
+			if (wait_event_freezable(dpmaif_ctrl->tx_wq,
+						 (!t7xx_tx_lists_are_all_empty(dpmaif_ctrl) &&
+						  dpmaif_ctrl->state == DPMAIF_STATE_PWRON) ||
+						 kthread_should_stop()))
 				continue;
 
 			if (kthread_should_stop())
 				break;
 		}
 
+		/* Park on a pending freeze here, outside the runtime-PM and MMIO
+		 * section below, so the PM freezer quiesces this thread before
+		 * dpm_suspend() runs the device suspend callbacks.
+		 * kthread_freezable_should_stop() also honours a concurrent
+		 * kthread_stop() while the thread is frozen.
+		 */
+		if (kthread_freezable_should_stop(NULL))
+			break;
+
 		ret = pm_runtime_resume_and_get(dpmaif_ctrl->dev);
 		if (ret < 0 && ret != -EACCES) {
 			/* Do not exit the thread: dpmaif_ctrl->tx_thread still
@@ -478,7 +496,9 @@ static int t7xx_dpmaif_tx_hw_push_thread(void *arg)
 			 */
 			dev_err_ratelimited(dpmaif_ctrl->dev,
 					    "Failed to resume for TX push: %d\n", ret);
-			msleep_interruptible(DPMAIF_TX_RESUME_RETRY_MS);
+			wait_event_freezable_timeout(dpmaif_ctrl->tx_wq,
+						     kthread_should_stop(),
+						     msecs_to_jiffies(DPMAIF_TX_RESUME_RETRY_MS));
 			continue;
 		}
 
-- 
2.43.0

[PATCH net v6 4/4] net: wwan: t7xx: complete NAPI on the not-started RX poll early return

[Tim JH Chen]

t7xx_dpmaif_napi_rx_poll() has an early return taken when the RX queue is
no longer started:

	if (!rxq->que_started) {
		atomic_set(&rxq->rx_processing, 0);
		pm_runtime_put_autosuspend(rxq->dpmaif_ctrl->dev);
		dev_err(..., "Work RXQ: %d has not been started\n", rxq->index);
		return work_done;
	}

work_done is 0 here, so the poll returns less than the budget without
calling napi_complete_done(). Per __napi_poll() (net/core/dev.c) a poll
that returns less than the budget is assumed to have completed the NAPI
itself, so the core does not reschedule it. NAPI_STATE_SCHED is therefore
left set and the NAPI is never polled again.

t7xx_dpmaif_rx_stop() clears que_started without completing the NAPI, on
the MD_STATE_EXCEPTION teardown path as well as on system suspend, so the
next poll takes this early return and strands NAPI_STATE_SCHED. A later
netdev close then hangs:

	t7xx_ccmni_close()
	  t7xx_ccmni_disable_napi()
	    napi_synchronize()	<- spins on NAPI_STATE_SCHED forever

napi_synchronize() runs with rtnl_lock held, so every subsequent rtnl
operation blocks and the network stack wedges (a hung task, not a soft
lockup).

Complete the NAPI on this early return so NAPI_STATE_SCHED is cleared and
napi_synchronize() can make progress. The sleep-lock retry branch just
below already calls napi_complete_done() before rescheduling, so only the
not-started branch needs the fix.

This is a pre-existing bug, independent of the TX/RX data path vs system
PM suspend race fixed earlier in this series. It was only reached once
that race stopped soft-locking the CPU before the teardown could run.

Fixes: d642b012df70 ("net: wwan: t7xx: Add data path interface")
Signed-off-by: Tim JH Chen <tim770802@gmail.com>
---
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
index 0e1174ee611d..6272956fe053 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
@@ -848,6 +848,12 @@ int t7xx_dpmaif_napi_rx_poll(struct napi_struct *napi, const int budget)
 		atomic_set(&rxq->rx_processing, 0);
 		pm_runtime_put_autosuspend(rxq->dpmaif_ctrl->dev);
 		dev_err(rxq->dpmaif_ctrl->dev, "Work RXQ: %d has not been started\n", rxq->index);
+		/* Returning work_done < budget without completing the NAPI would
+		 * leave NAPI_STATE_SCHED set, hanging a later napi_synchronize()
+		 * in t7xx_ccmni_disable_napi() (which holds rtnl_lock). Complete
+		 * it here so the queue is cleanly unscheduled after rx_stop().
+		 */
+		napi_complete_done(napi, work_done);
 		return work_done;
 	}
 
-- 
2.43.0