Re: [PATCH net-next v2 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: 1 · Medium: 0 · Low: 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] mpnic_wait_all_queues_idle() returns void and drops the
  -ETIMEDOUT from read_poll_timeout().
- [Low] mpnic_enable_twq() writes fls(size_mask) & MPNIC_TWQ_SIZE_SIZE (a
  4-bit field), and mpnic_enable_tcq() writes the same exponent into…

--

eth: mpnic: start and stop the Tx HW queues

This commit writes the Tx work queue and completion queue base addresses
and sizes into the hardware. It adds mpnic_enable() and mpnic_disable() to
turn the Tx queues on and off. It also adds mpnic_wait_all_queues_idle(),
which polls the TWQ, TQS, TDE and TCQ idle bitmaps, and mpnic_flush(),
which throws away any Tx work left in the rings.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 853976e11b032..aaa3a3e99a138 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> @@ -467,6 +468,180 @@ void mpnic_reset_netif_queues(struct mpnic_net *mpn)
>  	}
>  }
>  
> +static void mpnic_enable_twq(struct mpnic_dev *mpd, struct mpnic_ring *twq)
> +{
> +	u32 log_size = fls(twq->size_mask);
> +	u32 i = twq->q_idx;

[ ... ]

> +	/* Store descriptor ring address and size */
> +	mpnic_wr64(mpd, MPNIC_TWQ_BASE_ADDR(i, 0), twq->dma);
> +	mpnic_wr64(mpd, MPNIC_TWQ_SIZE(i, 0), log_size & MPNIC_TWQ_SIZE_SIZE);

[ ... ]

> +static void mpnic_enable_tcq(struct mpnic_dev *mpd,
> +			     struct mpnic_napi_vector *nv,
> +			     struct mpnic_ring *tcq)
> +{
> +	u32 log_size = fls(tcq->size_mask);
> +	u32 i = tcq->q_idx;

[ ... ]

> +	/* Store descriptor ring address and size */
> +	mpnic_wr64(mpd, MPNIC_TCQ_BASE_ADDR(i), tcq->dma);
> +	mpnic_wr64(mpd, MPNIC_TCQ_SIZE(i), log_size & MPNIC_TCQ_SIZE_SIZE);

[Severity: Low]
This isn't a bug with the current fixed 1024-entry rings, but
mpnic_enable_twq() and mpnic_enable_tcq() mask the same exponent with
fields of different widths in mpnic_csr.h:

  #define MPNIC_TWQ_SIZE_SIZE    CSR_GENMASK(3, 0)
  #define MPNIC_TCQ_SIZE_SIZE    CSR_GENMASK(4, 0)

For a 64K-entry ring (fls() == 16), TWQ_SIZE would get 0 and TCQ_SIZE
would get 16. Is this the intended encoding for each register?

fbnic has a comment for the 4-bit form: "Write lower 4 bits of log size
as 64K ring size is 0". Would a similar comment help here?

[ ... ]

> +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: High]
What happens to the Tx rings if this poll times out?

The commit message says every block "has to report idle before the
descriptors are safe to free". However, the -ETIMEDOUT from
read_poll_timeout() is only logged. mpnic_wait_all_queues_idle() returns
void, so a caller can't tell a timeout apart from a clean drain.

mpnic_stop() is added later in the series, in "eth: mpnic: add a netdevice
and basic Tx handling". It carries on whatever the result:

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

  mpnic_reset_netif_queues(mpn);
  mpnic_free_resources(mpn);

mpnic_disable_twq() and mpnic_disable_tcq() only clear the enable bits.
TWQ_BASE_ADDR and TCQ_BASE_ADDR still point at the rings.

After a timeout, mpnic_flush()->mpnic_clean_twq0(..., true, tail) unmaps
the in-flight Tx buffers and consumes their skbs. Then
mpnic_free_ring_resources() calls dma_free_coherent() on the TWQ and TCQ
descriptor memory.

Could a Tx engine that is still busy then fetch descriptors or payload from
freed memory and put it on the wire? Could the TCM write completion
descriptors into memory that now belongs to another user?

fbnic_wait_all_queues_idle() handles the same case differently. It turns
on Tx drop through fbnic_tx_flush(), polls again, and returns an int.
Would something similar be needed here? At minimum, could the error be
returned so the caller can avoid freeing the rings?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-linux-mpnic-v2-0-4badc9b58b9e%40gmail.com