Re: [PATCH net-next 5/8] eth: mpnic: start and stop the Tx HW queues

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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).

- [Medium] mpnic_wait_all_queues_idle()
  (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c:599) returns void.
- [Medium] mpnic_non_idle_queues() (mpnic_txrx.c:569-570) counts a block
  as idle when every idle register reads ~0ULL.

--

eth: mpnic: start and stop the Tx HW queues

This commit programs the Tx work and completion queue base addresses and
sizes into the hardware, and adds helpers to enable and disable the Tx
queues. It also adds mpnic_wait_all_queues_idle(), which polls the idle
bitmaps of the TWQ, TQS, TDE and TCQ blocks so that teardown can wait for
the DMA engines before the rings are freed.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index fe360a26a27be..cb4d1427434c9 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> +static u32 mpnic_non_idle_queues(struct mpnic_dev *mpd,
> +				 const struct mpnic_idle_regs *regs,
> +				 unsigned int nregs)
> +{
> +	u32 non_idle_bitmap = 0;
> +	unsigned int i, j;
> +
> +	for (i = 0; i < nregs; i++) {
> +		for (j = 0; j < regs[i].reg_cnt; j++) {
> +			if (mpnic_rd64(mpd, regs[i].reg_base + 2 * j) !=
> +			    ~0ULL) {
> +				non_idle_bitmap |= BIT(i);
> +				break;
> +			}

[Severity: Medium]
Could this report a device that can't be reached as idle? The idle test
here is ~0ULL, and mpnic_rd64() returns that same value once CSR access
has been disabled:

drivers/net/ethernet/meta/mpnic/mpnic_pci.c:mpnic_rd64() {
	u32 __iomem *csr = READ_ONCE(mpd->uc_addr0);
	...
	if (!csr)
		return ~0ULL;
	...
	mpnic_mmio_err(mpd, reg);

	return ~0ULL;
}

mpnic_wr64() also drops writes silently in that state:

	if (csr)
		writeq(val, csr + reg);

After mpnic_mmio_err() clears uc_addr0, the writes in mpnic_disable()
that clear MPNIC_TWQ_CTL_ENABLE and MPNIC_TCQ_CTL never reach the device.
On the first poll, mpnic_wait_all_queues_idle() then sees all four blocks
as idle and returns without a timeout. The caller can't tell a confirmed
idle state apart from an unreadable device, so it goes on to free the
rings.

mpnic_mmio_err() logs "awaiting reset", which suggests the device may be
recoverable rather than gone. Suppose the device can still bus-master
after two all-ones reads in a row, for example after a transient MMIO
failure. Could its still-enabled queues then DMA into ring memory that
has already been freed?

[ ... ]

> +void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd)
> +{
> +	static const struct mpnic_idle_regs queues[] = {
> +		{ MPNIC_TWQ_IDLE(0), MPNIC_TWQ_IDLE_CNT, "TWQ" },
> +		{ MPNIC_TQS_IDLE(0), MPNIC_TQS_IDLE_CNT, "TQS" },
> +		{ MPNIC_TDE_IDLE(0), MPNIC_TDE_IDLE_CNT, "TDE" },
> +		{ MPNIC_TCQ_IDLE(0), MPNIC_TCQ_IDLE_CNT, "TCQ" },
> +	};
> +	u32 non_idle_bitmap;
> +	int err;
> +
> +	err = read_poll_timeout(mpnic_non_idle_queues, non_idle_bitmap,
> +				!non_idle_bitmap, 20, 500000, false, mpd,
> +				queues, ARRAY_SIZE(queues));
> +	if (err)
> +		mpnic_idle_dump(mpd, queues, ARRAY_SIZE(queues),
> +				non_idle_bitmap, err);
> +}

[Severity: Medium]
What happens to the rings when this poll times out? The -ETIMEDOUT in
err is only passed to mpnic_idle_dump() for logging and then dropped.
Because the function returns void, no caller can act on the timeout.

The commit message says:

  every one of the four blocks a packet passes through on its way out
  has to report idle before the descriptors are safe to free.

Later in the series, though, mpnic_stop() runs the teardown with no
condition between the steps:

	mpnic_disable(mpn);
	mpnic_wait_all_queues_idle(mpn->mpd);
	mpnic_flush(mpn);

	mpnic_reset_netif_queues(mpn);
	mpnic_free_resources(mpn);

After a timeout, mpnic_flush() calls mpnic_clean_twq0() with
qt->sub0.tail. That unmaps every pending TWD and frees the in-flight
skbs. mpnic_free_ring_resources() then calls dma_free_coherent() on the
TWQ and TCQ descriptor rings, whose addresses mpnic_enable_twq() and
mpnic_enable_tcq() handed to the hardware.

If a block is still busy after 500 ms, can the device still read the
freed skb data and descriptors? Can it write completions into the freed
TCQ memory?

The final version of the series (the "eth: mpnic: add basic Rx handling"
commit) still returns void and still frees unconditionally. It also adds
the Rx HPQ/PPQ/RCQ blocks to this wait, which would expose the Rx rings
and page pool pages the same way.

Should mpnic_wait_all_queues_idle() return the error? Teardown could then
avoid freeing memory the device may still be using.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-linux-mpnic-v1-0-236844f53072%40gmail.com