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

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

[Tim JH Chen]

Several driver contexts call pm_runtime_resume_and_get() and then access
hardware registers without being quiesced during system suspend. System
suspend does not honour the runtime PM reference they hold, so they can
touch the hardware while the device suspend callbacks tear 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 any of these contexts holds its PM
reference the runtime suspend callback cannot run. Only system suspend,
which ignores that reference, is exposed.

Quiesce all hardware-accessing contexts with the PM freezer, which runs
before dpm_suspend() invokes the device suspend callbacks.

The TX push kthread (t7xx_dpmaif_tx_hw_push_thread):
  - Call set_freezable() at thread start.
  - Replace wait_event_interruptible() with wait_event_freezable() so the
    idle wait is also a freeze point.
  - Add try_to_freeze() before the pm_runtime_resume_and_get() / MMIO
    section so continuous TX traffic still reaches a freeze point.
  - Add a freezing(current) check inside the DRB-ring-full retry loop in
    t7xx_do_tx_hw_push(). When the TX-done workqueue is frozen first, it
    stops draining completed DRBs; the ring stays full and the kthread
    loops in the retry branch indefinitely, never reaching the
    try_to_freeze() above. Returning from t7xx_do_tx_hw_push() on
    freezing(current) lets the caller release the PM sleep lock and the
    runtime PM reference before the kthread is parked at try_to_freeze().

The TX-done (md_dpmaif_tx*_worker), BAT-release
(dpmaif_bat_release_work_queue), CLDMA TX (md_hif*_tx*_worker), and
CLDMA RX (md_hif*_rx*_worker) workqueues:
  - Mark all four WQ_FREEZABLE so the workqueue freezer drains and parks
    their pending work before the device suspend callbacks run.

Tasks and work items are thawed only after the resume callbacks have
re-armed the hardware, so none of these contexts 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>
---
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. Add a freezing(current) check in that branch so
    the kthread returns to its caller, which releases the PM sleep lock
    and runtime PM reference before try_to_freeze() parks the thread.
    (Simon Horman)
  - Extend WQ_FREEZABLE to the BAT-release workqueue
    (dpmaif_bat_release_work_queue) and the CLDMA TX/RX workqueues
    (md_hif*_tx*_worker, md_hif*_rx*_worker), which also access hardware
    registers and must not run after dpm_suspend() tears the device down.
    (Simon Horman)
  - The -EACCES usage-count underflow in pm_runtime_resume_and_get()
    callers and the stale kthread pointer on early thread exit are
    pre-existing issues; they will be addressed in a separate series.
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_cldma.c     |  4 ++--
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c |  2 +-
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 26 +++++++++++++++++-----
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
index e10cb4f9104e..3d7712126761 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
@@ -1313,7 +1313,7 @@ int t7xx_cldma_init(struct cldma_ctrl *md_ctrl)
 		md_cd_queue_struct_init(&md_ctrl->txq[i], md_ctrl, MTK_TX, i);
 		md_ctrl->txq[i].worker =
 			alloc_ordered_workqueue("md_hif%d_tx%d_worker",
-					WQ_MEM_RECLAIM | (i ? 0 : WQ_HIGHPRI),
+					WQ_MEM_RECLAIM | WQ_FREEZABLE | (i ? 0 : WQ_HIGHPRI),
 					md_ctrl->hif_id, i);
 		if (!md_ctrl->txq[i].worker)
 			goto err_workqueue;
@@ -1327,7 +1327,7 @@ int t7xx_cldma_init(struct cldma_ctrl *md_ctrl)
 
 		md_ctrl->rxq[i].worker =
 			alloc_ordered_workqueue("md_hif%d_rx%d_worker",
-						WQ_MEM_RECLAIM,
+						WQ_MEM_RECLAIM | WQ_FREEZABLE,
 						md_ctrl->hif_id, i);
 		if (!md_ctrl->rxq[i].worker)
 			goto err_workqueue;
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
index 5af90ca6e063..0fe2dd1363a4 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
@@ -1088,7 +1088,7 @@ static void t7xx_dpmaif_bat_release_work(struct work_struct *work)
 int t7xx_dpmaif_bat_rel_wq_alloc(struct dpmaif_ctrl *dpmaif_ctrl)
 {
 	dpmaif_ctrl->bat_release_wq = alloc_workqueue("dpmaif_bat_release_work_queue",
-						      WQ_MEM_RECLAIM | WQ_PERCPU,
+						      WQ_MEM_RECLAIM | WQ_PERCPU | WQ_FREEZABLE,
 						      1);
 	if (!dpmaif_ctrl->bat_release_wq)
 		return -ENOMEM;
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
index 236d632cf591..cce71c827e7b 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>
@@ -421,6 +422,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) {
+			/* If a freeze is pending the TX-done worker may already be
+			 * frozen and unable to drain the DRB ring; return to the
+			 * caller so PM resources are released before try_to_freeze().
+			 */
+			if (freezing(current))
+				return;
 			usleep_range(10, 20);
 			cond_resched();
 			continue;
@@ -447,19 +454,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 +633,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 v5] net: wwan: t7xx: fix race between TX path and system PM suspend

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 3 (1) · Medium: 0 (1) · Low: 0

- [High] Incomplete fix: the commit message claims to "Quiesce all
  hardware-accessing contexts with the PM freezer" and that "none of…
- [High] Marking the CLDMA TX/RX workers, the DPMAIF TX-done worker and
  the BAT-release workqueue WQ_FREEZABLE creates blocking dependencies…
- [High] The TX push kthread is parked with plain try_to_freeze(), which
  calls __refrigerator(false) (include/linux/freezer.h:56-64).

Pre-existing issues:
- [High] t7xx_dpmaif_tx_hw_push_thread() can end its own lifetime
  ('return ret' when pm_runtime_resume_and_get() fails with anything…
- [Medium] In t7xx_dpmaif_tx_hw_push_thread() the -EACCES return of
  pm_runtime_resume_and_get() is treated as success, but…

--

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

This makes the DPMAIF TX push kthread freezable (set_freezable(),
wait_event_freezable(), try_to_freeze(), plus a freezing(current) bail-out
in the DRB-ring-full retry loop) and marks the CLDMA TX/RX workers, the
DPMAIF TX-done worker and the BAT-release workqueue WQ_FREEZABLE, so the PM
freezer parks them before the device suspend callbacks run.

A few questions about the freezer-based approach below.

[Severity: High]

The commit message says:

  "Quiesce all hardware-accessing contexts with the PM freezer, which runs
   before dpm_suspend() invokes the device suspend callbacks."

and

  "Tasks and work items are thawed only after the resume callbacks have
   re-armed the hardware, so none of these contexts can issue MMIO against
   a torn-down or not-yet-rearmed device."

Are all such contexts really covered?  t7xx_cldma_send_skb() has the same
pattern this patch is trying to quiesce:

drivers/net/wwan/t7xx/t7xx_hif_cldma.c:t7xx_cldma_send_skb() {
	ret = pm_runtime_resume_and_get(md_ctrl->dev);
	if (ret < 0 && ret != -EACCES)
		return ret;

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

and it is reached from contexts the freezer never touches:

  fsm_main_thread()      (kthread_run in t7xx_state_monitor.c, no
                          set_freezable(), plain wait_event_interruptible)
  port_ctl_rx_thread()   (kthread_run in t7xx_port_ctrl_msg.c, same)
  md_hk_wq              (WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_HIGHPRI, not
                          freezable)

After this patch the only set_freezable() in the driver is the one added to
t7xx_dpmaif_tx_hw_push_thread(), so those two kthreads inherit PF_NOFREEZE
from kthreadd and keep running through the whole freeze window.  A modem
MHCCIF interrupt during that window appends an FSM command
(t7xx_fsm_recv_md_intr() -> t7xx_fsm_append_cmd()) and wakes the FSM
kthread, which then drives CLDMA start/stop and control-message MMIO.

The DPMAIF receive side looks similarly uncovered:

  t7xx_dpmaif_irq_rx_done()   takes the runtime PM reference from the
                              non-freezable threaded IRQ handler and
                              napi_schedule()s
  t7xx_dpmaif_napi_rx_poll()  writes registers via
                              t7xx_dpmaif_clr_ip_busy_sts() and
                              t7xx_dpmaif_dlq_unmask_rx_done() from softirq

The freezer cannot park a softirq, and t7xx_dpmaif_suspend() only calls
t7xx_dpmaif_rx_stop() after it has already run
t7xx_dpmaif_hw_stop_all_txq()/t7xx_dpmaif_hw_stop_all_rxq() and
t7xx_dpmaif_disable_irq(), with no napi_disable()/napi_synchronize()
anywhere in the PM callbacks.

Would explicit quiescing in the PM callbacks (napi_disable() /
napi_synchronize(), plus gating or making the control kthreads freezable) be
needed for those paths, rather than more freezer participation?

> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
> index 2917cee9b8028..b70ec84d50a99 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
> @@ -1316,7 +1316,7 @@ int t7xx_cldma_init(struct cldma_ctrl *md_ctrl)
>  		md_cd_queue_struct_init(&md_ctrl->txq[i], md_ctrl, MTK_TX, i);
>  		md_ctrl->txq[i].worker =
>  			alloc_ordered_workqueue("md_hif%d_tx%d_worker",
> -					WQ_MEM_RECLAIM | (i ? 0 : WQ_HIGHPRI),
> +					WQ_MEM_RECLAIM | WQ_FREEZABLE | (i ? 0 : WQ_HIGHPRI),
>  					md_ctrl->hif_id, i);
>  		if (!md_ctrl->txq[i].worker)
>  			goto err_workqueue;

[Severity: High]

Can marking these four workqueues WQ_FREEZABLE block contexts that are not
themselves frozen?

freeze_workqueues_begin() sets workqueue_freezing and wq_adjust_max_active()
forces max_active to 0 for every WQ_FREEZABLE workqueue, so items queued
during the freeze window sit on pwq->inactive_works until thaw_workqueues()
at the very end of resume.  A flush_work() on such an item links its barrier
behind the inactive item, so the flush blocks until thaw.  The WQ_MEM_RECLAIM
rescuer only walks pool->worklist, so it cannot drain inactive items either.

There are synchronous waiters on exactly these items that the freezer does
not freeze:

drivers/net/wwan/t7xx/t7xx_hif_cldma.c:t7xx_cldma_stop() {
	if (md_ctrl->is_late_init) {
		for (i = 0; i < CLDMA_TXQ_NUM; i++)
			flush_work(&md_ctrl->txq[i].cldma_work);

		for (i = 0; i < CLDMA_RXQ_NUM; i++)
			flush_work(&md_ctrl->rxq[i].cldma_work);
	}
}

t7xx_cldma_stop() is reached from the non-freezable FSM kthread
(fsm_routine_start() -> t7xx_lk_stage_event_handling(), fsm_routine_stopping()),
from t7xx_md_exception() and from t7xx_cldma_exit() on the remove path.

There is also a budget wait with no timeout:

drivers/net/wwan/t7xx/t7xx_hif_cldma.c:t7xx_cldma_send_skb() {
	ret = wait_event_interruptible_exclusive(queue->req_wq, queue->budget > 0);
}

The only place that replenishes queue->budget is t7xx_cldma_gpd_tx_collect()
(the sole "queue->budget++"), which runs from t7xx_cldma_tx_done() on the
md_hif%d_tx%d_worker this hunk freezes.  That wait is held with a runtime PM
reference and t7xx_pci_disable_sleep() in effect.

The same shape exists for the two DPMAIF workqueues touched below:
t7xx_dpmaif_bat_wq_rel() flushes bat_release_work with no preceding cancel,
and t7xx_dpmaif_txq_flush_rel() does cancel_work_sync() followed by
flush_work() on an item that t7xx_dpmaif_irq_tx_done() can re-queue after
the cancel.

If the blocked waiter is t7xx_cldma_exit() -> t7xx_cldma_stop() on an
unbind/hot-unplug, it holds device_lock(), and dpm_prepare() (which runs
after the freezer) then blocks in wait_for_device_probe()/device_prepare(),
so the suspend can never reach thaw_processes().  Can this hang the suspend
outright?

Separately, t7xx_pci_pm_prepare() waits on init_done with
T7XX_INIT_TIMEOUT; if the handshake path is stuck on a frozen workqueue,
does that turn into an aborted suspend after the timeout?

> @@ -1330,7 +1330,7 @@ int t7xx_cldma_init(struct cldma_ctrl *md_ctrl)
>  
>  		md_ctrl->rxq[i].worker =
>  			alloc_ordered_workqueue("md_hif%d_rx%d_worker",
> -						WQ_MEM_RECLAIM,
> +						WQ_MEM_RECLAIM | WQ_FREEZABLE,
>  						md_ctrl->hif_id, i);
>  		if (!md_ctrl->rxq[i].worker)
>  			goto err_workqueue;
> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> index 5af90ca6e0631..0fe2dd1363a4b 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> @@ -1088,7 +1088,7 @@ static void t7xx_dpmaif_bat_release_work(struct work_struct *work)
>  int t7xx_dpmaif_bat_rel_wq_alloc(struct dpmaif_ctrl *dpmaif_ctrl)
>  {
>  	dpmaif_ctrl->bat_release_wq = alloc_workqueue("dpmaif_bat_release_work_queue",
> -						      WQ_MEM_RECLAIM | WQ_PERCPU,
> +						      WQ_MEM_RECLAIM | WQ_PERCPU | WQ_FREEZABLE,
>  						      1);
>  	if (!dpmaif_ctrl->bat_release_wq)
>  		return -ENOMEM;
> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
> index 236d632cf591b..cce71c827e7b2 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c

[ ... ]

> @@ -421,6 +422,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) {
> +			/* If a freeze is pending the TX-done worker may already be
> +			 * frozen and unable to drain the DRB ring; return to the
> +			 * caller so PM resources are released before try_to_freeze().
> +			 */
> +			if (freezing(current))
> +				return;
>  			usleep_range(10, 20);
>  			cond_resched();
>  			continue;
> @@ -447,19 +454,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]

Should this be kthread_freezable_should_stop() instead of a bare
try_to_freeze()?

try_to_freeze() calls __refrigerator(false), and inside __refrigerator() the
loop condition is:

kernel/freezer.c:__refrigerator() {
	...
	for (;;) {
		bool freeze;
		...
		freeze = freezing(current) && !(check_kthr_stop && kthread_should_stop());
	...
}

With check_kthr_stop false a pending stop is ignored, and the task sits in
TASK_FROZEN, which wake_up_process()'s TASK_NORMAL mask does not match.  So
a concurrent

drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c:t7xx_dpmaif_tx_thread_rel() {
	if (dpmaif_ctrl->tx_thread)
		kthread_stop(dpmaif_ctrl->tx_thread);
}

blocks in wait_for_completion(&kthread->exited) until thaw_processes().

kthread_freezable_should_stop() documents this case:

kernel/kthread.c:
 * kthread_should_stop() for freezable kthreads, which will enter
 * refrigerator if necessary.  This function is safe from kthread_stop() /
 * freezer deadlock and freezable kthreads should use this function instead
 * of calling try_to_freeze() directly.

The only callers of t7xx_dpmaif_tx_thread_rel() are the probe error path and
remove (t7xx_ccmni_exit() -> t7xx_dpmaif_hif_exit()), both of which hold
device_lock()/the probe count.  If dpm_prepare() then blocks on
wait_for_device_probe()/device_lock() for the same device, does the suspend
ever reach thaw_processes()?

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

[Severity: Medium]

This isn't a bug introduced by this patch, and the changelog already notes
it is deferred, but for the record: pm_runtime_resume_and_get() already
dropped the usage count before returning -EACCES:

include/linux/pm_runtime.h:pm_runtime_get_active() {
	ret = __pm_runtime_resume(dev, RPM_GET_PUT | rpmflags);
	if (ret < 0) {
		pm_runtime_put_noidle(dev);
		return ret;
	}
}

The loop body still ends with an unconditional
pm_runtime_put_autosuspend(dpmaif_ctrl->dev), so does each -EACCES
iteration drop a reference the thread does not hold and drive usage_count
negative?

[Severity: High]

This is also pre-existing rather than something this patch introduces, but
the "return ret" here ends the thread's own lifetime while
dpmaif_ctrl->tx_thread still points at that task.  The thread is created
with kthread_run() and no extra task reference is kept, so kthreadd can reap
the task_struct.  A later t7xx_dpmaif_tx_thread_rel() then does:

drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c:t7xx_dpmaif_tx_thread_rel() {
	if (dpmaif_ctrl->tx_thread)
		kthread_stop(dpmaif_ctrl->tx_thread);
}

and kthread_stop() immediately does get_task_struct(k) and to_kthread(k) on
that pointer.  kthread_stop()'s kerneldoc covers this:

kernel/kthread.c:
 * If threadfn() may call kthread_exit() itself, the caller must ensure
 * task_struct can't go away.

Can this be a use-after-free of the task_struct when a hard error (for
example -EINVAL once dev->power.runtime_error is set after a failed resume)
makes the thread self-exit and the device is later unbound?  Before that,
TX push is permanently dead with nothing reported.

Would clearing dpmaif_ctrl->tx_thread on exit, holding a task reference, or
not self-exiting at all fit better here, given that this thread's PM
handling is being reworked anyway?

>  
>  		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);
>  	}
> @@ -617,7 +633,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;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907082938.7500-1-tim770802%40gmail.com