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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] eth: mpnic: hardware pipeline deadlock from incorrect teardown sequence
- [High] eth: mpnic: DMA use-after-free due to ignored queue idle timeout
--

commit f74c6bf4e3ce87eee577108084c59c43728d6d01
Author: Daniel Zahka <daniel.zahka@gmail.com>

eth: mpnic: start and stop the Tx HW queues

This patch points the hardware at the descriptor rings and provides functions
for turning the Tx queues on and off. Teardown waits for the DMA engines to
go idle before the ring memory can be 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

[ ... ]

> @@ -460,6 +461,180 @@ void mpnic_reset_netif_queues(struct mpnic_net *mpn)
>  	}
>  }
>  
> +static void mpnic_enable_twq(struct mpnic_dev *mpd, struct mpnic_ring *twq)

[ ... ]

> +void mpnic_enable(struct mpnic_net *mpn)
> +{
> +	struct mpnic_dev *mpd = mpn->mpd;
> +	int i, j;
> +
> +	for (i = 0; i < mpn->num_napi; i++) {
> +		struct mpnic_napi_vector *nv = mpn->napi[i];
> +
> +		for (j = 0; j < nv->txt_count; j++) {
> +			mpnic_enable_twq(mpd, &nv->qt[j].sub0);
> +			mpnic_enable_tcq(mpd, nv, &nv->qt[j].cmpl);
> +		}
> +	}
> +
> +	mpnic_wrfl(mpd);
> +}

[Severity: High]
Does enabling the Transmit Work Queue (TWQ) before the Transmit Completion
Queue (TCQ) allow in-flight descriptors to complete against a disabled TCQ?

Could we swap the initialization order in mpnic_enable so the TCQ is ready
before the hardware picks up new work?

> +static void mpnic_disable_twq(struct mpnic_dev *mpd, struct mpnic_ring *txr)
> +{
> +	u64 twq_ctl = mpnic_rd64(mpd, MPNIC_TWQ_CTL(txr->q_idx, 0));
> +
> +	twq_ctl &= ~MPNIC_TWQ_CTL_ENABLE;
> +	mpnic_wr64(mpd, MPNIC_TWQ_CTL(txr->q_idx, 0), twq_ctl);
> +}
> +
> +static void mpnic_disable_tcq(struct mpnic_dev *mpd, struct mpnic_ring *txr)
> +{
> +	mpnic_wr64(mpd, MPNIC_TCQ_CTL(txr->q_idx), 0);
> +	mpnic_wr64(mpd, MPNIC_TIM_INTR_MASK(txr->q_idx),
> +		   MPNIC_TIM_INTR_MASK_MASK);
> +}
> +
> +void mpnic_disable(struct mpnic_net *mpn)
> +{
> +	struct mpnic_dev *mpd = mpn->mpd;
> +	int i, j;
> +
> +	for (i = 0; i < mpn->num_napi; i++) {
> +		struct mpnic_napi_vector *nv = mpn->napi[i];
> +
> +		for (j = 0; j < nv->txt_count; j++) {
> +			mpnic_disable_twq(mpd, &nv->qt[j].sub0);
> +			mpnic_disable_tcq(mpd, &nv->qt[j].cmpl);
> +		}
> +	}
> +
> +	mpnic_wrfl(mpd);
> +}

[Severity: High]
Does shutting down the TCQ concurrently with the TWQ stall the hardware
pipeline during teardown?

If the TWQ is disabled, packets already in the pipeline still need to be
completed. By disabling the TCQ immediately in mpnic_disable, the DMA engine
(TDE) is blocked from writing completions to host memory.

Will this prevent the pipeline from ever reaching an idle state, guaranteeing
that the subsequent wait for idle queues times out? Should we disable the
TWQ, wait for the pipeline to drain, and then disable the TCQ?

[ ... ]

> +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]
Does returning void here prevent callers from handling hardware wedges safely?

If the 500ms timeout is reached in read_poll_timeout(), mpnic_wait_all_queues_idle
logs an error but the caller will unconditionally proceed to free the DMA-mapped
descriptor rings.

Could freeing this memory while the DMA engine is still active cause a DMA
use-after-free, leading to IOMMU faults or memory corruption? Should this
function return the error so callers can either leak the memory or issue a
hard device reset if the device fails to idle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922-linux-mpnic-v1-0-236844f53072@gmail.com?part=5