Re: [PATCH net-next 7/8] eth: mpnic: implement Rx queue allocation and cleanup

[Daniel Zahka]

On Wed Sep 23, 2026 at 10:05 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · 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).
>
> - [Medium] The Rx engines are added to an idle wait whose timeout is only
>   logged, and teardown then frees memory the device writes into.
> - [Low] The two comments about Rx buffer batching disagree with each
>   other and with the code.
>
> --
>
> eth: mpnic: implement Rx queue allocation and cleanup
>
> This adds the Rx side of the mpnic queue triad: a header buffer queue
> and a payload buffer queue, both fed from one page pool per triad, plus
> an Rx completion queue. Buffers are posted to the device in batches, and
> the device is set up to leave XDP headroom and skb_shared_info tailroom.
> Teardown releases any partially assembled frame and the buffers still
> posted.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index f4ad8caf6ce66..6d2123bee97b4 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>
> [ ... ]
>
>> @@ -335,6 +336,109 @@ static void mpnic_clean_tcq(struct mpnic_napi_vector *nv,
>
> [ ... ]
>
>> +/* Descriptors are only handed to the device in whole batches, so the slot
>> + * the device is working on and everything up to the next batch boundary
>> + * stay untouched while it does.
>> + */
>> +static unsigned int mpnic_bdq_desc_unused(struct mpnic_ring *bdq)
>> +{
>> +	return (ALIGN_DOWN(bdq->head - 1, MPNIC_BDQ_BATCH_SIZE) - bdq->tail) &
>> +	       bdq->size_mask;
>> +}
>> +
>> +static unsigned int __mpnic_fill_bdq(struct mpnic_ring *bdq)
>> +{
>> +	unsigned int i = bdq->tail;
>> +	unsigned int count;
>> +
>> +	for (count = mpnic_bdq_desc_unused(bdq); count; count--) {
>> +		struct page *page;
>> +
>> +		page = page_pool_dev_alloc_pages(bdq->page_pool);
>> +		if (!page)
>> +			break;
>> +
>> +		bdq->rx_buf[i] = page;
>> +		mpnic_bd_prep(bdq, i, page);
>> +
>> +		i++;
>> +		i &= bdq->size_mask;
>> +	}
>> +
>> +	return i;
>> +}
>> +
>> +static void __mpnic_bdq_commit_tail(struct mpnic_ring *bdq, unsigned int tail)
>> +{
>> +	if (bdq->tail != tail) {
>> +		bdq->tail = tail;
>> +
>> +		writeq(tail, bdq->doorbell);
>> +	}
>> +}
>
> [Severity: Low]
> This isn't a bug, but is the comment on mpnic_bdq_desc_unused() accurate?
>
> __mpnic_fill_bdq() stops as soon as page_pool_dev_alloc_pages() returns
> NULL. __mpnic_bdq_commit_tail() then writes that tail to the doorbell,
> so a tail that is not on a 64 descriptor boundary can reach the device.
> In that case descriptors are not handed over in whole batches.
>
> It also looks like the slot at head - 1 stays untouched because of the
> "head - 1" term. Whole-batch posting is not what protects it. The
> ALIGN_DOWN only holds back up to 63 more slots that have already been
> consumed.
>
> The comment on MPNIC_BDQ_BATCH_SIZE in mpnic_txrx.h gives a different
> reason for batching:
>
>  * doorbell. The device consumes whatever the doorbell points at, this is
>  * purely to keep the driver from writing the CSR for every descriptor.
>
> Could the two comments be made consistent with each other and with what
> the refill code actually does?
>
> [ ... ]
>
>> @@ -760,6 +1081,9 @@ void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd)
>>  		{ 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" },
>> +		{ MPNIC_HPQ_IDLE(0), MPNIC_HPQ_IDLE_CNT, "HPQ" },
>> +		{ MPNIC_PPQ_IDLE(0), MPNIC_PPQ_IDLE_CNT, "PPQ" },
>> +		{ MPNIC_RCQ_IDLE(0), MPNIC_RCQ_IDLE_CNT, "RCQ" },
>>  	};
>>  	u32 non_idle_bitmap;
>>  	int err;
>
> [Severity: Medium]
> What happens in mpnic_stop() if one of the new Rx engines is still busy
> when this wait times out?
>
> mpnic_wait_all_queues_idle() returns void. It only logs on timeout:
>
> 	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);
>
> mpnic_stop() then keeps tearing down:
>
> 	mpnic_disable(mpn);
> 	mpnic_wait_all_queues_idle(mpn->mpd);
> 	mpnic_flush(mpn);
>
> 	mpnic_reset_netif_queues(mpn);
> 	mpnic_free_resources(mpn);
>
> mpnic_flush()->mpnic_clean_bdq() hands every posted HPQ/PPQ page back to
> the pool with page_pool_put_full_page(). After that,
> mpnic_free_resources()->mpnic_free_nv_resources()->
> mpnic_free_rx_qt_resources() frees the RCQ ring with dma_free_coherent()
> and calls page_pool_destroy(), which unmaps and frees the pages.
>
> The earlier commit "eth: mpnic: start and stop the Tx HW queues" says
> disabling a queue "only stops the device picking up new work, it does
> not tell us that the work already fetched has been retired". If so, a
> DMA_FROM_DEVICE write could still be in flight after a timeout. Examples
> are descriptors the prefetcher has already fetched, or a frame the RDE
> is partway through.
>
> Could such a write land in RCQ memory or Rx pages that have already been
> unmapped and possibly reused? That would cause an IOMMU fault, or silent
> memory corruption on systems without an IOMMU. Before this patch, no
> device-written Rx buffers were covered by this best-effort wait.
>
> Whether this can happen depends on whether a non-idle Rx engine can
> still complete DMA after the timeout. fbnic_down() follows the same
> ordering. Would it make sense to return the timeout to mpnic_stop() and
> avoid freeing device-writable memory (or reset the device first) when
> the queues never go idle?
>

Same complaint addressed already.