Re: [PATCH net-next v7 04/18] net: ethernet: cortina: Normalize free queue pointers on rebuild

[netdev-bot+sashiko@kernel.org]

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] geth_resize_freeq() now sets a minimum queue order from the
  hardware read pointer: `new_order = max_t(unsigned int, new_order,…

--

net: ethernet: cortina: Normalize free queue pointers on rebuild

This commit masks the software free queue page indices to the active
queue order. It also sets the software write pointer from the retained
hardware read pointer before a rebuilt ring is filled. Finally,
geth_resize_freeq() now raises the shared free queue order so the
replacement ring is big enough to hold the retained read pointer.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 5fdbe987dc975..fa51f2b54073f 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1099,6 +1106,10 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	}
>  
>  	new_order = min(15, ilog2(new_size - 1) + 1);
> +	rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
> +	if (rptr >= BIT(15))
> +		return -EIO;
> +	new_order = max_t(unsigned int, new_order, fls(rptr));

[Severity: Medium]

Can this floor push the shared free queue up to order 15 whatever ring
sizes were requested?

Any retained read pointer from 16384 to 32767 gives fls(rptr) == 15. At
order 15 the ring has 32768 descriptors. With 4K pages and 2K fragments,
geth_setup_freeq() then needs 16384 pages (about 64 MiB), allocated one
at a time:

geth_resize_freeq()
  geth_setup_freeq()
    for (pn = 0; pn < pages; pn++)
      geth_freeq_alloc_map_page()
        page = alloc_page(GFP_ATOMIC);

This runs in process context under RTNL, but GFP_ATOMIC cannot do direct
reclaim. Gemini boards usually have 32 to 128 MiB of RAM. A later patch
in this series, "Scale Gemini RX queues to system memory", seems to
address the memory pressure from these queues.

The pointer can be inherited from firmware, or carried over from an
earlier driver instance across rmmod/insmod. If the allocation then
fails, freeq_ring stays NULL. DMA is stopped, so the hardware read
pointer does not move.

Won't every later gmac_open() or ethtool -G compute the same floor and
fail again? gmac_open() treats any error other than -EBUSY as fatal:

	if (err && (err != -EBUSY)) {
		netdev_err(netdev, "could not resize freeq\n");
		goto err_stop_phy;
	}

Both ports share this queue. Would networking on both ports stay down
until the hardware pointer changes? In the same way, a retained pointer
of BIT(15) or more makes every open return -EIO.

The commit message says:

    A requested shrink may therefore retain a larger shared queue. A high
    pointer inherited from firmware can impose the same allocation floor.
    If allocation fails, leave the queue disabled and return the error;

It does not say that the floor can reach 64 MiB of GFP_ATOMIC page
allocations. It also does not say that the failure persists across
retries.

The design also assumes the read pointer cannot be reset. The commit
message itself says this is unverified. gemini_ethernet_init() already
writes the whole register:

	writel(0, geth->base + GLOBAL_SWFQ_RWPTR_REG);

If that write does reset the read pointer, is the floor still needed for
the firmware-inherited case? If it does not, does that write have any
effect on the read pointer half?

>  	dev_dbg(geth->dev, "set shared queue to size %d order %d\n",
>  		new_size, new_order);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-gemini-ethernet-fixes-3-v7-0-9c32a1a46aec%40kernel.org