[PATCH net v4] net: wwan: t7xx: fix race between TX path and system PM suspend

[PATCH net v4] net: wwan: t7xx: fix race between TX path and system PM suspend

[Tim JH Chen]

Two DPMAIF TX contexts run pm_runtime_resume_and_get() followed by MMIO
independently of the PM core: the TX push kthread
(t7xx_dpmaif_tx_hw_push_thread) and the TX-done work
(t7xx_dpmaif_tx_done). Neither is stopped during a system suspend
transition, and system suspend does not honour the runtime PM reference
they hold, so they can touch the hardware while t7xx_dpmaif_suspend()
tears it down. With ASPM L1 enabled and repeated suspend/resume cycles
this ends 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 either context holds its PM
reference the runtime suspend callback cannot run. Only system suspend,
which ignores that reference, is exposed.

Quiesce both contexts with the PM freezer, which runs before dpm_suspend()
invokes the device suspend callbacks:

  - The push kthread is made freezable: set_freezable() and
    wait_event_freezable() for the idle wait, plus try_to_freeze() before
    the pm_runtime section so it also reaches a freeze point under
    continuous TX traffic.
  - The TX-done workqueue is marked WQ_FREEZABLE so the workqueue freezer
    drains it before suspend; this also parks its self-requeue until thaw.

Tasks are thawed only after the resume callbacks have re-armed the
hardware, so neither context can issue MMIO against a torn-down or
not-yet-rearmed device. No lock is shared with the PM callbacks, so this
cannot deadlock.

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>
---
v3 -> v4:
  - Drop the tx_pm_lock / state-snapshot approach entirely and use the PM
    freezer for both TX contexts instead. The previous approach deadlocked
    through the runtime PM wait queue (t7xx_dpmaif_suspend() is also the
    .runtime_suspend callback) and opened ISR windows by writing
    dpmaif_ctrl->state in suspend/resume.
  - Also cover t7xx_dpmaif_tx_done() (WQ_FREEZABLE), which has the same
    pm_runtime + MMIO pattern as the kthread.
  - Trim the changelog/commit message.
v2 -> v3: process fixes (Fixes tag, changelog placement).
v1 -> v2: save/restore pre-suspend state; wrap pm_runtime with a mutex.

 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
index 236d632cf591..804bd730c40f 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>
@@ -447,19 +448,28 @@ 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;
 		}
 
+		/* Freeze here, outside the runtime-PM and MMIO section below, so
+		 * the system suspend freezer parks this thread before the device
+		 * suspend callbacks tear the DPMAIF hardware down.
+		 */
+		if (try_to_freeze())
+			continue;
+
 		ret = pm_runtime_resume_and_get(dpmaif_ctrl->dev);
 		if (ret < 0 && ret != -EACCES)
 			return ret;
@@ -617,7 +627,7 @@ int t7xx_dpmaif_txq_init(struct dpmaif_tx_queue *txq)
 	}
 
 	txq->worker = alloc_ordered_workqueue("md_dpmaif_tx%d_worker",
-				WQ_MEM_RECLAIM | (txq->index ? 0 : WQ_HIGHPRI),
+				WQ_MEM_RECLAIM | WQ_FREEZABLE | (txq->index ? 0 : WQ_HIGHPRI),
 				txq->index);
 	if (!txq->worker)
 		return -ENOMEM;
-- 
2.43.0

Re: [PATCH net v4] net: wwan: t7xx: fix race between TX path and system PM suspend

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says:

Hi Tim,

While I think the issues that are flagged as pre-existing below
can be considered in the context of possible follow-up, the other
two issues - the first and last one - do look like they effect this patch.

---
net: wwan: t7xx: fix race between TX path and system PM suspend

This commit addresses a CPU soft lockup that occurs during system suspend
when ASPM L1 is enabled. It aims to quiesce the DPMAIF TX contexts by
using the PM freezer, marking the TX push kthread as freezable and the
TX-done workqueue with WQ_FREEZABLE. This prevents them from touching
hardware while the suspend callback tears it down.

> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
> index 236d632cf591b..804bd730c40fa 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c

[ ... ]

> @@ -447,19 +448,28 @@ 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;
>  		}
>  
> +		/* Freeze here, outside the runtime-PM and MMIO section below, so
> +		 * the system suspend freezer parks this thread before the device
> +		 * suspend callbacks tear the DPMAIF hardware down.
> +		 */
> +		if (try_to_freeze())
> +			continue;
> +

[Severity: High]
Does this setup cause a freeze deadlock?

Since the tx worker queue is now freezable, if the worker freezes first, it
stops processing TX completions. The DRB ring stays full and no space is freed.

Meanwhile, if there are pending skbs, t7xx_do_tx_hw_push() repeatedly calls
t7xx_txq_burst_send_skb(), which returns 0 when the ring is full:

t7xx_do_tx_hw_push() {
    ...
    drb_send_cnt = t7xx_txq_burst_send_skb(txq);
    if (drb_send_cnt <= 0) {
        usleep_range(10, 20);
        cond_resched();
        continue;
    }
    ...
}

Because this do-while loop inside t7xx_do_tx_hw_push() never exits when the
DRB ring is full, the kthread never loops back to reach this try_to_freeze()
call. Will this cause the freezer to time out and abort the system suspend?

>  		ret = pm_runtime_resume_and_get(dpmaif_ctrl->dev);
>  		if (ret < 0 && ret != -EACCES)
>  			return ret;

[Severity: High]
This is a pre-existing issue, but does this lead to a runtime PM usage
counter underflow when pm_runtime_resume_and_get() returns -EACCES?

pm_runtime_resume_and_get() internally decrements the usage counter back to
its original value if it returns an error. By ignoring -EACCES here, the
code proceeds and unconditionally executes pm_runtime_put_autosuspend() at
the end of the loop, dropping the usage count a second time.

A similar issue exists in t7xx_dpmaif_tx_done() where the TX done work item
also ignores -EACCES and continues. Could this lead to a negative reference
count when runtime PM is disabled?

[Severity: High]
This is a pre-existing issue, but does returning early here cause a
use-after-free later?

If pm_runtime_resume_and_get() returns an error other than -EACCES, the
function returns early. This terminates the kthread and invokes do_exit(),
which frees the task_struct since kthread_run() does not hold an extra
reference.

Later, when the device is unbound, t7xx_dpmaif_tx_thread_rel() calls
kthread_stop(dpmaif_ctrl->tx_thread) on the freed task pointer:

t7xx_dpmaif_tx_thread_rel() {
    ...
    if (dpmaif_ctrl->tx_thread)
        kthread_stop(dpmaif_ctrl->tx_thread);
    ...
}

Will this cause a panic?

[ ... ]

> @@ -617,7 +627,7 @@ int t7xx_dpmaif_txq_init(struct dpmaif_tx_queue *txq)
>  	}
>  
>  	txq->worker = alloc_ordered_workqueue("md_dpmaif_tx%d_worker",
> -				WQ_MEM_RECLAIM | (txq->index ? 0 : WQ_HIGHPRI),
> +				WQ_MEM_RECLAIM | WQ_FREEZABLE | (txq->index ? 0 : WQ_HIGHPRI),
>  				txq->index);
>  	if (!txq->worker)
>  		return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but are there other hardware-accessing workqueues
that also need to be marked as freezable to prevent this system suspend race?

For example, the bat_release_wq is allocated without WQ_FREEZABLE:

t7xx_dpmaif_bat_rel_wq_alloc() {
    ...
    dpmaif_ctrl->bat_release_wq = alloc_workqueue("dpmaif_bat_release_work_queue",
                                                  WQ_MEM_RECLAIM | WQ_PERCPU, 1);
    ...
}

And its worker ignores -EACCES from runtime PM and accesses the hardware
directly:

t7xx_dpmaif_bat_release_work() {
    ...
    if (ret < 0 && ret != -EACCES)
        return;

    t7xx_pci_disable_sleep(dpmaif_ctrl->t7xx_dev);
    ...
}

Similarly, the CLDMA TX and RX workqueues are allocated without WQ_FREEZABLE
in t7xx_cldma_init(). Could these unfrozen workers execute pending work
against a suspended or torn-down device and cause the same soft lockups?