[PATCH net v6 0/4] net: wwan: t7xx: fix DPMAIF data path vs system PM suspend

[PATCH net v6 0/4] net: wwan: t7xx: fix DPMAIF data path vs system PM suspend

[Tim JH Chen]

This series fixes a race between the t7xx DPMAIF data path and system PM
suspend, plus three pre-existing bugs uncovered while fixing it.

With ASPM L1 enabled and repeated suspend/resume cycles, several DPMAIF
data-plane contexts take a runtime PM reference and then access device
registers. System suspend ignores that reference, so these contexts can
touch the hardware while the suspend callback is tearing it down. The
observable symptom is a CPU soft lockup in the TX push kthread:

  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]

Patch 3 is the actual fix: it quiesces the DPMAIF data-plane contexts
across system suspend (a freezable TX push kthread plus an explicit drain
of the TX-done workers and the in-flight NAPI RX poll in the suspend
callback), rather than marking the data-plane workqueues freezable, which
the v5 review showed can deadlock suspend.

Patches 1, 2 and 4 are pre-existing bugs found while developing the fix,
each with its own Fixes: tag and independent of the main race:

  1 - runtime PM usage-count underflow on the -EACCES path
  2 - use-after-free from the TX push kthread exiting on resume failure
  4 - a NAPI that is never completed on the "RX queue not started" early
      return; a later napi_synchronize() under rtnl_lock then hangs the
      network stack. Patch 4 is argued from the NAPI contract (a poll
      returning < budget must call napi_complete_done()); it is not tied
      to a specific reproducer.

Only the DPMAIF data path is touched; the CLDMA control path uses a
different mechanism and is out of scope (t7xx_hif_cldma.c is unchanged).

The data-path fix (patch 3) was tested with 500+ suspend/resume cycles
with a SIM registered and ASPM L1 enabled.

v5 -> v6:
  - Drop the "freezer as a global quiesce" direction: remove every
    WQ_FREEZABLE annotation added in v4/v5. Marking the data-plane
    workqueues freezable can deadlock the suspend, because a
    flush_work()/cancel_work_sync() issued from a context the freezer
    does not freeze (the FSM kthread, or an unbind holding device_lock)
    blocks until thaw_workqueues(). (Reported on v5 review.)
  - Instead quiesce the DPMAIF data plane explicitly in the system
    suspend callback: mask interrupts, cancel_work_sync() the TX-done
    workers, call t7xx_dpmaif_rx_stop() to drain the in-flight NAPI RX
    poll (the freezer cannot park a softirq), then cancel
    bat_release_work and stop the hardware last.
  - Keep the PM freezer only for the lone TX push kthread, and use
    kthread_freezable_should_stop() instead of try_to_freeze(), so a
    concurrent kthread_stop() is honoured while the thread is frozen.
  - Fold in the pre-existing fixes previously deferred to a separate
    series: the -EACCES runtime PM usage-count underflow (patch 1), the
    TX push kthread self-exit use-after-free (patch 2), and a NAPI that
    is never completed on the not-started RX poll early return, which
    hangs a later napi_synchronize() under rtnl_lock (patch 4). This
    revision is therefore a 4-patch series.
  - Do not touch t7xx_hif_cldma.c. Restrict the commit messages to the
    DPMAIF data path; the CLDMA control path uses a different mechanism
    and is called out as out of scope.
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. (Simon Horman)
  - Extend WQ_FREEZABLE to the BAT-release and CLDMA TX/RX workqueues.
    (Simon Horman) [reverted in v6, see above]
  - Note the -EACCES underflow and the stale kthread pointer as
    pre-existing issues to be addressed separately. [done in v6, patches 1-2]
v3 -> v4:
  - Drop the tx_pm_lock / state-snapshot approach entirely and use the PM
    freezer instead. The previous approach deadlocked through the runtime
    PM wait queue and opened ISR windows by writing dpmaif_ctrl->state in
    suspend/resume.
v2 -> v3: process fixes (Fixes tag, changelog placement).
v1 -> v2: save/restore pre-suspend state; wrap pm_runtime with a mutex.

Tim JH Chen (4):
  net: wwan: t7xx: fix runtime PM usage count underflow on -EACCES
  net: wwan: t7xx: do not exit the TX push kthread on resume failure
  net: wwan: t7xx: fix race between TX/RX data path and system PM
    suspend
  net: wwan: t7xx: complete NAPI on the not-started RX poll early return

 drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c    | 24 +++++++++-
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c |  9 +++-
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 55 ++++++++++++++++++----
 3 files changed, 77 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH net v6 1/4] net: wwan: t7xx: fix runtime PM usage count underflow on -EACCES

[Tim JH Chen]

t7xx_dpmaif_tx_hw_push_thread(), t7xx_dpmaif_tx_done() and
t7xx_dpmaif_bat_release_work() treat -EACCES from
pm_runtime_resume_and_get() as success and proceed to access the
hardware. That is intentional, but pm_runtime_resume_and_get() has
already dropped the usage count it took before returning -EACCES, so the
unconditional pm_runtime_put_autosuspend() at the end of each context
drops a reference that was never held and drives the usage count
negative.

Only balance the reference when it was actually taken.

Fixes: 46e8f49ed7b3 ("net: wwan: t7xx: Introduce power management")
Signed-off-by: Tim JH Chen <tim770802@gmail.com>
---
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c |  3 ++-
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 10 ++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
index 5af90ca6e063..0e1174ee611d 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
@@ -1082,7 +1082,8 @@ static void t7xx_dpmaif_bat_release_work(struct work_struct *work)
 	}
 
 	t7xx_pci_enable_sleep(dpmaif_ctrl->t7xx_dev);
-	pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
+	if (ret != -EACCES)
+		pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
 }
 
 int t7xx_dpmaif_bat_rel_wq_alloc(struct dpmaif_ctrl *dpmaif_ctrl)
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
index 236d632cf591..bd6116a8c541 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
@@ -160,12 +160,16 @@ static void t7xx_dpmaif_tx_done(struct work_struct *work)
 	struct dpmaif_tx_queue *txq = container_of(work, struct dpmaif_tx_queue, dpmaif_tx_work);
 	struct dpmaif_ctrl *dpmaif_ctrl = txq->dpmaif_ctrl;
 	struct dpmaif_hw_info *hw_info;
+	bool pm_ref;
 	int ret;
 
 	ret = pm_runtime_resume_and_get(dpmaif_ctrl->dev);
 	if (ret < 0 && ret != -EACCES)
 		return;
 
+	/* -EACCES means no reference was taken; only balance a real one. */
+	pm_ref = !ret;
+
 	/* The device may be in low power state. Disable sleep if needed */
 	t7xx_pci_disable_sleep(dpmaif_ctrl->t7xx_dev);
 	if (t7xx_pci_sleep_disable_complete(dpmaif_ctrl->t7xx_dev)) {
@@ -185,7 +189,8 @@ static void t7xx_dpmaif_tx_done(struct work_struct *work)
 	}
 
 	t7xx_pci_enable_sleep(dpmaif_ctrl->t7xx_dev);
-	pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
+	if (pm_ref)
+		pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
 }
 
 static void t7xx_setup_msg_drb(struct dpmaif_ctrl *dpmaif_ctrl, unsigned int q_num,
@@ -467,7 +472,8 @@ static int t7xx_dpmaif_tx_hw_push_thread(void *arg)
 		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);
+		if (ret != -EACCES)
+			pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
 	}
 
 	return 0;
-- 
2.43.0

[PATCH net v6 2/4] net: wwan: t7xx: do not exit the TX push kthread on resume failure

[Tim JH Chen]

t7xx_dpmaif_tx_hw_push_thread() returns, ending the kthread, when
pm_runtime_resume_and_get() fails with anything other than -EACCES.
kthread_run() keeps no extra reference to the task, so the task_struct
can be reaped while dpmaif_ctrl->tx_thread still points at it. A later
t7xx_dpmaif_tx_thread_rel() then calls kthread_stop() on the stale
pointer, which does get_task_struct() on freed memory: a use-after-free.
It also stops TX permanently and silently.

Log the failure and retry after a short back-off instead of exiting, so
the thread stays alive until kthread_stop() tears it down.

Fixes: 46e8f49ed7b3 ("net: wwan: t7xx: Introduce power management")
Signed-off-by: Tim JH Chen <tim770802@gmail.com>
---
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
index bd6116a8c541..2a405bc74312 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
@@ -447,6 +447,11 @@ static void t7xx_do_tx_hw_push(struct dpmaif_ctrl *dpmaif_ctrl)
 		 (dpmaif_ctrl->state == DPMAIF_STATE_PWRON));
 }
 
+/* Back-off before retrying a failed runtime PM resume in the TX push
+ * kthread, so a persistent error does not busy-loop.
+ */
+#define DPMAIF_TX_RESUME_RETRY_MS	20
+
 static int t7xx_dpmaif_tx_hw_push_thread(void *arg)
 {
 	struct dpmaif_ctrl *dpmaif_ctrl = arg;
@@ -466,8 +471,16 @@ static int t7xx_dpmaif_tx_hw_push_thread(void *arg)
 		}
 
 		ret = pm_runtime_resume_and_get(dpmaif_ctrl->dev);
-		if (ret < 0 && ret != -EACCES)
-			return ret;
+		if (ret < 0 && ret != -EACCES) {
+			/* Do not exit the thread: dpmaif_ctrl->tx_thread still
+			 * points at this task and t7xx_dpmaif_tx_thread_rel()
+			 * will call kthread_stop() on it. Back off and retry.
+			 */
+			dev_err_ratelimited(dpmaif_ctrl->dev,
+					    "Failed to resume for TX push: %d\n", ret);
+			msleep_interruptible(DPMAIF_TX_RESUME_RETRY_MS);
+			continue;
+		}
 
 		t7xx_pci_disable_sleep(dpmaif_ctrl->t7xx_dev);
 		t7xx_do_tx_hw_push(dpmaif_ctrl);
-- 
2.43.0

[PATCH net v6 3/4] net: wwan: t7xx: fix race between TX/RX data path and system PM suspend

[Tim JH Chen]

Several DPMAIF data-plane contexts call pm_runtime_resume_and_get() and
then access hardware registers. System suspend ignores the runtime PM
reference they hold, so with ASPM L1 enabled and repeated suspend/resume
cycles they can touch the device while the suspend callback tears it
down, ending 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 the DPMAIF data-plane contexts across system suspend:

 - Make the TX push kthread freezable (set_freezable(),
   wait_event_freezable(), kthread_freezable_should_stop()) so the PM
   freezer parks it before dpm_suspend() runs the device suspend
   callbacks. kthread_freezable_should_stop() also lets a concurrent
   kthread_stop() proceed while the thread is frozen, and a
   freezing(current) bail-out in the DRB-ring-full retry loop keeps the
   thread from looping there under sustained TX.

 - The suspend callback masks interrupts and drains the TX-done workers
   (cancel_work_sync(); their producer irq_tx_done is masked and
   cancel_work_sync() also blocks a self-requeue). It then calls
   t7xx_dpmaif_rx_stop(), which clears que_started and waits for the
   in-flight NAPI RX poll to finish, so that poll -- which writes
   registers via t7xx_dpmaif_clr_ip_busy_sts() /
   t7xx_dpmaif_dlq_unmask_rx_done() -- cannot run after the hardware is
   torn down. bat_release_work is cancelled only after rx_stop(), since
   its sole producer is that NAPI poll.

The data-plane workqueues are intentionally left non-freezable: marking
them WQ_FREEZABLE would let a flush_work()/cancel_work_sync() from a
context the freezer does not freeze (the FSM kthread, or an unbind
holding device_lock) block until thaw_workqueues(), which can hang the
suspend. Draining them from the suspend callback avoids that.

This covers the DPMAIF data path that produces the observed soft lockup;
the CLDMA control path uses a different mechanism and is out of scope.

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>
---
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c    | 24 +++++++++++++++--
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 30 ++++++++++++++++++----
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c
index 7ff33c1d6ac7..b5a857e940b7 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c
@@ -410,12 +410,32 @@ static int t7xx_dpmaif_stop(struct dpmaif_ctrl *dpmaif_ctrl)
 static int t7xx_dpmaif_suspend(struct t7xx_pci_dev *t7xx_dev, void *param)
 {
 	struct dpmaif_ctrl *dpmaif_ctrl = param;
+	unsigned int i;
 
+	/* Stop new TX and mask interrupts first, so nothing re-arms the
+	 * contexts drained below.
+	 */
 	t7xx_dpmaif_tx_stop(dpmaif_ctrl);
-	t7xx_dpmaif_hw_stop_all_txq(&dpmaif_ctrl->hw_info);
-	t7xx_dpmaif_hw_stop_all_rxq(&dpmaif_ctrl->hw_info);
 	t7xx_dpmaif_disable_irq(dpmaif_ctrl);
+
+	/* irq_tx_done is masked now and cancel_work_sync() also blocks a
+	 * self-requeue, so the TX-done workers can be drained here.
+	 */
+	for (i = 0; i < DPMAIF_TXQ_NUM; i++)
+		cancel_work_sync(&dpmaif_ctrl->txq[i].dpmaif_tx_work);
+
+	/* t7xx_dpmaif_rx_stop() clears que_started and waits for the
+	 * in-flight NAPI poll (rx_processing) to finish, so no poll issues
+	 * MMIO after this point. It is also the sole producer of
+	 * bat_release_work, so cancel that work only after rx_stop();
+	 * otherwise a residual poll re-queues it and it runs against
+	 * torn-down hardware.
+	 */
 	t7xx_dpmaif_rx_stop(dpmaif_ctrl);
+	cancel_work_sync(&dpmaif_ctrl->bat_release_work);
+
+	t7xx_dpmaif_hw_stop_all_txq(&dpmaif_ctrl->hw_info);
+	t7xx_dpmaif_hw_stop_all_rxq(&dpmaif_ctrl->hw_info);
 	return 0;
 }
 
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
index 2a405bc74312..450e030fc696 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>
@@ -426,6 +427,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) {
+			/* Bail out promptly on a pending freeze so the caller can
+			 * drop its runtime-PM reference and this thread can reach
+			 * the freeze point instead of looping here under load.
+			 */
+			if (freezing(current))
+				return;
 			usleep_range(10, 20);
 			cond_resched();
 			continue;
@@ -457,19 +464,30 @@ 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;
 		}
 
+		/* Park on a pending freeze here, outside the runtime-PM and MMIO
+		 * section below, so the PM freezer quiesces this thread before
+		 * dpm_suspend() runs the device suspend callbacks.
+		 * kthread_freezable_should_stop() also honours a concurrent
+		 * kthread_stop() while the thread is frozen.
+		 */
+		if (kthread_freezable_should_stop(NULL))
+			break;
+
 		ret = pm_runtime_resume_and_get(dpmaif_ctrl->dev);
 		if (ret < 0 && ret != -EACCES) {
 			/* Do not exit the thread: dpmaif_ctrl->tx_thread still
@@ -478,7 +496,9 @@ static int t7xx_dpmaif_tx_hw_push_thread(void *arg)
 			 */
 			dev_err_ratelimited(dpmaif_ctrl->dev,
 					    "Failed to resume for TX push: %d\n", ret);
-			msleep_interruptible(DPMAIF_TX_RESUME_RETRY_MS);
+			wait_event_freezable_timeout(dpmaif_ctrl->tx_wq,
+						     kthread_should_stop(),
+						     msecs_to_jiffies(DPMAIF_TX_RESUME_RETRY_MS));
 			continue;
 		}
 
-- 
2.43.0

[PATCH net v6 4/4] net: wwan: t7xx: complete NAPI on the not-started RX poll early return

[Tim JH Chen]

t7xx_dpmaif_napi_rx_poll() has an early return taken when the RX queue is
no longer started:

	if (!rxq->que_started) {
		atomic_set(&rxq->rx_processing, 0);
		pm_runtime_put_autosuspend(rxq->dpmaif_ctrl->dev);
		dev_err(..., "Work RXQ: %d has not been started\n", rxq->index);
		return work_done;
	}

work_done is 0 here, so the poll returns less than the budget without
calling napi_complete_done(). Per __napi_poll() (net/core/dev.c) a poll
that returns less than the budget is assumed to have completed the NAPI
itself, so the core does not reschedule it. NAPI_STATE_SCHED is therefore
left set and the NAPI is never polled again.

t7xx_dpmaif_rx_stop() clears que_started without completing the NAPI, on
the MD_STATE_EXCEPTION teardown path as well as on system suspend, so the
next poll takes this early return and strands NAPI_STATE_SCHED. A later
netdev close then hangs:

	t7xx_ccmni_close()
	  t7xx_ccmni_disable_napi()
	    napi_synchronize()	<- spins on NAPI_STATE_SCHED forever

napi_synchronize() runs with rtnl_lock held, so every subsequent rtnl
operation blocks and the network stack wedges (a hung task, not a soft
lockup).

Complete the NAPI on this early return so NAPI_STATE_SCHED is cleared and
napi_synchronize() can make progress. The sleep-lock retry branch just
below already calls napi_complete_done() before rescheduling, so only the
not-started branch needs the fix.

This is a pre-existing bug, independent of the TX/RX data path vs system
PM suspend race fixed earlier in this series. It was only reached once
that race stopped soft-locking the CPU before the teardown could run.

Fixes: d642b012df70 ("net: wwan: t7xx: Add data path interface")
Signed-off-by: Tim JH Chen <tim770802@gmail.com>
---
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
index 0e1174ee611d..6272956fe053 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
@@ -848,6 +848,12 @@ int t7xx_dpmaif_napi_rx_poll(struct napi_struct *napi, const int budget)
 		atomic_set(&rxq->rx_processing, 0);
 		pm_runtime_put_autosuspend(rxq->dpmaif_ctrl->dev);
 		dev_err(rxq->dpmaif_ctrl->dev, "Work RXQ: %d has not been started\n", rxq->index);
+		/* Returning work_done < budget without completing the NAPI would
+		 * leave NAPI_STATE_SCHED set, hanging a later napi_synchronize()
+		 * in t7xx_ccmni_disable_napi() (which holds rtnl_lock). Complete
+		 * it here so the queue is cleanly unscheduled after rx_stop().
+		 */
+		napi_complete_done(napi, work_done);
 		return work_done;
 	}
 
-- 
2.43.0

Re: [PATCH net v6 1/4] net: wwan: t7xx: fix runtime PM usage count underflow on -EACCES

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  t7xx_cldma_send_skb() (drivers/net/wwan/t7xx/t7xx_hif_cldma.c) returns…
- [Low] At this commit, t7xx_dpmaif_tx_hw_push_thread() still runs `if
  (ret < 0 && ret != -EACCES) return ret;` when…

--

net: wwan: t7xx: fix runtime PM usage count underflow on -EACCES

This patch changes t7xx_dpmaif_tx_hw_push_thread(), t7xx_dpmaif_tx_done()
and t7xx_dpmaif_bat_release_work() so they skip
pm_runtime_put_autosuspend() when pm_runtime_resume_and_get() returned
-EACCES. In that case no usage count reference is held.

> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> index 5af90ca6e0631..0e1174ee611df 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> @@ -1082,7 +1082,8 @@ static void t7xx_dpmaif_bat_release_work(struct work_struct *work)
>  	}
>  
>  	t7xx_pci_enable_sleep(dpmaif_ctrl->t7xx_dev);
> -	pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
> +	if (ret != -EACCES)
> +		pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
>  }

[Severity: High]
Are there other sites in this driver with the same unbalanced put that
still need fixing?

In this same file, t7xx_dpmaif_irq_rx_done() accepts -EACCES and still
schedules NAPI with no reference held:

t7xx_dpmaif_irq_rx_done() {
    ...
	ret = pm_runtime_resume_and_get(ctrl->dev);
	if (ret < 0 && ret != -EACCES) {
		dev_err(ctrl->dev, "Failed to resume device: %d\n", ret);
		return;
	}
	napi_schedule(&rxq->napi);
}

t7xx_dpmaif_napi_rx_poll() then drops a reference unconditionally, both
on the !rxq->que_started exit and on the work_done < budget completion
path:

t7xx_dpmaif_napi_rx_poll() {
    ...
	if (!rxq->que_started) {
		atomic_set(&rxq->rx_processing, 0);
		pm_runtime_put_autosuspend(rxq->dpmaif_ctrl->dev);
    ...
	} else if (work_done < budget) {
		...
		t7xx_pci_enable_sleep(rxq->dpmaif_ctrl->t7xx_dev);
		pm_runtime_put_autosuspend(rxq->dpmaif_ctrl->dev);
    ...
}

The get runs in IRQ context and the put runs in the NAPI poll, so a local
ret check can't be used. Would this need per-rxq state that is set in
t7xx_dpmaif_irq_rx_done() and checked in t7xx_dpmaif_napi_rx_poll()?

t7xx_cldma_send_skb() in t7xx_hif_cldma.c follows the same pattern. It
accepts -EACCES, and every exit goes through allow_sleep:

allow_sleep:
	t7xx_pci_enable_sleep(md_ctrl->t7xx_dev);
	pm_runtime_put_autosuspend(md_ctrl->dev);
	return ret;

There, ret gets overwritten later (-EIO, -ETIMEDOUT, or the result of
wait_event_interruptible_exclusive()). It would need a separate flag, like
the pm_ref this patch adds to t7xx_dpmaif_tx_done().

pm_runtime_get_active() has already called pm_runtime_put_noidle() before
it returns -EACCES. So each extra put either triggers the "Runtime PM
usage count underflow!" warning, or drops a reference that another context
holds (TX push thread, tx_done work, NAPI). Could that let the device
autosuspend while the other context is still doing MMIO?

As far as I can tell, the rest of the series doesn't cover these sites
either. At the end of the series, t7xx_dpmaif_napi_rx_poll() still puts
unconditionally (the last patch only adds napi_complete_done()), and CLDMA
is described as out of scope.

[ ... ]

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

[ ... ]

> @@ -467,7 +472,8 @@ static int t7xx_dpmaif_tx_hw_push_thread(void *arg)
>  		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);
> +		if (ret != -EACCES)
> +			pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
>  	}

[Severity: Low]
This isn't a bug introduced by this patch, but just above this hunk the
thread still exits if the resume fails:

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

After the kthread returns, would TX stall for good?
t7xx_dpmaif_tx_send_skb() only enqueues the skb and wakes tx_wq.

dpmaif_ctrl->tx_thread also keeps pointing at the exited task, and no
reference was taken with get_task_struct(). Can the later kthread_stop()
in t7xx_dpmaif_tx_thread_rel() then touch a task_struct that has already
been freed?

A later patch in this series, "net: wwan: t7xx: do not exit the TX push
kthread on resume failure", replaces the return with
dev_err_ratelimited(), wait_event_freezable_timeout() and continue. That
fixes this in the final state of the series.

[Severity: High]
This is a pre-existing issue, but this patch keeps treating -EACCES as
success. Can t7xx_cldma_send_skb() in t7xx_hif_cldma.c return -EACCES
after it has already handed the skb to the TX ring?

When pm_runtime_resume_and_get() returns -EACCES, ret keeps that value.
On the first loop iteration with queue->budget > 0, nothing reassigns it:

t7xx_cldma_send_skb() {
    ...
	ret = pm_runtime_resume_and_get(md_ctrl->dev);
	if (ret < 0 && ret != -EACCES)
		return ret;
    ...
		if (queue->budget > 0 && !tx_req->skb) {
			...
			t7xx_cldma_gpd_handle_tx_request(queue, tx_req, skb);
			...
			spin_lock_irqsave(&md_ctrl->cldma_lock, flags);
			t7xx_cldma_hw_start_send(md_ctrl, qno, tx_req);
			spin_unlock_irqrestore(&md_ctrl->cldma_lock, flags);

			break;
		}
    ...
	return ret;
}

By this point t7xx_cldma_gpd_handle_tx_request() has set
tx_req->skb = skb. The -EACCES goes back up through
t7xx_port_send_raw_skb() and t7xx_port_send_ccci_skb(), and the callers
free the skb on error:

t7xx_port_fastboot_write():  dev_kfree_skb(tx_skb)
t7xx_port_ctrl_tx():         dev_kfree_skb(cloned)
port_ctl_send_msg_to_md():   dev_kfree_skb_any(skb)

Later, t7xx_cldma_gpd_tx_collect() takes req->skb, unmaps it and calls
dev_kfree_skb_any() on it again.

Does this double free the skb? Could the device also DMA from the freed
memory before the second free?

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

Re: [PATCH net v6 3/4] net: wwan: t7xx: fix race between TX/RX data path and system PM suspend

[netdev-bot+sashiko]

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

Critical: 0 · High: 4 · Medium: 4 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] t7xx_dpmaif_suspend()
  (drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c:424-425) now calls…
- [High] The fix relies on t7xx_dpmaif_rx_stop() to "clear que_started
  and wait for the in-flight NAPI poll", so that no poll does MMIO or…
- [High] t7xx_dpmaif_suspend() assumes that after
  t7xx_dpmaif_disable_irq() "nothing re-arms the contexts drained below"…
- [High] Runtime suspend can now deadlock.
- [Medium] The commit message says "kthread_freezable_should_stop() also
  lets a concurrent kthread_stop() proceed while the thread is frozen".
- [Medium] The patch moves t7xx_dpmaif_hw_stop_all_txq() and
  t7xx_dpmaif_hw_stop_all_rxq() from the start of t7xx_dpmaif_suspend()…
- [Medium] In t7xx_do_tx_hw_push() (t7xx_hif_dpmaif_tx.c:416-455), the
  new freezing(current) bail-out at line 434 only sits in the…
- [Medium] The new cancel_work_sync(&dpmaif_ctrl->bat_release_work) in
  t7xx_dpmaif_suspend() (t7xx_hif_dpmaif.c:435) can discard a pending…

--

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

This patch makes the DPMAIF TX push kthread freezable. It also reworks
t7xx_dpmaif_suspend() to mask interrupts, drain the TX-done workers, stop
RX and cancel bat_release_work before the hardware queues are stopped.
The aim is to keep data-plane contexts off the device while the suspend
callback tears it down, which showed up as a soft lockup with ASPM L1.

> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c
> index 7ff33c1d6ac75..b5a857e940b7c 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c
> @@ -410,12 +410,32 @@ static int t7xx_dpmaif_stop(struct dpmaif_ctrl *dpmaif_ctrl)
>  static int t7xx_dpmaif_suspend(struct t7xx_pci_dev *t7xx_dev, void *param)
>  {
>  	struct dpmaif_ctrl *dpmaif_ctrl = param;
> +	unsigned int i;
>  
> +	/* Stop new TX and mask interrupts first, so nothing re-arms the
> +	 * contexts drained below.
> +	 */
>  	t7xx_dpmaif_tx_stop(dpmaif_ctrl);
> -	t7xx_dpmaif_hw_stop_all_txq(&dpmaif_ctrl->hw_info);
> -	t7xx_dpmaif_hw_stop_all_rxq(&dpmaif_ctrl->hw_info);
>  	t7xx_dpmaif_disable_irq(dpmaif_ctrl);

[Severity: High]
Does masking here really stop everything that can re-arm the contexts
drained below?

t7xx_dpmaif_disable_irq() only writes the PCIe MAC mask through
t7xx_pcie_mac_clear_int(). It does not call synchronize_irq(), and it
leaves dpmaif_ctrl->state unchanged.

Suppose a DPMAIF interrupt fired just before suspend. The hardirq handler
has already masked and returned IRQ_WAKE_THREAD, so the threaded handler
may still be running:

t7xx_dpmaif_isr_thread()
	t7xx_dpmaif_irq_cb(isr_para);
	t7xx_pcie_mac_set_int(dpmaif_ctrl->t7xx_dev, isr_para->pcie_int);

>From t7xx_dpmaif_irq_cb(), that thread can still reach:

- t7xx_dpmaif_irq_tx_done() -> queue_work(dpmaif_tx_work), after the
  cancel_work_sync() loop below has finished.
- t7xx_dpmaif_irq_rx_done() -> napi_schedule(), after
  t7xx_dpmaif_rx_stop(). The device is still RPM_ACTIVE during system
  suspend, so pm_runtime_resume_and_get() returns at once.

The mask_ulq_intr() MMIO poll inside the thread can take up to 10ms,
which makes this window realistic. IRQ threads are not frozen, and
suspend_device_irqs() only runs later, in the noirq phase.

The thread then calls t7xx_pcie_mac_set_int() unconditionally. That
unmasks the interrupt suspend has just masked.

Could TX-done work, NAPI polls and a re-enabled interrupt then reach the
hardware after hw_stop_all_txq/rxq and the H2D suspend handshake? Is a
synchronize_irq() on the DPMAIF vectors needed after disable_irq()?

> +
> +	/* irq_tx_done is masked now and cancel_work_sync() also blocks a
> +	 * self-requeue, so the TX-done workers can be drained here.
> +	 */
> +	for (i = 0; i < DPMAIF_TXQ_NUM; i++)
> +		cancel_work_sync(&dpmaif_ctrl->txq[i].dpmaif_tx_work);

[Severity: High]
Can this cancel_work_sync() discard the only code that unmasks the
queue's UL-DONE interrupt?

The ISR masks UL-DONE for each queue before it queues the work:

t7xx_dpmaif_irq_cb()
  t7xx_dpmaif_hw_get_intr_cnt()
    t7xx_dpmaif_hw_check_tx_intr()
		for_each_set_bit(index, &value, DPMAIF_TXQ_NUM)
			t7xx_dpmaif_mask_ulq_intr(hw_info, index);

At runtime, only t7xx_dpmaif_tx_done() unmasks it, and only on its final
pass:

		if (ret == -EAGAIN ||
		    (t7xx_dpmaif_ul_clr_done(hw_info, txq->index) &&
		     t7xx_dpmaif_drb_ring_not_empty(txq))) {
			queue_work(dpmaif_ctrl->txq[txq->index].worker,
				   &dpmaif_ctrl->txq[txq->index].dpmaif_tx_work);
			/* Give the device time to enter the low power state */
			t7xx_dpmaif_clr_ip_busy_sts(hw_info);
		} else {
			t7xx_dpmaif_clr_ip_busy_sts(hw_info);
			t7xx_dpmaif_unmask_ulq_intr(hw_info, txq->index);
		}

The unmask is skipped in two cases:

- The work is pending when suspend runs. It is removed and never
  executes.
- The work is running and takes the requeue branch. cancel_work_sync()
  blocks the requeue, as the new comment notes.

t7xx_dpmaif_resume() then does:

	t7xx_dpmaif_start_txrx_qs(dpmaif_ctrl);
	t7xx_dpmaif_enable_irq(dpmaif_ctrl);
	t7xx_dpmaif_unmask_dlq_intr(dpmaif_ctrl);
	t7xx_dpmaif_start_hw(&dpmaif_ctrl->hw_info);

That restores the DL masks but not UL-DONE, and the cancelled work is
never requeued. The only other unmask, t7xx_dpmaif_init_intr(), runs only
on a full re-init.

Would the queue then get no further UL-DONE interrupts? In that case
t7xx_dpmaif_tx_release() never runs again, the DRB ring and tx_budget are
never reclaimed, and TX on that queue stalls until the modem is
re-initialised. This callback is also used for runtime suspend, so system
suspend is not the only trigger.

[Severity: High]
Could this cancel_work_sync() deadlock on the runtime suspend path?
Runtime suspend calls the same callback:

t7xx_pci_pm_runtime_suspend()
  __t7xx_pci_pm_suspend()
    entity->suspend()
      t7xx_dpmaif_suspend()

rpm_suspend() sets RPM_SUSPENDING before it calls the callback. The
DPMAIF interrupt stays enabled until t7xx_dpmaif_disable_irq() runs here.
The ISR only checks dpmaif_ctrl->state, so a UL-DONE interrupt in that
window queues dpmaif_tx_work. The work then does:

t7xx_dpmaif_tx_done()
  pm_runtime_resume_and_get()
    rpm_resume()
      /* sleeps uninterruptibly while runtime_status == RPM_SUSPENDING */

Meanwhile the suspend callback sits in cancel_work_sync() waiting for
that same work. The callback cannot return, so the status never leaves
RPM_SUSPENDING. t7xx_dpmaif_bat_release_work() makes the same
pm_runtime_resume_and_get() call, and it is also cancelled synchronously
below.

The commit message says:

  Runtime suspend is already safe: while any of these contexts holds its PM
  reference the runtime suspend callback cannot run.

That covers contexts that already hold a reference. Does it cover a
context that is still trying to take one? Before this patch the suspend
callback never waited on these works, so this cycle could not form.

> +
> +	/* t7xx_dpmaif_rx_stop() clears que_started and waits for the
> +	 * in-flight NAPI poll (rx_processing) to finish, so no poll issues
> +	 * MMIO after this point. It is also the sole producer of
> +	 * bat_release_work, so cancel that work only after rx_stop();
> +	 * otherwise a residual poll re-queues it and it runs against
> +	 * torn-down hardware.
> +	 */
>  	t7xx_dpmaif_rx_stop(dpmaif_ctrl);

[Severity: High]
Does t7xx_dpmaif_rx_stop() give the guarantee this comment and the commit
message describe ("clears que_started and waits for the in-flight NAPI RX
poll to finish")? It does the two steps in the other order:

t7xx_dpmaif_rx_stop() {
	...
		timeout = readx_poll_timeout_atomic(atomic_read, &rxq->rx_processing, value,
						    !value, 0, DPMAIF_CHECK_INIT_TIMEOUT_US);
		if (timeout)
			dev_err(dpmaif_ctrl->dev, "Stop RX SW failed\n");

		/* Ensure RX processing has stopped before we set rxq->que_started to false */
		smp_mb();
		rxq->que_started = false;
	...
}

The poll side sets rx_processing first and reads que_started afterwards:

t7xx_dpmaif_napi_rx_poll() {
	atomic_set(&rxq->rx_processing, 1);
	/* Ensure rx_processing is changed to 1 before actually begin RX flow */
	smp_mb();

	if (!rxq->que_started) {
	...
}

Take a poll that starts after the rx_processing check but before the
que_started store. It sees que_started == true and runs a full pass:

- t7xx_dpmaif_rx_start() -> t7xx_dpmaifq_rx_notify_hw() ->
  queue_work(bat_release_work)
- t7xx_dpmaif_clr_ip_busy_sts()
- t7xx_dpmaif_dlq_unmask_rx_done()

The timeout case is a second way in, since the timeout is only logged.
rx_processing stays at 1 across once_more and budget-exhausted repolls.
The DL hardware queues now stay enabled until after rx_stop(), so
sustained downlink could keep rx_processing set for the whole
DPMAIF_CHECK_INIT_TIMEOUT_US. rx_stop() would then return with a poll
still in progress.

In either case, could that poll do MMIO and requeue bat_release_work
after the cancel_work_sync() below and after hw_stop_all_rxq()?

> +	cancel_work_sync(&dpmaif_ctrl->bat_release_work);

[Severity: Medium]
Can this cancel drop a BAT refill that nothing replays on resume?
t7xx_dpmaif_bat_release_work() is the code that refills consumed RX
buffers:

	if (t7xx_pci_sleep_disable_complete(dpmaif_ctrl->t7xx_dev)) {
		t7xx_dpmaif_bat_release_and_add(rxq);
		t7xx_dpmaif_frag_bat_release_and_add(rxq);
	}

Its only producer is t7xx_dpmaifq_rx_notify_hw(), called from the NAPI
poll. t7xx_dpmaif_resume() does not requeue it.

Suppose heavy RX just before suspend used up the BAT ring. After resume
the hardware has no free entry to place a packet in. So no DL-DONE
interrupt fires, no NAPI poll runs, and the refill never happens.
DPF_INTR_DL_BATCNT_LEN_ERR only unmasks itself again.

Would RX then stay stalled after resume? If the ring was not fully used
up, the next poll recovers it. So this depends on the ring running dry
first.

> +
> +	t7xx_dpmaif_hw_stop_all_txq(&dpmaif_ctrl->hw_info);
> +	t7xx_dpmaif_hw_stop_all_rxq(&dpmaif_ctrl->hw_info);

[Severity: Medium]
The hardware queue-disable calls move from the start of the callback to
the end:

  old: tx_stop -> hw_stop_all_txq -> hw_stop_all_rxq -> disable_irq
       -> rx_stop
  new: tx_stop -> disable_irq -> cancel TX work -> rx_stop
       -> cancel bat work -> hw_stop_all_txq -> hw_stop_all_rxq

The commit message does not mention this reorder. Was it intentional?

DL hardware is now still enabled during t7xx_dpmaif_rx_stop(). Sustained
downlink can keep NAPI returning full budget, which makes the rx_stop()
timeout above reachable. Before the patch DL was stopped first, so NAPI
could drain.

There is a second effect. Once rx_stop() marks the queues stopped, PIT
entries the hardware produces before hw_stop_all_rxq() are not consumed.
The "middle PIT sync" wait in t7xx_dpmaif_hw_stop_all_rxq() now runs with
software consumption already stopped:

	count = 0;
	do {
		wr_idx = ioread32(hw_info->pcie_base + DPMAIF_AO_DL_PIT_WR_IDX);
		wr_idx &= DPMAIF_DL_RD_WR_IDX_MSK;
		rd_idx = ioread32(hw_info->pcie_base + DPMAIF_AO_DL_PIT_RD_IDX);
		rd_idx &= DPMAIF_DL_RD_WR_IDX_MSK;

		if (wr_idx == rd_idx)
			return 0;
	} while (++count < DPMAIF_MAX_CHECK_COUNT);

The caller also ignores its return value. Is that wait still expected to
succeed with the new order?

>  	return 0;
>  }
>  
> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
> index 2a405bc743121..450e030fc6962 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c

[ ... ]

> @@ -426,6 +427,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) {
> +			/* Bail out promptly on a pending freeze so the caller can
> +			 * drop its runtime-PM reference and this thread can reach
> +			 * the freeze point instead of looping here under load.
> +			 */
> +			if (freezing(current))
> +				return;

[Severity: Medium]
The freeze check only covers the drb_send_cnt <= 0 branch. On the
success path the loop calls t7xx_dpmaif_ul_update_hw_drb_cnt() and
cond_resched(), then repeats on:

	} while (!t7xx_tx_lists_are_all_empty(dpmaif_ctrl) && !kthread_should_stop() &&
		 (dpmaif_ctrl->state == DPMAIF_STATE_PWRON));

That condition does not check for a pending freeze.

Consider skbs that arrive fast enough to keep the lists non-empty but not
fast enough to fill the ring. Forwarded traffic, or TCP ACKs and
retransmits sent from softirq after userspace is frozen, would do it.
Does the thread ever get back to kthread_freezable_should_stop() in
t7xx_dpmaif_tx_hw_push_thread() in that case?

If it doesn't, freeze_kernel_threads() hits freeze_timeout_msecs and
system suspend aborts. Before this patch the thread was not freezable, so
it could not block the freezer.

Should the loop condition check freezing(current) as well?

>  			usleep_range(10, 20);
>  			cond_resched();
>  			continue;
> @@ -457,19 +464,30 @@ static int t7xx_dpmaif_tx_hw_push_thread(void *arg)

[ ... ]

> +		/* Park on a pending freeze here, outside the runtime-PM and MMIO
> +		 * section below, so the PM freezer quiesces this thread before
> +		 * dpm_suspend() runs the device suspend callbacks.
> +		 * kthread_freezable_should_stop() also honours a concurrent
> +		 * kthread_stop() while the thread is frozen.
> +		 */
> +		if (kthread_freezable_should_stop(NULL))
> +			break;

[Severity: Medium]
Is the last sentence of this comment accurate? The commit message makes
the same claim:

  kthread_freezable_should_stop() also lets a concurrent
  kthread_stop() proceed while the thread is frozen

In the refrigerator the thread is in TASK_FROZEN. kthread_stop() does:

	set_bit(KTHREAD_SHOULD_STOP, &kthread->flags);
	kthread_unpark(k);
	set_tsk_thread_flag(k, TIF_NOTIFY_SIGNAL);
	wake_up_process(k);
	wait_for_completion(&kthread->exited);

wake_up_process() uses TASK_NORMAL, and ttwu_state_match() does not wake
a TASK_FROZEN task. A saved_state match only updates saved_state.

__refrigerator(true) re-checks kthread_should_stop() only after it has
been woken. That handles a stop issued before the thread freezes, not
one issued while it is frozen.

So during the freeze window, this kthread_stop() now blocks until
thaw_processes():

t7xx_ccmni_exit()
  t7xx_dpmaif_hif_exit()
    t7xx_dpmaif_sw_release()
      t7xx_dpmaif_tx_thread_rel()
        kthread_stop()

The commit message mentions unbind holding device_lock as a non-frozen
context. If this runs from such a context, could dpm_suspend() or
dpm_resume() end up waiting on that same lock?

The kerneldoc for kthread_freezable_should_stop() makes a similar claim,
which may be where this wording came from.

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

Re: [PATCH net v6 4/4] net: wwan: t7xx: complete NAPI on the not-started RX poll early return

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The handshake between t7xx_dpmaif_rx_stop() and
  t7xx_dpmaif_napi_rx_poll() can let a poll run past the stop.
- [Low] The Fixes: tag points at the wrong commit.
- [Low] The new comment in t7xx_dpmaif_napi_rx_poll() says the hang
  happens in "napi_synchronize() in t7xx_ccmni_disable_napi() (which…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the
  !que_started early return in t7xx_dpmaif_napi_rx_poll() (lines…

--

net: wwan: t7xx: complete NAPI on the not-started RX poll early return

The patch adds a napi_complete_done() call to the !que_started early
return in t7xx_dpmaif_napi_rx_poll(). Without that call, a poll that
returns 0 after t7xx_dpmaif_rx_stop() leaves NAPI_STATE_SCHED set. A later
napi_synchronize() in t7xx_ccmni_disable_napi() then hangs.

> Fixes: d642b012df70 ("net: wwan: t7xx: Add data path interface")

[Severity: Low]
Is this the right commit for the Fixes: tag?

At d642b012df70, drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c has no NAPI
code. RX ran from a workqueue, so returning early on !que_started was fine
at that point.

The NAPI ->poll() form came from 5545b7b9f294 ("net: wwan: t7xx: Add NAPI
support"). That is t7xx_dpmaif_napi_rx_poll(napi, budget), with the
dev_err() followed by return work_done and no napi_complete_done().

Should it be this instead?

Fixes: 5545b7b9f294 ("net: wwan: t7xx: Add NAPI support")

d642b012df70 is in v5.19, and 5545b7b9f294 was merged for v6.2. With the
current tag, stable tooling would try to apply this to 6.1.y, which has no
t7xx_dpmaif_napi_rx_poll().

> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> index 0e1174ee611df..6272956fe0535 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> @@ -848,6 +848,12 @@ int t7xx_dpmaif_napi_rx_poll(struct napi_struct *napi, const int budget)
>  		atomic_set(&rxq->rx_processing, 0);
>  		pm_runtime_put_autosuspend(rxq->dpmaif_ctrl->dev);
>  		dev_err(rxq->dpmaif_ctrl->dev, "Work RXQ: %d has not been started\n", rxq->index);
> +		/* Returning work_done < budget without completing the NAPI would
> +		 * leave NAPI_STATE_SCHED set, hanging a later napi_synchronize()
> +		 * in t7xx_ccmni_disable_napi() (which holds rtnl_lock). Complete
> +		 * it here so the queue is cleanly unscheduled after rx_stop().
> +		 */

[Severity: Low]
Is the "(which holds rtnl_lock)" part of this comment accurate?
t7xx_ccmni_disable_napi() doesn't take any lock:

drivers/net/wwan/t7xx/t7xx_netdev.c:t7xx_ccmni_disable_napi() {
	if (!ctlb->is_napi_en)
		return;

	for (i = 0; i < RXQ_NUM; i++) {
		napi_synchronize(ctlb->napi[i]);
		napi_disable(ctlb->napi[i]);
	}
	...
}

rtnl is held only when the core reaches it through .ndo_stop, which is
t7xx_ccmni_close().

The other caller is the modem state path. It runs in FSM context without
rtnl:

t7xx_ccmni_md_state_callback()    /* MD_STATE_EXCEPTION / MD_STATE_STOPPED */
  t7xx_ccmni_post_stop()
    t7xx_ccmni_disable_napi()
      napi_synchronize()

On the exception path, this is the first napi_synchronize() to hit a
stranded NAPI_STATE_SCHED. By then que_started has already been cleared
through
t7xx_dpmaif_md_state_callback()->t7xx_dpmaif_stop()->t7xx_dpmaif_stop_sw()->t7xx_dpmaif_rx_stop().

The t7xx driver never takes rtnl_lock itself. Could the comment say that
rtnl_lock is held only on the ndo_stop path, or drop the lock reference?

> +		napi_complete_done(napi, work_done);
>  		return work_done;
>  	}

[Severity: Medium]
This isn't a bug introduced by this patch, but this early return still
doesn't undo an earlier t7xx_pci_disable_sleep() from the same scheduling
cycle. It doesn't call t7xx_pci_enable_sleep(), and it doesn't clear
rxq->sleep_lock_pending.

There are two ways to reach it with a sleep-disable vote still
outstanding.

(a) An earlier poll used up its budget or set once_more. It returned with
sleep_disable_count incremented and rx_processing still 1.

(b) An earlier poll took the retry branch:

	ret = try_wait_for_completion(&t7xx_dev->sleep_lock_acquire);
	if (!ret) {
		napi_complete_done(napi, work_done);
		rxq->sleep_lock_pending = true;
		napi_schedule(napi);
		return work_done;
	}

In both cases t7xx_dpmaif_rx_stop() can time out and clear que_started
anyway:

		timeout = readx_poll_timeout_atomic(atomic_read, &rxq->rx_processing, value,
						    !value, 0, DPMAIF_CHECK_INIT_TIMEOUT_US);
		if (timeout)
			dev_err(dpmaif_ctrl->dev, "Stop RX SW failed\n");
		...
		rxq->que_started = false;

Can the next poll then take this early return with sleep_disable_count
still raised, or with sleep_lock_pending stale?

sleep_disable_count is never reset. If it stays raised,
t7xx_dev_set_sleep_capability(true) wouldn't run again. Later
t7xx_pci_disable_sleep() calls would also skip the count == 1 DS_LOCK
handshake.

The accounting also looks unbalanced apart from this branch. Each repoll
after a budget-exhausting or once_more return calls
t7xx_pci_disable_sleep() again, because sleep_lock_pending is false. The
completion branch calls t7xx_pci_enable_sleep() only once.

This patch doesn't make things worse. In case (b), the stale flag now makes
the next poll skip t7xx_pci_disable_sleep() and call
t7xx_pci_enable_sleep() on completion, which cancels out the leaked count.

[Severity: High]
Can a new poll still get past t7xx_dpmaif_rx_stop() through this check?
The current ordering doesn't seem able to keep a newly starting poll out.

t7xx_dpmaif_rx_stop() loads rx_processing, does smp_mb(), and then stores
que_started = false. The poll stores rx_processing = 1, does smp_mb(), and
then loads que_started. Because the stop side does a load and then a
store, this interleaving is allowed:

CPU0 (t7xx_dpmaif_rx_stop)        CPU1 (t7xx_dpmaif_napi_rx_poll)
reads rx_processing == 0
                                  atomic_set(&rxq->rx_processing, 1);
                                  smp_mb();
                                  reads que_started == true
smp_mb();
rxq->que_started = false;
returns

A new poll can be scheduled here even after the IRQ is masked. If an RX
IRQ arrived during the previous poll, NAPIF_STATE_MISSED is set. That
poll's napi_complete_done() then reschedules it with __napi_schedule(), and
only afterwards does it store rx_processing = 0.

CPU1 would then run the full RX path while the rest of
t7xx_dpmaif_suspend() runs. That includes:

t7xx_dpmaifq_rx_notify_hw()
  queue_work(dpmaif_ctrl->bat_release_wq, &dpmaif_ctrl->bat_release_work);

This can run after cancel_work_sync(&dpmaif_ctrl->bat_release_work). The
PIT/BAT updates and the MMIO in t7xx_dpmaif_clr_ip_busy_sts() and
t7xx_dpmaif_dlq_unmask_rx_done() can also run at the same time as
t7xx_dpmaif_hw_stop_all_rxq() and the H2D suspend requests.

That conflicts with what the earlier patch in this series ("net: wwan:
t7xx: fix race between TX/RX data path and system PM suspend") relies on
in t7xx_dpmaif_suspend():

	/* t7xx_dpmaif_rx_stop() clears que_started and waits for the
	 * in-flight NAPI poll (rx_processing) to finish, so no poll issues
	 * MMIO after this point. It is also the sole producer of
	 * bat_release_work, so cancel that work only after rx_stop();
	 ...
	 */
	t7xx_dpmaif_rx_stop(dpmaif_ctrl);
	cancel_work_sync(&dpmaif_ctrl->bat_release_work);

The same path can also be reached on a modem exception through
t7xx_dpmaif_stop_sw().

The load-then-store order in t7xx_dpmaif_rx_stop() dates back to
d642b012df70, but this series now depends on it to guarantee that no
MMIO happens after the stop.

Would this ordering close the window?

- In t7xx_dpmaif_rx_stop(): store que_started = false with WRITE_ONCE(),
  then smp_mb(), then wait for rx_processing == 0.
- In t7xx_dpmaif_napi_rx_poll(): read que_started with READ_ONCE().

>  
>

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