[PATCH net-next v2 00/11] net: ethernet: cortina: Fix Gemini RX buffer management

[PATCH net-next v2 00/11] net: ethernet: cortina: Fix Gemini RX buffer management

[Linus Walleij]

The Gemini ethernet uses a custom software free queue shared by both
ports. Its page metadata, DMA mappings and fragment references are not
managed consistently as pages move through the free queue and RX queues.

Correct the DMA address bookkeeping and synchronization.

Track exact fragment mappings with an XArray and keep
in-flight page metadata in bitmap-allocated slots until all hardware
references have been claimed.

Serialize queue resize against the refill interrupt, harden fragment
validation and teardown, then scale the default queue sizes for systems
with 32 or 64 MiB of RAM.

This series wasn't so complex to begin with but Sashiko and the other
LLMs just find more and more corner cases. It's for the better I
guess.

Before these patches the network driver would crash under strong load
but it does not happen any more after. Tested on the D-Link DIR-685
playing back media and transfering new media using ksmbd while
issuing ping storms.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
Changes in v2:
- Replace the page_pool conversion with several smaller fixes using the
  existing free queue page allocator.
- Split DMA bookkeeping, XArray lookup, bitmap allocation, cursor rotation,
  synchronization, validation and teardown into individual patches.
- Drain the threaded free queue interrupt before resizing the shared queue.
- Allocate and DMA-map refill pages outside the IRQ-disabled free queue
  lock.
- Reject and unwind partial initial free queue fills, reset their hardware
  pointers and leave the refill interrupt masked so setup can be retried.
- Avoid full-page CPU synchronization after fragments enter the network
  stack.
- Bound RX lengths to the posted fragment and account rejected lengths as
  receive errors.
- Keep the fixes, and keep the add-on RAM sizing patch.
- Link to v1: https://patch.msgid.link/20260920-gemini-ethernet-fixes-3-v1-0-3a2a50a83d89@kernel.org

---
Linus Walleij (11):
      net: ethernet: cortina: Keep shared free queue parent-owned
      net: ethernet: cortina: Drain free queue IRQ before resize
      net: ethernet: cortina: Correct free queue DMA mappings
      net: ethernet: cortina: Index free queue fragments with XArray
      net: ethernet: cortina: Preserve in-flight free queue pages
      net: ethernet: cortina: Rotate free queue page allocation
      net: ethernet: cortina: Synchronize RX fragments for the CPU
      net: ethernet: cortina: Validate RX fragment lengths
      net: ethernet: cortina: Release partial RX frames on stop
      net: ethernet: cortina: Scale Gemini RX queues to system memory
      net: ethernet: cortina: Use guard helpers for locking

 drivers/net/ethernet/cortina/gemini.c | 577 +++++++++++++++++++++-------------
 1 file changed, 358 insertions(+), 219 deletions(-)
---
base-commit: 2ff4ca33660706f4092866b220eb0c94940f7e0c
change-id: 20260919-gemini-ethernet-fixes-3-f0403653f23a

Best regards,
--  
Linus Walleij <linusw@kernel.org>

[PATCH net-next v2 01/11] net: ethernet: cortina: Keep shared free queue parent-owned

[Linus Walleij]

The software free queue is shared by both Ethernet ports, but each
child removal tears it down. The parent removal then tears it down once
more. This can destroy the queue while the sibling port is active and can
free it repeatedly.

Depopulate both port devices before cleaning up the queue, and perform
that cleanup only from the parent. Disable the free-queue interrupt after
the managed handlers have drained, since its threaded handler can
re-enable it. Guard cleanup for probe and removal paths where no queue was
allocated.

Reported-by: Myeonghun Pak <mhun512@gmail.com>
Resolves: https://lore.kernel.org/netdev/20260917192835.58126-1-mhun512@gmail.com/
Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 2fe7fd0202d2..31bcd41c17fa 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1004,6 +1004,9 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 	unsigned int pages = len >> fpp_order;
 	unsigned int pn;
 
+	if (!geth->freeq_ring)
+		return;
+
 	writew(readw(geth->base + GLOBAL_SWFQ_RWPTR_REG),
 	       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
 	writel(0, geth->base + GLOBAL_SW_FREEQ_BASE_SIZE_REG);
@@ -2391,7 +2394,6 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
 		unregister_netdev(port->netdev);
 	}
 	clk_disable_unprepare(port->pclk);
-	geth_cleanup_freeq(port->geth);
 }
 
 static void gemini_ethernet_init(struct gemini_ethernet *geth)
@@ -2683,6 +2685,8 @@ static void gemini_ethernet_remove(struct platform_device *pdev)
 {
 	struct gemini_ethernet *geth = platform_get_drvdata(pdev);
 
+	devm_of_platform_depopulate(&pdev->dev);
+	writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
 	geth_cleanup_freeq(geth);
 	geth->initialized = false;
 }

-- 
2.55.0

Re: [PATCH net-next v2 01/11] net: ethernet: cortina: Keep shared free queue parent-owned

[netdev-bot+sashiko]

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] The commit message is incomplete for a stable-relevant memory-
  safety fix.
- [Low] At this commit, the new guard `if (!geth->freeq_ring) return;` in
  geth_cleanup_freeq() treats NULL as meaning 'no queue'.
- [Low] The patch moves all shared global-register teardown in the remove
  path so it runs after devm_of_platform_depopulate().
- [Low] The concern is that GLOBAL_INTERRUPT_SELECT_4_REG (0xFF000003,
  line 2429) routes the free-queue-empty interrupt to int1 (port1's…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  geth->port0/geth->port1 are written only in…
- [High] This problem wasn't introduced by this patch, but
  gemini_port_remove() calls phy_disconnect(port->netdev->phydev) before…
- [High] At this commit, geth_cleanup_freeq() forces each free-queue
  page's refcount to zero with `while (page_ref_count(gpage->page) > 0)…

--

net: ethernet: cortina: Keep shared free queue parent-owned

This commit stops each Gemini port child device from tearing down the
software free queue that both ports share. Only the parent's
gemini_ethernet_remove() now does the cleanup, after it depopulates the
children and disables the free-queue interrupt. It also adds a NULL check
on freeq_ring in geth_cleanup_freeq().

Resolves: https://lore.kernel.org/netdev/20260917192835.58126-1-mhun512@gmail.com/

> net: ethernet: cortina: Keep shared free queue parent-owned
>
> The software free queue is shared by both Ethernet ports, but each
> child removal tears it down. The parent removal then tears it down once
> more. This can destroy the queue while the sibling port is active and can
> free it repeatedly.

[ ... ]

> Reported-by: Myeonghun Pak <mhun512@gmail.com>
> Resolves: https://lore.kernel.org/netdev/20260917192835.58126-1-mhun512@gmail.com/

[Severity: Low]
Is the order described here accurate for the common case?

Before this patch, gemini_ethernet_remove() called geth_cleanup_freeq()
first, while both netdevs were still registered. Devres then depopulated
the children, and each gemini_port_remove() freed the queue again. So a
plain rmmod or parent unbind could free the queue up to three times, not
only after a single-port unbind.

This fixes a repeated free of the shared queue. Should it carry a Fixes:
tag? The per-child geth_cleanup_freeq(port->geth) call removed here was
added by:

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")

Also, Resolves: isn't a standard trailer. Should it be Closes:, placed
directly after Reported-by:? That is what checkpatch expects.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 2fe7fd0202d26..31bcd41c17fad 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1004,6 +1004,9 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  	unsigned int pages = len >> fpp_order;
>  	unsigned int pn;
>  
> +	if (!geth->freeq_ring)
> +		return;
> +

[Severity: Low]
This guard treats a NULL freeq_ring as "no queue allocated". However,
geth_cleanup_freeq() itself does not keep that rule:

	kfree(geth->freeq_pages);

	dma_free_coherent(geth->dev,
			  sizeof(*geth->freeq_ring) << geth->freeq_order,
			  geth->freeq_ring, geth->freeq_dma_base);

Both freeq_pages and freeq_ring are left dangling afterwards. Only the
error path in geth_setup_freeq() sets freeq_ring to NULL. Can the guard
detect a queue that has already been cleaned up?

No second cleanup call seems reachable at this commit. The later patch in
this series, "net: ethernet: cortina: Preserve in-flight free queue
pages", clears freeq_ring, freeq_pages, freeq_page_bitmap and
num_freeq_pages at the end of geth_cleanup_freeq() and on every
geth_setup_freeq() error path.

>  	writew(readw(geth->base + GLOBAL_SWFQ_RWPTR_REG),
>  	       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
>  	writel(0, geth->base + GLOBAL_SW_FREEQ_BASE_SIZE_REG);

[Severity: High]
This isn't a bug introduced by this patch. Further down in
geth_cleanup_freeq(), the refcount of every free queue page is forced to
zero:

		gpage = &geth->freeq_pages[pn];
		while (page_ref_count(gpage->page) > 0)
			put_page(gpage->page);

Does this also drop the references held by skb fragments? gmac_rx() hands
these pages to the stack through skb_fill_page_desc() and
napi_gro_frags(). Those skbs can outlive the queue, so this looks like a
use-after-free of in-flight RX pages.

This is fixed later in the series by "net: ethernet: cortina: Preserve
in-flight free queue pages". There, geth_freeq_release_pages() drops only
the outstanding per-fragment hardware references and the queue's base
reference. The related DMA map/unmap size mismatch in this function is
fixed by "net: ethernet: cortina: Correct free queue DMA mappings".

[ ... ]

> @@ -2391,7 +2394,6 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
>  		unregister_netdev(port->netdev);
>  	}
>  	clk_disable_unprepare(port->pclk);
> -	geth_cleanup_freeq(port->geth);
>  }

[Severity: High]
This is a pre-existing issue. The new devm_of_platform_depopulate() call
in gemini_ethernet_remove() now leads straight here. What happens if the
interface is still up when it is removed?

gemini_port_remove() does:

	if (port->netdev) {
		phy_disconnect(port->netdev->phydev);
		unregister_netdev(port->netdev);
	}

phy_disconnect()->phy_detach()->phy_detach_internal() runs:

	phydev->attached_dev->phydev = NULL;

For an IFF_UP interface, unregister_netdev() then calls ndo_stop:

gemini_ethernet_remove()
  devm_of_platform_depopulate()
    gemini_ethernet_port_remove()
      gemini_port_remove()
        phy_disconnect()              /* netdev->phydev = NULL */
        unregister_netdev()
          gmac_stop()
            phy_stop(netdev->phydev)  /* NULL */

Can phy_stop() oops when it dereferences phydev->attached_dev?

Unbinding a single port reaches the same path. The order dates back to
3e813d61401a ("net: gemini: Clean up phy registration") and is unchanged
at the end of the series.

[Severity: High]
This isn't a bug introduced by this patch. Now that a sibling port is
meant to keep running after the other port is removed, what happens to
geth->port0 and geth->port1?

They are only set in gemini_ethernet_port_probe():

	if (!id)
		geth->port0 = port;
	else
		geth->port1 = port;

Nothing clears them: not gemini_port_remove(), and not the probe error
paths after the assignment (devm_request_threaded_irq(),
gmac_setup_phy() or register_netdev() failure).

The port struct is the netdev_priv() of a netdev allocated with
devm_alloc_etherdev_mqs() on the port device. It is freed when that port
unbinds or fails probe.

geth_resize_freeq() then dereferences the sibling unconditionally:

	if (netdev->dev_id == 0)
		other_netdev = geth->port1->netdev;
	else
		other_netdev = geth->port0->netdev;

It follows up with netif_running(other_netdev) and
other_port->rxq_order. Could this be a use-after-free after, for
example, a sysfs unbind of one port followed by ip link set up or
ethtool -G on the surviving port?

The same seems possible in two other cases:

- port0 fails probe (for example, no PHY is found) and eth1 is then
  brought up.
- During depopulate, one child has been freed while the other is still
  registered.

These dereferences are still there at the end of the series, and more are
added: synchronize_irq(geth->port1->irq) in geth_resize_freeq(), and
geth->port0->rxq_order / geth->port1->rxq_order in geth_setup_freeq().

[Severity: Low]
gemini_ethernet_init() routes the FreeQ interrupt to int1, which is
port1's line:

	writel(0xFF000003, geth->base + GLOBAL_INTERRUPT_SELECT_4_REG);

With the per-port cleanup gone, unbinding only port1 leaves
SWFQ_EMPTY_INT_BIT enabled while port0 keeps using the shared queue. No
handler is left on int1. Is that a problem in practice?

>From the code it looks harmless:

- port0's gemini_port_irq() is registered with IRQF_SHARED and checks the
  global SWFQ_EMPTY bit in GLOBAL_INTERRUPT_STATUS_4_REG, whatever the
  routing.
- gmac_napi_poll() also refills the queue through geth_fill_freeq().
- free_irq() masks port1's line at the interrupt controller.

Before this patch, the same scenario was a use-after-free of the freed
queue.

[ ... ]

> @@ -2683,6 +2685,8 @@ static void gemini_ethernet_remove(struct platform_device *pdev)
>  {
>  	struct gemini_ethernet *geth = platform_get_drvdata(pdev);
>  
> +	devm_of_platform_depopulate(&pdev->dev);
> +	writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
>  	geth_cleanup_freeq(geth);

[Severity: Low]
When these global register writes run, both children have already gone
through gemini_port_remove() and called clk_disable_unprepare(port->pclk).

The gmac0-gate and gmac1-gate clocks in drivers/clk/clk-gemini.c are
registered with flags 0, so they really do switch off. The parent node in
gemini.dtsi has no clocks property.

Does the global register block at 0x60000000 depend on either GMAC gate?
If so, some accesses could be lost or could stall the bus:

- the writel() to GLOBAL_INTERRUPT_ENABLE_4_REG here
- the GLOBAL_SWFQ_RWPTR_REG and GLOBAL_SW_FREEQ_BASE_SIZE_REG accesses in
  geth_cleanup_freeq()

Before this patch, the parent's cleanup ran while those clocks were still
enabled.

This may be fine. gemini_ethernet_probe() already reads
GLOBAL_TOE_VERSION_REG before any child enables PCLK, which suggests the
global block has its own clock.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org

[PATCH net-next v2 02/11] net: ethernet: cortina: Drain free queue IRQ before resize

[Linus Walleij]

Masking the software free queue interrupt does not stop a threaded
handler which was already woken. That handler can refill the queue while
resize frees its ring and page metadata, then re-enable the interrupt
after teardown.

Both net devices are stopped while the shared queue is resized. Drain
the interrupt routed through port 1 after masking it, then mask it again
in case the threaded handler re-enabled it before completing.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 31bcd41c17fa..e5531e41ae9a 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1030,6 +1030,21 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 			  geth->freeq_ring, geth->freeq_dma_base);
 }
 
+static void geth_set_freeq_irq(struct gemini_ethernet *geth, bool enable)
+{
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&geth->irq_lock, flags);
+	val = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
+	if (enable)
+		val |= SWFQ_EMPTY_INT_BIT;
+	else
+		val &= ~SWFQ_EMPTY_INT_BIT;
+	writel(val, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
+	spin_unlock_irqrestore(&geth->irq_lock, flags);
+}
+
 /**
  * geth_resize_freeq() - resize the software queue depth
  * @port: the port requesting the change
@@ -1047,8 +1062,6 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	struct net_device *other_netdev;
 	unsigned int new_size = 0;
 	unsigned int new_order;
-	unsigned long flags;
-	u32 en;
 	int ret;
 
 	if (netdev->dev_id == 0)
@@ -1079,13 +1092,10 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	if (geth->freeq_order == new_order)
 		return 0;
 
-	spin_lock_irqsave(&geth->irq_lock, flags);
-
-	/* Disable the software queue IRQs */
-	en = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
-	en &= ~SWFQ_EMPTY_INT_BIT;
-	writel(en, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
-	spin_unlock_irqrestore(&geth->irq_lock, flags);
+	/* The threaded handler can already be running and re-enable the IRQ. */
+	geth_set_freeq_irq(geth, false);
+	synchronize_irq(geth->port1->irq);
+	geth_set_freeq_irq(geth, false);
 
 	/* Drop the old queue */
 	if (geth->freeq_ring)
@@ -1099,10 +1109,7 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	 * after probe(), this is where the interrupts get turned on
 	 * in the first place.
 	 */
-	spin_lock_irqsave(&geth->irq_lock, flags);
-	en |= SWFQ_EMPTY_INT_BIT;
-	writel(en, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
-	spin_unlock_irqrestore(&geth->irq_lock, flags);
+	geth_set_freeq_irq(geth, true);
 
 	return ret;
 }

-- 
2.55.0

Re: [PATCH net-next v2 02/11] net: ethernet: cortina: Drain free queue IRQ before resize

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 2 (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] The new sequence in geth_resize_freeq() (lines 1096-1098) is
  mask, synchronize_irq(port1->irq), mask.
- [Medium] At this commit, geth_resize_freeq() calls
  geth_set_freeq_irq(geth, true) (line 1112) without checking the return…
- [Low] The commit fixes a use-after-free race, but it has no Fixes:
  trailer.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  geth_resize_freeq() assumes the peer port object is valid.
- [Medium] This problem wasn't introduced by this patch, but at this
  commit geth_resize_freeq() sets `geth->freeq_order = new_order` (line…
- [Low] This problem wasn't introduced by this patch, but the kernel-doc
  of geth_resize_freeq() is wrong.

--

net: ethernet: cortina: Drain free queue IRQ before resize

This commit changes how geth_resize_freeq() disables the SWFQ empty
interrupt. It used to only mask it. It now masks it, calls synchronize_irq()
on port 1's IRQ, and masks it again, so that a threaded handler which is
already running gets drained. The read-modify-write of
GLOBAL_INTERRUPT_ENABLE_4_REG moves into a new helper, geth_set_freeq_irq().

> Assisted-by: LLM
> Signed-off-by: Linus Walleij <linusw@kernel.org>

[Severity: Low]
The commit message describes a fix for a race, but there is no Fixes: tag.
In the race, gemini_port_irq_thread() refills the queue while
geth_resize_freeq() tears down freeq_ring and freeq_pages.

The mask-only sequence being replaced in geth_resize_freeq() appears to
date back to the original driver submission. Should this carry a tag so
stable and backport tooling can pick it up?

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 31bcd41c17fad..e5531e41ae9a3 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1030,6 +1030,21 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)

[ ... ]

>  /**
>   * geth_resize_freeq() - resize the software queue depth
>   * @port: the port requesting the change

[Severity: Low]
This isn't a bug introduced by this patch, but the kernel-doc for
geth_resize_freeq() says:

 * This gets called at least once during probe() so the device queue gets
 * "resized" from the hardware defaults.

The only callers appear to be gmac_open() and gmac_set_ringparam(). Neither
gemini_ethernet_port_probe() nor gemini_ethernet_init() calls it.

This patch reworks the IRQ handling in this function. Could the comment be
updated at the same time? It is also unchanged at the end of the series.

> @@ -1047,8 +1062,6 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	struct net_device *other_netdev;
>  	unsigned int new_size = 0;
>  	unsigned int new_order;
> -	unsigned long flags;
> -	u32 en;
>  	int ret;
>
>  	if (netdev->dev_id == 0)

[Severity: High]
This is a pre-existing issue, but geth_resize_freeq() dereferences the peer
port here without checking it. The patch adds another unchecked dereference
further down with synchronize_irq(geth->port1->irq).

Can geth->port1 or geth->port0 be NULL, or point to freed memory, at this
point?

If the peer ethernet-port node is missing, the pointer stays NULL. The
binding doesn't require either child node.

Several in-tree boards declare port@1 with no phy-mode or phy-handle:
gemini-dlink-dns-313, nas4210b, nas4220b, rut1xx, wbd111 and
dlink-dir-685. The ns2502 and verbatim boards inherit port@1 from
gemini.dtsi in the same state. gemini_ethernet_port_probe() publishes the
pointer before the steps that can fail:

	if (!id)
		geth->port0 = port;
	else
		geth->port1 = port;
	...
	ret = gmac_setup_phy(netdev);
	if (ret) {
		netdev_err(netdev,
			   "PHY init failed\n");
		goto unprepare;
	}

On those boards gmac_setup_phy() returns -ENODEV. Devres then frees the
netdev from devm_alloc_etherdev_mqs() that contains port, and geth->port1 is
never cleared.

Wouldn't a later ip link set eth0 up or ethtool -G eth0 then go through
gmac_open()->geth_resize_freeq() and read freed memory? That would happen
via other_netdev, netif_running(other_netdev), other_port->rxq_order, and
now geth->port1->irq.

gmac_open() also treats a spurious -EBUSY from the netif_running() check as
success. The interface would then come up without a configured free queue.

This still appears to be present at the end of the series.

[ ... ]

> @@ -1079,13 +1092,10 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	if (geth->freeq_order == new_order)
>  		return 0;

[Severity: Medium]
This isn't a bug introduced by this patch, but geth_resize_freeq() stores
the new order before geth_setup_freeq() succeeds. It doesn't restore the
old order on failure:

	geth->freeq_order = new_order;
	ret = geth_setup_freeq(geth);

geth_setup_freeq() can fail, for example in dma_alloc_coherent() or in
alloc_page(GFP_ATOMIC) inside geth_freeq_alloc_map_page(). After such a
failure freeq_ring is NULL, and GLOBAL_SW_FREEQ_BASE_SIZE_REG is still 0
from geth_cleanup_freeq().

The next gmac_open() or ethtool -G would compute the same order. Could it
then take this early return with 0, so that gmac_open() goes on to
gmac_setup_rxq() and gmac_start_dma() without a valid shared free queue?

Later in this series, "net: ethernet: cortina: Preserve in-flight free
queue pages" changes the check to the following and NULLs freeq_ring on the
setup error paths, which resolves this:

	if (geth->freeq_ring && geth->freeq_order == new_order)

>
> -	spin_lock_irqsave(&geth->irq_lock, flags);
> -
> -	/* Disable the software queue IRQs */
> -	en = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
> -	en &= ~SWFQ_EMPTY_INT_BIT;
> -	writel(en, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
> -	spin_unlock_irqrestore(&geth->irq_lock, flags);
> +	/* The threaded handler can already be running and re-enable the IRQ. */
> +	geth_set_freeq_irq(geth, false);
> +	synchronize_irq(geth->port1->irq);
> +	geth_set_freeq_irq(geth, false);

[Severity: Medium]
Does this sequence fully close the window described in the commit message?

synchronize_irq() only waits for threads that were already woken. After its
refill, the drained gemini_port_irq_thread() re-enables the interrupt
unconditionally:

	/* ACK queue interrupt */
	writel(irqmask, geth->base + GLOBAL_INTERRUPT_STATUS_4_REG);
	/* Enable queue interrupt again */
	irqmask |= readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
	writel(irqmask, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);

Suppose SWFQ_EMPTY is asserted, and the hardirq is handled after that
thread finishes but before the second geth_set_freeq_irq(geth, false). Then
gemini_port_irq() sees:

	if (val & en & SWFQ_EMPTY_INT_BIT) {

It masks the bit and returns IRQ_WAKE_THREAD. The second mask then clears a
bit that is already clear, and nothing waits for the newly woken thread.

Couldn't that thread run geth_fill_freeq() on freeq_pages and freeq_ring
while geth_cleanup_freeq() frees them? It could also race with
geth_setup_freeq() rebuilding them with the new freeq_order, and then
re-enable the interrupt after teardown.

The sequence and the unconditional re-enable in the thread appear to be
unchanged at the end of the series.

Would something other than a second mask be more robust? One option is
disable_irq(geth->port1->irq) and enable_irq() around the resize. Another is
state protected by irq_lock that stops the thread from re-enabling the
interrupt during a resize.

>
>  	/* Drop the old queue */
>  	if (geth->freeq_ring)
> @@ -1099,10 +1109,7 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	 * after probe(), this is where the interrupts get turned on
>  	 * in the first place.
>  	 */
> -	spin_lock_irqsave(&geth->irq_lock, flags);
> -	en |= SWFQ_EMPTY_INT_BIT;
> -	writel(en, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
> -	spin_unlock_irqrestore(&geth->irq_lock, flags);
> +	geth_set_freeq_irq(geth, true);

[Severity: Medium]
Should this re-enable be skipped when geth_setup_freeq() returns an error?

By this point geth_cleanup_freeq() has already done the following:

- set wptr = rptr
- written 0 to GLOBAL_SW_FREEQ_BASE_SIZE_REG
- kfree()d freeq_pages and freed the ring, without NULLing either pointer

The geth_setup_freeq() error paths kfree() the new freeq_pages without
NULLing it, and they set freeq_ring to NULL. A dma_alloc_coherent() failure
leaves the stale freeq_pages from cleanup in place.

Setup failure looks easy to hit. For example, ethtool -G ethX rx 16384 asks
for an order 15 queue backed by many alloc_page(GFP_ATOMIC) pages.

The queue has just been emptied, and resize never ACKs
GLOBAL_INTERRUPT_STATUS_4_REG. If SWFQ_EMPTY status is latched, this
re-enable wakes gemini_port_irq_thread()->geth_fill_freeq(geth, true).

Can that walk freeq_pages[pn] in freed memory using the new, larger
freeq_order? Could it call page_ref_count() and page_ref_add() on stale page
pointers, and write through the NULL or freed freeq_ring in
geth_freeq_alloc_map_page()?

Later in this series, "net: ethernet: cortina: Preserve in-flight free
queue pages" resolves this. It returns early on error before the re-enable,
and it NULLs freeq_pages and freeq_ring in the cleanup and error paths.

>
>  	return ret;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org

[PATCH net-next v2 03/11] net: ethernet: cortina: Correct free queue DMA mappings

[Linus Walleij]

The free queue maps complete pages and splits each mapping between its
fragment descriptors. The mapping variable is advanced while filling the
descriptors and that advanced address is then saved as the page mapping.

Replacement also reads the old address after overwriting the descriptor
with the new mapping.

Keep the page DMA base separate from the fragment iterator. Unmap the old
page through its saved metadata before replacing it, and use PAGE_SIZE for
every map and unmap operation.

Reject mappings that cannot fit in the 32-bit hardware descriptors and
derive fragment offsets from the saved DMA base rather than assuming
DMA addresses are page aligned.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 85 ++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index e5531e41ae9a..fa5513b53ea8 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -725,17 +725,13 @@ static int gmac_setup_rxq(struct net_device *netdev)
 }
 
 static struct gmac_queue_page *
-gmac_get_queue_page(struct gemini_ethernet *geth,
-		    struct gemini_ethernet_port *port,
-		    dma_addr_t addr)
+gmac_get_queue_page(struct gemini_ethernet *geth, dma_addr_t addr)
 {
+	unsigned int frag_len = 1 << geth->freeq_frag_order;
 	struct gmac_queue_page *gpage;
-	dma_addr_t mapping;
+	unsigned int offset;
 	int i;
 
-	/* Only look for even pages */
-	mapping = addr & PAGE_MASK;
-
 	if (!geth->freeq_pages) {
 		dev_err_ratelimited(geth->dev,
 				    "try to get page with no page list\n");
@@ -745,7 +741,12 @@ gmac_get_queue_page(struct gemini_ethernet *geth,
 	/* Look up a ring buffer page from virtual mapping */
 	for (i = 0; i < geth->num_freeq_pages; i++) {
 		gpage = &geth->freeq_pages[i];
-		if (gpage->mapping == mapping)
+		if (!gpage->page || addr < gpage->mapping)
+			continue;
+
+		offset = addr - gpage->mapping;
+		if (offset <= PAGE_SIZE - frag_len &&
+		    !(offset & (frag_len - 1)))
 			return gpage;
 	}
 
@@ -757,7 +758,7 @@ static void gmac_cleanup_rxq(struct net_device *netdev)
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
 	struct gemini_ethernet *geth = port->geth;
 	struct gmac_rxdesc *rxd = port->rxq_ring;
-	static struct gmac_queue_page *gpage;
+	struct gmac_queue_page *gpage;
 	struct nontoe_qhdr __iomem *qhdr;
 	void __iomem *dma_reg;
 	void __iomem *ptr_reg;
@@ -788,8 +789,7 @@ static void gmac_cleanup_rxq(struct net_device *netdev)
 		if (!mapping)
 			continue;
 
-		/* Freeq pointers are one page off */
-		gpage = gmac_get_queue_page(geth, port, mapping + PAGE_SIZE);
+		gpage = gmac_get_queue_page(geth, mapping);
 		if (!gpage) {
 			dev_err(geth->dev, "could not find page\n");
 			continue;
@@ -809,6 +809,7 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 	struct gmac_queue_page *gpage;
 	unsigned int fpp_order;
 	unsigned int frag_len;
+	dma_addr_t page_mapping;
 	dma_addr_t mapping;
 	struct page *page;
 	int i;
@@ -818,9 +819,17 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 	if (!page)
 		return NULL;
 
-	mapping = dma_map_single(geth->dev, page_address(page),
-				 PAGE_SIZE, DMA_FROM_DEVICE);
-	if (dma_mapping_error(geth->dev, mapping)) {
+	page_mapping = dma_map_single(geth->dev, page_address(page),
+				      PAGE_SIZE, DMA_FROM_DEVICE);
+	if (dma_mapping_error(geth->dev, page_mapping)) {
+		put_page(page);
+		return NULL;
+	}
+	if (page_mapping > U32_MAX - (PAGE_SIZE - 1)) {
+		dev_err_ratelimited(geth->dev,
+				    "freeq DMA mapping exceeds 32 bits\n");
+		dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
+				 DMA_FROM_DEVICE);
 		put_page(page);
 		return NULL;
 	}
@@ -833,20 +842,11 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 	 */
 	frag_len = 1 << geth->freeq_frag_order; /* Usually 2048 */
 	fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
-	freeq_entry = geth->freeq_ring + (pn << fpp_order);
-	dev_dbg(geth->dev, "allocate page %d fragment length %d fragments per page %d, freeq entry %p\n",
-		 pn, frag_len, (1 << fpp_order), freeq_entry);
-	for (i = (1 << fpp_order); i > 0; i--) {
-		freeq_entry->word2.buf_adr = mapping;
-		freeq_entry++;
-		mapping += frag_len;
-	}
-
 	/* If the freeq entry already has a page mapped, then unmap it. */
 	gpage = &geth->freeq_pages[pn];
 	if (gpage->page) {
-		mapping = geth->freeq_ring[pn << fpp_order].word2.buf_adr;
-		dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);
+		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
+				 DMA_FROM_DEVICE);
 		/* This should be the last reference to the page so it gets
 		 * released
 		 */
@@ -854,11 +854,21 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 	}
 
 	/* Then put our new mapping into the page table */
-	dev_dbg(geth->dev, "page %d, DMA addr: %08x, page %p\n",
-		pn, (unsigned int)mapping, page);
-	gpage->mapping = mapping;
+	gpage->mapping = page_mapping;
 	gpage->page = page;
 
+	freeq_entry = geth->freeq_ring + (pn << fpp_order);
+	dev_dbg(geth->dev, "allocate page %d fragment length %d fragments per page %d, freeq entry %p\n",
+		pn, frag_len, (1 << fpp_order), freeq_entry);
+	mapping = page_mapping;
+	for (i = (1 << fpp_order); i > 0; i--) {
+		freeq_entry->word2.buf_adr = mapping;
+		freeq_entry++;
+		mapping += frag_len;
+	}
+	dev_dbg(geth->dev, "page %d, DMA addr: %pad, page %p\n",
+		pn, &page_mapping, page);
+
 	return page;
 }
 
@@ -927,7 +937,6 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
 static int geth_setup_freeq(struct gemini_ethernet *geth)
 {
 	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
-	unsigned int frag_len = 1 << geth->freeq_frag_order;
 	unsigned int len = 1 << geth->freeq_order;
 	unsigned int pages = len >> fpp_order;
 	union queue_threshold qt;
@@ -974,12 +983,11 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 err_freeq_alloc:
 	while (pn > 0) {
 		struct gmac_queue_page *gpage;
-		dma_addr_t mapping;
 
 		--pn;
-		mapping = geth->freeq_ring[pn << fpp_order].word2.buf_adr;
-		dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);
 		gpage = &geth->freeq_pages[pn];
+		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
+				 DMA_FROM_DEVICE);
 		put_page(gpage->page);
 	}
 
@@ -999,7 +1007,6 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 {
 	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
-	unsigned int frag_len = 1 << geth->freeq_frag_order;
 	unsigned int len = 1 << geth->freeq_order;
 	unsigned int pages = len >> fpp_order;
 	unsigned int pn;
@@ -1013,12 +1020,10 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 
 	for (pn = 0; pn < pages; pn++) {
 		struct gmac_queue_page *gpage;
-		dma_addr_t mapping;
-
-		mapping = geth->freeq_ring[pn << fpp_order].word2.buf_adr;
-		dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);
 
 		gpage = &geth->freeq_pages[pn];
+		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
+				 DMA_FROM_DEVICE);
 		while (page_ref_count(gpage->page) > 0)
 			put_page(gpage->page);
 	}
@@ -1503,8 +1508,6 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 
 		frag_len = word0.bits.buffer_size;
 		frame_len = word1.bits.byte_count;
-		page_offs = mapping & ~PAGE_MASK;
-
 		if (word3.bits32 & SOF_BIT) {
 			if (skb) {
 				napi_free_frags(&port->napi);
@@ -1523,14 +1526,14 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 			goto err_drop;
 		}
 
-		/* Freeq pointers are one page off */
-		gpage = gmac_get_queue_page(geth, port, mapping + PAGE_SIZE);
+		gpage = gmac_get_queue_page(geth, mapping);
 		if (!gpage) {
 			dev_err_ratelimited(geth->dev,
 					    "could not find mapping\n");
 			goto err_drop;
 		}
 		page = gpage->page;
+		page_offs = mapping - gpage->mapping;
 
 		if (word3.bits32 & SOF_BIT) {
 			skb = gmac_skb_if_good_frame(port, word0, frame_len);

-- 
2.55.0

Re: [PATCH net-next v2 03/11] net: ethernet: cortina: Correct free queue DMA mappings

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (3) · Medium: 2 (1) · Low: 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] At this commit, the replacement path in
  geth_freeq_alloc_map_page() calls a full…
- [Medium] At this commit, gmac_rx() and gmac_cleanup_rxq() read
  gpage->page and gpage->mapping without holding geth->freeq_lock.
- [Low] This patch fixes a user-visible bug that dates back to the
  driver's introduction, but its commit message has no Fixes: tag and…

Pre-existing issues:
- [High] At this commit, geth_fill_freeq() calls
  geth_freeq_alloc_map_page(geth, pn) whenever page_ref_count(page) > 1.
- [High] At this commit, free-queue pages are mapped DMA_FROM_DEVICE once
  and never synced.
- [High] At this commit, geth_cleanup_freeq() runs 'while
  (page_ref_count(gpage->page) > 0) put_page(gpage->page);'.
- [Medium] At this commit, gmac_rx() uses lengths from the hardware
  descriptor without bounds checks.

--

net: ethernet: cortina: Correct free queue DMA mappings

geth_freeq_alloc_map_page() now keeps the page DMA base separate from the
fragment iterator. When replacing a page, it unmaps the old page through
gpage->mapping using PAGE_SIZE, and it rejects mappings that don't fit the
32-bit descriptors. gmac_get_queue_page() now matches addresses by their
offset from the saved base, and the "one page off" lookup adjustment in
gmac_rx() and gmac_cleanup_rxq() is removed.

> Replacement also reads the old address after overwriting the descriptor
> with the new mapping.

[Severity: Low]
This patch looks like it fixes a user-visible bug that dates back to the
original driver, but there is no Fixes: tag. Should it carry:

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")

Before this patch, the first population stored gpage->mapping as base +
PAGE_SIZE. The "Freeq pointers are one page off" lookup in gmac_rx() and
gmac_cleanup_rxq() compensated for that.

On replacement, the old code re-read the descriptor after it already held
the new base. It then unmapped the new buffer with frag_len even though
that buffer was handed to hardware. The old mapping was never unmapped,
and gpage->mapping was stored as the new base without the extra PAGE_SIZE.

After any replacement, gmac_rx() would either fail the lookup ("could not
find mapping", frame dropped, page reference never put) or match a
neighbouring slot and attach the fragment to the wrong struct page.

Could the commit message describe these effects as well as the mechanics?

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

[ ... ]

> @@ -818,9 +819,17 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
>  	if (!page)
>  		return NULL;
>  
> -	mapping = dma_map_single(geth->dev, page_address(page),
> -				 PAGE_SIZE, DMA_FROM_DEVICE);
> -	if (dma_mapping_error(geth->dev, mapping)) {
> +	page_mapping = dma_map_single(geth->dev, page_address(page),
> +				      PAGE_SIZE, DMA_FROM_DEVICE);

[Severity: High]
This is a pre-existing issue, but free queue pages are mapped once here
with DMA_FROM_DEVICE and never synced after that.

gmac_rx() passes page + page_offs to skb_fill_page_desc() and
napi_gro_frags() without calling dma_sync_single_range_for_cpu().

geth_fill_freeq() re-posts pages whose refcount has dropped back to 1 with
only a reference bump, and never calls dma_sync_single_for_device():

geth_fill_freeq() {
    ...
		/* Add one reference per fragment in the page */
		page_ref_add(page, 1 << fpp_order);
    ...
}

On non-coherent Gemini, can cache lines the CPU filled while reading the
previous packet hide data the device writes into the reused page?

This seems to be fixed later in the series. "net: ethernet: cortina:
Synchronize RX fragments for the CPU" adds dma_sync_single_range_for_cpu()
in geth_freeq_claim(). "net: ethernet: cortina: Preserve in-flight free
queue pages" makes geth_fill_freeq() map a fresh page every time.

> +	if (dma_mapping_error(geth->dev, page_mapping)) {

[ ... ]

> @@ -833,20 +842,11 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,

[ ... ]

>  	/* If the freeq entry already has a page mapped, then unmap it. */
>  	gpage = &geth->freeq_pages[pn];
>  	if (gpage->page) {
> -		mapping = geth->freeq_ring[pn << fpp_order].word2.buf_adr;
> -		dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);
> +		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
> +				 DMA_FROM_DEVICE);

[Severity: Medium]
This new unmap covers the whole PAGE_SIZE and does not pass
DMA_ATTR_SKIP_CPU_SYNC. It only runs when page_ref_count(page) > 1 in
geth_fill_freeq(), so other fragments of the page are still owned by the
stack or waiting in an RX ring.

Where the implicit device-to-CPU sync is a cache invalidate or a swiotlb
copy-back, can this throw away CPU writes to fragments the stack already
owns?

The old code was also wrong here, since it unmapped the new mapping. This
looks to be fixed later in the series by "net: ethernet: cortina:
Synchronize RX fragments for the CPU". That commit syncs each fragment
when it is claimed. It only tears the page down, with
dma_unmap_single_attrs(..., DMA_ATTR_SKIP_CPU_SYNC), after every fragment
has been claimed.

>  		/* This should be the last reference to the page so it gets
>  		 * released
>  		 */

[Severity: High]
This isn't a bug introduced by this patch, but geth_fill_freeq() only
replaces a page while it still has fragment references outstanding:

geth_fill_freeq() {
    ...
		if (page_ref_count(page) > 1) {
    ...
			page = geth_freeq_alloc_map_page(geth, pn);
    ...
}

Some of those references can belong to fragments whose completion
descriptors are still in either port's RX ring. That doesn't match this
comment's claim that the put_page() drops the last reference.

After gpage->mapping and gpage->page are overwritten, gmac_get_queue_page()
only knows the new mapping. A pending descriptor that carries the old
buf_adr would then fail the lookup. gmac_rx() would go to err_drop with
page == NULL, and gmac_cleanup_rxq() would print "could not find page".

Is the fragment reference on the old page leaked in both cases?

This seems to be fixed later in the series by "net: ethernet: cortina:
Preserve in-flight free queue pages". It keeps every fragment DMA address
in an XArray until geth_freeq_claim() consumes it, and it only unmaps the
page once all fragments are claimed.

[ ... ]

> @@ -1013,12 +1020,10 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  
>  	for (pn = 0; pn < pages; pn++) {
>  		struct gmac_queue_page *gpage;
> -		dma_addr_t mapping;
> -
> -		mapping = geth->freeq_ring[pn << fpp_order].word2.buf_adr;
> -		dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);
>  
>  		gpage = &geth->freeq_pages[pn];
> +		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
> +				 DMA_FROM_DEVICE);
>  		while (page_ref_count(gpage->page) > 0)
>  			put_page(gpage->page);

[Severity: High]
This is a pre-existing issue, but this loop drops references the queue
doesn't own. gmac_rx() hands fragment references to skbs through
skb_fill_page_desc(), and those skbs can outlive the queue.
geth_cleanup_freeq() runs from geth_resize_freeq() on an ethtool ring
resize, and on removal.

Can this underflow the refcount of pages still attached to skbs? Once the
last put_page() frees the page, the loop also keeps reading
page_ref_count() on memory that may already be reallocated.

This looks to be fixed later in the series by "net: ethernet: cortina:
Preserve in-flight free queue pages". There, geth_freeq_release_pages()
only puts the per-fragment references still owned by hardware plus the
base reference.

>  	}

[ ... ]

> @@ -1503,8 +1508,6 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  
>  		frag_len = word0.bits.buffer_size;
>  		frame_len = word1.bits.byte_count;
> -		page_offs = mapping & ~PAGE_MASK;
> -
>  		if (word3.bits32 & SOF_BIT) {
>  			if (skb) {
>  				napi_free_frags(&port->napi);
> @@ -1523,14 +1526,14 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  			goto err_drop;
>  		}
>  
> -		/* Freeq pointers are one page off */
> -		gpage = gmac_get_queue_page(geth, port, mapping + PAGE_SIZE);
> +		gpage = gmac_get_queue_page(geth, mapping);
>  		if (!gpage) {
>  			dev_err_ratelimited(geth->dev,
>  					    "could not find mapping\n");
>  			goto err_drop;
>  		}
>  		page = gpage->page;
> +		page_offs = mapping - gpage->mapping;

[Severity: Medium]
gmac_get_queue_page() and these two lines read gpage->mapping and
gpage->page without holding geth->freeq_lock. Meanwhile geth_fill_freeq()
can replace both fields under that lock, from the other port's NAPI poll
or from the SWFQ IRQ thread:

geth_freeq_alloc_map_page() {
    ...
	gpage->mapping = page_mapping;
	gpage->page = page;
    ...
}

What happens if the slot is replaced between the lookup and this read?
Could page_offs become a wrapped value that is then passed with the new
page to skb_fill_page_desc()?

Before this patch, page_offs came from the descriptor (mapping &
~PAGE_MASK) and always stayed within one page.

This seems to be fixed later in the series by "net: ethernet: cortina:
Index free queue fragments with XArray". It does the lookup, the
validation and the page/offset snapshot in geth_freeq_claim() while
holding freeq_lock.

[Severity: Medium]
This isn't a bug introduced by this patch, but the descriptor lengths used
further down in gmac_rx() are not bounds checked:

			page_offs += NET_IP_ALIGN;
			frag_len -= NET_IP_ALIGN;
    ...
		if (word3.bits32 & EOF_BIT)
			frag_len = frame_len - skb->len;
    ...
		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);

frag_len underflows if buffer_size is smaller than NET_IP_ALIGN, or if
byte_count is smaller than the bytes already collected.
gmac_get_queue_page() only checks where the fragment starts.

Can a malformed descriptor then produce an skb fragment that runs past the
end of the page?

This looks to be fixed later in the series by "net: ethernet: cortina:
Validate RX fragment lengths", which rejects these cases in gmac_rx().

>  
>  		if (word3.bits32 & SOF_BIT) {
>  			skb = gmac_skb_if_good_frame(port, word0, frame_len);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org

[PATCH net-next v2 04/11] net: ethernet: cortina: Index free queue fragments with XArray

[Linus Walleij]

RX descriptors return the DMA address of the free queue fragment used by
the hardware. The driver currently recovers its page by scanning every
free queue page and comparing address ranges.

This isn't very efficient...

Instead index each fragment in an XArray using its DMA address in
fragment units.

Retain the complete DMA base in the page metadata and validate the exact
fragment address after lookup. Serialize metadata replacement with RX
lookup so the caller obtains the page belonging to the descriptor even
when the queue is being refilled.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 109 ++++++++++++++++++++++++----------
 1 file changed, 79 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index fa5513b53ea8..3d729f4c34ef 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -37,6 +37,7 @@
 #include <linux/ethtool.h>
 #include <linux/tcp.h>
 #include <linux/u64_stats_sync.h>
+#include <linux/xarray.h>
 
 #include <linux/in.h>
 #include <linux/ip.h>
@@ -165,6 +166,7 @@ struct gemini_ethernet {
 	struct gmac_rxdesc *freeq_ring;
 	dma_addr_t	freeq_dma_base;
 	struct gmac_queue_page	*freeq_pages;
+	struct xarray	freeq_mappings;
 	unsigned int	num_freeq_pages;
 	spinlock_t	freeq_lock; /* Locks queue from reentrance */
 };
@@ -724,32 +726,46 @@ static int gmac_setup_rxq(struct net_device *netdev)
 	return 0;
 }
 
-static struct gmac_queue_page *
-gmac_get_queue_page(struct gemini_ethernet *geth, dma_addr_t addr)
+static unsigned long
+geth_freeq_mapping_index(const struct gemini_ethernet *geth,
+			 dma_addr_t mapping)
+{
+	return (unsigned long)(mapping >> geth->freeq_frag_order);
+}
+
+static struct page *geth_freeq_lookup(struct gemini_ethernet *geth,
+				      dma_addr_t mapping,
+				      unsigned int *page_offs)
 {
 	unsigned int frag_len = 1 << geth->freeq_frag_order;
 	struct gmac_queue_page *gpage;
-	unsigned int offset;
-	int i;
-
-	if (!geth->freeq_pages) {
-		dev_err_ratelimited(geth->dev,
-				    "try to get page with no page list\n");
-		return NULL;
-	}
+	unsigned long index;
+	unsigned long flags;
+	dma_addr_t page_mapping;
+	struct page *page;
+	bool valid;
 
-	/* Look up a ring buffer page from virtual mapping */
-	for (i = 0; i < geth->num_freeq_pages; i++) {
-		gpage = &geth->freeq_pages[i];
-		if (!gpage->page || addr < gpage->mapping)
-			continue;
+	index = geth_freeq_mapping_index(geth, mapping);
 
-		offset = addr - gpage->mapping;
-		if (offset <= PAGE_SIZE - frag_len &&
-		    !(offset & (frag_len - 1)))
-			return gpage;
-	}
+	spin_lock_irqsave(&geth->freeq_lock, flags);
+	gpage = xa_load(&geth->freeq_mappings, index);
+	if (!gpage || !gpage->page)
+		goto err_unlock;
+
+	page = gpage->page;
+	page_mapping = gpage->mapping;
+	valid = mapping >= page_mapping &&
+		mapping - page_mapping <= PAGE_SIZE - frag_len &&
+		!((mapping - page_mapping) & (frag_len - 1));
+	if (!valid)
+		goto err_unlock;
+
+	*page_offs = mapping - page_mapping;
+	spin_unlock_irqrestore(&geth->freeq_lock, flags);
+	return page;
 
+err_unlock:
+	spin_unlock_irqrestore(&geth->freeq_lock, flags);
 	return NULL;
 }
 
@@ -758,11 +774,12 @@ static void gmac_cleanup_rxq(struct net_device *netdev)
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
 	struct gemini_ethernet *geth = port->geth;
 	struct gmac_rxdesc *rxd = port->rxq_ring;
-	struct gmac_queue_page *gpage;
 	struct nontoe_qhdr __iomem *qhdr;
 	void __iomem *dma_reg;
 	void __iomem *ptr_reg;
+	unsigned int page_offs;
 	dma_addr_t mapping;
+	struct page *page;
 	union dma_rwptr rw;
 	unsigned int r, w;
 
@@ -789,13 +806,13 @@ static void gmac_cleanup_rxq(struct net_device *netdev)
 		if (!mapping)
 			continue;
 
-		gpage = gmac_get_queue_page(geth, mapping);
-		if (!gpage) {
+		page = geth_freeq_lookup(geth, mapping, &page_offs);
+		if (!page) {
 			dev_err(geth->dev, "could not find page\n");
 			continue;
 		}
 		/* Release the RX queue reference to the page */
-		put_page(gpage->page);
+		put_page(page);
 	}
 
 	dma_free_coherent(geth->dev, sizeof(*port->rxq_ring) << port->rxq_order,
@@ -808,10 +825,12 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 	struct gmac_rxdesc *freeq_entry;
 	struct gmac_queue_page *gpage;
 	unsigned int fpp_order;
+	unsigned int fragments;
 	unsigned int frag_len;
 	dma_addr_t page_mapping;
 	dma_addr_t mapping;
 	struct page *page;
+	int ret;
 	int i;
 
 	/* First allocate and DMA map a single page */
@@ -842,8 +861,27 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 	 */
 	frag_len = 1 << geth->freeq_frag_order; /* Usually 2048 */
 	fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
-	/* If the freeq entry already has a page mapped, then unmap it. */
+	fragments = 1 << fpp_order;
+
 	gpage = &geth->freeq_pages[pn];
+	for (i = 0; i < fragments; i++) {
+		mapping = page_mapping + i * frag_len;
+		ret = xa_insert(&geth->freeq_mappings,
+				geth_freeq_mapping_index(geth, mapping),
+				gpage, GFP_ATOMIC);
+		if (ret)
+			goto err_mappings;
+	}
+
+	if (gpage->page) {
+		for (i = 0; i < fragments; i++) {
+			mapping = gpage->mapping + i * frag_len;
+			xa_erase(&geth->freeq_mappings,
+				 geth_freeq_mapping_index(geth, mapping));
+		}
+	}
+
+	/* If the freeq entry already has a page mapped, then unmap it. */
 	if (gpage->page) {
 		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
 				 DMA_FROM_DEVICE);
@@ -870,6 +908,17 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 		pn, &page_mapping, page);
 
 	return page;
+
+err_mappings:
+	while (i--) {
+		mapping = page_mapping + i * frag_len;
+		xa_erase(&geth->freeq_mappings,
+			 geth_freeq_mapping_index(geth, mapping));
+	}
+	dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
+			 DMA_FROM_DEVICE);
+	put_page(page);
+	return NULL;
 }
 
 /**
@@ -990,6 +1039,7 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 				 DMA_FROM_DEVICE);
 		put_page(gpage->page);
 	}
+	xa_destroy(&geth->freeq_mappings);
 
 	kfree(geth->freeq_pages);
 err_freeq:
@@ -1027,6 +1077,7 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 		while (page_ref_count(gpage->page) > 0)
 			put_page(gpage->page);
 	}
+	xa_destroy(&geth->freeq_mappings);
 
 	kfree(geth->freeq_pages);
 
@@ -1471,7 +1522,6 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 	unsigned int consumed = 0;
 	unsigned int frame_len, frag_len;
 	struct gmac_rxdesc *rx = NULL;
-	struct gmac_queue_page *gpage;
 	unsigned int received = 0;
 	bool dropping = port->rx_dropping;
 	union gmac_rxdesc_0 word0;
@@ -1526,14 +1576,12 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 			goto err_drop;
 		}
 
-		gpage = gmac_get_queue_page(geth, mapping);
-		if (!gpage) {
+		page = geth_freeq_lookup(geth, mapping, &page_offs);
+		if (!page) {
 			dev_err_ratelimited(geth->dev,
 					    "could not find mapping\n");
 			goto err_drop;
 		}
-		page = gpage->page;
-		page_offs = mapping - gpage->mapping;
 
 		if (word3.bits32 & SOF_BIT) {
 			skb = gmac_skb_if_good_frame(port, word0, frame_len);
@@ -2683,6 +2731,7 @@ static int gemini_ethernet_probe(struct platform_device *pdev)
 
 	spin_lock_init(&geth->irq_lock);
 	spin_lock_init(&geth->freeq_lock);
+	xa_init(&geth->freeq_mappings);
 
 	/* The children will use this */
 	platform_set_drvdata(pdev, geth);

-- 
2.55.0

Re: [PATCH net-next v2 04/11] net: ethernet: cortina: Index free queue fragments with XArray

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 1 (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).

- [High] At this commit, geth_fill_freeq() calls
  geth_freeq_alloc_map_page() for any consumed free-queue slot whose…
- [Medium] The new freeq_mappings XArray is created with a plain
  xa_init(), so xa_insert()/xa_erase() take xa_lock with plain…

Pre-existing issues:
- [High] Free-queue pages are mapped once with dma_map_single(...,
  DMA_FROM_DEVICE) in geth_freeq_alloc_map_page().
- [Medium] gmac_rx() uses hardware-reported lengths without bounds
  checks.

--

net: ethernet: cortina: Index free queue fragments with XArray

This commit replaces the linear scan in gmac_get_queue_page() with an
XArray indexed by DMA address in fragment units. The new
geth_freeq_lookup() validates the exact fragment address under
freeq_lock. When a page is replaced, geth_freeq_alloc_map_page() inserts
the fragment indices of the new page and erases those of the old one.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index fa5513b53ea8f..3d729f4c34ef1 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -724,32 +726,46 @@ static int gmac_setup_rxq(struct net_device *netdev)

[ ... ]

> +static struct page *geth_freeq_lookup(struct gemini_ethernet *geth,
> +				      dma_addr_t mapping,
> +				      unsigned int *page_offs)
>  {

[ ... ]

> +	spin_lock_irqsave(&geth->freeq_lock, flags);
> +	gpage = xa_load(&geth->freeq_mappings, index);
> +	if (!gpage || !gpage->page)
> +		goto err_unlock;
> +
> +	page = gpage->page;
> +	page_mapping = gpage->mapping;
> +	valid = mapping >= page_mapping &&
> +		mapping - page_mapping <= PAGE_SIZE - frag_len &&
> +		!((mapping - page_mapping) & (frag_len - 1));
> +	if (!valid)
> +		goto err_unlock;
> +
> +	*page_offs = mapping - page_mapping;
> +	spin_unlock_irqrestore(&geth->freeq_lock, flags);
> +	return page;

[Severity: High]
This isn't a bug introduced by this patch, but is a DMA sync missing
here?

geth_freeq_alloc_map_page() maps the free queue pages once with
dma_map_single(..., DMA_FROM_DEVICE). There are no dma_sync_* calls
anywhere in gemini.c.

geth_freeq_lookup() returns the fragment to gmac_rx() without calling
dma_sync_single_range_for_cpu(). gmac_rx() then passes it to GRO via
skb_fill_page_desc().

When page_ref_count(page) == 1, geth_fill_freeq() also gives a
still-mapped page back to the hardware without a
dma_sync_single_range_for_device():

	/* Add one reference per fragment in the page */
	page_ref_add(page, 1 << fpp_order);

On the non-coherent Gemini (FA526), could the CPU read stale cache lines
from an earlier frame after the device has DMAed a new frame into a
recycled page?

This seems to be fixed later in the series. "net: ethernet: cortina:
Synchronize RX fragments for the CPU" adds
dma_sync_single_range_for_cpu() in geth_freeq_claim(), and the in-flight
page rework stops recycling mapped pages.

[ ... ]

> @@ -842,8 +861,27 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
>  	 */
>  	frag_len = 1 << geth->freeq_frag_order; /* Usually 2048 */
>  	fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
> -	/* If the freeq entry already has a page mapped, then unmap it. */
> +	fragments = 1 << fpp_order;
> +
>  	gpage = &geth->freeq_pages[pn];
> +	for (i = 0; i < fragments; i++) {
> +		mapping = page_mapping + i * frag_len;
> +		ret = xa_insert(&geth->freeq_mappings,
> +				geth_freeq_mapping_index(geth, mapping),
> +				gpage, GFP_ATOMIC);

[Severity: Medium]
Will lockdep report inconsistent xa_lock usage here?

gemini_ethernet_probe() sets up freeq_mappings with a plain xa_init().
That means xa_insert() and xa_erase() take xa_lock with a plain
spin_lock().

geth_setup_freeq() calls this helper without holding freeq_lock. It runs
in process context with softirqs enabled:

	for (pn = 0; pn < pages; pn++)
		if (!geth_freeq_alloc_map_page(geth, pn))
			goto err_freeq_alloc;

The same lock is also taken from NAPI softirq context each time a page
with refcount > 1 is replaced:

gmac_napi_poll()
  geth_fill_freeq(geth, true)
    geth_freeq_alloc_map_page()
      xa_insert() / xa_erase()

Lockdep would record SOFTIRQ-ON-W and then IN-SOFTIRQ-W for the same
lock. Under CONFIG_PROVE_LOCKING that produces an inconsistent lock state
report, and lockdep then disables itself.

A real same-CPU deadlock does not look reachable.
geth_resize_freeq() returns -EBUSY if the other netdev is running, and
the opening port's NAPI is not enabled yet during setup.

"net: ethernet: cortina: Preserve in-flight free queue pages" later in
the series moves every xa_insert() and xa_erase() under freeq_lock with
irqsave. That seems to resolve this.

> +		if (ret)
> +			goto err_mappings;
> +	}
> +
> +	if (gpage->page) {
> +		for (i = 0; i < fragments; i++) {
> +			mapping = gpage->mapping + i * frag_len;
> +			xa_erase(&geth->freeq_mappings,
> +				 geth_freeq_mapping_index(geth, mapping));
> +		}
> +	}

[Severity: High]
Can this erase lookup entries that RX descriptors still refer to?

geth_fill_freeq() calls geth_freeq_alloc_map_page() for any consumed slot
whose page still holds extra references:

		if (page_ref_count(page) > 1) {
			...
			page = geth_freeq_alloc_map_page(geth, pn);

A refcount above 1 also covers pages whose fragments the hardware has
already placed in a port RX queue that gmac_rx() has not processed yet.
For example, gmac_rx() may have run out of budget, or the descriptor may
be in the other port's RX ring.

This loop then removes every fragment index of the old gpage->mapping.
The old page is unmapped and only its base reference is dropped:

		/* This should be the last reference to the page so it gets
		 * released
		 */
		put_page(gpage->page);

Later, when gmac_rx() reaches one of those pending descriptors, xa_load()
in geth_freeq_lookup() returns NULL and gmac_rx() takes this path:

		page = geth_freeq_lookup(geth, mapping, &page_offs);
		if (!page) {
			dev_err_ratelimited(geth->dev,
					    "could not find mapping\n");
			goto err_drop;
		}

Here page == NULL, so the "if (page) put_page(page)" under err_drop is
skipped. Does that drop a valid frame and leak the fragment reference
that page_ref_add() took? The old page is no longer in freeq_pages, so
geth_cleanup_freeq() cannot reclaim it either.

gmac_cleanup_rxq() has the same leak on its "could not find page"
continue path.

The old linear scan failed the same way, but the commit message says:

    Serialize metadata replacement with RX lookup so the caller obtains
    the page belonging to the descriptor even when the queue is being
    refilled.

The freeq_lock in geth_freeq_lookup() only makes each individual lookup
atomic. It does not help a descriptor whose page was replaced by a refill
that has already finished. Is that sentence accurate?

This looks fixed later in the series by "net: ethernet: cortina: Preserve
in-flight free queue pages". That patch erases each XArray entry only
when its fragment is claimed, keeps a per-page fragment count, and does
not reuse slots while fragments are still outstanding.

[ ... ]

> @@ -1526,14 +1576,12 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  			goto err_drop;
>  		}
>  
> -		gpage = gmac_get_queue_page(geth, mapping);
> -		if (!gpage) {
> +		page = geth_freeq_lookup(geth, mapping, &page_offs);
> +		if (!page) {
>  			dev_err_ratelimited(geth->dev,
>  					    "could not find mapping\n");
>  			goto err_drop;
>  		}
> -		page = gpage->page;
> -		page_offs = mapping - gpage->mapping;
>  
>  		if (word3.bits32 & SOF_BIT) {
>  			skb = gmac_skb_if_good_frame(port, word0, frame_len);

[Severity: Medium]
This is a pre-existing issue, but geth_freeq_lookup() only checks the
start of the fragment (page_offs). It does not check the extent that
gmac_rx() attaches next. Can the lengths reported by the hardware
underflow or overrun the fragment?

		if (word3.bits32 & SOF_BIT) {
			...
			page_offs += NET_IP_ALIGN;
			frag_len -= NET_IP_ALIGN;
			...
		if (word3.bits32 & EOF_BIT)
			frag_len = frame_len - skb->len;
		...
		if (frag_len == 0 && net_ratelimit())
			netdev_err(netdev, "Received fragment with len = 0\n");

		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);

frag_len wraps if word0.bits.buffer_size < NET_IP_ALIGN, or if
byte_count < skb->len. Nothing compares frag_len with the 2 KiB fragment
boundary or with PAGE_SIZE - page_offs. A zero length fragment is logged
but still appended.

It isn't clear whether the hardware can actually report such values.
If it does, could a malformed descriptor attach a fragment that runs
into the sibling fragment (possibly still owned by the device) or past
the end of the page?

This looks addressed later in the series by "net: ethernet: cortina:
Validate RX fragment lengths".

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org

[PATCH net-next v2 05/11] net: ethernet: cortina: Preserve in-flight free queue pages

[Linus Walleij]

The software free queue and the two per-port RX queues have independent
lifetimes. A free queue ring position becomes reusable once hardware has
taken its buffer address, but the resulting RX descriptor can remain
unprocessed for longer.

With the default 2K fragments, the sequence is:

1. Free queue entries 2n and 2n + 1 contain the first and second halves
   of page A.
2. Hardware consumes those entries and copies their DMA addresses into RX
   descriptors, potentially in the RX queues of different ports.
3. The free queue read pointer advances, making entries 2n and 2n + 1
   available for refill.
4. NAPI may not yet have processed the RX descriptors referring to page A,
   so their DMA-to-page association must remain valid.
5. Refilling the same free queue positions with page B must therefore not
   replace the metadata describing page A.

The driver currently indexes page metadata by free queue position. Step 5
therefore overwrites page A's association with page B. When NAPI later
processes an old descriptor containing page A's DMA address, the driver can
no longer find the mapping. This may explain long-standing reports of the
driver failing to find RX mappings.

This is not just caused by splitting a 4K page into two 2K
fragments: the metadata would also have to outlive its free queue position
with one full-page buffer per entry, but the 2K split makes the lifetime more
involved because the two halves of one page are independently consumed and
may be waiting in different port RX queues. The page cannot be unmapped
until both fragment references have been claimed.

Allocate page metadata slots independently of free queue positions and
track occupied slots with a bitmap. Keep each exact fragment DMA address in
the XArray until software claims its descriptor. Once both fragments have
been claimed, unmap the complete page and release the base reference owned
by the free queue.

Give each posted fragment its own page reference. Queue and error cleanup
can then release precisely the outstanding hardware references without
dropping references held by delivered skbs.

Allocate and DMA-map candidate pages before taking the free queue lock.
Re-read the hardware pointers under the lock and publish one complete page
at a time, so refilling no longer performs a potentially multi-megabyte
allocation batch with interrupts disabled.

Require the initial fill to populate every usable queue entry. If mapping
or metadata allocation stops the fill early, rewind the hardware write
pointer, release the partial population and fail setup. Leave the refill
interrupt masked and require a live ring for the resize fast path so a
later open retries queue setup.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 296 ++++++++++++++++++++--------------
 1 file changed, 173 insertions(+), 123 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 3d729f4c34ef..8d216fa45287 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -14,6 +14,7 @@
  * Gary Chen & Ch Hsu Storlink Semiconductor
  */
 #include <linux/kernel.h>
+#include <linux/bitmap.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/net.h>
@@ -88,11 +89,13 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 /**
  * struct gmac_queue_page - page buffer per-page info
  * @page: the page struct
- * @mapping: the dma address handle
+ * @mapping: DMA address of the first fragment
+ * @fragments: number of fragments not yet claimed from the hardware
  */
 struct gmac_queue_page {
 	struct page *page;
 	dma_addr_t mapping;
+	unsigned int fragments;
 };
 
 struct gmac_txq {
@@ -168,6 +171,7 @@ struct gemini_ethernet {
 	struct gmac_queue_page	*freeq_pages;
 	struct xarray	freeq_mappings;
 	unsigned int	num_freeq_pages;
+	unsigned long	*freeq_page_bitmap;
 	spinlock_t	freeq_lock; /* Locks queue from reentrance */
 };
 
@@ -733,15 +737,32 @@ geth_freeq_mapping_index(const struct gemini_ethernet *geth,
 	return (unsigned long)(mapping >> geth->freeq_frag_order);
 }
 
-static struct page *geth_freeq_lookup(struct gemini_ethernet *geth,
-				      dma_addr_t mapping,
-				      unsigned int *page_offs)
+static int geth_freeq_alloc_slot(struct gemini_ethernet *geth)
+{
+	unsigned int slot;
+
+	lockdep_assert_held(&geth->freeq_lock);
+
+	slot = find_first_zero_bit(geth->freeq_page_bitmap,
+				   geth->num_freeq_pages);
+	if (slot == geth->num_freeq_pages)
+		return -ENOSPC;
+
+	__set_bit(slot, geth->freeq_page_bitmap);
+
+	return slot;
+}
+
+static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
+				     dma_addr_t mapping,
+				     unsigned int *page_offs)
 {
 	unsigned int frag_len = 1 << geth->freeq_frag_order;
 	struct gmac_queue_page *gpage;
 	unsigned long index;
 	unsigned long flags;
 	dma_addr_t page_mapping;
+	unsigned int slot;
 	struct page *page;
 	bool valid;
 
@@ -749,7 +770,7 @@ static struct page *geth_freeq_lookup(struct gemini_ethernet *geth,
 
 	spin_lock_irqsave(&geth->freeq_lock, flags);
 	gpage = xa_load(&geth->freeq_mappings, index);
-	if (!gpage || !gpage->page)
+	if (!gpage || !gpage->page || !gpage->fragments)
 		goto err_unlock;
 
 	page = gpage->page;
@@ -760,6 +781,17 @@ static struct page *geth_freeq_lookup(struct gemini_ethernet *geth,
 	if (!valid)
 		goto err_unlock;
 
+	xa_erase(&geth->freeq_mappings, index);
+	if (!--gpage->fragments) {
+		slot = gpage - geth->freeq_pages;
+		dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
+				 DMA_FROM_DEVICE);
+		gpage->page = NULL;
+		gpage->mapping = 0;
+		__clear_bit(slot, geth->freeq_page_bitmap);
+		put_page(page);
+	}
+
 	*page_offs = mapping - page_mapping;
 	spin_unlock_irqrestore(&geth->freeq_lock, flags);
 	return page;
@@ -806,7 +838,7 @@ static void gmac_cleanup_rxq(struct net_device *netdev)
 		if (!mapping)
 			continue;
 
-		page = geth_freeq_lookup(geth, mapping, &page_offs);
+		page = geth_freeq_claim(geth, mapping, &page_offs);
 		if (!page) {
 			dev_err(geth->dev, "could not find page\n");
 			continue;
@@ -819,30 +851,22 @@ static void gmac_cleanup_rxq(struct net_device *netdev)
 			  port->rxq_ring, port->rxq_dma_base);
 }
 
-static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
-					      int pn)
+static int geth_freeq_map_page(struct gemini_ethernet *geth,
+			       struct page **pagep,
+			       dma_addr_t *page_mappingp)
 {
-	struct gmac_rxdesc *freeq_entry;
-	struct gmac_queue_page *gpage;
-	unsigned int fpp_order;
-	unsigned int fragments;
-	unsigned int frag_len;
 	dma_addr_t page_mapping;
-	dma_addr_t mapping;
 	struct page *page;
-	int ret;
-	int i;
 
-	/* First allocate and DMA map a single page */
 	page = alloc_page(GFP_ATOMIC);
 	if (!page)
-		return NULL;
+		return -ENOMEM;
 
 	page_mapping = dma_map_single(geth->dev, page_address(page),
 				      PAGE_SIZE, DMA_FROM_DEVICE);
 	if (dma_mapping_error(geth->dev, page_mapping)) {
 		put_page(page);
-		return NULL;
+		return -ENOMEM;
 	}
 	if (page_mapping > U32_MAX - (PAGE_SIZE - 1)) {
 		dev_err_ratelimited(geth->dev,
@@ -850,9 +874,34 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 		dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
 				 DMA_FROM_DEVICE);
 		put_page(page);
-		return NULL;
+		return -EOVERFLOW;
 	}
 
+	*pagep = page;
+	*page_mappingp = page_mapping;
+
+	return 0;
+}
+
+static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int pn,
+			       struct page *page, dma_addr_t page_mapping)
+{
+	struct gmac_rxdesc *freeq_entry;
+	struct gmac_queue_page *gpage;
+	unsigned int fpp_order;
+	unsigned int fragments;
+	unsigned int frag_len;
+	dma_addr_t mapping;
+	int ret;
+	int slot;
+	int i;
+
+	lockdep_assert_held(&geth->freeq_lock);
+
+	slot = geth_freeq_alloc_slot(geth);
+	if (slot < 0)
+		return slot;
+
 	/* The assign the page mapping (physical address) to the buffer address
 	 * in the hardware queue. PAGE_SHIFT on ARM is 12 (1 page is 4096 bytes,
 	 * 4k), and the default RX frag order is 11 (fragments are up 20 2048
@@ -863,7 +912,9 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 	fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
 	fragments = 1 << fpp_order;
 
-	gpage = &geth->freeq_pages[pn];
+	gpage = &geth->freeq_pages[slot];
+	gpage->page = page;
+	gpage->mapping = page_mapping;
 	for (i = 0; i < fragments; i++) {
 		mapping = page_mapping + i * frag_len;
 		ret = xa_insert(&geth->freeq_mappings,
@@ -872,28 +923,8 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 		if (ret)
 			goto err_mappings;
 	}
-
-	if (gpage->page) {
-		for (i = 0; i < fragments; i++) {
-			mapping = gpage->mapping + i * frag_len;
-			xa_erase(&geth->freeq_mappings,
-				 geth_freeq_mapping_index(geth, mapping));
-		}
-	}
-
-	/* If the freeq entry already has a page mapped, then unmap it. */
-	if (gpage->page) {
-		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
-				 DMA_FROM_DEVICE);
-		/* This should be the last reference to the page so it gets
-		 * released
-		 */
-		put_page(gpage->page);
-	}
-
-	/* Then put our new mapping into the page table */
-	gpage->mapping = page_mapping;
-	gpage->page = page;
+	gpage->fragments = fragments;
+	page_ref_add(page, fragments);
 
 	freeq_entry = geth->freeq_ring + (pn << fpp_order);
 	dev_dbg(geth->dev, "allocate page %d fragment length %d fragments per page %d, freeq entry %p\n",
@@ -907,7 +938,7 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 	dev_dbg(geth->dev, "page %d, DMA addr: %pad, page %p\n",
 		pn, &page_mapping, page);
 
-	return page;
+	return 0;
 
 err_mappings:
 	while (i--) {
@@ -915,20 +946,20 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
 		xa_erase(&geth->freeq_mappings,
 			 geth_freeq_mapping_index(geth, mapping));
 	}
-	dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
-			 DMA_FROM_DEVICE);
-	put_page(page);
-	return NULL;
+	gpage->page = NULL;
+	gpage->mapping = 0;
+	__clear_bit(slot, geth->freeq_page_bitmap);
+	return ret;
 }
 
 /**
  * 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.
  */
-static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
+static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
 {
 	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
 	unsigned int count = 0;
@@ -940,47 +971,71 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
 	/* Mask for page */
 	m_pn = (1 << (geth->freeq_order - fpp_order)) - 1;
 
-	spin_lock_irqsave(&geth->freeq_lock, flags);
-
-	rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
-	pn = (refill ? rw.bits.wptr : rw.bits.rptr) >> fpp_order;
-	epn = (rw.bits.rptr >> fpp_order) - 1;
-	epn &= m_pn;
-
 	/* Loop over the freeq ring buffer entries */
-	while (pn != epn) {
-		struct gmac_queue_page *gpage;
+	for (;;) {
+		dma_addr_t page_mapping;
 		struct page *page;
+		int ret;
 
-		gpage = &geth->freeq_pages[pn];
-		page = gpage->page;
+		ret = geth_freeq_map_page(geth, &page, &page_mapping);
+		if (ret)
+			break;
 
-		dev_dbg(geth->dev, "fill entry %d page ref count %d add %d refs\n",
-			pn, page_ref_count(page), 1 << fpp_order);
+		spin_lock_irqsave(&geth->freeq_lock, flags);
 
-		if (page_ref_count(page) > 1) {
-			unsigned int fl = (pn - epn) & m_pn;
+		rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
+		pn = rw.bits.wptr >> fpp_order;
+		epn = (rw.bits.rptr >> fpp_order) - 1;
+		epn &= m_pn;
+		if (pn == epn) {
+			ret = -ENOSPC;
+		} else {
+			ret = geth_freeq_add_page(geth, pn, page,
+						  page_mapping);
+			if (!ret) {
+				count += 1 << fpp_order;
+				pn++;
+				pn &= m_pn;
+				writew(pn << fpp_order,
+				       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
+			}
+		}
 
-			if (fl > 64 >> fpp_order)
-				break;
+		spin_unlock_irqrestore(&geth->freeq_lock, flags);
 
-			page = geth_freeq_alloc_map_page(geth, pn);
-			if (!page)
-				break;
+		if (ret) {
+			dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
+					 DMA_FROM_DEVICE);
+			put_page(page);
+			break;
 		}
-
-		/* Add one reference per fragment in the page */
-		page_ref_add(page, 1 << fpp_order);
-		count += 1 << fpp_order;
-		pn++;
-		pn &= m_pn;
 	}
 
-	writew(pn << fpp_order, geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
+	return count;
+}
 
-	spin_unlock_irqrestore(&geth->freeq_lock, flags);
+static void geth_freeq_release_pages(struct gemini_ethernet *geth)
+{
+	unsigned int pn;
 
-	return count;
+	for (pn = 0; pn < geth->num_freeq_pages; pn++) {
+		struct gmac_queue_page *gpage;
+		struct page *page;
+
+		gpage = &geth->freeq_pages[pn];
+		if (!gpage->page)
+			continue;
+
+		page = gpage->page;
+		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
+				 DMA_FROM_DEVICE);
+		while (gpage->fragments) {
+			put_page(page);
+			gpage->fragments--;
+		}
+		put_page(page);
+	}
+	xa_destroy(&geth->freeq_mappings);
 }
 
 static int geth_setup_freeq(struct gemini_ethernet *geth)
@@ -988,10 +1043,16 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
 	unsigned int len = 1 << geth->freeq_order;
 	unsigned int pages = len >> fpp_order;
+	unsigned int page_slots = pages;
+	unsigned int expected;
 	union queue_threshold qt;
 	union dma_skb_size skbsz;
 	unsigned int filled;
-	unsigned int pn;
+
+	if (geth->port0)
+		page_slots += 1 << geth->port0->rxq_order;
+	if (geth->port1)
+		page_slots += 1 << geth->port1->rxq_order;
 
 	geth->freeq_ring = dma_alloc_coherent(geth->dev,
 		sizeof(*geth->freeq_ring) << geth->freeq_order,
@@ -1004,19 +1065,18 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 	}
 
 	/* Allocate a mapping to page look-up index */
-	geth->freeq_pages = kzalloc_objs(*geth->freeq_pages, pages);
+	geth->freeq_pages = kzalloc_objs(*geth->freeq_pages, page_slots);
 	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;
 
-	dev_info(geth->dev, "allocate %d pages for queue\n", pages);
-	for (pn = 0; pn < pages; pn++)
-		if (!geth_freeq_alloc_map_page(geth, pn))
-			goto err_freeq_alloc;
-
-	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;
 
 	qt.bits32 = readl(geth->base + GLOBAL_QUEUE_THRESHOLD_REG);
 	qt.bits.swfq_empty = 32;
@@ -1029,19 +1089,16 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 
 	return 0;
 
-err_freeq_alloc:
-	while (pn > 0) {
-		struct gmac_queue_page *gpage;
-
-		--pn;
-		gpage = &geth->freeq_pages[pn];
-		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
-				 DMA_FROM_DEVICE);
-		put_page(gpage->page);
-	}
-	xa_destroy(&geth->freeq_mappings);
-
+err_freeq_bitmap:
+	writew(readw(geth->base + GLOBAL_SWFQ_RWPTR_REG),
+	       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
+	geth_freeq_release_pages(geth);
+	bitmap_free(geth->freeq_page_bitmap);
+	geth->freeq_page_bitmap = NULL;
+err_freeq_pages:
 	kfree(geth->freeq_pages);
+	geth->freeq_pages = NULL;
+	geth->num_freeq_pages = 0;
 err_freeq:
 	dma_free_coherent(geth->dev,
 			  sizeof(*geth->freeq_ring) << geth->freeq_order,
@@ -1056,11 +1113,6 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
  */
 static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 {
-	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
-	unsigned int len = 1 << geth->freeq_order;
-	unsigned int pages = len >> fpp_order;
-	unsigned int pn;
-
 	if (!geth->freeq_ring)
 		return;
 
@@ -1068,22 +1120,18 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 	       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
 	writel(0, geth->base + GLOBAL_SW_FREEQ_BASE_SIZE_REG);
 
-	for (pn = 0; pn < pages; pn++) {
-		struct gmac_queue_page *gpage;
-
-		gpage = &geth->freeq_pages[pn];
-		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
-				 DMA_FROM_DEVICE);
-		while (page_ref_count(gpage->page) > 0)
-			put_page(gpage->page);
-	}
-	xa_destroy(&geth->freeq_mappings);
+	geth_freeq_release_pages(geth);
 
+	bitmap_free(geth->freeq_page_bitmap);
+	geth->freeq_page_bitmap = NULL;
 	kfree(geth->freeq_pages);
+	geth->freeq_pages = NULL;
+	geth->num_freeq_pages = 0;
 
 	dma_free_coherent(geth->dev,
 			  sizeof(*geth->freeq_ring) << geth->freeq_order,
 			  geth->freeq_ring, geth->freeq_dma_base);
+	geth->freeq_ring = NULL;
 }
 
 static void geth_set_freeq_irq(struct gemini_ethernet *geth, bool enable)
@@ -1145,7 +1193,7 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	new_order = min(15, ilog2(new_size - 1) + 1);
 	dev_dbg(geth->dev, "set shared queue to size %d order %d\n",
 		new_size, new_order);
-	if (geth->freeq_order == new_order)
+	if (geth->freeq_ring && geth->freeq_order == new_order)
 		return 0;
 
 	/* The threaded handler can already be running and re-enable the IRQ. */
@@ -1160,6 +1208,8 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	/* Allocate a new queue with the desired order */
 	geth->freeq_order = new_order;
 	ret = geth_setup_freeq(geth);
+	if (ret)
+		return ret;
 
 	/* Restart the interrupts - NOTE if this is the first resize
 	 * after probe(), this is where the interrupts get turned on
@@ -1576,7 +1626,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");
@@ -1673,7 +1723,7 @@ static int gmac_napi_poll(struct napi_struct *napi, int budget)
 	port->freeq_refill += freeq_consumed;
 	if (port->freeq_refill > freeq_threshold) {
 		port->freeq_refill -= freeq_threshold;
-		geth_fill_freeq(geth, true);
+		geth_fill_freeq(geth);
 	}
 
 	if (!reschedule && received < budget &&
@@ -2403,7 +2453,7 @@ static irqreturn_t gemini_port_irq_thread(int irq, void *data)
 
 	geth = port->geth;
 	/* The queue is half empty so refill it */
-	geth_fill_freeq(geth, true);
+	geth_fill_freeq(geth);
 
 	spin_lock_irqsave(&geth->irq_lock, flags);
 	/* ACK queue interrupt */

-- 
2.55.0

Re: [PATCH net-next v2 05/11] net: ethernet: cortina: Preserve in-flight free queue pages

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 3 (1) · Low: 3

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_fill_freeq() no longer recycles pages, and the commit
  message does not say so.
- [Medium] The metadata slot pool (num_freeq_pages / freeq_page_bitmap)
  is sized once in geth_setup_freeq() as pages + (1 << port0->rxq_order)…
- [Medium] In this patch, geth_freeq_claim() hands a fragment to the
  stack without dma_sync_single_range_for_cpu().
- [Low] Commit "net: ethernet: cortina: Preserve in-flight free queue
  pages" fixes an RX bug that has been in the driver since it was added,…
- [Low] When the free queue is rebuilt, geth_fill_freeq() starts at pn =
  wptr >> fpp_order, and geth_cleanup_freeq() has set wptr to the…
- [Low] geth_freeq_alloc_slot() runs find_first_zero_bit() from bit 0 on
  every page add, under the IRQ-disabled freeq_lock, and rescans the…

Pre-existing issues:
- [High] geth_fill_freeq() takes pn straight from the hardware SWFQ write
  pointer (pn = rw.bits.wptr >> fpp_order) and masks it with m_pn only…
- [High] geth_resize_freeq() dereferences the partner port
  unconditionally (other_netdev = geth->port1->netdev /…
- [Medium] gmac_stop() sets port->rx_skb = NULL without
  napi_free_frags(), so a partly assembled napi->skb and its fragment…

--

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

Free queue page metadata is no longer indexed by ring position.
Metadata slots are now allocated from a bitmap, and each fragment's DMA
address stays in the XArray until its RX descriptor is claimed. Each
posted fragment owns a page reference, and refill now allocates and maps
pages outside the free queue lock and publishes them one page at a time.

> The driver currently indexes page metadata by free queue position. Step 5
> therefore overwrites page A's association with page B. When NAPI later
> processes an old descriptor containing page A's DMA address, the driver can
> no longer find the mapping. This may explain long-standing reports of the
> driver failing to find RX mappings.

[Severity: Low]
This describes an RX bug that seems to have existed since the driver was
added. Should the patch carry a Fixes: tag?

In the parent tree, geth_fill_freeq() calls geth_freeq_alloc_map_page(geth,
pn) whenever page_ref_count(page) > 1. That function erases the old page's
XArray entries and unmaps and puts gpage->page. RX descriptors pointing at
that page may still be waiting for NAPI, so the later lookup fails, the
frame is dropped and the per-fragment reference leaks.

The position-indexed metadata dates back to the original driver commit:

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")

The fix also depends on the earlier XArray and DMA-mapping patches in this
series. Could the commit message say so?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 3d729f4c34ef..8d216fa45287 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -733,15 +737,32 @@ geth_freeq_mapping_index(const struct gemini_ethernet *geth,
>  	return (unsigned long)(mapping >> geth->freeq_frag_order);
>  }
>  
> -static struct page *geth_freeq_lookup(struct gemini_ethernet *geth,
> -				      dma_addr_t mapping,
> -				      unsigned int *page_offs)
> +static int geth_freeq_alloc_slot(struct gemini_ethernet *geth)
> +{
> +	unsigned int slot;
> +
> +	lockdep_assert_held(&geth->freeq_lock);
> +
> +	slot = find_first_zero_bit(geth->freeq_page_bitmap,
> +				   geth->num_freeq_pages);

[Severity: Low]
Every page add scans the bitmap from bit 0 while freeq_lock is held with
interrupts disabled, so the occupied low slots are rescanned each time. Is
a first-fit scan intended here?

The next commit in the series, "net: ethernet: cortina: Rotate free queue
page allocation", appears to change this to find_next_zero_bit() starting
at freeq_page_cursor and wrapping around.

> +	if (slot == geth->num_freeq_pages)
> +		return -ENOSPC;

[ ... ]

> @@ -760,6 +781,17 @@ static struct page *geth_freeq_lookup(struct gemini_ethernet *geth,
>  	if (!valid)
>  		goto err_unlock;
>  
> +	xa_erase(&geth->freeq_mappings, index);
> +	if (!--gpage->fragments) {
> +		slot = gpage - geth->freeq_pages;
> +		dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
> +				 DMA_FROM_DEVICE);

[Severity: Medium]
geth_freeq_claim() returns the claimed fragment without calling
dma_sync_single_range_for_cpu() for it. The first fragment of a page is
therefore handed to the stack with no CPU sync at all.

When the last fragment is claimed, this dma_unmap_single() does CPU cache
maintenance over the whole PAGE_SIZE mapping. That range includes the
sibling fragment, which may already belong to an skb in the stack.

Can this corrupt or hide data in a fragment the CPU already owns?
geth_freeq_release_pages() also does a whole-page unmap with CPU sync.

This seems to be fixed later in the series by "net: ethernet: cortina:
Synchronize RX fragments for the CPU". That patch syncs each fragment
range in geth_freeq_claim() and unmaps with DMA_ATTR_SKIP_CPU_SYNC. It
also makes geth_freeq_release_pages() sync only the fragments still in
the XArray.

> +		gpage->page = NULL;
> +		gpage->mapping = 0;
> +		__clear_bit(slot, geth->freeq_page_bitmap);
> +		put_page(page);
> +	}

[ ... ]

> @@ -940,47 +971,71 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
>  	/* Mask for page */
>  	m_pn = (1 << (geth->freeq_order - fpp_order)) - 1;
>  
> -	spin_lock_irqsave(&geth->freeq_lock, flags);
> -
> -	rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
> -	pn = (refill ? rw.bits.wptr : rw.bits.rptr) >> fpp_order;
> -	epn = (rw.bits.rptr >> fpp_order) - 1;
> -	epn &= m_pn;
> -
>  	/* Loop over the freeq ring buffer entries */
> -	while (pn != epn) {
> -		struct gmac_queue_page *gpage;
> +	for (;;) {
> +		dma_addr_t page_mapping;
>  		struct page *page;
> +		int ret;
>  
> -		gpage = &geth->freeq_pages[pn];
> -		page = gpage->page;
> +		ret = geth_freeq_map_page(geth, &page, &page_mapping);
> +		if (ret)
> +			break;
>  
> -		dev_dbg(geth->dev, "fill entry %d page ref count %d add %d refs\n",
> -			pn, page_ref_count(page), 1 << fpp_order);
> +		spin_lock_irqsave(&geth->freeq_lock, flags);
>  
> -		if (page_ref_count(page) > 1) {
> -			unsigned int fl = (pn - epn) & m_pn;
> +		rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
> +		pn = rw.bits.wptr >> fpp_order;
> +		epn = (rw.bits.rptr >> fpp_order) - 1;
> +		epn &= m_pn;

[Severity: High]
This isn't a bug introduced by this patch, but pn comes straight from the
hardware write pointer. It is only masked with m_pn after the first
geth_freeq_add_page() call. When the free queue shrinks, can the first
page be written past the end of geth->freeq_ring?

geth_cleanup_freeq() leaves wptr equal to the old rptr and clears the base
register, but it does not reset the pointers:

geth_cleanup_freeq()
    writew(readw(geth->base + GLOBAL_SWFQ_RWPTR_REG),
           geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
    writel(0, geth->base + GLOBAL_SW_FREEQ_BASE_SIZE_REG);

geth_setup_freeq() then allocates a smaller coherent ring and calls
geth_fill_freeq() before it programs the new size.

Take an old order of 11, rptr at 1000 and a new order of 8. Then pn is
500, and geth_freeq_add_page() does:

    freeq_entry = geth->freeq_ring + (pn << fpp_order);
    ...
    freeq_entry->word2.buf_adr = mapping;

This writes about 16KB into a 4KB buffer. The loop then wraps back into
range and fills exactly the expected count, so setup reports success.

One way to reach this: run traffic, bring both interfaces down, then run
ethtool -G rx 64 on eth0 (fast path) and then on eth1. The eth1 change goes
through gmac_set_ringparam()->geth_resize_freeq(). This assumes the
hardware rptr survives the BASE_SIZE write, which the new error path in
geth_setup_freeq() also relies on.

The parent code used the same unmasked index. Since this calculation is
being rewritten, should pn also be masked with m_pn, as epn is?

[Severity: Low]
What happens here if the hardware rptr is odd when the ring is rebuilt,
for example after an odd number of 2K entries were consumed?

geth_cleanup_freeq() sets wptr to rptr. With fpp_order = 1 and rptr = 37,
pn is 18 and epn is 17. The first page goes into entries 36 and 37, and
entry 36 is behind the hardware read pointer, so it is never consumed.

The final wptr is 34, so a later refill writes page 18 again and overwrites
entry 36. Fragment 0 of the original page is never claimed. Its bitmap
slot, XArray entry, page and DMA mapping stay held until the next
geth_freeq_release_pages().

The old position-indexed code reclaimed that position on refill.

> +		if (pn == epn) {
> +			ret = -ENOSPC;
> +		} else {
> +			ret = geth_freeq_add_page(geth, pn, page,
> +						  page_mapping);
> +			if (!ret) {
> +				count += 1 << fpp_order;
> +				pn++;
> +				pn &= m_pn;
> +				writew(pn << fpp_order,
> +				       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
> +			}
> +		}

[Severity: Medium]
Each loop iteration calls geth_freeq_map_page() before checking whether
the ring has room. If the ring is already full, the pn == epn branch
unmaps and frees the page that was just mapped. Does every call to
geth_fill_freeq() now allocate, map, unmap and free one extra page?

There is also a second change. The parent code reused a page in place once
its refcount dropped back to 1, calling only page_ref_add() with no
alloc_page() or dma_map_single(). Now every refilled page costs
alloc_page(GFP_ATOMIC) plus a full-page dma_map_single(), and a full unmap
and free once both fragments are claimed.

This runs at line rate from gmac_napi_poll() and gemini_port_irq_thread().

The commit message describes the change as a latency improvement and does
not mention that page recycling is gone. Was that intended? Could the ring
space be checked before a page is mapped?

[ ... ]

> @@ -988,10 +1043,16 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
>  	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
>  	unsigned int len = 1 << geth->freeq_order;
>  	unsigned int pages = len >> fpp_order;
> +	unsigned int page_slots = pages;
> +	unsigned int expected;
>  	union queue_threshold qt;
>  	union dma_skb_size skbsz;
>  	unsigned int filled;
> -	unsigned int pn;
> +
> +	if (geth->port0)
> +		page_slots += 1 << geth->port0->rxq_order;
> +	if (geth->port1)
> +		page_slots += 1 << geth->port1->rxq_order;

[Severity: Medium]
The slot pool is sized from the rxq_order values at setup time. Can
gmac_set_ringparam() later increase port->rxq_order without the pool being
rebuilt?

gmac_set_ringparam() updates port->rxq_order before it calls
geth_resize_freeq(). The rebuild is then skipped in two cases. The first
is the fast path:

geth_resize_freeq()
    if (geth->freeq_ring && geth->freeq_order == new_order)
        return 0;

The second is the -EBUSY return when the other port is running, which
gmac_open() accepts:

gmac_open()
    if (err && (err != -EBUSY)) {

In both cases gmac_setup_rxq() then programs the larger RX ring.

For example, a rebuild with RX orders 9 and 7 gives a freeq order of 11
and 1664 slots. Raising the second port to order 8 keeps order 11, so the
fast path is taken. The worst-case number of outstanding pages is then
about 1023 + 511 + 255 = 1789.

Would geth_freeq_alloc_slot() then return -ENOSPC, leaving the free queue
empty under load until NAPI claims free some slots?

[Severity: High]
This is a pre-existing issue, but geth->port1 can be NULL or dangling at
this point. Is it safe to read geth->port1->rxq_order?

gemini_ethernet_port_probe() assigns geth->port1 = port before
gmac_setup_phy() and register_netdev(), and its error path does not clear
the pointer.

gemini.dtsi always defines ethernet-port@1. Several boards (dlink-dns-313,
nas4210b, nas4220b, rut1xx, wbd111, dir-685) give it no phy-mode or
phy-handle. On those boards gmac_setup_phy() returns -ENODEV, devres frees
the netdev, and geth->port1 is left pointing at freed memory.

Opening eth0 then does:

gmac_open()
  geth_resize_freeq()
    other_netdev = geth->port1->netdev;
    ...
    synchronize_irq(geth->port1->irq);
  geth_setup_freeq()
    page_slots += 1 << geth->port1->rxq_order;

If the partner port's probe fails before the assignment, the pointer is
NULL and geth_resize_freeq() dereferences it.

The dereferences in geth_resize_freeq() predate this patch. The new
page_slots calculation reads the same pointer again and can produce a
garbage shift and a wrong pool size. Should the probe error path clear
geth->port0/port1?

[ ... ]

> @@ -1576,7 +1626,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);

[Severity: Medium]
This isn't a bug introduced by this patch, but what releases a partially
assembled frame if the port is stopped in the middle of one? The claimed
fragment references end up in that skb.

gmac_stop() does:

    port->rx_skb = NULL;

It does not call napi_free_frags(), so napi->skb and its fragment page
references survive into the next open.

This looks fixed later in the series by "net: ethernet: cortina: Release
partial RX frames on stop", where gmac_stop() calls napi_free_frags() when
port->rx_skb is set.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org

[PATCH net-next v2 06/11] net: ethernet: cortina: Rotate free queue page allocation

[Linus Walleij]

The free queue metadata bitmap currently searches for a free slot from
zero for every page allocation. Under sustained traffic this repeatedly
scans occupied low-numbered slots.

Remember the position after the most recently allocated slot and wrap the
bitmap search at its end. This keeps allocation cost distributed across
the metadata table.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 8d216fa45287..385b78fd6653 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -172,6 +172,7 @@ struct gemini_ethernet {
 	struct xarray	freeq_mappings;
 	unsigned int	num_freeq_pages;
 	unsigned long	*freeq_page_bitmap;
+	unsigned int	freeq_page_cursor;
 	spinlock_t	freeq_lock; /* Locks queue from reentrance */
 };
 
@@ -743,12 +744,20 @@ static int geth_freeq_alloc_slot(struct gemini_ethernet *geth)
 
 	lockdep_assert_held(&geth->freeq_lock);
 
-	slot = find_first_zero_bit(geth->freeq_page_bitmap,
-				   geth->num_freeq_pages);
-	if (slot == geth->num_freeq_pages)
-		return -ENOSPC;
+	slot = find_next_zero_bit(geth->freeq_page_bitmap,
+				  geth->num_freeq_pages,
+				  geth->freeq_page_cursor);
+	if (slot == geth->num_freeq_pages) {
+		slot = find_first_zero_bit(geth->freeq_page_bitmap,
+					   geth->freeq_page_cursor);
+		if (slot == geth->freeq_page_cursor)
+			return -ENOSPC;
+	}
 
 	__set_bit(slot, geth->freeq_page_bitmap);
+	geth->freeq_page_cursor = slot + 1;
+	if (geth->freeq_page_cursor == geth->num_freeq_pages)
+		geth->freeq_page_cursor = 0;
 
 	return slot;
 }
@@ -1072,6 +1081,7 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 	if (!geth->freeq_page_bitmap)
 		goto err_freeq_pages;
 	geth->num_freeq_pages = page_slots;
+	geth->freeq_page_cursor = 0;
 
 	expected = len - (1 << fpp_order);
 	filled = geth_fill_freeq(geth);

-- 
2.55.0

[PATCH net-next v2 07/11] net: ethernet: cortina: Synchronize RX fragments for the CPU

[Linus Walleij]

Free queue pages use streaming DMA mappings with DMA_FROM_DEVICE. The RX
path reads packet data after the device has filled a fragment without
first transferring ownership of that range to the CPU.

Synchronize each complete free queue fragment when its RX descriptor is
claimed.

Once all fragments have been claimed, tear down the full-page mapping with
DMA_ATTR_SKIP_CPU_SYNC. Each range has already moved to the CPU domain, and
another full-page synchronization could invalidate a sibling fragment
already being consumed by the network stack.

During queue teardown, synchronize only XArray entries still owned by the
hardware before the skip-sync unmap. Claimed fragments may remain attached
to skbs and must not be synchronized again.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 385b78fd6653..d677d7431ab2 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -790,11 +790,15 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
 	if (!valid)
 		goto err_unlock;
 
+	dma_sync_single_range_for_cpu(geth->dev, page_mapping,
+				      mapping - page_mapping, frag_len,
+				      DMA_FROM_DEVICE);
 	xa_erase(&geth->freeq_mappings, index);
 	if (!--gpage->fragments) {
 		slot = gpage - geth->freeq_pages;
-		dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
-				 DMA_FROM_DEVICE);
+		dma_unmap_single_attrs(geth->dev, page_mapping, PAGE_SIZE,
+				       DMA_FROM_DEVICE,
+				       DMA_ATTR_SKIP_CPU_SYNC);
 		gpage->page = NULL;
 		gpage->mapping = 0;
 		__clear_bit(slot, geth->freeq_page_bitmap);
@@ -1025,19 +1029,34 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
 
 static void geth_freeq_release_pages(struct gemini_ethernet *geth)
 {
+	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
+	unsigned int frag_len = 1 << geth->freeq_frag_order;
 	unsigned int pn;
 
 	for (pn = 0; pn < geth->num_freeq_pages; pn++) {
 		struct gmac_queue_page *gpage;
+		dma_addr_t mapping;
 		struct page *page;
+		unsigned int i;
 
 		gpage = &geth->freeq_pages[pn];
 		if (!gpage->page)
 			continue;
 
 		page = gpage->page;
-		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
-				 DMA_FROM_DEVICE);
+		for (i = 0; i < (1 << fpp_order); i++) {
+			mapping = gpage->mapping + i * frag_len;
+			if (xa_load(&geth->freeq_mappings,
+				    geth_freeq_mapping_index(geth, mapping)) != gpage)
+				continue;
+
+			dma_sync_single_range_for_cpu(geth->dev, gpage->mapping,
+						      i * frag_len, frag_len,
+						      DMA_FROM_DEVICE);
+		}
+		dma_unmap_single_attrs(geth->dev, gpage->mapping, PAGE_SIZE,
+				       DMA_FROM_DEVICE,
+				       DMA_ATTR_SKIP_CPU_SYNC);
 		while (gpage->fragments) {
 			put_page(page);
 			gpage->fragments--;

-- 
2.55.0

Re: [PATCH net-next v2 07/11] net: ethernet: cortina: Synchronize RX fragments for the CPU

[netdev-bot+sashiko]

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] The commit message for "net: ethernet: cortina: Synchronize RX
  fragments for the CPU" presents the change as a bug fix.

--

net: ethernet: cortina: Synchronize RX fragments for the CPU

This commit adds a dma_sync_single_range_for_cpu() for each free queue
fragment when geth_freeq_claim() claims it. It also changes the final
full-page unmap to use DMA_ATTR_SKIP_CPU_SYNC. During teardown,
geth_freeq_release_pages() now syncs only the fragments the hardware still
owns before doing a skip-sync unmap.

> Free queue pages use streaming DMA mappings with DMA_FROM_DEVICE. The RX
> path reads packet data after the device has filled a fragment without
> first transferring ownership of that range to the CPU.

[Severity: Low]

Could the commit message say which configurations are affected and what
the user-visible symptom is? As written, it reads like a fix for RX data
corruption, but there is no Fixes: tag.

The missing sync_for_cpu seems to go back to the original driver, commit
4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet").
That commit added gmac_rx() and the claim/unmap logic.

This IP only appears on the Gemini SoC (ARCH_GEMINI, ARCH_MULTI_V4, FA526
core, no outer cache). On Gemini, arch_sync_dma_for_cpu() in
arch/arm/mm/dma-mapping.c does:

	if (dir != DMA_TO_DEVICE) {
		outer_inv_range(paddr, paddr + size);

		dma_cache_maint_page(paddr, size, dir, dmac_unmap_area);
	}

outer_inv_range() does nothing here. dmac_unmap_area resolves to
fa_dma_unmap_area in arch/arm/mm/cache-fa.S:

SYM_TYPED_FUNC_START(fa_dma_unmap_area)
	ret	lr
SYM_FUNC_END(fa_dma_unmap_area)

So the new dma_sync_single_range_for_cpu() in geth_freeq_claim() does not
seem to do any cache maintenance on this hardware. The old full-page
dma_unmap_single() did none either.

> Once all fragments have been claimed, tear down the full-page mapping with
> DMA_ATTR_SKIP_CPU_SYNC. Each range has already moved to the CPU domain, and
> another full-page synchronization could invalidate a sibling fragment
> already being consumed by the network stack.

Given the above, can the sibling-invalidation case happen on FA526 at
all?

If this fixes real RX corruption on some configuration, should it carry
"Fixes: 4d5ae32f5e1e"? If it is a DMA API correctness change with no
functional effect on Gemini, could the commit message say so? That would
let stable backporters tell the two cases apart.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org

[PATCH net-next v2 08/11] net: ethernet: cortina: Validate RX fragment lengths

[Linus Walleij]

RX descriptor lengths are used to adjust the page offset and populate skb
fragments without checking that the resulting range remains inside the
posted free queue fragment. A zero-length descriptor is logged but is still
appended.

The backing page is larger than the default 2 KiB DMA fragment, so checking
only the page boundary would allow a malformed descriptor for the first
half of a page to consume data from the sibling fragment. That fragment may
still be owned by the device.

Reject a short initial fragment before applying NET_IP_ALIGN, reject frame
length underflow and ranges extending beyond either the DMA fragment or the
page, and drop zero-length fragments instead of adding them to the skb.
Count these drops as receive and length errors.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index d677d7431ab2..258bb44d5570 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1593,6 +1593,7 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
 	unsigned short m = (1 << port->rxq_order) - 1;
 	struct gemini_ethernet *geth = port->geth;
+	unsigned int freeq_frag_len = 1 << geth->freeq_frag_order;
 	void __iomem *ptr_reg = port->rxq_rwptr;
 	unsigned int frag_nr = port->rx_frag_nr;
 	struct sk_buff *skb = port->rx_skb;
@@ -1667,6 +1668,9 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 			if (!skb)
 				goto err_drop;
 
+			if (frag_len < NET_IP_ALIGN)
+				goto err_length;
+
 			page_offs += NET_IP_ALIGN;
 			frag_len -= NET_IP_ALIGN;
 			frag_nr = 0;
@@ -1675,15 +1679,26 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 			goto err_drop;
 		}
 
-		if (word3.bits32 & EOF_BIT)
+		if (word3.bits32 & EOF_BIT) {
+			if (frame_len < skb->len)
+				goto err_length;
 			frag_len = frame_len - skb->len;
+		}
 
 		/* append page frag to skb */
 		if (frag_nr == MAX_SKB_FRAGS)
 			goto err_drop;
+		if (frag_len > freeq_frag_len -
+			       (page_offs & (freeq_frag_len - 1)) ||
+		    frag_len > PAGE_SIZE - page_offs)
+			goto err_length;
 
-		if (frag_len == 0 && net_ratelimit())
-			netdev_err(netdev, "Received fragment with len = 0\n");
+		if (!frag_len) {
+			if (net_ratelimit())
+				netdev_err(netdev,
+					   "Received fragment with len = 0\n");
+			goto err_length;
+		}
 
 		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);
 		skb->len += frag_len;
@@ -1698,6 +1713,11 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 		}
 		goto next_desc;
 
+err_length:
+		if (!dropping) {
+			port->stats.rx_errors++;
+			port->stats.rx_length_errors++;
+		}
 err_drop:
 		if (skb) {
 			napi_free_frags(&port->napi);

-- 
2.55.0

Re: [PATCH net-next v2 08/11] net: ethernet: cortina: Validate RX fragment lengths

[netdev-bot+sashiko]

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] The new err_length label in gmac_rx() counts a frame as bad
  (rx_errors++, rx_length_errors++) after gmac_skb_if_good_frame() has…
- [Low] The commit "net: ethernet: cortina: Validate RX fragment lengths"
  fixes real defects in gmac_rx()…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but in
  drivers/net/ethernet/cortina/gemini.h the comment on `union…

--

net: ethernet: cortina: Validate RX fragment lengths

This patch adds bounds checks on RX descriptor lengths in gmac_rx(). It
rejects a short first fragment, an EOF frame length underflow, fragments
that run past the DMA fragment or the page, and zero-length fragments.
These drops are counted as rx_errors and rx_length_errors.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index d677d7431ab2..258bb44d5570 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1667,6 +1668,9 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  			if (!skb)
>  				goto err_drop;
>  
> +			if (frag_len < NET_IP_ALIGN)
> +				goto err_length;
> +
>  			page_offs += NET_IP_ALIGN;
>  			frag_len -= NET_IP_ALIGN;

[Severity: Low]
This isn't a bug introduced by this patch, but the comment on
buffer_size in union gmac_rxdesc_0 in gemini.h looks like it was copied
from the field next to it:

drivers/net/ethernet/cortina/gemini.h:
		/* bit 15:0 number of descriptors used for the current frame */
		unsigned int buffer_size:16;
		/* bit 21:16 number of descriptors used for the current frame */
		unsigned int desc_count:6;

gmac_rx() treats word0.bits.buffer_size as a byte length. It subtracts
NET_IP_ALIGN from it and, with this patch, compares it against the
freeq fragment size and PAGE_SIZE.

Could the comment be changed to describe a per-buffer byte count? This
doesn't affect the new bounds arithmetic.

[ ... ]

> -		if (word3.bits32 & EOF_BIT)
> +		if (word3.bits32 & EOF_BIT) {
> +			if (frame_len < skb->len)
> +				goto err_length;
>  			frag_len = frame_len - skb->len;
> +		}

[Severity: Low]
Should this commit have a Fixes: tag?

Without this check, frame_len < skb->len at EOF wraps frag_len to a huge
unsigned value. That value then goes into skb_fill_page_desc(),
skb->len and skb->data_len.

The EOF computation and the unchecked skb_fill_page_desc() and skb->len
updates date back to the original driver:

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")

Earlier RX fixes to gmac_rx() carry that tag, for example 6520198c430c
("net: ethernet: cortina: Count RX drops once per frame").

The commit uses the Assisted-by workflow, and step 6 of
Documentation/process/coding-assistants.rst also asks for a Fixes tag on
such fixes. The trailers here only have Assisted-by and Signed-off-by.

[ ... ]

> @@ -1698,6 +1713,11 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  		}
>  		goto next_desc;
>  
> +err_length:
> +		if (!dropping) {
> +			port->stats.rx_errors++;
> +			port->stats.rx_length_errors++;
> +		}

[Severity: Medium]
Does this count the same frame as both a good packet and a bad packet?

Each new goto err_length runs after gmac_skb_if_good_frame() has already
returned an skb for the SOF descriptor. By then that function has done:

drivers/net/ethernet/cortina/gemini.c:gmac_skb_if_good_frame() {
    ...
update_exit:
	port->stats.rx_bytes += frame_len;
	port->stats.rx_packets++;
	return skb;
}

So when a frame hits any of the new checks, it gets rx_packets and
rx_bytes from the SOF path, then rx_errors and rx_length_errors here,
then rx_dropped in err_drop. gmac_get_stats64() then reports it in both
groups.

In include/uapi/linux/if_link.h, rx_packets counts good packets and
rx_errors counts bad ones. Before this patch the two never overlapped,
because gmac_skb_if_good_frame() rejects error frames before it touches
rx_packets.

The same header defines rx_length_errors for 802.3 in terms of in-range
length, out-of-range length field and frame-too-long errors. An
inconsistent DMA descriptor length doesn't seem to fit any of those.

Would it be better either to undo the good-packet accounting here, or
to count these drops only in rx_dropped?

>  err_drop:
>  		if (skb) {
>  			napi_free_frags(&port->napi);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org

[PATCH net-next v2 09/11] net: ethernet: cortina: Release partial RX frames on stop

[Linus Walleij]

A NAPI poll can retain a partially assembled frame across invocations. The
stop path clears that pointer without releasing its fragment references,
leaking every page already attached to the frame.

Free a pending fragment skb after disabling NAPI and before clearing the
saved RX state.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 258bb44d5570..a7096690e4be 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -2077,6 +2077,8 @@ static int gmac_stop(struct net_device *netdev)
 	gmac_disable_tx_rx(netdev);
 	gmac_stop_dma(port);
 	napi_disable(&port->napi);
+	if (port->rx_skb)
+		napi_free_frags(&port->napi);
 	port->rx_skb = NULL;
 	port->rx_frag_nr = 0;
 	port->rx_dropping = false;

-- 
2.55.0

Re: [PATCH net-next v2 09/11] net: ethernet: cortina: Release partial RX frames on stop

[netdev-bot+sashiko]

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] The commit message understates what this patch fixes.
- [Low] The patch fixes a real, user-visible defect in gmac_stop() but
  has no Fixes: tag.
- [Low] The new discard in gmac_stop() drops the pending partial frame
  with napi_free_frags() but does not increment port->stats.rx_dropped.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but gmac_stop()
  cancels the RX coalesce hrtimer as its first step…

--

net: ethernet: cortina: Release partial RX frames on stop

A NAPI poll can leave a partially assembled RX frame in napi->skb. This
patch frees that frame in gmac_stop() with napi_free_frags(), after
napi_disable() and before port->rx_skb and the saved RX state are
cleared.

> A NAPI poll can retain a partially assembled frame across invocations. The
> stop path clears that pointer without releasing its fragment references,
> leaking every page already attached to the frame.

[Severity: Low]
Does this describe only part of what the patch fixes?

Before this patch, gmac_stop() cleared only port->rx_skb, and napi->skb
still pointed at the half-built skb. napi_disable() and napi_enable() do
not touch napi->skb, so the stale skb is still there on the next open.

After ifup, gmac_rx() starts with skb = NULL and frag_nr = 0. The first
SOF descriptor then gets the old skb back:

gmac_rx()
  gmac_skb_if_good_frame()
    napi_get_frags()   <- returns the stale napi->skb unchanged

At EOF, the length math uses the stale skb->len:

    if (frame_len < skb->len)
        goto err_length;
    frag_len = frame_len - skb->len;

Either the good frame is dropped as a length error, or
skb_fill_page_desc(skb, 0, ...) overwrites frag 0 and sets nr_frags to
1. The second case leaks the old fragment pages. It also leaves data_len
larger than the bytes the frags actually hold.

That skb is then passed to napi_gro_frags(). If the remaining fragment is
shorter than ETH_HLEN, this path can reach the BUG_ON() on
skb_copy_bits() failure:

napi_frags_skb()->skb_gro_header_slow()->pskb_may_pull()->__pskb_pull_tail()

Could the commit message say that stale RX state was carried into the
next open, and that the next received frame could be corrupted or hit
a BUG_ON()? Right now it only mentions a page leak.

> Assisted-by: LLM
> Signed-off-by: Linus Walleij <linusw@kernel.org>

[Severity: Low]
Should this have a Fixes: tag?

The incomplete reset in gmac_stop() appears to come from commit
06937db21ee3 ("net: ethernet: cortina: Make RX SKB per-port"). That
commit added port->rx_skb = NULL without calling napi_free_frags(). It
carries its own tag, Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver
for Gemini gigabit ethernet").

The napi_disable() call that never releases napi->skb goes back to
4d5ae32f5e1e.

Without a Fixes: tag, stable trees could pick up 06937db21ee3 but not
this follow-up. The stale napi->skb reuse after ifdown/ifup would then
stay in stable kernels.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 258bb44d5570..a7096690e4be 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2077,6 +2077,8 @@ static int gmac_stop(struct net_device *netdev)
>  	gmac_disable_tx_rx(netdev);

[Severity: High]
This isn't a bug introduced by this patch, but can the RX coalesce
hrtimer be re-armed after gmac_stop() cancels it?

gmac_stop() cancels the timer first. It masks the port interrupts much
later:

    hrtimer_cancel(&port->rx_coalesce_timer);
    netif_tx_stop_all_queues(netdev);
    gmac_disable_tx_rx(netdev);
    ...
    gmac_enable_irq(netdev, 0);
    ...
    free_irq(netdev->irq, netdev);

The window between hrtimer_cancel() and gmac_enable_irq(netdev, 0)
includes the mdelay(10) in gmac_disable_tx_rx(). If rx_coalesce_nsecs is
nonzero, an RX interrupt in that window runs:

gmac_irq()
    gmac_enable_rx_irq(netdev, 0);
    ...
    hrtimer_start(&port->rx_coalesce_timer, ktime,
                  HRTIMER_MODE_REL);

gmac_set_coalesce() puts no upper bound on rx_coalesce_usecs. The timer
can therefore stay pending for seconds after gmac_stop() returns.

On the next gmac_open(), hrtimer_setup() would memset a timer that is
still linked in the timerqueue, corrupting the hrtimer rbtree.

On unbind or remove, dev_close()->gmac_stop() is followed by
free_netdev(). That frees the private area holding the armed timer.
gmac_coalesce_delay_expired() would later run napi_schedule(&port->napi)
on freed memory.

Would it work to mask the port interrupts with gmac_enable_irq(netdev,
0), or call free_irq(), before the hrtimer_cancel()? Then nothing could
re-arm the timer once it has been cancelled.

>  	gmac_stop_dma(port);
>  	napi_disable(&port->napi);
> +	if (port->rx_skb)
> +		napi_free_frags(&port->napi);

[Severity: Low]
Should this discard also increment port->stats.rx_dropped?

Both existing places in gmac_rx() that discard the pending partial frame
count it. One is the SOF-while-open path:

    if (skb) {
        napi_free_frags(&port->napi);
        port->stats.rx_dropped++;
        ...

The other is the err_drop path, which follows the same pattern.

gmac_skb_if_good_frame() already counted this frame in rx_packets and
rx_bytes at SOF. A frame discarded here therefore shows up as received
and never as dropped.

If a counter is added, the update would need to sit inside
u64_stats_update_begin/end(&port->rx_stats_syncp). gmac_get_stats64()
reads these fields under that seqcount, and this is a 32-bit platform.

>  	port->rx_skb = NULL;
>  	port->rx_frag_nr = 0;
>  	port->rx_dropping = false;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org

[PATCH net-next v2 10/11] net: ethernet: cortina: Scale Gemini RX queues to system memory

[Linus Walleij]

The default RX queues contain 512 descriptors per port. The shared
software free queue is sized at twice their combined capacity, requiring
4 MiB of page fragments even on Gemini systems with only 32 or 64 MiB of
RAM.

Choose smaller default RX queues on memory-constrained systems. Use 256
descriptors per port and a 2 MiB free queue with at most 64 MiB of RAM,
and 128 descriptors per port with a 1 MiB free queue with at most 32 MiB.
Keep the existing defaults on systems with more memory.

This preserves the existing relationship between RX descriptor capacity
and the free queue. Users can still override the defaults with ethtool.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index a7096690e4be..910de9925949 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/bitmap.h>
 #include <linux/init.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/net.h>
 #include <linux/platform_device.h>
@@ -35,6 +36,7 @@
 #include <linux/skbuff.h>
 #include <linux/phy.h>
 #include <linux/crc32.h>
+#include <linux/sizes.h>
 #include <linux/ethtool.h>
 #include <linux/tcp.h>
 #include <linux/u64_stats_sync.h>
@@ -470,6 +472,17 @@ static int gmac_pick_rx_max_len(unsigned int max_l3_len)
 	return -1;
 }
 
+static unsigned int gmac_pick_rxq_order(void)
+{
+	if (totalram_pages() <= (SZ_32M >> PAGE_SHIFT))
+		return DEFAULT_GMAC_RXQ_ORDER - 2;
+
+	if (totalram_pages() <= (SZ_64M >> PAGE_SHIFT))
+		return DEFAULT_GMAC_RXQ_ORDER - 1;
+
+	return DEFAULT_GMAC_RXQ_ORDER;
+}
+
 static int gmac_init(struct net_device *netdev)
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
@@ -538,7 +551,7 @@ static int gmac_init(struct net_device *netdev)
 	writel(sw_weigh.bits32,
 	       port->dma_base + GMAC_TX_WEIGHTING_CTRL_1_REG);
 
-	port->rxq_order = DEFAULT_GMAC_RXQ_ORDER;
+	port->rxq_order = gmac_pick_rxq_order();
 	port->txq_order = DEFAULT_GMAC_TXQ_ORDER;
 	port->rx_coalesce_nsecs = DEFAULT_RX_COALESCE_NSECS;
 

-- 
2.55.0

[PATCH net-next v2 11/11] net: ethernet: cortina: Use guard helpers for locking

[Linus Walleij]

Use cleanup guard helpers to scope the driver's spinlocks
automatically.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 144 +++++++++++++---------------------
 1 file changed, 53 insertions(+), 91 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 910de9925949..4fdd7478601f 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 #include <linux/cache.h>
+#include <linux/cleanup.h>
 #include <linux/interrupt.h>
 #include <linux/reset.h>
 #include <linux/clk.h>
@@ -240,46 +241,37 @@ static void gmac_update_config0_reg(struct net_device *netdev,
 				    u32 val, u32 vmask)
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
-	unsigned long flags;
 	u32 reg;
 
-	spin_lock_irqsave(&port->config_lock, flags);
+	guard(spinlock_irqsave)(&port->config_lock);
 
 	reg = readl(port->gmac_base + GMAC_CONFIG0);
 	reg = (reg & ~vmask) | val;
 	writel(reg, port->gmac_base + GMAC_CONFIG0);
-
-	spin_unlock_irqrestore(&port->config_lock, flags);
 }
 
 static void gmac_enable_tx_rx(struct net_device *netdev)
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
-	unsigned long flags;
 	u32 reg;
 
-	spin_lock_irqsave(&port->config_lock, flags);
+	guard(spinlock_irqsave)(&port->config_lock);
 
 	reg = readl(port->gmac_base + GMAC_CONFIG0);
 	reg &= ~CONFIG0_TX_RX_DISABLE;
 	writel(reg, port->gmac_base + GMAC_CONFIG0);
-
-	spin_unlock_irqrestore(&port->config_lock, flags);
 }
 
 static void gmac_disable_tx_rx(struct net_device *netdev)
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
-	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&port->config_lock, flags);
-
-	val = readl(port->gmac_base + GMAC_CONFIG0);
-	val |= CONFIG0_TX_RX_DISABLE;
-	writel(val, port->gmac_base + GMAC_CONFIG0);
-
-	spin_unlock_irqrestore(&port->config_lock, flags);
+	scoped_guard(spinlock_irqsave, &port->config_lock) {
+		val = readl(port->gmac_base + GMAC_CONFIG0);
+		val |= CONFIG0_TX_RX_DISABLE;
+		writel(val, port->gmac_base + GMAC_CONFIG0);
+	}
 
 	mdelay(10);	/* let GMAC consume packet */
 }
@@ -287,10 +279,9 @@ static void gmac_disable_tx_rx(struct net_device *netdev)
 static void gmac_set_flow_control(struct net_device *netdev, bool tx, bool rx)
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
-	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&port->config_lock, flags);
+	guard(spinlock_irqsave)(&port->config_lock);
 
 	val = readl(port->gmac_base + GMAC_CONFIG0);
 	val &= ~CONFIG0_FLOW_CTL;
@@ -299,8 +290,6 @@ static void gmac_set_flow_control(struct net_device *netdev, bool tx, bool rx)
 	if (rx)
 		val |= CONFIG0_FLOW_RX;
 	writel(val, port->gmac_base + GMAC_CONFIG0);
-
-	spin_unlock_irqrestore(&port->config_lock, flags);
 }
 
 static void gmac_adjust_link(struct net_device *netdev)
@@ -782,7 +771,6 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
 	unsigned int frag_len = 1 << geth->freeq_frag_order;
 	struct gmac_queue_page *gpage;
 	unsigned long index;
-	unsigned long flags;
 	dma_addr_t page_mapping;
 	unsigned int slot;
 	struct page *page;
@@ -790,10 +778,11 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
 
 	index = geth_freeq_mapping_index(geth, mapping);
 
-	spin_lock_irqsave(&geth->freeq_lock, flags);
+	guard(spinlock_irqsave)(&geth->freeq_lock);
+
 	gpage = xa_load(&geth->freeq_mappings, index);
 	if (!gpage || !gpage->page || !gpage->fragments)
-		goto err_unlock;
+		return NULL;
 
 	page = gpage->page;
 	page_mapping = gpage->mapping;
@@ -801,7 +790,7 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
 		mapping - page_mapping <= PAGE_SIZE - frag_len &&
 		!((mapping - page_mapping) & (frag_len - 1));
 	if (!valid)
-		goto err_unlock;
+		return NULL;
 
 	dma_sync_single_range_for_cpu(geth->dev, page_mapping,
 				      mapping - page_mapping, frag_len,
@@ -819,12 +808,7 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
 	}
 
 	*page_offs = mapping - page_mapping;
-	spin_unlock_irqrestore(&geth->freeq_lock, flags);
 	return page;
-
-err_unlock:
-	spin_unlock_irqrestore(&geth->freeq_lock, flags);
-	return NULL;
 }
 
 static void gmac_cleanup_rxq(struct net_device *netdev)
@@ -990,7 +974,6 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
 	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
 	unsigned int count = 0;
 	unsigned int pn, epn;
-	unsigned long flags;
 	union dma_rwptr rw;
 	unsigned int m_pn;
 
@@ -1007,28 +990,26 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
 		if (ret)
 			break;
 
-		spin_lock_irqsave(&geth->freeq_lock, flags);
-
-		rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
-		pn = rw.bits.wptr >> fpp_order;
-		epn = (rw.bits.rptr >> fpp_order) - 1;
-		epn &= m_pn;
-		if (pn == epn) {
-			ret = -ENOSPC;
-		} else {
-			ret = geth_freeq_add_page(geth, pn, page,
-						  page_mapping);
-			if (!ret) {
-				count += 1 << fpp_order;
-				pn++;
-				pn &= m_pn;
-				writew(pn << fpp_order,
-				       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
+		scoped_guard(spinlock_irqsave, &geth->freeq_lock) {
+			rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
+			pn = rw.bits.wptr >> fpp_order;
+			epn = (rw.bits.rptr >> fpp_order) - 1;
+			epn &= m_pn;
+			if (pn == epn) {
+				ret = -ENOSPC;
+			} else {
+				ret = geth_freeq_add_page(geth, pn, page,
+							  page_mapping);
+				if (!ret) {
+					count += 1 << fpp_order;
+					pn++;
+					pn &= m_pn;
+					writew(pn << fpp_order,
+					       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
+				}
 			}
 		}
 
-		spin_unlock_irqrestore(&geth->freeq_lock, flags);
-
 		if (ret) {
 			dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
 					 DMA_FROM_DEVICE);
@@ -1178,17 +1159,16 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 
 static void geth_set_freeq_irq(struct gemini_ethernet *geth, bool enable)
 {
-	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&geth->irq_lock, flags);
+	guard(spinlock_irqsave)(&geth->irq_lock);
+
 	val = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
 	if (enable)
 		val |= SWFQ_EMPTY_INT_BIT;
 	else
 		val &= ~SWFQ_EMPTY_INT_BIT;
 	writel(val, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
-	spin_unlock_irqrestore(&geth->irq_lock, flags);
 }
 
 /**
@@ -1267,12 +1247,11 @@ static void gmac_tx_irq_enable(struct net_device *netdev,
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
 	struct gemini_ethernet *geth = port->geth;
-	unsigned long flags;
 	u32 val, mask;
 
 	netdev_dbg(netdev, "%s device %d\n", __func__, netdev->dev_id);
 
-	spin_lock_irqsave(&geth->irq_lock, flags);
+	guard(spinlock_irqsave)(&geth->irq_lock);
 
 	mask = GMAC0_IRQ0_TXQ0_INTS << (6 * netdev->dev_id + txq);
 
@@ -1282,8 +1261,6 @@ static void gmac_tx_irq_enable(struct net_device *netdev,
 	val = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_0_REG);
 	val = en ? val | mask : val & ~mask;
 	writel(val, geth->base + GLOBAL_INTERRUPT_ENABLE_0_REG);
-
-	spin_unlock_irqrestore(&geth->irq_lock, flags);
 }
 
 static void gmac_tx_irq(struct net_device *netdev, unsigned int txq_num)
@@ -1516,12 +1493,11 @@ static void gmac_enable_irq(struct net_device *netdev, int enable)
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
 	struct gemini_ethernet *geth = port->geth;
-	unsigned long flags;
 	u32 val, mask;
 
 	netdev_dbg(netdev, "%s device %d %s\n", __func__,
 		   netdev->dev_id, enable ? "enable" : "disable");
-	spin_lock_irqsave(&geth->irq_lock, flags);
+	guard(spinlock_irqsave)(&geth->irq_lock);
 
 	mask = GMAC0_IRQ0_2 << (netdev->dev_id * 2);
 	val = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_0_REG);
@@ -1537,27 +1513,22 @@ static void gmac_enable_irq(struct net_device *netdev, int enable)
 	val = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
 	val = enable ? (val | mask) : (val & ~mask);
 	writel(val, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
-
-	spin_unlock_irqrestore(&geth->irq_lock, flags);
 }
 
 static void gmac_enable_rx_irq(struct net_device *netdev, int enable)
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
 	struct gemini_ethernet *geth = port->geth;
-	unsigned long flags;
 	u32 val, mask;
 
 	netdev_dbg(netdev, "%s device %d %s\n", __func__, netdev->dev_id,
 		   enable ? "enable" : "disable");
-	spin_lock_irqsave(&geth->irq_lock, flags);
+	guard(spinlock_irqsave)(&geth->irq_lock);
 	mask = DEFAULT_Q0_INT_BIT << netdev->dev_id;
 
 	val = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_1_REG);
 	val = enable ? (val | mask) : (val & ~mask);
 	writel(val, geth->base + GLOBAL_INTERRUPT_ENABLE_1_REG);
-
-	spin_unlock_irqrestore(&geth->irq_lock, flags);
 }
 
 static struct sk_buff *gmac_skb_if_good_frame(struct gemini_ethernet_port *port,
@@ -1622,17 +1593,16 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 	union gmac_rxdesc_3 word3;
 	struct page *page = NULL;
 	unsigned int page_offs;
-	unsigned long flags;
 	unsigned short r, w;
 	union dma_rwptr rw;
 	dma_addr_t mapping;
 
-	spin_lock_irqsave(&geth->irq_lock, flags);
-	rw.bits32 = readl(ptr_reg);
-	/* Reset interrupt as all packages until here are taken into account */
-	writel(DEFAULT_Q0_INT_BIT << netdev->dev_id,
-	       geth->base + GLOBAL_INTERRUPT_STATUS_1_REG);
-	spin_unlock_irqrestore(&geth->irq_lock, flags);
+	scoped_guard(spinlock_irqsave, &geth->irq_lock) {
+		rw.bits32 = readl(ptr_reg);
+		/* Reset interrupt as all packages until here are taken into account */
+		writel(DEFAULT_Q0_INT_BIT << netdev->dev_id,
+		       geth->base + GLOBAL_INTERRUPT_STATUS_1_REG);
+	}
 
 	r = rw.bits.rptr;
 	w = rw.bits.wptr;
@@ -1871,9 +1841,8 @@ static void gmac_update_hw_stats(struct net_device *netdev)
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
 	unsigned int rx_discards, rx_mcast, rx_bcast;
 	struct gemini_ethernet *geth = port->geth;
-	unsigned long flags;
 
-	spin_lock_irqsave(&geth->irq_lock, flags);
+	guard(spinlock_irqsave)(&geth->irq_lock);
 	u64_stats_update_begin(&port->ir_stats_syncp);
 
 	rx_discards = readl(port->gmac_base + GMAC_IN_DISCARDS);
@@ -1894,7 +1863,6 @@ static void gmac_update_hw_stats(struct net_device *netdev)
 	       geth->base + GLOBAL_INTERRUPT_STATUS_4_REG);
 
 	u64_stats_update_end(&port->ir_stats_syncp);
-	spin_unlock_irqrestore(&geth->irq_lock, flags);
 }
 
 /**
@@ -1980,13 +1948,14 @@ static irqreturn_t gmac_irq(int irq, void *data)
 		gmac_update_hw_stats(netdev);
 
 	if (val & (GMAC0_RX_OVERRUN_INT_BIT << (netdev->dev_id * 8))) {
-		spin_lock(&geth->irq_lock);
-		writel(GMAC0_RX_OVERRUN_INT_BIT << (netdev->dev_id * 8),
-		       geth->base + GLOBAL_INTERRUPT_STATUS_4_REG);
-		u64_stats_update_begin(&port->ir_stats_syncp);
-		++port->stats.rx_fifo_errors;
-		u64_stats_update_end(&port->ir_stats_syncp);
-		spin_unlock(&geth->irq_lock);
+		scoped_guard(spinlock, &geth->irq_lock) {
+			writel(GMAC0_RX_OVERRUN_INT_BIT <<
+			       (netdev->dev_id * 8),
+			       geth->base + GLOBAL_INTERRUPT_STATUS_4_REG);
+			u64_stats_update_begin(&port->ir_stats_syncp);
+			++port->stats.rx_fifo_errors;
+			u64_stats_update_end(&port->ir_stats_syncp);
+		}
 	}
 
 	return orr ? IRQ_HANDLED : IRQ_NONE;
@@ -2251,16 +2220,14 @@ static int gmac_set_features(struct net_device *netdev,
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
 	int enable = features & NETIF_F_RXCSUM;
-	unsigned long flags;
 	u32 reg;
 
-	spin_lock_irqsave(&port->config_lock, flags);
+	guard(spinlock_irqsave)(&port->config_lock);
 
 	reg = readl(port->gmac_base + GMAC_CONFIG0);
 	reg = enable ? reg | CONFIG0_RX_CHKSUM : reg & ~CONFIG0_RX_CHKSUM;
 	writel(reg, port->gmac_base + GMAC_CONFIG0);
 
-	spin_unlock_irqrestore(&port->config_lock, flags);
 	return 0;
 }
 
@@ -2513,20 +2480,17 @@ static irqreturn_t gemini_port_irq_thread(int irq, void *data)
 	unsigned long irqmask = SWFQ_EMPTY_INT_BIT;
 	struct gemini_ethernet_port *port = data;
 	struct gemini_ethernet *geth;
-	unsigned long flags;
 
 	geth = port->geth;
 	/* The queue is half empty so refill it */
 	geth_fill_freeq(geth);
 
-	spin_lock_irqsave(&geth->irq_lock, flags);
+	guard(spinlock_irqsave)(&geth->irq_lock);
 	/* ACK queue interrupt */
 	writel(irqmask, geth->base + GLOBAL_INTERRUPT_STATUS_4_REG);
 	/* Enable queue interrupt again */
 	irqmask |= readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
 	writel(irqmask, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
-	spin_unlock_irqrestore(&geth->irq_lock, flags);
-
 	return IRQ_HANDLED;
 }
 
@@ -2538,7 +2502,7 @@ static irqreturn_t gemini_port_irq(int irq, void *data)
 	u32 val, en;
 
 	geth = port->geth;
-	spin_lock(&geth->irq_lock);
+	guard(spinlock)(&geth->irq_lock);
 
 	val = readl(geth->base + GLOBAL_INTERRUPT_STATUS_4_REG);
 	en = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
@@ -2554,8 +2518,6 @@ static irqreturn_t gemini_port_irq(int irq, void *data)
 		ret = IRQ_WAKE_THREAD;
 	}
 
-	spin_unlock(&geth->irq_lock);
-
 	return ret;
 }
 

-- 
2.55.0

Re: [PATCH net-next v2 11/11] net: ethernet: cortina: Use guard helpers for locking

[netdev-bot+sashiko]

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] The patch goes against netdev's documented policy on cleanup.h
  and on standalone clean-ups.
- [Low] gmac_rx() now mixes goto-based cleanup with a cleanup.h helper,
  which subsystem/cleanup.md and include/linux/cleanup.h (lines 148-153)…

--

net: ethernet: cortina: Use guard helpers for locking

This commit converts the spinlock lock and unlock pairs in the Gemini
ethernet driver to guard() and scoped_guard() from linux/cleanup.h. The
goto based unlock path in geth_freeq_claim() is replaced with early
returns. No functional change is intended.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 910de9925949..4fdd7478601f 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -790,10 +778,11 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
>  
>  	index = geth_freeq_mapping_index(geth, mapping);
>  
> -	spin_lock_irqsave(&geth->freeq_lock, flags);
> +	guard(spinlock_irqsave)(&geth->freeq_lock);
> +
>  	gpage = xa_load(&geth->freeq_mappings, index);
>  	if (!gpage || !gpage->page || !gpage->fragments)
> -		goto err_unlock;
> +		return NULL;

[Severity: Low]
Does this conversion fit the netdev guidance on cleanup.h?
Documentation/process/maintainer-netdev.rst says:

  Use of guard() is discouraged within any function longer than 20 lines,
  scoped_guard() is considered more readable. Using normal lock/unlock is
  still (weakly) preferred.

geth_freeq_claim() is about 46 lines long. Its function-scope guard()
covers xa_load(), xa_erase(), dma_unmap_single_attrs() and put_page().
There are also two early return NULL exits, which now release freeq_lock
implicitly.

gmac_update_hw_stats(), gmac_enable_irq() and the gemini_port_irq()
hardirq handler are also longer than 20 lines and now use a
function-scope guard().

The same document also says:

  Netdev discourages patches which perform simple clean-ups, which are not
  in the context of other work.

The commit message only says "Use cleanup guard helpers to scope the
driver's spinlocks automatically". The patch has no functional change, and
it touches the RX and IRQ paths in gmac_rx(), gmac_irq() and
gemini_port_irq(). That may make stable backports harder.

Would it be better to keep the explicit spin_lock_irqsave() and
spin_unlock_irqrestore() calls in the longer functions? Or use
scoped_guard() there, or fold this into other work on the driver?

[ ... ]

> @@ -1622,17 +1593,16 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,

[ ... ]

> -	spin_lock_irqsave(&geth->irq_lock, flags);
> -	rw.bits32 = readl(ptr_reg);
> -	/* Reset interrupt as all packages until here are taken into account */
> -	writel(DEFAULT_Q0_INT_BIT << netdev->dev_id,
> -	       geth->base + GLOBAL_INTERRUPT_STATUS_1_REG);
> -	spin_unlock_irqrestore(&geth->irq_lock, flags);
> +	scoped_guard(spinlock_irqsave, &geth->irq_lock) {
> +		rw.bits32 = readl(ptr_reg);
> +		/* Reset interrupt as all packages until here are taken into account */
> +		writel(DEFAULT_Q0_INT_BIT << netdev->dev_id,
> +		       geth->base + GLOBAL_INTERRUPT_STATUS_1_REG);
> +	}
>  
>  	r = rw.bits.rptr;
>  	w = rw.bits.wptr;

[Severity: Low]
Does this mix goto based cleanup with a cleanup.h helper in gmac_rx()?

Before this patch gmac_rx() used only gotos. Its RX loop already relies on
goto based cleanup through the err_length, err_drop and next_desc labels:

err_drop:
		if (skb) {
			napi_free_frags(&port->napi);
			skb = NULL;
			frag_nr = 0;
		}

		if (page)
			put_page(page);

include/linux/cleanup.h says:

  the expectation is that usage of "goto" and cleanup helpers is never
  mixed in the same function. I.e. for a given routine, convert all
  resources that need a "goto" cleanup to scope-based cleanup, or
  convert none of them.

Nothing goes wrong today. The scoped_guard() block closes before the while
loop, and none of the gotos cross its scope.

If the locked section were later moved into the loop, or changed to
guard(), those gotos would jump across a cleanup scope.

Could gmac_rx() keep the explicit spin_lock_irqsave() and
spin_unlock_irqrestore() pair? Another option is to move the locked rwptr
read and interrupt ack into a small helper.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org

Re: [PATCH net-next v2 00/11] net: ethernet: cortina: Fix Gemini RX buffer management

[Paolo Abeni]

On 9/28/26 10:50, Linus Walleij wrote:
> The Gemini ethernet uses a custom software free queue shared by both
> ports. Its page metadata, DMA mappings and fragment references are not
> managed consistently as pages move through the free queue and RX queues.
> 
> Correct the DMA address bookkeeping and synchronization.
> 
> Track exact fragment mappings with an XArray and keep
> in-flight page metadata in bitmap-allocated slots until all hardware
> references have been claimed.
> 
> Serialize queue resize against the refill interrupt, harden fragment
> validation and teardown, then scale the default queue sizes for systems
> with 32 or 64 MiB of RAM.
> 
> This series wasn't so complex to begin with but Sashiko and the other
> LLMs just find more and more corner cases. It's for the better I
> guess.
> 
> Before these patches the network driver would crash under strong load
> but it does not happen any more after. Tested on the D-Link DIR-685
> playing back media and transfering new media using ksmbd while
> issuing ping storms.
> 
> Assisted-by: LLM
> Signed-off-by: Linus Walleij <linusw@kernel.org>
The vast majority of clashiko findings are actually fixes in this
very patch series or pre-existing issues, but I think it has a valid
point on patch 5 (WRT rebuilding ring with odd number of entries
consumed).

Also it looks like patch 11 is mostly against the current netdev
guidance for guard() usage:

https://elixir.bootlin.com/linux/v7.2.8/source/Documentation/process/maintainer-netdev.rst#L407

Thanks,

Paolo

Re: [PATCH net-next v2 00/11] net: ethernet: cortina: Fix Gemini RX buffer management

[Linus Walleij]

On Thu, Oct 1, 2026 at 11:51 AM Paolo Abeni <pabeni@redhat.com> wrote:

> > Before these patches the network driver would crash under strong load
> > but it does not happen any more after. Tested on the D-Link DIR-685
> > playing back media and transfering new media using ksmbd while
> > issuing ping storms.
> >
> > Assisted-by: LLM
> > Signed-off-by: Linus Walleij <linusw@kernel.org>

> The vast majority of clashiko findings are actually fixes in this
> very patch series or pre-existing issues, but I think it has a valid
> point on patch 5 (WRT rebuilding ring with odd number of entries
> consumed).
>
> Also it looks like patch 11 is mostly against the current netdev
> guidance for guard() usage:

OK I'll look closely at patch 5 and drop patch 11 and respin.

What I did was to kind-of-sort-of run the patches through
sashiko before submitting them, which makes it find
less.

Thanks Paolo,
Linus Walleij