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

[Daniel Zahka]

On Sun Sep 27, 2026 at 8:01 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · 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] In `mpnic_stop()`
>   (drivers/net/ethernet/meta/mpnic/mpnic_netdev.c), Rx teardown frees…
>
> --
>
> eth: mpnic: implement Rx queue allocation and cleanup
>
> This commit adds the Rx side of the mpnic queue triad: a header buffer
> queue, a payload buffer queue and a completion queue. All three share
> one page pool per triad. It also adds the enable/disable, fill and
> flush paths, and adds the HPQ, PPQ and RCQ blocks to the idle wait done
> on teardown.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index b1fb0d3902a7a..389a9e5476c6e 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>
> [ ... ]
>
>> @@ -767,6 +1084,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;
>> @@ -779,15 +1099,31 @@ void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd)
>>  				non_idle_bitmap, err);
>>  }
>>  
>> +static void mpnic_clean_bdq(struct mpnic_ring *bdq)
>> +{
>> +	unsigned int head = bdq->head;
>> +
>> +	while (head != bdq->tail) {
>> +		struct page *page = bdq->rx_buf[head];
>> +
>> +		page_pool_put_full_page(page->pp, page, false);
>
> [Severity: Medium]
> What happens to these Rx buffers if the idle poll times out?
> mpnic_wait_all_queues_idle() returns void. On timeout it only logs:
>
> 	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 carries on with teardown either way:
>
> 	mpnic_disable(mpn);
> 	mpnic_wait_all_queues_idle(mpn->mpd);
> 	mpnic_flush(mpn);
>
> 	mpnic_reset_netif_queues(mpn);
> 	mpnic_free_resources(mpn);
>
> Two things happen after that:
>
>   - mpnic_flush()->mpnic_clean_bdq() gives every page still posted in
>     [head, tail) back to the page pool.
>   - mpnic_free_resources()->mpnic_free_rx_qt_resources() frees the HPQ,
>     PPQ and RCQ descriptor rings with dma_free_coherent(), then calls
>     page_pool_destroy().
>
> Before this patch, the same timeout-and-continue path only covered Tx,
> where the device reads host memory. Now there are DMA_FROM_DEVICE page
> pool buffers and an RCQ that the device writes to.
>
> If the device has not quiesced when the poll gives up, could it write
> frame data into freed or reused pages, or write completions into the
> freed RCQ ring?
>
> Separately, the earlier commit "eth: mpnic: start and stop the Tx HW
> queues" says every block a packet passes through must report idle
> before memory is safe to free. The Rx idle table here covers HPQ, PPQ
> and RCQ, but it has no Rx DMA engine entry to match TDE on the Tx side.
> The only RDE registers defined are CFG, CTL and the MEM_INIT ones.
>
> Does RCQ reporting idle guarantee that the RDE has no writes in flight?
> If not, should an RDE idle entry be polled here as well?
>
> This code is still the same at the end of the series, in "eth: mpnic:
> add basic Rx handling".
>
>> +
>> +		head++;
>> +		head &= bdq->size_mask;
>> +	}
>> +
>> +	bdq->head = head;
>> +}
>
> [ ... ]

Same feedback addressed in v1 series.