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