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

[netdev-bot+sashiko@kernel.org]

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