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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Cached doorbell pointers let queue-head MMIO writes reach BAR0
  after the driver has marked the device as gone.
- [Low] Concern (baseline: rejected): mpnic_alloc_ring_desc() stores
  `count - 1` into the u16 ring->size_mask (line 334) without checking…
- [Low] Concern (baseline: rejected): mpnic_clean_tcq() takes HEAD0 from
  the device-written completion descriptor (line 163) and passes it…

--

eth: mpnic: implement Tx queue allocation and cleanup

This adds the Tx ring structures and allocates queue triads per NAPI
vector, along with their descriptor memory and interrupts. It also adds
Tx completion processing, where one completion reports a work queue head
and can release many packets. Nothing enables the queues in hardware yet.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> new file mode 100644
> index 0000000000000..fe360a26a27be
> --- /dev/null
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> @@ -0,0 +1,496 @@

[ ... ]

> +static void mpnic_commit_cq_head(struct mpnic_ring *cmpl)
> +{
> +	u32 head = cmpl->head;
> +
> +	/* The tail shadows the last value written to the doorbell, so a
> +	 * completion queue which has not moved costs no MMIO write.
> +	 */
> +	if (cmpl->tail != head) {
> +		cmpl->tail = head;
> +		writeq(head & cmpl->size_mask, cmpl->doorbell);
> +	}
> +}

[Severity: Low]
Can this doorbell write still reach BAR0 after the driver has marked the
device as gone?

mpnic_alloc_napi_vector() reads mpd->uc_addr0 once and stores raw BAR0
addresses in the rings:

    mpnic_ring_init(&nv->qt[0].sub0, &uc_addr[MPNIC_TWQ_TAIL(idx, 0)], idx);
    mpnic_ring_init(&nv->qt[0].cmpl, &uc_addr[MPNIC_TCQ_HEAD(idx)], idx);

When mpnic_rd64() sees all-ones reads, mpnic_mmio_err() in mpnic_pci.c
clears only the accessor pointer:

    WRITE_ONCE(mpd->uc_addr0, NULL);

BAR0 stays mapped through pcim_iomap_region(). mpnic_wr64() checks for
the NULL pointer, but mpnic_commit_cq_head() always writes through
cmpl->doorbell.

So suppose one mpnic_poll() run still has unprocessed completions that
were posted before the failure. The TCQ head doorbell write goes out,
while the mpnic_nv_irq_rearm() write through mpnic_wr64() is dropped.

The scaffolding commit says that once the device is gone, no further
accesses reach the bus. Should that also cover the doorbells?

Later in the series, mpnic_tx_doorbell() and __mpnic_bdq_commit_tail()
use the same cached-pointer pattern. Would a mpnic_present() check help
in those places and here?

The comment in mpnic_alloc_napi_vector() already says the doorbells
cannot notice the window going away, and fbnic does the same thing. The
practical effect is probably limited to posted writes to a dead link.

> +static void mpnic_clean_tcq(struct mpnic_napi_vector *nv,
> +			    struct mpnic_q_triad *qt, int napi_budget)
> +{

[ ... ]

> +	while ((*raw_tcd & cpu_to_le64(MPNIC_TCD_DONE)) == done) {
> +		u64 tcd;
> +
> +		dma_rmb();
> +
> +		tcd = le64_to_cpu(*raw_tcd);
> +		head0 = FIELD_GET(MPNIC_TCD_TYPE0_HEAD0, tcd);

[ ... ]

> +	cmpl->head = head;
> +
> +	if (head0 >= 0)
> +		mpnic_clean_twq0(nv, napi_budget, &qt->sub0, false, head0);
> +}

[Severity: Low]
This isn't a bug with a conforming device, but head0 comes straight from
the completion descriptor the device writes. It goes to
mpnic_clean_twq0() without any check against sub0.tail.

If the device ever reported a head past the posted tail,
mpnic_clean_twq0() would reach an empty slot and dereference NULL:

    struct sk_buff *skb = ring->tx_buf[head];
    ...
    desc_cnt = MPNIC_XMIT_CB(skb)->desc_count;

The completion walk loop is also not bounded by the NAPI budget. In
practice the device cannot post past the CQ head, and that head is only
committed after the loop.

Stale entries from an earlier bringup do not look reachable. The
descriptor memory is freshly zeroed by dma_alloc_coherent(), the head
and tail are reset to 0 later in the series, and at head 0 the expected
DONE bit is 1.

Would a cheap sanity check on head0, or a NULL check on skb, be worth
adding as hardening?

[ ... ]

> +static int mpnic_alloc_ring_desc(struct mpnic_net *mpn,
> +				 struct mpnic_ring *ring, u32 count)
> +{

[ ... ]

> +	ring->size_mask = count - 1;
> +	ring->size = size;

[Severity: Low]
This isn't a bug with the current callers, but count is a u32 while
ring->size_mask is a u16 in struct mpnic_ring. Nothing here checks that
count is a non-zero power of two no larger than 65536.

This cannot be reached today. The only caller passes mpn->txq_size,
which is only ever set to MPNIC_TXQ_SIZE_DEFAULT (1024), and there is
no set_ringparam path.

If a runtime ring size setter is added later, a count that is zero, not
a power of two, or above 65536 would break the masked indexes in
mpnic_clean_twq0() and mpnic_clean_tcq().

Would a WARN_ON_ONCE() or an explicit check here be worthwhile?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-linux-mpnic-v1-0-236844f53072%40gmail.com