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

[Paolo Abeni]

On 6/1/26 3:52 AM, Tim JH Chen wrote:
> When system suspend is triggered while the DPMAIF TX kthread
> (t7xx_dpmaif_tx_hw_push_thread) is running, a deadlock can occur
> leading to a CPU soft lockup.
> 
> The root cause is two-fold:
> 
> 1. t7xx_dpmaif_suspend() calls t7xx_dpmaif_tx_stop() which only stops
>    the TX work-queue items (by clearing txq->que_started and waiting on
>    txq->tx_processing). It does NOT signal the kthread and does NOT
>    update dpmaif_ctrl->state, which stays DPMAIF_STATE_PWRON.
> 
> 2. The kthread's state guard is only checked at the top of each loop
>    iteration. If the thread already passed this guard, it proceeds
>    unconditionally to call pm_runtime_resume_and_get() — which tries to
>    acquire dev->power.lock also contended by the system PM suspend path.
> 
> The result is a spinlock deadlock observed as:
> 
>   watchdog: BUG: soft lockup - CPU#N stuck for 26s! [dpmaif_tx_hw_pu]
>   RIP: _raw_spin_unlock_irqrestore
>   Call Trace:
>     __pm_runtime_resume+0x5b/0x80
>     t7xx_dpmaif_tx_hw_push_thread+0xc4 [mtk_t7xx]
> 
> The condition requires ASPM L1 enabled on the endpoint (which extends
> the time pm_runtime_resume_and_get() holds dev->power.lock during L1.2
> link retraining) and hundreds of repeated suspend/resume cycles to
> trigger reliably.
> 
> Fix by introducing tx_pm_lock (struct mutex) and several coordinated
> changes:
> 
> t7xx_dpmaif_suspend():
>   After t7xx_dpmaif_tx_stop(), acquire tx_pm_lock. Under the lock,
>   snapshot dpmaif_ctrl->state into pre_suspend_state (capturing the
>   modem state atomically with respect to the kthread's PM section),
>   then set DPMAIF_STATE_PWROFF via WRITE_ONCE(). Release the lock
>   and call wake_up() so any sleeping kthread re-evaluates the
>   wait_event condition and exits.
> 
>   t7xx_dpmaif_suspend() acquires tx_pm_lock without holding any PM
>   lock. While it waits, the kthread may call pm_runtime_resume_and_get()
>   which briefly takes and releases dev->power.lock independently.
>   Because the suspend callback does not compete for dev->power.lock at
>   this point, the original spinlock deadlock cannot occur. Suspend
>   latency increases by at most one TX burst drain time, which is
>   bounded by the DRB ring depth.
> 
> t7xx_dpmaif_resume():
>   When pre_suspend_state is DPMAIF_STATE_PWRON, re-arm the HW fully
>   (start_txrx_qs, enable_irq, unmask_dlq_intr, start_hw) before
>   publishing the new state. This ensures the kthread cannot issue
>   ul_update_hw_drb_cnt() MMIO writes before UL_ALL_Q_EN is set by
>   t7xx_dpmaif_start_hw(). Publish the restored state under tx_pm_lock
>   to serialise with the kthread's under-lock state check. Wake up the
>   kthread only after HW and state are both consistent.
> 
>   When pre_suspend_state is DPMAIF_STATE_PWROFF (modem was already
>   stopped or in exception before suspend), skip HW re-arming entirely
>   to avoid leaving DMA engines running while the MD state machine
>   considers the modem inactive.
> 
> t7xx_dpmaif_tx_hw_push_thread():
>   Hold tx_pm_lock across the [state check -> pm_runtime_resume_and_get
>   -> pm_runtime_put_autosuspend] sequence. A second READ_ONCE() state
>   check under the lock closes the TOCTOU window between the wait_event
>   guard at the loop top and the pm_runtime call. READ_ONCE() is used
>   in all unguarded state reads in this function.
> 
> t7xx_dpmaif_start() / t7xx_dpmaif_stop():
>   Use WRITE_ONCE() for state writes to match the READ_ONCE() reads
>   used throughout the driver and prevent compiler optimisations from
>   obscuring concurrent access.
> 
> t7xx_do_tx_hw_push():
>   Use READ_ONCE() in the do/while termination condition to match the
>   WRITE_ONCE() annotations on the write side.
> 
> t7xx_dpmaif_tx_thread_init():
>   Initialise tx_pm_lock with mutex_init().
> 
> Note: t7xx_dpmaif_start() and t7xx_dpmaif_stop() (called from the
> MD-FSM kthread via t7xx_dpmaif_md_state_callback()) do not hold
> tx_pm_lock. A race where the FSM transitions the modem to
> DPMAIF_STATE_PWROFF concurrently with the TX kthread's last burst is
> pre-existing and not introduced by this patch; the do/while condition
> in t7xx_do_tx_hw_push() now re-checks state with READ_ONCE() at each
> iteration boundary, limiting exposure to at most one burst.
> 
> Tested: no soft lockup observed over 500+ suspend/resume cycles with
> SIM registered and ASPM L1 enabled (previously triggered in < 300).
> 
> Fixes: 46e8f49ed7b3 ("net: wwan: t7xx: Introduce power management")
> Signed-off-by: Tim JH Chen <tim.jh.chen@xxxxxxxxxx>

The above is way too verbose and hints that this patch should likely 
be split in a series.

Note that process wise there are still several problems:
- missing revision number in the suby prefix
- mismatch between from email message and SoB
- new revision MUST NOT be in reply-to of older ones.

Please try to be accurate with your next resubmission, or we will
have to delay processing this patch for an additional while.

Sashiko has still quite a bit of concerns:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260601015231.3211764-1-tim.jh.chen%40wnc.com.tw

/P