* Re: [PATCH net v5] net: wwan: t7xx: fix race between TX path and system PM suspend
2026-09-07 8:29 [PATCH net v5] net: wwan: t7xx: fix race between TX path and system PM suspend Tim JH Chen
@ 2026-09-10 11:30 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-10 11:30 UTC (permalink / raw)
To: tim770802
Cc: netdev, pabeni, simon.horman, haijun.liu,
chandrashekar.devegowda, ricardo.martinez, loic.poulain,
ryazanov.s.a, johannes, andrew+netdev, davem, edumazet, kuba,
linux-kernel, tim.jh.chen, Chih.Hung.Huang
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
^ permalink raw reply [flat|nested] 2+ messages in thread