Re: [PATCH net-next v6 07/15] net: ethernet: cortina: Preserve in-flight free queue pages

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: cortina: Preserve in-flight free queue pages

This patch stops indexing free queue page metadata by ring position.
Metadata slots now come from a pool tracked by a bitmap, and each fragment's
DMA address stays in the XArray until software claims it. Each posted
fragment holds its own page reference, the page is unmapped once both
fragments are claimed, and free queue sizing now accounts for both RX rings.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index c1c42e1f93f42..9602429388b16 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -910,88 +955,146 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,

[ ... ]

> -static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
> +static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
>  {

[ ... ]

> +	/* Loop over the freeq ring buffer entries */
> +	for (;;) {
> +		dma_addr_t page_mapping;
> +		struct page *page;
> +		int ret;
>  
> -	spin_lock_irqsave(&geth->freeq_lock, flags);
> +		spin_lock_irqsave(&geth->freeq_lock, flags);
> +		rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
> +		rptr = rw.bits.rptr & ring_mask;
> +		wptr = rw.bits.wptr & ring_mask;
> +		space = CIRC_SPACE(wptr, rptr, len) >= fragments;
> +		spin_unlock_irqrestore(&geth->freeq_lock, flags);
> +		if (!space)
> +			break;
>  
> -	rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
> -	pn = (refill ? rw.bits.wptr : rw.bits.rptr) >> fpp_order;
> -	pn &= m_pn;
> -	epn = (rw.bits.rptr >> fpp_order) - 1;
> -	epn &= m_pn;
> +		ret = geth_freeq_map_page(geth, &page, &page_mapping);
> +		if (ret)
> +			break;

[Severity: Low]
This isn't a functional bug, but since this patch has a Fixes: tag, I have
a couple of questions about how it is meant to go to stable.

geth_freeq_claim() depends on freeq_mappings and
geth_freeq_mapping_index(). Both come from the earlier "Index free queue
fragments with XArray" patch, which has no Fixes: tag. Would this patch
apply and build on its own in a stable tree without that prerequisite?

This patch also removes position-based recycling. Every refill now maps a
fresh page through geth_freeq_map_page(), and geth_freeq_claim() unmaps
and frees each page once its last fragment is claimed.

The commit message says "A later patch restores recycling independently of
queue position". The later "Recycle claimed free queue pages" patch in this
series does that, and it is also tagged Fixes: 4d5ae32f5e1e. Should the
two patches be kept together for backports, so stable doesn't end up with
only the allocate-on-every-refill state?

[ ... ]

> @@ -1595,7 +1687,7 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  			goto err_drop;
>  		}
>  
> -		page = geth_freeq_lookup(geth, mapping, &page_offs);
> +		page = geth_freeq_claim(geth, mapping, &page_offs);
>  		if (!page) {
>  			dev_err_ratelimited(geth->dev,
>  					    "could not find mapping\n");

[Severity: Medium]
Can free queue pages leak if the hardware uses a posted buffer but the
driver never claims it?

With this patch, three things are released together: the page's slot in
freeq_page_bitmap, its streaming DMA mapping and its per-fragment
references. That happens only when geth_freeq_claim() sees every
fragment's exact DMA address, or when geth_freeq_release_pages() tears
down the whole free queue:

geth_freeq_claim() {
    ...
	if (!--gpage->fragments) {
		...
		__clear_bit(slot, geth->freeq_page_bitmap);
		put_page(page);
	}
    ...
}

gmac_rx() has two paths that drop a descriptor without claiming anything.
One is the "could not find mapping" case above. The other is the zero
address case just before it:

	if (!mapping) {
		if (net_ratelimit())
			netdev_err(netdev,
				   "rxq[%u]: HW BUG: zero DMA desc\n",
				   r);
		goto err_drop;
	}

If the hardware used a free queue fragment for one of these descriptors,
that fragment is never claimed.

gmac_cleanup_rxq() only walks the RX ring between rptr and wptr. A buffer
the DMA engine took from the shared free queue but had not yet written into
the ring before gmac_stop() would also stay unclaimed.

These slots come back only when the free queue is rebuilt. That doesn't
happen while the peer port is running, or on a reopen with the same order,
because geth_resize_freeq() returns 0 early in both cases.

geth_freeq_page_slots() sizes the pool as the free queue pages plus both RX
ring sizes, which leaves little slack. Once enough slots are pinned,
geth_freeq_alloc_slot() returns -ENOSPC:

	slot = find_first_zero_bit(geth->freeq_page_bitmap,
				   geth->num_freeq_pages);
	if (slot == geth->num_freeq_pages)
		return -ENOSPC;

and geth_fill_freeq() stops refilling the shared queue. Could RX on both
ports eventually starve this way?

The old position-indexed scheme got the metadata slot back the next time
that position was refilled.

The code doesn't make it clear whether the hardware really uses a posted
buffer in these error cases, so this may not be reachable in practice. The
same behaviour seems to remain at the end of the series, where slots are
still only released through a claim followed by recycle or discard.