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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1 (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] geth_setup_freeq() sets the software write pointer to the
  hardware read pointer kept from the previous queue, using…
- [Low] geth_setup_freeq() allocates geth->freeq_pages with
  kzalloc_objs() sized by page_slots.

Pre-existing issues:
- [Low] The kernel-doc for geth_fill_freeq() says 'Usually the interrupt
  to refill the queue happens when the queue is half empty.' The SWFQ…

--

net: ethernet: cortina: Preserve in-flight free queue pages

This patch allocates free queue page metadata slots independently of free
queue positions. Occupied slots are tracked with a bitmap, and each fragment
DMA address stays in the XArray until its RX descriptor is claimed. The patch
also sizes the metadata pool for the free queue plus both RX rings, and
requires the initial fill to populate every usable entry.

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

[ ... ]

> @@ -910,20 +952,20 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,

[ ... ]

>  /**
>   * geth_fill_freeq() - Fill the freeq with empty fragments to use
>   * @geth: the ethernet adapter
> - * @refill: whether to reset the queue by filling in all freeq entries or
> - * just refill it, usually the interrupt to refill the queue happens when
> - * the queue is half empty.
> + *
> + * Usually the interrupt to refill the queue happens when the queue is half
> + * empty.
>   */

[Severity: Low]
This problem was not introduced by this patch. The "half empty" wording comes
from the old kernel-doc, and it doesn't seem to match the code.
geth_setup_freeq() programs the SWFQ empty interrupt with:

    qt.bits.swfq_empty = 32;

gemini.h documents that field as the "Software Free Queue Empty Threshold".

Only the NAPI refill in gmac_napi_poll() uses a half-queue threshold:

    freeq_threshold = 1 << (geth->freeq_order - 1);

gemini_port_irq_thread() has the same "half empty" comment. Could both
comments be fixed while this kernel-doc is being rewritten?

[ ... ]

> @@ -1003,27 +1106,22 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
>  	}
>  
>  	/* Allocate page metadata referenced by the mapping index */
> -	geth->freeq_pages = kzalloc_objs(*geth->freeq_pages, pages);
> +	geth->freeq_pages = kzalloc_objs(*geth->freeq_pages, page_slots);

[Severity: Low]
Could this allocation become hard to satisfy at larger ring sizes?

page_slots now includes both RX ring sizes, added in
geth_freeq_page_slots():

    if (geth->port0 && geth->port0->netdev)
        slots += 1 << geth->port0->rxq_order;
    if (geth->port1 && geth->port1->netdev)
        slots += 1 << geth->port1->rxq_order;

struct gmac_queue_page has also grown to 12 bytes on 32-bit Gemini.

At the ethtool maximum (rxq_order 15 on both ports, freeq_order 15) this is
16384 + 32768 + 32768 = 81920 entries. That is about 960 KiB of physically
contiguous kmalloc memory (order 8). Before this patch it was 128 KiB.

With rxq_order 11 on both ports it needs 96 KiB (order 5). The old code
needed 32 KiB.

If the allocation fails under fragmentation, geth_resize_freeq() fails, and
gmac_open() or the ethtool -G request returns -ENOMEM. Only the CPU accesses
this array, so would kvcalloc() and kvfree() be a better fit?

>  	if (!geth->freeq_pages)
>  		goto err_freeq;
> -	geth->num_freeq_pages = pages;
> -

[ ... ]

> +	geth->freeq_page_bitmap = bitmap_zalloc(page_slots, GFP_KERNEL);
> +	if (!geth->freeq_page_bitmap)
> +		goto err_freeq_pages;
> +	geth->num_freeq_pages = page_slots;
>  
>  	/* Match the software pointer to the retained hardware pointer. */
>  	rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
>  	SET_WPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG, rptr & (len - 1));
>  
> -	filled = geth_fill_freeq(geth, false);
> -	if (!filled)
> -		goto err_freeq_alloc;
> +	expected = len - (1 << fpp_order);
> +	filled = geth_fill_freeq(geth);
> +	if (filled != expected)
> +		goto err_freeq_bitmap;

[Severity: Medium]
Can the first page posted here straddle the hardware read pointer?

rptr is the read-only hardware SWFQ read pointer left over from the previous
queue. geth_cleanup_freeq() only sets wptr = rptr, and geth_resize_freeq()
keeps rptr through:

    new_order = max_t(unsigned int, new_order, fls(rptr));

If the hardware consumed an odd number of 2K fragments before the resize,
rptr is odd. geth_fill_freeq() then rounds the write pointer down to a page
index:

    pn = (rw.bits.wptr >> fpp_order) & m_pn;

geth_freeq_add_page() writes both fragments at freeq_ring + (pn << fpp_order).
So fragment 0 ends up in entry rptr - 1, behind the hardware read pointer.

For example, take len = 16 and rptr = 5:

- Page A goes into entries 4 and 5, and the fill ends at wptr = 2.
- The hardware reads entries 5..15, then 0 and 1.
- A later refill at rptr >= 8 writes page B into entries 4 and 5.
- The fragment 0 address of page A never comes back in an RX descriptor.

geth_freeq_claim() is the only path that releases a slot, and only when
gpage->fragments drops to zero. Does page A then keep its XArray entry, DMA
mapping, page references and freeq_page_bitmap bit until the queue is torn
down?

The old position-indexed code dropped the stale page when that position was
refilled, so this looks new with this patch. The later recycle patch in the
series only recycles pages whose fragments count reaches 0, so the problem
seems to remain at the end of the series.

The check above also counts the fragment that the hardware cannot see:

    expected = len - (1 << fpp_order);

So it passes in this case. Is this sentence in the commit message accurate
when rptr is not page aligned?

    Require the initial fill to populate every usable
    queue entry.

The impact looks bounded to one page and one slot per free queue lifetime.
Later refills always write page-aligned wptr values, and each setup gets a
fresh bitmap. It does use up one slot of the headroom computed by
geth_freeq_page_slots().

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-gemini-ethernet-fixes-3-v5-0-b5c9e2f7b8a3%40kernel.org