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

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Sun Sep 27, 2026 at 8:01 PM EDT, netdev-bot+sashiko wrote:
> 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?
>

The code is correct. TWQ has a max ring size of 64k, and that has an
encoding of 0 (like fbnic). TCQ actually supports rings as large as
256k, but it does not have/need the 0 encoding for that, as there is an
extra bit in the size field.

> 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?

This was addressed many times in the v1 sashiko review.