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

[PATCH net-next v3 00/12] 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 v3:
- Add Fixes tags throughout the series, using the original driver commit
  where no more specific offending commit exists, and replace Resolves with
  Closes on the lifetime fix. I did not retarget the series to net
  because there are dependencies in net-next and we are getting closer
  to the merge window.
- Keep PHY-less port 1 bound as the shared free queue IRQ owner and clear
  stale parent port pointers.
- Disable the port 1 Linux IRQ throughout free queue resizing so no newly
  woken threaded handler can race queue teardown or setup.
- Keep failed queue rebuilds quiescent in the IRQ-drain patch.
- Serialize linear DMA lookups before converting them to an XArray.
- Keep all XArray mutation under the IRQ-safe free queue lock.
- Defer page unmapping until fragment lifetime is tracked, and fold DMA
  synchronization into that fix.
- Check free queue space before allocating and mapping a candidate page.
- Rebuild free queue metadata when RX rings grow and roll back rejected
  ring changes.
- Recycle released pages with explicit DMA synchronization using the
  existing free queue allocator.
- Account inconsistent RX descriptor lengths as drops rather than wire
  length errors.
- Count partial RX frames discarded during stop as dropped.
- Drop the cleanup guard conversion because it is not recommended for
  network drivers.
- Link to v2: https://patch.msgid.link/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78@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 (12):
      net: ethernet: cortina: Keep PHY-less port bound for shared IRQ
      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: Rebuild free queue metadata for RX ring changes
      net: ethernet: cortina: Rotate free queue page allocation
      net: ethernet: cortina: Recycle claimed free queue pages
      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

 drivers/net/ethernet/cortina/gemini.c | 632 +++++++++++++++++++++++++---------
 1 file changed, 467 insertions(+), 165 deletions(-)
---
base-commit: 071876fd50482a68603a9460d80dd6dd58827ee1
change-id: 20260919-gemini-ethernet-fixes-3-f0403653f23a

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

[PATCH net-next v3 01/12] net: ethernet: cortina: Keep PHY-less port bound for shared IRQ

[Linus Walleij]

The software free queue interrupt is routed through port 1 even on
boards which only connect a PHY to port 0. Since PHY setup was moved
into probe, the PHY-less port fails to bind after installing its parent
pointer. This leaves a dangling port pointer and releases the threaded
handler needed by the shared queue.

Keep children without a PHY description bound as IRQ-only ports. Clear
parent pointers on genuine probe failures and removal, and reject queue
setup cleanly if the port providing the shared interrupt is absent.

Fixes: 3e813d61401a ("net: gemini: Clean up phy registration")
Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 2fe7fd0202d2..2be3e9051981 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1048,10 +1048,15 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	u32 en;
 	int ret;
 
+	/* The software free queue interrupt is routed through port 1. */
+	if (!geth->port1)
+		return -ENODEV;
+
 	if (netdev->dev_id == 0)
-		other_netdev = geth->port1->netdev;
+		other_port = geth->port1;
 	else
-		other_netdev = geth->port0->netdev;
+		other_port = geth->port0;
+	other_netdev = other_port ? other_port->netdev : NULL;
 
 	if (other_netdev && netif_running(other_netdev))
 		return -EBUSY;
@@ -1062,7 +1067,6 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 		   new_size,
 		   port->rxq_order);
 	if (other_netdev) {
-		other_port = netdev_priv(other_netdev);
 		new_size += 1 << (other_port->rxq_order + 1);
 		netdev_dbg(other_netdev, "port %d size: %d order %d\n",
 			   other_netdev->dev_id,
@@ -2384,6 +2388,16 @@ static irqreturn_t gemini_port_irq(int irq, void *data)
 	return ret;
 }
 
+static void gemini_port_clear(struct gemini_ethernet_port *port)
+{
+	struct gemini_ethernet *geth = port->geth;
+
+	if (!port->id && geth->port0 == port)
+		geth->port0 = NULL;
+	else if (port->id && geth->port1 == port)
+		geth->port1 = NULL;
+}
+
 static void gemini_port_remove(struct gemini_ethernet_port *port)
 {
 	if (port->netdev) {
@@ -2392,6 +2406,7 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
 	}
 	clk_disable_unprepare(port->pclk);
 	geth_cleanup_freeq(port->geth);
+	gemini_port_clear(port);
 }
 
 static void gemini_ethernet_init(struct gemini_ethernet *geth)
@@ -2598,6 +2613,13 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
 	if (ret)
 		goto unprepare;
 
+	if (!of_property_present(np, "phy-handle") &&
+	    !of_phy_is_fixed_link(np)) {
+		dev_info(dev, "no PHY, keeping port for shared IRQ\n");
+		port->netdev = NULL;
+		return 0;
+	}
+
 	ret = gmac_setup_phy(netdev);
 	if (ret) {
 		netdev_err(netdev,
@@ -2612,6 +2634,7 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
 	return 0;
 
 unprepare:
+	gemini_port_clear(port);
 	clk_disable_unprepare(port->pclk);
 	return ret;
 }

-- 
2.55.0

[PATCH net-next v3 02/12] 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>
Closes: https://lore.kernel.org/netdev/20260917192835.58126-1-mhun512@gmail.com/
Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
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 2be3e9051981..15ad9f721dd5 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);
@@ -2405,7 +2408,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);
 	gemini_port_clear(port);
 }
 
@@ -2706,6 +2708,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

[PATCH net-next v3 03/12] net: ethernet: cortina: Drain free queue IRQ before resize

[Linus Walleij]

The software free queue interrupt remains registered while both netdevs
are down. Merely masking the device source does not stop a threaded
handler which was already woken. That handler can refill the queue while
resize frees its ring and page metadata, or re-enable the source after
teardown.

The free queue interrupt is routed through port 1. Disable that Linux IRQ
for the entire resize. disable_irq() drains hard and threaded handlers and
prevents a new thread from being woken.

After the drain, mask the device source and rebuild the queue. If setup
succeeds, re-enable the source before the Linux IRQ. On failure, keep the
source masked, clear freed queue state and restore the Linux IRQ so a later
open retries setup safely.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 40 +++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 15ad9f721dd5..a2daf22e7698 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -984,6 +984,8 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 	}
 
 	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,
@@ -1024,10 +1026,28 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 	}
 
 	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)
+{
+	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);
 }
 
 /**
@@ -1047,8 +1067,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;
 
 	/* The software free queue interrupt is routed through port 1. */
@@ -1080,16 +1098,11 @@ 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;
 
-	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);
+	disable_irq(geth->port1->irq);
+	geth_set_freeq_irq(geth, false);
 
 	/* Drop the old queue */
 	if (geth->freeq_ring)
@@ -1103,10 +1116,9 @@ 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);
+	if (!ret)
+		geth_set_freeq_irq(geth, true);
+	enable_irq(geth->port1->irq);
 
 	return ret;
 }

-- 
2.55.0

[PATCH net-next v3 04/12] 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, so it unmaps the new buffer while leaving the old
mapping live.

Keep the page DMA base separate from the fragment iterator and remove the
invalid replacement unmap. Releasing mappings with in-flight fragments is
handled together with their lifetime later in the series. Use PAGE_SIZE
when unwinding mappings which have not reached the hardware.

Reject mappings that cannot fit in the 32-bit hardware descriptors and
derive fragment offsets from the saved DMA base instead of assuming
page-aligned DMA addresses. Snapshot the page, DMA base and offset under
the free queue lock so refill cannot replace them between lookup and use.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 106 ++++++++++++++++++----------------
 1 file changed, 56 insertions(+), 50 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index a2daf22e7698..809274aff8e5 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -724,32 +724,43 @@ static int gmac_setup_rxq(struct net_device *netdev)
 	return 0;
 }
 
-static struct gmac_queue_page *
-gmac_get_queue_page(struct gemini_ethernet *geth,
-		    struct gemini_ethernet_port *port,
-		    dma_addr_t addr)
+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;
-	dma_addr_t mapping;
+	unsigned long flags;
+	dma_addr_t page_mapping;
+	struct page *page = NULL;
 	int i;
 
-	/* Only look for even pages */
-	mapping = addr & PAGE_MASK;
-
+	spin_lock_irqsave(&geth->freeq_lock, flags);
 	if (!geth->freeq_pages) {
 		dev_err_ratelimited(geth->dev,
 				    "try to get page with no page list\n");
-		return NULL;
+		goto unlock;
 	}
 
 	/* 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)
-			return gpage;
+		if (!gpage->page || mapping < gpage->mapping)
+			continue;
+
+		page_mapping = gpage->mapping;
+		if (mapping - page_mapping > PAGE_SIZE - frag_len ||
+		    ((mapping - page_mapping) & (frag_len - 1)))
+			continue;
+
+		page = gpage->page;
+		*page_offs = mapping - page_mapping;
+		break;
 	}
 
-	return NULL;
+unlock:
+	spin_unlock_irqrestore(&geth->freeq_lock, flags);
+	return page;
 }
 
 static void gmac_cleanup_rxq(struct net_device *netdev)
@@ -757,11 +768,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;
-	static 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;
 
@@ -788,14 +800,13 @@ 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);
-		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,
@@ -809,6 +820,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 +830,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,31 +853,25 @@ 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;
+	gpage = &geth->freeq_pages[pn];
+	if (gpage->page)
+		put_page(gpage->page);
+
+	/* Then put our new mapping into the page table */
+	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);
+		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;
 	}
-
-	/* 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);
-		/* 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 */
-	dev_dbg(geth->dev, "page %d, DMA addr: %08x, page %p\n",
-		pn, (unsigned int)mapping, page);
-	gpage->mapping = mapping;
-	gpage->page = page;
+	dev_dbg(geth->dev, "page %d, DMA addr: %pad, page %p\n",
+		pn, &page_mapping, page);
 
 	return page;
 }
@@ -927,7 +941,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 +987,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);
 	}
 
@@ -1019,7 +1031,6 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 
 		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];
 		while (page_ref_count(gpage->page) > 0)
 			put_page(gpage->page);
@@ -1475,7 +1486,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;
@@ -1512,8 +1522,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);
@@ -1532,14 +1540,12 @@ 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);
-		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;
 
 		if (word3.bits32 & SOF_BIT) {
 			skb = gmac_skb_if_good_frame(port, word0, frame_len);

-- 
2.55.0

[PATCH net-next v3 05/12] 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.

Populate and erase the fragment index under the free queue lock. The
lookup remains serialized with refill and validates the exact fragment
address before returning the page and offset snapshot.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 103 +++++++++++++++++++++++++---------
 1 file changed, 75 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 809274aff8e5..ee604bc04fc3 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,43 +726,47 @@ static int gmac_setup_rxq(struct net_device *netdev)
 	return 0;
 }
 
+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 long index;
 	unsigned long flags;
 	dma_addr_t page_mapping;
-	struct page *page = NULL;
-	int i;
-
-	spin_lock_irqsave(&geth->freeq_lock, flags);
-	if (!geth->freeq_pages) {
-		dev_err_ratelimited(geth->dev,
-				    "try to get page with no page list\n");
-		goto unlock;
-	}
-
-	/* 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 || mapping < gpage->mapping)
-			continue;
-
-		page_mapping = gpage->mapping;
-		if (mapping - page_mapping > PAGE_SIZE - frag_len ||
-		    ((mapping - page_mapping) & (frag_len - 1)))
-			continue;
+	struct page *page;
+	bool valid;
 
-		page = gpage->page;
-		*page_offs = mapping - page_mapping;
-		break;
-	}
+	index = geth_freeq_mapping_index(geth, mapping);
 
-unlock:
+	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;
 }
 
 static void gmac_cleanup_rxq(struct net_device *netdev)
@@ -819,12 +825,16 @@ 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;
 
+	lockdep_assert_held(&geth->freeq_lock);
+
 	/* First allocate and DMA map a single page */
 	page = alloc_page(GFP_ATOMIC);
 	if (!page)
@@ -853,9 +863,26 @@ 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;
+	fragments = 1 << fpp_order;
+
 	gpage = &geth->freeq_pages[pn];
-	if (gpage->page)
+	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));
+		}
 		put_page(gpage->page);
+	}
 
 	/* Then put our new mapping into the page table */
 	gpage->mapping = page_mapping;
@@ -874,6 +901,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;
 }
 
 /**
@@ -945,6 +983,8 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 	unsigned int pages = len >> fpp_order;
 	union queue_threshold qt;
 	union dma_skb_size skbsz;
+	unsigned long flags;
+	struct page *page;
 	unsigned int filled;
 	unsigned int pn;
 
@@ -965,9 +1005,13 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 	geth->num_freeq_pages = pages;
 
 	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))
+	for (pn = 0; pn < pages; pn++) {
+		spin_lock_irqsave(&geth->freeq_lock, flags);
+		page = geth_freeq_alloc_map_page(geth, pn);
+		spin_unlock_irqrestore(&geth->freeq_lock, flags);
+		if (!page)
 			goto err_freeq_alloc;
+	}
 
 	filled = geth_fill_freeq(geth, false);
 	if (!filled)
@@ -994,6 +1038,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);
 	geth->freeq_pages = NULL;
@@ -1035,6 +1080,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);
 	geth->freeq_pages = NULL;
@@ -2714,6 +2760,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

[PATCH net-next v3 06/12] 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.

The pages use streaming DMA_FROM_DEVICE mappings. Synchronize each
fragment when claimed and unmap the page with DMA_ATTR_SKIP_CPU_SYNC after
both are claimed. This has no known visible effect on Gemini's FA526,
where CPU sync is currently a no-op, but follows the DMA API and avoids
resynchronizing a sibling on other implementations. During teardown,
synchronize only fragments still owned by hardware.

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.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 316 +++++++++++++++++++++-------------
 1 file changed, 193 insertions(+), 123 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index ee604bc04fc3..660e51634017 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,21 @@ static struct page *geth_freeq_lookup(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_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);
+		put_page(page);
+	}
+
 	*page_offs = mapping - page_mapping;
 	spin_unlock_irqrestore(&geth->freeq_lock, flags);
 	return page;
@@ -806,7 +842,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,32 +855,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;
 
-	lockdep_assert_held(&geth->freeq_lock);
-
-	/* 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,
@@ -852,9 +878,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
@@ -865,7 +916,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,
@@ -874,19 +927,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));
-		}
-		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",
@@ -900,7 +942,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--) {
@@ -908,20 +950,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;
@@ -933,47 +975,95 @@ 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;
-
-		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);
+		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;
+		spin_unlock_irqrestore(&geth->freeq_lock, flags);
+		if (pn == epn)
+			break;
 
-		if (page_ref_count(page) > 1) {
-			unsigned int fl = (pn - epn) & m_pn;
+		ret = geth_freeq_map_page(geth, &page, &page_mapping);
+		if (ret)
+			break;
 
-			if (fl > 64 >> fpp_order)
-				break;
+		spin_lock_irqsave(&geth->freeq_lock, flags);
 
-			page = geth_freeq_alloc_map_page(geth, pn);
-			if (!page)
-				break;
+		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);
+			}
 		}
 
-		/* Add one reference per fragment in the page */
-		page_ref_add(page, 1 << fpp_order);
-		count += 1 << fpp_order;
-		pn++;
-		pn &= m_pn;
+		spin_unlock_irqrestore(&geth->freeq_lock, flags);
+
+		if (ret) {
+			dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
+					 DMA_FROM_DEVICE);
+			put_page(page);
+			break;
+		}
 	}
 
-	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 fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
+	unsigned int frag_len = 1 << geth->freeq_frag_order;
+	unsigned int pn;
 
-	return count;
+	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;
+		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--;
+		}
+		put_page(page);
+	}
+	xa_destroy(&geth->freeq_mappings);
 }
 
 static int geth_setup_freeq(struct gemini_ethernet *geth)
@@ -981,12 +1071,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 long flags;
-	struct page *page;
 	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,
@@ -999,23 +1093,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;
-
-	dev_info(geth->dev, "allocate %d pages for queue\n", pages);
-	for (pn = 0; pn < pages; pn++) {
-		spin_lock_irqsave(&geth->freeq_lock, flags);
-		page = geth_freeq_alloc_map_page(geth, pn);
-		spin_unlock_irqrestore(&geth->freeq_lock, flags);
-		if (!page)
-			goto err_freeq_alloc;
-	}
+	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;
 
-	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;
@@ -1028,18 +1117,13 @@ 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;
@@ -1057,12 +1141,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;
-
 	if (!geth->freeq_ring)
 		return;
 
@@ -1070,18 +1148,10 @@ 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;
-		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];
-		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;
@@ -1586,7 +1656,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");
@@ -1683,7 +1753,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 &&
@@ -2413,7 +2483,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

[PATCH net-next v3 07/12] net: ethernet: cortina: Rebuild free queue metadata for RX ring changes

[Linus Walleij]

Changing an RX ring can leave the shared free queue order unchanged
while increasing the number of pages which may be in flight. The queue
resize fast path then retains a metadata pool sized for the old RX
rings. A failed resize also leaves the new RX order installed, and open
accepts -EBUSY and proceeds with that larger ring.

Derive the required metadata capacity in one helper and include it in
the resize fast path. Return success while another port is running only
when the existing queue is large enough. Otherwise propagate the
failure from open and restore the old RX order after a failed ethtool
request.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 48 ++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 660e51634017..2345d582cb59 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1066,21 +1066,31 @@ static void geth_freeq_release_pages(struct gemini_ethernet *geth)
 	xa_destroy(&geth->freeq_mappings);
 }
 
+static unsigned int
+geth_freeq_page_slots(struct gemini_ethernet *geth, unsigned int order)
+{
+	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
+	unsigned int slots = 1 << (order - fpp_order);
+
+	if (geth->port0 && geth->port0->netdev)
+		slots += 1 << geth->port0->rxq_order;
+	if (geth->port1 && geth->port1->netdev)
+		slots += 1 << geth->port1->rxq_order;
+
+	return slots;
+}
+
 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 page_slots;
 	unsigned int expected;
 	union queue_threshold qt;
 	union dma_skb_size skbsz;
 	unsigned int filled;
 
-	if (geth->port0)
-		page_slots += 1 << geth->port0->rxq_order;
-	if (geth->port1)
-		page_slots += 1 << geth->port1->rxq_order;
+	page_slots = geth_freeq_page_slots(geth, geth->freeq_order);
 
 	geth->freeq_ring = dma_alloc_coherent(geth->dev,
 		sizeof(*geth->freeq_ring) << geth->freeq_order,
@@ -1193,6 +1203,7 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	struct gemini_ethernet_port *other_port;
 	struct net_device *other_netdev;
 	unsigned int new_size = 0;
+	unsigned int page_slots;
 	unsigned int new_order;
 	int ret;
 
@@ -1206,9 +1217,6 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 		other_port = geth->port0;
 	other_netdev = other_port ? other_port->netdev : NULL;
 
-	if (other_netdev && netif_running(other_netdev))
-		return -EBUSY;
-
 	new_size = 1 << (port->rxq_order + 1);
 	netdev_dbg(netdev, "port %d size: %d order %d\n",
 		   netdev->dev_id,
@@ -1225,8 +1233,15 @@ 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_ring && geth->freeq_order == new_order)
-		return 0;
+	if (geth->freeq_ring) {
+		page_slots = geth_freeq_page_slots(geth, geth->freeq_order);
+		if (geth->freeq_order >= new_order &&
+		    geth->num_freeq_pages >= page_slots)
+			return 0;
+	}
+
+	if (other_netdev && netif_running(other_netdev))
+		return -EBUSY;
 
 	disable_irq(geth->port1->irq);
 	geth_set_freeq_irq(geth, false);
@@ -2008,10 +2023,7 @@ static int gmac_open(struct net_device *netdev)
 	phy_start(netdev->phydev);
 
 	err = geth_resize_freeq(port);
-	/* It's fine if it's just busy, the other port has set up
-	 * the freeq in that case.
-	 */
-	if (err && (err != -EBUSY)) {
+	if (err) {
 		netdev_err(netdev, "could not resize freeq\n");
 		goto err_stop_phy;
 	}
@@ -2370,14 +2382,20 @@ static int gmac_set_ringparam(struct net_device *netdev,
 			      struct netlink_ext_ack *extack)
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
+	unsigned int old_rxq_order;
 	int err = 0;
 
 	if (netif_running(netdev))
 		return -EBUSY;
 
 	if (rp->rx_pending) {
+		old_rxq_order = port->rxq_order;
 		port->rxq_order = min(15, ilog2(rp->rx_pending - 1) + 1);
 		err = geth_resize_freeq(port);
+		if (err) {
+			port->rxq_order = old_rxq_order;
+			return err;
+		}
 	}
 	if (rp->tx_pending) {
 		port->txq_order = min(15, ilog2(rp->tx_pending - 1) + 1);

-- 
2.55.0

[PATCH net-next v3 08/12] 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.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
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 2345d582cb59..87c537ae83e7 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;
 }
@@ -1110,6 +1119,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 v3 09/12] net: ethernet: cortina: Recycle claimed free queue pages

[Linus Walleij]

The lifetime fix releases each mapping after its last descriptor is
claimed. Refilling therefore allocates and maps a fresh page at line
rate even after the network stack has released all references to an old
page.

Keep the free queue base reference and streaming mapping after the
fragments are claimed. Once the page reference count returns to one,
synchronize the complete page for the device, restore its fragment
mappings and post it again. Fall back to allocating a new page while old
fragments remain in the stack.

This retains the existing free queue allocator without introducing a
page pool.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 130 +++++++++++++++++++++++++++-------
 1 file changed, 104 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 87c537ae83e7..a0186cc4b943 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -173,6 +173,7 @@ struct gemini_ethernet {
 	unsigned int	num_freeq_pages;
 	unsigned long	*freeq_page_bitmap;
 	unsigned int	freeq_page_cursor;
+	unsigned int	freeq_recycle_pending;
 	spinlock_t	freeq_lock; /* Locks queue from reentrance */
 };
 
@@ -762,6 +763,35 @@ static int geth_freeq_alloc_slot(struct gemini_ethernet *geth)
 	return slot;
 }
 
+static int geth_freeq_recycle_slot(struct gemini_ethernet *geth)
+{
+	unsigned int slot = geth->freeq_page_cursor;
+	unsigned int scanned;
+
+	lockdep_assert_held(&geth->freeq_lock);
+
+	if (!geth->freeq_recycle_pending)
+		return -ENOSPC;
+
+	for (scanned = 0; scanned < geth->num_freeq_pages; scanned++) {
+		struct gmac_queue_page *gpage = &geth->freeq_pages[slot];
+
+		if (gpage->page && !gpage->fragments &&
+		    page_ref_count(gpage->page) == 1) {
+			geth->freeq_page_cursor = slot + 1;
+			if (geth->freeq_page_cursor == geth->num_freeq_pages)
+				geth->freeq_page_cursor = 0;
+			geth->freeq_recycle_pending--;
+			return slot;
+		}
+
+		if (++slot == geth->num_freeq_pages)
+			slot = 0;
+	}
+
+	return -ENOSPC;
+}
+
 static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
 				     dma_addr_t mapping,
 				     unsigned int *page_offs)
@@ -771,7 +801,6 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
 	unsigned long index;
 	unsigned long flags;
 	dma_addr_t page_mapping;
-	unsigned int slot;
 	struct page *page;
 	bool valid;
 
@@ -794,16 +823,8 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
 				      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_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);
-		put_page(page);
-	}
+	if (!--gpage->fragments)
+		geth->freeq_recycle_pending++;
 
 	*page_offs = mapping - page_mapping;
 	spin_unlock_irqrestore(&geth->freeq_lock, flags);
@@ -896,25 +917,21 @@ static int geth_freeq_map_page(struct gemini_ethernet *geth,
 	return 0;
 }
 
-static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int pn,
-			       struct page *page, dma_addr_t page_mapping)
+static int geth_freeq_post_page(struct gemini_ethernet *geth, unsigned int pn,
+				struct gmac_queue_page *gpage, bool recycle)
 {
 	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 = gpage->mapping;
 	dma_addr_t mapping;
+	struct page *page = gpage->page;
 	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
@@ -925,9 +942,6 @@ static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int pn,
 	fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
 	fragments = 1 << fpp_order;
 
-	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,
@@ -936,11 +950,14 @@ static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int pn,
 		if (ret)
 			goto err_mappings;
 	}
+	if (recycle)
+		dma_sync_single_for_device(geth->dev, page_mapping, PAGE_SIZE,
+					   DMA_FROM_DEVICE);
 	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",
+	dev_dbg(geth->dev, "post 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--) {
@@ -959,9 +976,51 @@ static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int pn,
 		xa_erase(&geth->freeq_mappings,
 			 geth_freeq_mapping_index(geth, mapping));
 	}
-	gpage->page = NULL;
-	gpage->mapping = 0;
-	__clear_bit(slot, geth->freeq_page_bitmap);
+	return ret;
+}
+
+static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int pn,
+			       struct page *page, dma_addr_t page_mapping)
+{
+	struct gmac_queue_page *gpage;
+	int ret;
+	int slot;
+
+	lockdep_assert_held(&geth->freeq_lock);
+
+	slot = geth_freeq_alloc_slot(geth);
+	if (slot < 0)
+		return slot;
+
+	gpage = &geth->freeq_pages[slot];
+	gpage->page = page;
+	gpage->mapping = page_mapping;
+	ret = geth_freeq_post_page(geth, pn, gpage, false);
+	if (ret) {
+		gpage->page = NULL;
+		gpage->mapping = 0;
+		__clear_bit(slot, geth->freeq_page_bitmap);
+	}
+
+	return ret;
+}
+
+static int geth_freeq_recycle_page(struct gemini_ethernet *geth,
+				   unsigned int pn)
+{
+	int ret;
+	int slot;
+
+	lockdep_assert_held(&geth->freeq_lock);
+
+	slot = geth_freeq_recycle_slot(geth);
+	if (slot < 0)
+		return slot;
+
+	ret = geth_freeq_post_page(geth, pn, &geth->freeq_pages[slot], true);
+	if (ret)
+		geth->freeq_recycle_pending++;
+
 	return ret;
 }
 
@@ -980,6 +1039,7 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
 	unsigned long flags;
 	union dma_rwptr rw;
 	unsigned int m_pn;
+	bool recycle = true;
 
 	/* Mask for page */
 	m_pn = (1 << (geth->freeq_order - fpp_order)) - 1;
@@ -995,9 +1055,26 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
 		pn = rw.bits.wptr >> fpp_order;
 		epn = (rw.bits.rptr >> fpp_order) - 1;
 		epn &= m_pn;
+		ret = -ENOSPC;
+		if (pn != epn && recycle) {
+			ret = geth_freeq_recycle_page(geth, pn);
+			if (!ret) {
+				count += 1 << fpp_order;
+				pn++;
+				pn &= m_pn;
+				writew(pn << fpp_order,
+				       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
+			}
+			if (ret == -ENOSPC)
+				recycle = false;
+		}
 		spin_unlock_irqrestore(&geth->freeq_lock, flags);
 		if (pn == epn)
 			break;
+		if (!ret)
+			continue;
+		if (ret != -ENOSPC)
+			break;
 
 		ret = geth_freeq_map_page(geth, &page, &page_mapping);
 		if (ret)
@@ -1073,6 +1150,7 @@ static void geth_freeq_release_pages(struct gemini_ethernet *geth)
 		put_page(page);
 	}
 	xa_destroy(&geth->freeq_mappings);
+	geth->freeq_recycle_pending = 0;
 }
 
 static unsigned int

-- 
2.55.0

[PATCH net-next v3 10/12] 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.
Account these descriptor inconsistencies as drops rather than wire length
errors.

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index a0186cc4b943..6b333eb81f9c 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1697,6 +1697,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;
@@ -1771,6 +1772,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_drop;
+
 			page_offs += NET_IP_ALIGN;
 			frag_len -= NET_IP_ALIGN;
 			frag_nr = 0;
@@ -1779,15 +1783,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_drop;
 			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_drop;
 
-		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_drop;
+		}
 
 		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);
 		skb->len += frag_len;

-- 
2.55.0

[PATCH net-next v3 11/12] 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. Count the discarded frame as dropped under the RX
statistics sequence counter.

Fixes: 06937db21ee3 ("net: ethernet: cortina: Make RX SKB per-port")
Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/ethernet/cortina/gemini.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 6b333eb81f9c..2ebfbbe84eba 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -2173,6 +2173,12 @@ 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);
+		u64_stats_update_begin(&port->rx_stats_syncp);
+		port->stats.rx_dropped++;
+		u64_stats_update_end(&port->rx_stats_syncp);
+	}
 	port->rx_skb = NULL;
 	port->rx_frag_nr = 0;
 	port->rx_dropping = false;

-- 
2.55.0

[PATCH net-next v3 12/12] 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 2ebfbbe84eba..437f36b1738c 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>
@@ -471,6 +473,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);
@@ -539,7 +552,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

Re: [PATCH net-next v3 01/12] net: ethernet: cortina: Keep PHY-less port bound for shared IRQ

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 · 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] The new guard in geth_resize_freeq() only checks geth->port1,
  but the new gemini_port_clear() in the probe error path can now leave…
- [Medium] Port 0 still loses the shared free-queue handler when the IRQ-
  only port 1 is unbound or fails probe late.
- [Medium] The new `if (!geth->port1) return -ENODEV;` check in
  geth_resize_freeq() is a lockless check-then-use that does not…

--

net: ethernet: cortina: Keep PHY-less port bound for shared IRQ

The commit keeps Gemini ethernet port children that have no PHY
description bound as IRQ-only ports. This keeps the shared free queue
threaded IRQ, which is routed through port 1, registered. It also clears
the parent's port pointers on probe failure and removal, and makes
geth_resize_freeq() return -ENODEV when port 1 is absent.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 2fe7fd0202d26..2be3e90519811 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1048,10 +1048,15 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	u32 en;
>  	int ret;
>  
> +	/* The software free queue interrupt is routed through port 1. */
> +	if (!geth->port1)
> +		return -ENODEV;
> +

[Severity: High]
Is checking only geth->port1 enough here? With the new
gemini_port_clear() call in the unprepare path of
gemini_ethernet_port_probe(), geth->port0 can now be NULL by the time
port 1 probes. In that case the shared block is never initialized.

Take a board with PHYs on both ports:

port 0 probe
  geth->port0 = port;
  gemini_ethernet_init()          /* port1 NULL, returns early */
  gmac_setup_phy() fails          /* e.g. -ENODEV, PHY not on MDIO */
  unprepare: gemini_port_clear()  /* geth->port0 = NULL */

port 1 probe
  geth->port1 = port;
  gemini_ethernet_init()          /* port0 NULL, returns early again */
  register_netdev()               /* eth1 is registered */

gemini_ethernet_init() is the only place that programs the
GLOBAL_INTERRUPT_SELECT_* and SWFQ/HWFQ registers and sets
freeq_frag_order and freeq_order:

	if (geth->port0 && geth->port1)
		geth->initialized = true;
	else
		return;
	...
	geth->freeq_frag_order = DEFAULT_RX_BUF_ORDER;

When eth1 is opened, gmac_open()->geth_resize_freeq() passes this check
and calls geth_setup_freeq() while freeq_frag_order is still 0 from
devm_kzalloc(). That makes fpp_order equal to PAGE_SHIFT and
pages = 1024 >> 12 = 0. kzalloc_objs(..., 0) then returns ZERO_SIZE_PTR,
which passes the NULL check.

geth_fill_freeq() then does:

	m_pn = (1 << (geth->freeq_order - fpp_order)) - 1;
	...
		gpage = &geth->freeq_pages[pn];
		page = gpage->page;

The shift is out of range, and the page lookup goes through
ZERO_SIZE_PTR. Can this oops?

Later in the series, "Scale Gemini RX queues to system memory" sends the
same uninitialized state to geth_freeq_post_page(). That function writes
1 << 12 descriptors starting at geth->freeq_ring + (pn << 12) into a
ring sized for 1 << 10 entries. Would that write past the DMA coherent
allocation? As far as I can tell, nothing later in the series closes
this path.

Before this patch, geth->port0 stayed non-NULL (dangling), so
gemini_ethernet_init() still ran.

Should this check geth->initialized, or both ports, instead of only
geth->port1?

>  	if (netdev->dev_id == 0)
> -		other_netdev = geth->port1->netdev;
> +		other_port = geth->port1;
>  	else
> -		other_netdev = geth->port0->netdev;
> +		other_port = geth->port0;
> +	other_netdev = other_port ? other_port->netdev : NULL;

[Severity: Medium]
Is this check-then-use of geth->port1 safe if port 1 is probed or
removed at the same time? geth_resize_freeq() runs under RTNL from
gmac_open() and gmac_set_ringparam(). geth->port1 is written without
RTNL or any other common lock.

On the remove side, gemini_port_remove() skips this block for the
IRQ-only port:

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

so it never takes RTNL. gemini_port_clear() runs next, and devres frees
the port (it is part of the devm netdev allocation) right afterwards.

A concurrent open of eth0 can pass the guard and load
other_port = geth->port1. It then reads other_port->netdev and
other_port->rxq_order. Could those reads hit freed memory?

Later in the series, geth_resize_freeq() also reads geth->port1->irq
again for disable_irq() and enable_irq(). Could that be a NULL
dereference if port 1 is cleared between the reads?

On the probe side, gemini_ethernet_port_probe() publishes geth->port1
before gemini_ethernet_init() and before devm_request_threaded_irq().

Can an eth0 open in that window resize the queue and enable SWFQ_EMPTY
before the handler exists? gemini_ethernet_init() may still be writing
GLOBAL_INTERRUPT_ENABLE_4_REG, the SW free queue registers,
freeq_frag_order and freeq_order without irq_lock at that point.

>  
>  	if (other_netdev && netif_running(other_netdev))
>  		return -EBUSY;

[ ... ]

> @@ -2392,6 +2406,7 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
>  	}
>  	clk_disable_unprepare(port->pclk);
>  	geth_cleanup_freeq(port->geth);
> +	gemini_port_clear(port);
>  }

[Severity: Medium]
Can port 0 still lose the shared free queue handler here? Port 1 now
stays bound only to provide the SWFQ threaded handler. Nothing stops it
from being unbound while eth0 is running: the port driver does not set
suppress_bind_attrs, and the IRQ-only remove path does not check the
sibling port.

In this commit, unbinding the IRQ-only port 1 while eth0 is up also
calls geth_cleanup_freeq() unconditionally. That frees and unmaps the
shared ring and pages while port 0's RX DMA, and possibly
gemini_port_irq_thread(), still use them. Is that a use-after-free? The
later patch "net: ethernet: cortina: Keep shared free queue
parent-owned" removes this call.

Even with that later patch, this unbind still runs gemini_port_clear():

  echo 6000c000.ethernet-port > /sys/bus/platform/drivers/gemini-ethernet-port/unbind

devres then frees port 1's threaded IRQ. SWFQ_EMPTY_INT_BIT stays set in
GLOBAL_INTERRUPT_ENABLE_4_REG, but no handler is left on int1, and
gemini_port_irq_thread() is the only refill path.

Would port 0 RX stall once the posted fragments are used up, while eth0
still reports itself as running?

The same thing can happen if eth0 is opened after port 1 publishes
geth->port1, and port 1 then fails late in gmac_setup_phy() or
register_netdev(). The unprepare path clears geth->port1, and devres
frees the IRQ.

This looks like the "releases the threaded handler needed by the shared
queue" case the commit message describes, reached through the remove
path and the late probe failure path.

[ ... ]

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

Re: [PATCH net-next v3 02/12] net: ethernet: cortina: Keep shared free queue parent-owned

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch makes the parent device own the shared software
  free queue (SWFQ), but the IRQ handler that refills it is still owned…
- [Medium] gemini_port_clear() clears geth->port0/port1 without any
  synchronization against sibling readers in geth_resize_freeq().

Pre-existing issues:
- [Critical] At this commit, geth_cleanup_freeq() frees each queue page
  with `while (page_ref_count(gpage->page) > 0) put_page(gpage->page);`…
- [High] This bug was not introduced by this patch, but the new
  devm_of_platform_depopulate() call now reaches it directly.

--

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

This commit stops each port's remove path from tearing down the shared
software free queue. That cleanup now runs only from the parent remove
callback. The parent first depopulates both port devices and masks
GLOBAL_INTERRUPT_ENABLE_4_REG, then calls geth_cleanup_freeq(). That
function now returns early if no queue was allocated.

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

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 2be3e90519811..15ad9f721dd52 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);

[Severity: Critical]
This is a pre-existing issue, but gemini_ethernet_remove() is now the only
caller of geth_cleanup_freeq(). Could the page release loop further down
in this function over-release pages?

drivers/net/ethernet/cortina/gemini.c:geth_cleanup_freeq() {
    ...
		gpage = &geth->freeq_pages[pn];
		while (page_ref_count(gpage->page) > 0)
			put_page(gpage->page);
    ...
}

This loop also drops the references held by skbs that gmac_rx() passed up
the stack. Pages still referenced by sockets would then go back to the
allocator. The loop also reads the refcount of a page that may already
have been freed and reused.

If the parent is unbound, or the module is removed, while delivered skbs
are still queued, could this cause a use-after-free or a double free?

This appears to be fixed later in the series by "net: ethernet: cortina:
Preserve in-flight free queue pages". In that patch,
geth_freeq_release_pages() drops only the references the driver owns.

> @@ -2405,7 +2408,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);
>  	gemini_port_clear(port);
>  }

[Severity: Medium]
With the parent now owning the queue, what happens if only port1 is
unbound through sysfs while port0 is up?

The port driver does not set suppress_bind_attrs, so this can be done
from sysfs. After this change, gemini_port_remove() on port1 leaves
GLOBAL_SW_FREEQ_BASE_SIZE_REG programmed. It also leaves
SWFQ_EMPTY_INT_BIT set in GLOBAL_INTERRUPT_ENABLE_4_REG.

Devres then frees the port1 handler that gemini_ethernet_port_probe()
requested:

	ret = devm_request_threaded_irq(port->dev,
					port->irq,
					gemini_port_irq,
					gemini_port_irq_thread,
	...

However, gemini_ethernet_init() routes the free queue interrupt to the
port1 line:

	 *	FreeQ intr -----------> int1 ----> eth1

The queue-empty event would then arrive on an IRQ with no handler, while
port0 keeps using up fragments. A refill would only happen if an
unrelated int0 interrupt ran gemini_port_irq() for port0 and it noticed
the STATUS_4 bit. Once RX is starved, that may never happen.

Can port0 RX stall in this case?

Also, geth->port1 is now NULL, so reopening port0 would fail in
geth_resize_freeq() while the hardware queue is still live and armed:

	/* The software free queue interrupt is routed through port 1. */
	if (!geth->port1)
		return -ENODEV;

The new writel() to GLOBAL_INTERRUPT_ENABLE_4_REG runs only from
gemini_ethernet_remove(), not when port1 is unbound by itself. This looks
the same in the final state of the series.

[Severity: Medium]
What keeps gemini_port_clear() from running while the sibling port is
inside geth_resize_freeq()?

When gmac_open() runs for port0, it holds only RTNL, and
geth_resize_freeq() does:

	if (netdev->dev_id == 0)
		other_port = geth->port1;
	else
		other_port = geth->port0;
	other_netdev = other_port ? other_port->netdev : NULL;
	...
		new_size += 1 << (other_port->rxq_order + 1);

On the port1 unbind side, gemini_port_clear() runs after
unregister_netdev() has released RTNL. If port1 has no netdev, the unbind
path takes RTNL at no point. Devres then frees the netdev that contains
the port struct.

The platform bus takes no parent lock, so these two paths do not share a
lock.

Could geth_resize_freeq() read other_port->netdev or
other_port->rxq_order after the port struct has been freed?

The window gets wider later in the series. There, geth_resize_freeq()
reads geth->port1 again for enable_irq(geth->port1->irq) after
geth_setup_freeq(). geth_setup_freeq() can sleep in GFP_KERNEL and
dma_alloc_coherent() allocations.

If port1 is cleared during that window, would this be a NULL dereference?
Or a use-after-free plus an unbalanced enable_irq() on a freed IRQ?

In the final state of the series, geth_freeq_page_slots() also
dereferences geth->port0 and geth->port1 with only a NULL check.

> @@ -2706,6 +2708,8 @@ static void gemini_ethernet_remove(struct platform_device *pdev)
>  {
>  	struct gemini_ethernet *geth = platform_get_drvdata(pdev);
>  
> +	devm_of_platform_depopulate(&pdev->dev);

[Severity: High]
This isn't a bug introduced by this patch, but the new
devm_of_platform_depopulate() call now reaches it straight from the parent
remove. If a port's netdev is up, does gemini_port_remove() end up calling
phy_stop() with a NULL phydev?

gemini_port_remove()
  phy_disconnect(port->netdev->phydev)
    phy_detach()
      phydev->attached_dev->phydev = NULL;
  unregister_netdev(port->netdev)
    ... __dev_close_many()
      gmac_stop()
        phy_stop(netdev->phydev)   <-- now NULL
          struct net_device *dev = phydev->attached_dev;

phy_stop() does not check for NULL. This looks like a deterministic oops
when the parent or a port is unbound, or the module is removed, while the
interface is up.

The same ordering is present before this patch and in the final state of
the series.

> +	writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
>  	geth_cleanup_freeq(geth);
>  	geth->initialized = false;
>  }

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

Re: [PATCH net-next v3 03/12] 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 (1) · High: 1 · Medium: 1 (2) · 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).

- [High] The free queue drain is incomplete.
- [Medium] This patch adds two new reads of geth->port1 inside
  geth_resize_freeq(): disable_irq(geth->port1->irq) (line 1104) and…
- [Low] Comments about the initial free queue setup no longer match the
  code.

Pre-existing issues:
- [Critical] geth_cleanup_freeq() does `while
  (page_ref_count(gpage->page) > 0) put_page(gpage->page);` (line 1024).
- [Medium] Free queue pages are mapped with dma_map_single(...,
  PAGE_SIZE, DMA_FROM_DEVICE) in geth_freeq_alloc_map_page() (line 822).
- [Medium] Rebuilding the free queue at a smaller order does not reset
  the hardware SWFQ pointers.

--

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

This patch disables port 1's Linux IRQ for the whole of geth_resize_freeq().
The aim is to drain the threaded free queue handler before the ring and page
metadata are torn down and rebuilt. It also adds a geth_set_freeq_irq()
helper, re-enables SWFQ_EMPTY_INT_BIT only when setup succeeds, and clears
the freed free queue state.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 15ad9f721dd52..a2daf22e76989 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -984,6 +984,8 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
>  	}
>  
>  	kfree(geth->freeq_pages);
> +	geth->freeq_pages = NULL;
> +	geth->num_freeq_pages = 0;
>  err_freeq:

[Severity: Medium]
This isn't a bug introduced by this patch, but the free queue pages are
mapped with PAGE_SIZE in geth_freeq_alloc_map_page():

    mapping = dma_map_single(geth->dev, page_address(page),
                             PAGE_SIZE, DMA_FROM_DEVICE);

The err_freeq_alloc unwind just above here in geth_setup_freeq() and the
teardown loop in geth_cleanup_freeq() both unmap with frag_len, which is
normally 2048:

    dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);

Shouldn't the unmap size match the PAGE_SIZE used when mapping? As written,
this would trigger DMA-API debug warnings. On swiotlb or non-coherent setups
it would also only partly unmap and sync the page.

The commit "net: ethernet: cortina: Correct free queue DMA mappings" later
in the series appears to fix this.

[ ... ]

> @@ -1024,10 +1026,28 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  	}
>  
>  	kfree(geth->freeq_pages);
> +	geth->freeq_pages = NULL;
> +	geth->num_freeq_pages = 0;

[Severity: Critical]
This is a pre-existing issue, but the loop just above in geth_cleanup_freeq()
drops every reference on each free queue page:

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

gmac_rx() gives fragment references from these pages to skbs:

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

Those skbs can still be in GRO or in socket receive queues when a resize
runs, for example on open after an RX ring change, or on ethtool -G.

Could this free pages that in-flight skbs still reference? The later skb
free would then call put_page() on a freed or reallocated page.

The commit "net: ethernet: cortina: Preserve in-flight free queue pages"
later in the series appears to fix this by giving each posted fragment its
own reference.

>  
>  	dma_free_coherent(geth->dev,
>  			  sizeof(*geth->freeq_ring) << geth->freeq_order,
>  			  geth->freeq_ring, geth->freeq_dma_base);
> +	geth->freeq_ring = NULL;
> +}

[Severity: Medium]
This is also a pre-existing issue. When the queue is rebuilt at a smaller
order, can the hardware SWFQ pointers end up outside the new ring?

geth_cleanup_freeq() only sets wptr equal to rptr and clears the base/size
register:

    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);

The only place that writes 0 to GLOBAL_SWFQ_RWPTR_REG is
gemini_ethernet_init().

After geth_resize_freeq() sets freeq_order = new_order, geth_setup_freeq()
calls geth_fill_freeq(geth, false). That starts from the old pointer and does
not mask it with the new m_pn:

    pn = (refill ? rw.bits.wptr : rw.bits.rptr) >> fpp_order;
    ...
    gpage = &geth->freeq_pages[pn];

If rptr had advanced past the end of the smaller ring, wouldn't this index
past the newly allocated freeq_pages[] and write freeq_ring entries out of
bounds? One way to get there is to run traffic, take both ports down, and
shrink the RX rings with ethtool -G.

Whether this happens depends on the hardware keeping rptr when
GLOBAL_SW_FREEQ_BASE_SIZE_REG is reprogrammed, and the driver code doesn't
show that. The final state of the series still derives pn from an unmasked
wptr, so later patches don't seem to fix this.

[ ... ]

> @@ -1080,16 +1098,11 @@ 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;

[Severity: Low]
This isn't a bug, but the new freeq_ring check forces the first setup
whatever freeq_order holds. That makes the sentinel in gemini_ethernet_init()
redundant, and its comment no longer matches the code:

    /* This makes the queue resize on probe() so that we
     * set up and enable the queue IRQ. FIXME: fragile.
     */
    geth->freeq_order = 1;

The kernel-doc for geth_resize_freeq() also still says "This gets called at
least once during probe()". The only callers are gmac_open() and
gmac_set_ringparam(). That probe() wording was there before this patch.

Could these comments be updated?

>  
> -	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);
> +	disable_irq(geth->port1->irq);
> +	geth_set_freeq_irq(geth, false);

[Severity: High]
Does draining only port1->irq here cover every instance of the free queue
handler?

gemini_ethernet_port_probe() registers the same handler pair on both ports'
IRQ lines:

    ret = devm_request_threaded_irq(port->dev,
                                    port->irq,
                                    gemini_port_irq,
                                    gemini_port_irq_thread,
                                    IRQF_SHARED,
                                    port_names[port->id],
                                    port);

gemini_port_irq() only reads the global status and enable registers and
never checks port->id:

    val = readl(geth->base + GLOBAL_INTERRUPT_STATUS_4_REG);
    en = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);

    if (val & en & SWFQ_EMPTY_INT_BIT) {
        ...
        ret = IRQ_WAKE_THREAD;
    }

Port 0's line stays enabled during the resize. It is shared with gmac_irq,
which gmac_open() requests before the resize, and irqpoll or misrouted IRQ
polling can also run it through try_one_irq().

If that handler runs between disable_irq(port1) and
geth_set_freeq_irq(geth, false), it wakes port 0's thread. Nothing here waits
for that thread. The thread then runs:

gemini_port_irq_thread()
    geth_fill_freeq(geth, true)
        gpage = &geth->freeq_pages[pn];

This can run while geth_cleanup_freeq() frees freeq_pages and sets it to
NULL, or while geth_setup_freeq() is still sleeping in its allocations.

Afterwards the thread ORs SWFQ_EMPTY_INT_BIT back into
GLOBAL_INTERRUPT_ENABLE_4_REG unconditionally, even if setup failed. The next
SWFQ interrupt would then run geth_fill_freeq() with freeq_pages set to NULL.

The commit message says:

  "disable_irq() drains hard and threaded handlers and prevents a new
  thread from being woken."

Isn't that true only for port 1's registration? Would gemini_port_irq() need
to filter SWFQ handling by port->id, or would both ports' IRQs need to be
disabled? The final state of the series still drains only port1->irq with the
same handler on both lines.

>  
>  	/* Drop the old queue */
>  	if (geth->freeq_ring)
> @@ -1103,10 +1116,9 @@ 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);
> +	if (!ret)
> +		geth_set_freeq_irq(geth, true);
> +	enable_irq(geth->port1->irq);

[Severity: Medium]
Can geth->port1 be NULL by the time this line runs?

The only NULL check is at the top of geth_resize_freeq():

    if (!geth->port1)
        return -ENODEV;

Between that check and this enable_irq() there is a long window where the
task can sleep. disable_irq() waits for the threaded handler, then
geth_cleanup_freeq() and geth_setup_freeq() run, with dma_alloc_coherent()
and GFP_KERNEL allocations.

Only RTNL serializes geth_resize_freeq(). gemini_port_clear() sets
geth->port1 = NULL without holding RTNL in two places:

- gemini_port_remove(), after unregister_netdev() returns, or with no netdev
  at all for a PHY-less port 1
- the probe unprepare error path

devm then frees port 1's IRQ action and the port memory. For example:

CPU0 (eth0 open)                 CPU1 (port 1 unbind or probe failure)
geth_resize_freeq()
  geth->port1 is non-NULL
  disable_irq(geth->port1->irq)
  geth_setup_freeq() sleeps
                                 gemini_port_clear()
                                   geth->port1 = NULL;
                                 devm frees the IRQ and the port
  enable_irq(geth->port1->irq)   <- NULL dereference

Probe has a second window. gemini_ethernet_port_probe() sets
geth->port1 = port before it calls devm_request_threaded_irq(). A resize from
port 0 in that window calls disable_irq() on a descriptor with no action.

Won't the first __setup_irq() then reset the depth through irq_startup()?
That would cancel the disable, and the later enable_irq() would hit the
"Unbalanced enable for IRQ" warning.

Before this patch, geth->port1 was only read at the top of the function.
These two re-reads are unchanged in the final state of the series.

[ ... ]

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

Re: [PATCH net-next v3 04/12] 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 (2) · Medium: 3 (2) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In geth_freeq_alloc_map_page()
  (drivers/net/ethernet/cortina/gemini.c:856-862), replacing a page…
- [Medium] geth_freeq_alloc_map_page() maps PAGE_SIZE (line 833).
- [Medium] geth_freeq_lookup() takes freeq_lock with spin_lock_irqsave()
  (line 738) and then linearly scans all num_freeq_pages entries (line…

Pre-existing issues:
- [High] geth_cleanup_freeq() (lines 1034-1036) runs 'while
  (page_ref_count(gpage->page) > 0) put_page(gpage->page);'.
- [High] gmac_rx() takes frag_len from the 16-bit hardware buffer_size
  (line 1523).
- [Medium] This problem wasn't introduced by this patch, but
  gmac_napi_poll() (line 1629-1635) wraps the whole gmac_rx() call in…
- [Medium] Freeq pages are mapped DMA_FROM_DEVICE once.

--

net: ethernet: cortina: Correct free queue DMA mappings

This commit keeps the free queue page DMA base separate from the fragment
iterator, drops the incorrect unmap on page replacement, and unwinds setup
mappings with PAGE_SIZE. It also replaces gmac_get_queue_page() with
geth_freeq_lookup(), which works out the fragment offset from the saved DMA
base under freeq_lock and rejects mappings that don't fit in 32 bits.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index a2daf22e76989..809274aff8e56 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -724,32 +724,43 @@ 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)
>  {
> +	unsigned int frag_len = 1 << geth->freeq_frag_order;
>  	struct gmac_queue_page *gpage;
> -	dma_addr_t mapping;
> +	unsigned long flags;
> +	dma_addr_t page_mapping;
> +	struct page *page = NULL;
>  	int i;
>  
> -	/* Only look for even pages */
> -	mapping = addr & PAGE_MASK;
> -
> +	spin_lock_irqsave(&geth->freeq_lock, flags);

[Severity: Medium]
This isn't a bug introduced by this patch, but on 32-bit PREEMPT_RT
kernels gmac_napi_poll() wraps the whole gmac_rx() call in the
rx_stats_syncp writer section:

gmac_napi_poll()
    u64_stats_update_begin(&port->rx_stats_syncp);
    received = gmac_rx(napi->dev, budget, &freeq_consumed, &reschedule);
    ...
    u64_stats_update_end(&port->rx_stats_syncp);

On 32-bit, u64_stats_update_begin() disables preemption, and on
PREEMPT_RT a spinlock_t can sleep.

Can the freeq_lock taken here for each descriptor trigger a "sleeping
function called from invalid context" splat with
CONFIG_DEBUG_ATOMIC_SLEEP?

The baseline gmac_rx() already took irq_lock and called napi_gro_frags()
inside the same section. The pattern is still there at the end of the
series, where geth_freeq_claim() runs inside it.

>  	if (!geth->freeq_pages) {
>  		dev_err_ratelimited(geth->dev,
>  				    "try to get page with no page list\n");
> -		return NULL;
> +		goto unlock;
>  	}
>  
>  	/* 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)
> -			return gpage;
> +		if (!gpage->page || mapping < gpage->mapping)
> +			continue;
> +
> +		page_mapping = gpage->mapping;
> +		if (mapping - page_mapping > PAGE_SIZE - frag_len ||
> +		    ((mapping - page_mapping) & (frag_len - 1)))
> +			continue;
> +
> +		page = gpage->page;
> +		*page_offs = mapping - page_mapping;
> +		break;
>  	}

[Severity: Medium]
gmac_rx() calls geth_freeq_lookup() once per RX descriptor in NAPI
context. Doesn't this put a linear scan over all num_freeq_pages entries,
with interrupts disabled, on the RX hot path?

The next patch in the series, "Index free queue fragments with XArray",
replaces this scan with an xa_load() in geth_freeq_claim().

[ ... ]

> @@ -833,31 +853,25 @@ 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;
> +	gpage = &geth->freeq_pages[pn];
> +	if (gpage->page)
> +		put_page(gpage->page);
> +
> +	/* Then put our new mapping into the page table */
> +	gpage->mapping = page_mapping;
> +	gpage->page = page;

[Severity: Medium]
With the replacement unmap gone, what releases the old page's PAGE_SIZE
streaming mapping here?

geth_fill_freeq() takes this path whenever page_ref_count(page) > 1:

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

The old gpage->mapping is overwritten right after the put_page(), and
geth_cleanup_freeq() only unmaps the address currently posted in the ring.

At this commit, does every replacement leak one DMA mapping when an IOMMU,
swiotlb or dma-debug is in use?

The commit message says these mappings are released later in the series.
After "Preserve in-flight free queue pages" and "Recycle claimed free queue
pages", geth_freeq_release_pages() unmaps every live slot with PAGE_SIZE.
The old code also left the old mapping live.

>  
>  	freeq_entry = geth->freeq_ring + (pn << fpp_order);

[ ... ]

> @@ -1019,7 +1031,6 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  
>  		mapping = geth->freeq_ring[pn << fpp_order].word2.buf_adr;
>  		dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);

[Severity: Medium]
geth_freeq_alloc_map_page() maps PAGE_SIZE, and this patch changes the
geth_setup_freeq() error path to unmap with gpage->mapping and PAGE_SIZE.

Should geth_cleanup_freeq() do the same, rather than using frag_len and
the address read back from the descriptor? As it stands, the two release
sites for the same mapping disagree, and dma-debug would warn about a
size mismatch on ifdown or remove.

At the end of the series, geth_cleanup_freeq() goes through
geth_freeq_release_pages(), which calls
dma_unmap_single_attrs(geth->dev, gpage->mapping, PAGE_SIZE,
DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC).

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

[Severity: High]
This isn't a bug introduced by this patch, but can this loop drop
references held by skbs that gmac_rx() already passed to the stack through
napi_gro_frags()?

It keeps calling put_page() until the refcount reaches zero. That can free
pages the stack is still using, and page_ref_count() is read again after
the final put_page().

Later in the series, geth_freeq_release_pages() drops only the
gpage->fragments references plus one base reference, and leaves
skb-owned references alone.

[ ... ]

> @@ -1532,14 +1540,12 @@ 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);
> -		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;

[Severity: High]
This isn't a bug introduced by this patch, but the length passed to
skb_fill_page_desc() further down in gmac_rx() comes straight from the
hardware descriptor:

		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;
		...
		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);

Can frag_len underflow here? That would happen if buffer_size is smaller
than NET_IP_ALIGN, or if frame_len is smaller than skb->len.

Nothing checks the result against the 2 KiB fragment size or against
PAGE_SIZE - page_offs. Could a malformed descriptor expose adjacent memory
to the stack?

The "Validate RX fragment lengths" patch later in the series adds these
checks.

[Severity: Medium]
This is a pre-existing issue, but the freeq pages are mapped
DMA_FROM_DEVICE once, and gemini.c has no dma_sync_* calls at this commit.

geth_fill_freeq() reposts reused pages with only page_ref_add(). gmac_rx()
then hands fragments written by the hardware to skb_fill_page_desc()
without syncing them for the CPU.

Could this give the stack stale cache contents on non-coherent DMA setups?

Later in the series, geth_freeq_claim() calls
dma_sync_single_range_for_cpu() for each claimed fragment.
geth_freeq_post_page() syncs for the device when recycling, and
geth_freeq_release_pages() syncs fragments still owned by the hardware
before unmapping. A later commit also notes that the CPU sync is a no-op
on Gemini's FA526.

>  
>  		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/20261002-gemini-ethernet-fixes-3-v3-0-3e1f25890ae8%40kernel.org

Re: [PATCH net-next v3 05/12] 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: 0 (2) · Medium: 1 (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] The commit message frames this patch as an efficiency change
  but tags it as a bug fix.
- [Low] In `geth_setup_freeq()`, the comment above the `freeq_pages`
  allocation still reads `/* Allocate a mapping to page look-up index…

Pre-existing issues:
- [High] `geth_fill_freeq()` replaces a free queue page whose
  `page_ref_count() > 1` by calling `geth_freeq_alloc_map_page()`.
- [High] `geth_cleanup_freeq()` runs `while (page_ref_count(gpage->page)
  > 0) put_page(gpage->page);`.
- [Medium] Free queue pages are mapped once with
  `dma_map_single(DMA_FROM_DEVICE)` and then reused.

--

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

geth_freeq_lookup() used to scan every free queue page linearly. This
commit replaces that scan with an XArray keyed by each fragment's DMA
address in fragment units. Entries are inserted and erased in
geth_freeq_alloc_map_page(), which now asserts that freeq_lock is held,
and geth_setup_freeq() now takes the lock around page allocation.

> This isn't very efficient...
>
> Instead index each fragment in an XArray using its DMA address in
> fragment units.

[ ... ]

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

[Severity: Medium]
Is this meant to be a bug fix? The only reason given is efficiency, but
the Fixes: tag marks the patch for stable backport.

Before this patch, the linear scan in geth_freeq_lookup() returned the
same page and offset as the XArray lookup in every case, and failed in
the same cases. The only thing that changes is the lookup cost. No
user-visible failure or latency numbers are mentioned.

Documentation/process/maintainer-netdev.rst asks every bug fix to say
how the issue was found and validated, and how the fix was tested. For
hardware drivers it also asks for the hardware it was tested on.

The later patch "net: ethernet: cortina: Preserve in-flight free queue
pages" relies on this XArray to keep exact fragment DMA addresses until
they are claimed. If the Fixes: tag is there to mark this as a stable
prerequisite, could the commit message say so? Otherwise, should the tag
be dropped and the patch sent to net-next as an optimisation?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 809274aff8e56..ee604bc04fc37 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

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

[ ... ]

> +	index = geth_freeq_mapping_index(geth, mapping);
>
> -unlock:
> +	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: Medium]
This is a pre-existing issue, but is a CPU-direction DMA sync missing
before the fragment goes back to the stack?

The free queue pages are mapped once in geth_freeq_alloc_map_page() with
dma_map_single(..., DMA_FROM_DEVICE) and then reused. geth_fill_freeq()
reposts a page whose refcount is 1 with only:

	page_ref_add(page, 1 << fpp_order);

There is no dma_sync_single_for_device() call. After this lookup
succeeds, gmac_rx() passes the fragment to skb_fill_page_desc() without
calling dma_sync_single_range_for_cpu().

On a non-coherent system, could the CPU read stale cache lines instead
of the new DMA data?

A later commit message in the series says the CPU sync is currently a
no-op on Gemini's FA526, so the impact on the target SoC may be limited.

This is addressed later in the series. "net: ethernet: cortina: Preserve
in-flight free queue pages" adds dma_sync_single_range_for_cpu() to
geth_freeq_claim(). "net: ethernet: cortina: Recycle claimed free queue
pages" adds dma_sync_single_for_device() on recycle in
geth_freeq_post_page().

[ ... ]

> @@ -853,9 +863,26 @@ 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;
> +	fragments = 1 << fpp_order;
> +
>  	gpage = &geth->freeq_pages[pn];
> -	if (gpage->page)
> +	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));
> +		}
>  		put_page(gpage->page);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but what happens to fragments
of the old page that are still waiting in an RX queue?

geth_fill_freeq() replaces the page when its refcount is above 1:

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

A refcount above 1 can mean that some of the old page's fragments are
still unprocessed in either port's RX queue. This block erases all of
the old page's XArray entries and drops the base reference. For those
descriptors, geth_freeq_lookup() then returns NULL.

gmac_rx() logs "could not find mapping" and jumps to err_drop with page
still NULL. gmac_cleanup_rxq() logs "could not find page" and skips
put_page().

Does this leak the per-fragment reference added by page_ref_add(), and
with it the old page?

The old linear scan failed the same way, because gpage->page had already
been overwritten. This seems to be fixed later in the series by "net:
ethernet: cortina: Preserve in-flight free queue pages". There,
geth_freeq_claim() erases only the claimed fragment's index, and a page
is reposted only when gpage->fragments is zero and its refcount is 1.

[ ... ]

> @@ -965,9 +1005,13 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
>  	geth->num_freeq_pages = pages;
>
>  	dev_info(geth->dev, "allocate %d pages for queue\n", pages);

[Severity: Low]
This isn't a bug, but the comment just above this hunk in
geth_setup_freeq() still reads:

	/* Allocate a mapping to page look-up index */
	geth->freeq_pages = kzalloc_objs(*geth->freeq_pages, pages);

After this patch the lookup goes through
xa_load(&geth->freeq_mappings, index), and freeq_pages is only the
metadata table that the XArray entries point into. Could the comment be
updated?

> -	for (pn = 0; pn < pages; pn++)
> -		if (!geth_freeq_alloc_map_page(geth, pn))
> +	for (pn = 0; pn < pages; pn++) {
> +		spin_lock_irqsave(&geth->freeq_lock, flags);
> +		page = geth_freeq_alloc_map_page(geth, pn);
> +		spin_unlock_irqrestore(&geth->freeq_lock, flags);
> +		if (!page)
>  			goto err_freeq_alloc;
> +	}

[ ... ]

> @@ -1035,6 +1080,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);

[Severity: High]
This is a pre-existing issue, but can this loop free pages that SKBs
still reference?

gmac_rx() gives fragment references to the stack through
skb_fill_page_desc(). Those SKBs can still be queued in the stack when
the free queue is torn down, for example on a resize that calls
geth_cleanup_freeq() before geth_setup_freeq().

Dropping the refcount all the way to zero would free pages the stack is
still using. The loop also reads page_ref_count() again after the final
put_page().

This changes later in the series with "net: ethernet: cortina: Preserve
in-flight free queue pages". There, geth_freeq_release_pages() drops only
the gpage->fragments references owned by the hardware plus the single
base reference.

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

Re: [PATCH net-next v3 06/12] net: ethernet: cortina: Preserve in-flight free queue pages

[netdev-bot+sashiko]

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

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

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, page_slots in geth_setup_freeq() depends on
  port0/port1 rxq_order.
- [Low] Two problems with the commit message of this patch.
- [Low] geth_setup_freeq() now accepts a free queue with zero usable
  entries.

Pre-existing issues:
- [High] Out-of-bounds write into DMA ring memory when the free queue
  shrinks.
- [Medium] gmac_stop() clears port->rx_skb, rx_frag_nr and rx_dropping
  without calling napi_free_frags().
- [Medium] At this commit, gmac_rx() passes lengths taken from the
  hardware descriptor to skb_fill_page_desc() without bounding them to…

--

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

This patch stops tying free queue page metadata to free queue ring
positions. Metadata slots now come from a bitmap, so a refill no longer
overwrites pages that unprocessed RX descriptors still point to. Each
posted fragment gets its own page reference and per-fragment DMA sync,
pages are allocated and mapped outside the free queue lock, and the
initial fill must populate every usable entry.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index ee604bc04fc37..660e51634017b 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -760,6 +781,21 @@ static struct page *geth_freeq_lookup(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);

[Severity: Medium]

This isn't a bug introduced by this patch. Now that only the claimed
fragment is synced for the CPU, can gmac_rx() still produce a fragment
length that goes past it?

gmac_rx() takes the lengths directly from the hardware descriptor:

	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;
	...
	skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);

frag_len wraps around if buffer_size is less than NET_IP_ALIGN, or if
frame_len is less than skb->len.

Could an oversized frag then cover the sibling fragment, which is not
synced or is still owned by the device, or run past the end of the page?

A later commit in this series, "net: ethernet: cortina: Validate RX
fragment lengths", looks like it handles this by dropping such
descriptors.

> +	xa_erase(&geth->freeq_mappings, index);

[ ... ]

> @@ -933,47 +975,95 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)

[ ... ]

> -		if (page_ref_count(page) > 1) {
> -			unsigned int fl = (pn - epn) & m_pn;
> +		ret = geth_freeq_map_page(geth, &page, &page_mapping);
> +		if (ret)
> +			break;
>  
> -			if (fl > 64 >> fpp_order)
> -				break;
> +		spin_lock_irqsave(&geth->freeq_lock, flags);
>  
> -			page = geth_freeq_alloc_map_page(geth, pn);
> -			if (!page)
> -				break;

[Severity: Low]

The commit message says:

  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.

Does this describe the old refill accurately?

The removed code re-posted any page whose page_ref_count() had dropped
back to 1, without allocating. It called geth_freeq_alloc_map_page() only
while the fill level was at or below 64 >> fpp_order (32 pages, about
128 KiB).

The old geth_setup_freeq() also took and dropped the lock once per page.
The setup-time geth_fill_freeq(geth, false) did not allocate anything.

The commit message also does not say that page recycling and the fill
cap are removed here. With this patch, every refill iteration calls
geth_freeq_map_page(), which means alloc_page(GFP_ATOMIC) plus
dma_map_single(). geth_freeq_claim() unmaps and puts the page once its
last fragment is claimed, and the loop keeps going until the ring is
full.

Could the commit message mention this change in behaviour?

A later commit in this series, "net: ethernet: cortina: Recycle claimed
free queue pages", brings back recycling and notes that the regression
came from this patch. The uncapped refill and this description of the
old code are still there at the end of the series.

> +		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);

[Severity: High]

This is a pre-existing issue, not one introduced by this patch. Can pn
point outside the ring here right after the free queue has been shrunk?

pn comes from the hardware wptr and is never masked with m_pn. Only epn
is masked. geth_cleanup_freeq() leaves wptr at the old ring's rptr:

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

geth_freeq_add_page() then writes the descriptor at:

	freeq_entry = geth->freeq_ring + (pn << fpp_order);

For example, with both ports down at the default rxq_order 9, the free
queue has order 11 (2048 entries). Suppose traffic has left rptr at
1500. "ethtool -G eth0 rx 128" keeps order 11. "ethtool -G eth1 rx 128"
then picks order 9, which is 512 entries in an 8 KiB ring:

  gmac_set_ringparam()
    geth_resize_freeq()
      geth_cleanup_freeq()      /* wptr = rptr = 1500 */
      geth_setup_freeq()
        geth_fill_freeq()       /* pn = 750, epn = 749 & 255 = 237 */
          geth_freeq_add_page(geth, 750, ...)

Would this write buf_adr into entries 1500 and 1501, about 16 KiB past
the end of the dma_alloc_coherent() buffer?

This assumes that writing GLOBAL_SW_FREEQ_BASE_SIZE_REG does not reset
wptr.

pn then wraps to 239, and the fill still returns count == expected
(510), so setup reports success. Page position 238 (entries 476 and
477) is never written, so the device and the driver no longer agree on
the ring contents.

The old code had the same unmasked pn and indexed freeq_pages[pn] the
same way. At the end of the series, geth_fill_freeq() still passes the
unmasked pn to both geth_freeq_recycle_page() and geth_freeq_add_page().
At that point the shrink can also happen when a failed growth setup is
followed by a request for a smaller ring.

Would masking pn with m_pn, or resetting the hardware pointers before
filling a new ring, avoid this?

[ ... ]

> @@ -981,12 +1071,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 long flags;
> -	struct page *page;
>  	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]

page_slots is now fixed by both ports' rxq_order at setup time. Can a
port later run a bigger RX ring than this slot pool was sized for?

geth_resize_freeq() still skips the rebuild whenever freeq_order is
unchanged:

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

It also returns -EBUSY when the other port is running, and gmac_open()
accepts that:

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

gmac_set_ringparam() keeps the new rxq_order whatever the resize
returns:

	port->rxq_order = min(15, ilog2(rp->rx_pending - 1) + 1);
	err = geth_resize_freeq(port);

For example, with port0 at order 8 and port1 at order 9, "ethtool -G
eth0 rx 512" keeps freeq_order 11. The pool stays at 1792 slots, but
2048 can be in flight.

geth_freeq_alloc_slot() would then return -ENOSPC, and refill would
stall until NAPI frees slots.

A later commit in this series, "net: ethernet: cortina: Rebuild free
queue metadata for RX ring changes", looks like it fixes this. It adds
geth_freeq_page_slots() and makes the fast path also require
num_freeq_pages >= page_slots. It returns -EBUSY only when the existing
queue is too small, makes gmac_open() propagate every resize error, and
restores the old rxq_order in gmac_set_ringparam() on failure.

[ ... ]

> @@ -999,23 +1093,18 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)

[ ... ]

> -	filled = geth_fill_freeq(geth, false);
> -	if (!filled)
> -		goto err_freeq_alloc;
> +	expected = len - (1 << fpp_order);
> +	filled = geth_fill_freeq(geth);
> +	if (filled != expected)
> +		goto err_freeq_bitmap;

[Severity: Low]

Does this check now accept a free queue with no usable entries?

Take a board where the other port has no netdev (the PHY-less probe
path). "ethtool -G ethX rx 1" sets rxq_order = min(15, ilog2(0) + 1),
which is 0. geth_resize_freeq() then computes new_size = 2 and
freeq_order = 1. That gives len = 2, and with two fragments per page,
expected = 0.

With m_pn = 0, geth_fill_freeq() sees pn == epn on its first check and
returns 0. filled == expected, so setup succeeds with no RX buffers
posted, and later refills cannot post any either.

The old "if (!filled)" check rejected this case with -ENOMEM.

[ ... ]

> @@ -1586,7 +1656,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) {

[Severity: Medium]

This is a pre-existing issue, but what happens to the fragments claimed
here if the interface is stopped partway through a frame?

gmac_stop() clears the partial frame state without freeing the skb:

	napi_disable(&port->napi);
	port->rx_skb = NULL;
	port->rx_frag_nr = 0;
	port->rx_dropping = false;

The partly built napi->skb still holds its claimed fragment page
references. On the next open, napi_get_frags() returns that stale skb
and gmac_rx() fills it again starting at frag 0.

Does this leak the page references held by the old frags?

A later commit in this series, "net: ethernet: cortina: Release partial
RX frames on stop", adds a napi_free_frags() call in gmac_stop() before
the state is cleared.

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

Re: [PATCH net-next v3 07/12] net: ethernet: cortina: Rebuild free queue metadata for RX ring changes

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 (1) · Low: 0 (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 Fixes: tag blames the wrong commit, and the main defect
  comes from the patch just before this one in the same series.
- [Medium] In geth_resize_freeq(), the fast path changed from
  `freeq_order == new_order` to `freeq_order >= new_order &&…
- [Medium] geth_freeq_page_slots() and geth_resize_freeq() count a port's
  RX ring whenever `geth->portN && geth->portN->netdev` is set.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  geth_resize_freeq() and the new geth_freeq_page_slots() read and…
- [Low] This problem wasn't introduced by this patch, but the kernel-doc
  of geth_resize_freeq() says it "gets called at least once during…

--

net: ethernet: cortina: Rebuild free queue metadata for RX ring changes

This patch moves the free queue metadata slot calculation into a new
helper, geth_freeq_page_slots(), and uses it in the geth_resize_freeq()
fast path. The check for the other port running now comes after that
fast path. gmac_open() no longer accepts -EBUSY, and
gmac_set_ringparam() puts the old rxq_order back if the resize fails.

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

[Severity: Medium]
Is this the right Fixes: target? The commit message describes a metadata
pool "sized for the old RX rings", but no such pool exists in 4d5ae32f5e1e.

The previous patch in this series, "net: ethernet: cortina: Preserve
in-flight free queue pages", added page_slots sizing of freeq_pages and
freeq_page_bitmap based on port0/port1 rxq_order. Before that patch,
num_freeq_pages was simply pages and depended only on freeq_order.

That patch did not change this fast path in geth_resize_freeq():

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

So the regression covers the commits from that patch up to this one.

For example, port0 rxq_order 9 and port1 rxq_order 8 give freeq_order 11
and 1792 slots. Raising port1 to order 9 keeps order 11 but needs 2048
slots. geth_freeq_alloc_slot() then returns -ENOSPC and refill stalls.

Could the metadata capacity part be squashed into "Preserve in-flight free
queue pages", or could the Fixes: tag point at that commit? The rxq_order
rollback in gmac_set_ringparam() and the -EBUSY change in gmac_open() do go
back to the original driver.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 660e51634017b..2345d582cb597 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1066,21 +1066,31 @@ static void geth_freeq_release_pages(struct gemini_ethernet *geth)
>  	xa_destroy(&geth->freeq_mappings);
>  }
>  
> +static unsigned int
> +geth_freeq_page_slots(struct gemini_ethernet *geth, unsigned int order)
> +{
> +	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
> +	unsigned int slots = 1 << (order - fpp_order);
> +
> +	if (geth->port0 && geth->port0->netdev)
> +		slots += 1 << geth->port0->rxq_order;
> +	if (geth->port1 && geth->port1->netdev)
> +		slots += 1 << geth->port1->rxq_order;

[Severity: Medium]
Can port->rxq_order still be 0 here even though port->netdev is set?

rxq_order is only set in gmac_init():

	port->rxq_order = DEFAULT_GMAC_RXQ_ORDER;

gmac_init() runs as .ndo_init from register_netdev() at the end of
gemini_ethernet_port_probe(). Before that point, probe has already set
port->netdev and published the port:

	geth->port1 = port;

It then requests the IRQ and sets up the PHY, which may hit
-EPROBE_DEFER, and only after that calls register_netdev().

If eth0 (rxq_order 9) is opened in that window while port1's rxq_order is
still 0, geth_setup_freeq() records num_freeq_pages = 1024 + 512 + 1 =
1537.

Once port1 registers with rxq_order 9, opening eth1 computes new_order 11
and page_slots = 1024 + 512 + 512 = 2048. The fast path check fails, and
because eth0 is running, geth_resize_freeq() returns -EBUSY. gmac_open()
now treats that as fatal:

	err = geth_resize_freeq(port);
	if (err) {
		netdev_err(netdev, "could not resize freeq\n");
		goto err_stop_phy;
	}

So eth1 fails to open every time until eth0 is brought down. Before this
patch, the order-only fast path returned 0 in this case.

Would checking that the netdev is registered, or setting rxq_order before
the port is published, avoid this?

> +
> +	return slots;
> +}
> +

[ ... ]

> @@ -1206,9 +1217,6 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  		other_port = geth->port0;
>  	other_netdev = other_port ? other_port->netdev : NULL;
>  
> -	if (other_netdev && netif_running(other_netdev))
> -		return -EBUSY;
> -
>  	new_size = 1 << (port->rxq_order + 1);

[ ... ]

> @@ -1225,8 +1233,15 @@ 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_ring && geth->freeq_order == new_order)
> -		return 0;
> +	if (geth->freeq_ring) {
> +		page_slots = geth_freeq_page_slots(geth, geth->freeq_order);
> +		if (geth->freeq_order >= new_order &&
> +		    geth->num_freeq_pages >= page_slots)
> +			return 0;
> +	}
> +
> +	if (other_netdev && netif_running(other_netdev))
> +		return -EBUSY;

[Severity: Medium]
Does this mean the shared free queue can now only grow?

The >= shortcut runs before the other-port-running check, so it is taken
even when both ports are down.

Suppose rx_pending is lowered with ethtool -G while both interfaces are
down. gmac_set_ringparam() calls geth_resize_freeq(), and new_order ends
up <= freeq_order. page_slots, computed with the smaller rxq_order, is <=
num_freeq_pages. The function returns 0 and keeps the larger ring and all
the pages posted to it.

Before this patch, the order mismatch forced a rebuild at the smaller
order when the other port was idle.

gmac_stop() does not free the freeq, and geth_cleanup_freeq() is otherwise
only reached from gemini_ethernet_remove(). The memory stays pinned until
unbind, which at order 14 is about 32 MiB of 2 KiB fragments.

The commit message also says:

  "Return success while another port is running only when the existing
  queue is large enough."

The code takes the >= path whether or not the other port is running.
Should the >= case apply only when other_netdev is running, with the exact
order match kept otherwise?

[Severity: Low]
This isn't a bug introduced by this patch, but since the fast path is
being rewritten here: the kernel-doc of geth_resize_freeq() says:

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

gemini_ethernet_init() has a matching comment:

	/* This makes the queue resize on probe() so that we
	 * set up and enable the queue IRQ. FIXME: fragile.
	 */
	geth->freeq_order = 1;

geth_resize_freeq() is only called from gmac_open() and
gmac_set_ringparam(); nothing in probe calls it. The fast path is also
gated on geth->freeq_ring, so the freeq_order = 1 sentinel is never
read. Could these comments be updated?

>  
>  	disable_irq(geth->port1->irq);

[Severity: Medium]
This is a pre-existing issue, but the new helper adds more unsynchronized
reads of geth->port0 and geth->port1. geth_resize_freeq() checks
geth->port1 once:

	if (!geth->port1)
		return -ENODEV;

It then dereferences geth->port1 again in several places:
disable_irq(geth->port1->irq), geth_freeq_page_slots(), and
enable_irq(geth->port1->irq) after geth_setup_freeq().

On the removal side, gemini_port_remove() calls unregister_netdev(), which
releases RTNL. gemini_port_clear() then does geth->port1 = NULL without
any lock shared with the open or ethtool paths, and devres frees the
netdev that contains the port.

The child driver doesn't set suppress_bind_attrs. Can a sysfs unbind of
port1 racing with "ip link set eth0 up" cause a NULL dereference at
enable_irq(geth->port1->irq)? Could it also read other_port->rxq_order or
other_port->netdev after they have been freed?

>  	geth_set_freeq_irq(geth, false);

[ ... ]

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

Re: [PATCH net-next v3 08/12] net: ethernet: cortina: Rotate free queue page allocation

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

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 commit "net: ethernet: cortina: Rotate free queue page
  allocation" carries `Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver…
- [Low] The commit message carries a Fixes tag and `Assisted-by: LLM`, so
  it presents itself as a netdev bug fix.
- [Low] `geth_cleanup_freeq()` and the error labels in
  `geth_setup_freeq()` (`err_freeq_bitmap` / `err_freeq_pages`) free the…

--

net: ethernet: cortina: Rotate free queue page allocation

This patch changes geth_freeq_alloc_slot() to start each bitmap search
just after the most recently allocated slot, wrapping at the end, instead
of always starting from zero. A new freeq_page_cursor field holds the
position, and geth_setup_freeq() resets it to 0.

> 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.

[ ... ]

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

[Severity: Medium]
Is this Fixes tag pointing at the right commit? The freeq_page_bitmap,
geth_freeq_alloc_slot() and its find_first_zero_bit() search are not in
4d5ae32f5e1e. They were added earlier in this series by "net: ethernet:
cortina: Preserve in-flight free queue pages".

The change also doesn't seem to alter behavior beyond where the search
starts. The old code returned -ENOSPC only when there was no zero bit in
[0, num_freeq_pages). The new code returns -ENOSPC only when both searches
fail, which is the same condition. A free slot is still found whenever one
exists.

This looks like an optimization of code added in the same unmerged series.
Could it be folded into "Preserve in-flight free queue pages", or sent
without a Fixes tag?

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

[Severity: Low]
The patch carries a Fixes tag, but the commit message doesn't say how the
scan cost was found or measured, or how the change was tested.
Documentation/process/maintainer-netdev.rst asks for this:

  All bug fixes must state how the issue was discovered and validated,
  and how the fix was tested.

For hardware drivers, it also asks that the testing statement name the
hardware the fix was tested on. Could that be added, including which
Gemini board was used?

Also, the message says the bitmap "currently searches for a free slot from
zero", but that behavior was added two patches earlier in this series.
Could the message say so?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 2345d582cb597..87c537ae83e7a 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -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);

[Severity: Low]
As far as I can tell this can't be reached today, but the wrap search
now passes freeq_page_cursor as the size argument. That means it relies
on the cursor matching the current bitmap.

geth_cleanup_freeq() and the error unwind in geth_setup_freeq() free the
bitmap and clear the count, but leave the cursor at its last value:

geth_cleanup_freeq() {
    ...
	bitmap_free(geth->freeq_page_bitmap);
	geth->freeq_page_bitmap = NULL;
	kfree(geth->freeq_pages);
	geth->freeq_pages = NULL;
	geth->num_freeq_pages = 0;
    ...
}

If geth_freeq_alloc_slot() ran in that state,
find_next_zero_bit(NULL, 0, cursor) would return 0. Then
find_first_zero_bit(NULL, cursor) with a non-zero cursor would read
through the NULL bitmap. The old find_first_zero_bit(bitmap, 0) returned
-ENOSPC without touching memory.

The current callers of geth_fill_freeq() appear to be guarded:

- geth_setup_freeq() resets the cursor after bitmap_zalloc() succeeds.
- NAPI only runs after a successful geth_resize_freeq().
- The SWFQ_EMPTY source stays masked after a failed resize.

Would it make sense to reset freeq_page_cursor to 0 next to the
num_freeq_pages = 0 assignments? Then the allocator wouldn't depend on
those outside guards.

> +		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;
>  }

[ ... ]

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

Re: [PATCH net-next v3 09/12] net: ethernet: cortina: Recycle claimed free queue pages

[netdev-bot+sashiko]

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

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

- [High] Pages held by the network stack now keep their free-queue
  metadata slot, so the free queue can run out of buffers and RX stops…
- [Medium] geth_freeq_recycle_slot() walks all geth->num_freeq_pages
  entries of geth->freeq_pages.
- [Low] The commit message and Fixes tag are inaccurate.

--

net: ethernet: cortina: Recycle claimed free queue pages

This patch keeps the free queue base reference and the DMA mapping after
all fragments of a page have been claimed. When the page refcount drops
back to one, the page is synced for the device and posted again. A fresh
page is allocated only when no claimed page can be recycled.

> The lifetime fix releases each mapping after its last descriptor is
> claimed. Refilling therefore allocates and maps a fresh page at line
> rate even after the network stack has released all references to an old
> page.

[ ... ]

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

[Severity: Low]
Is this Fixes tag accurate? The commit message does not say which commit
"the lifetime fix" is.

The original driver already reused pages in place. The baseline
geth_fill_freeq() reused a page when page_ref_count(page) <= 1. It only
called geth_freeq_alloc_map_page() when the page was still in use.

The allocate-and-map on every refill seems to come from an earlier patch
in this series, "net: ethernet: cortina: Preserve in-flight free queue
pages". That patch's geth_freeq_claim() unmaps the page, puts it and
clears its slot on the last claim.

This patch also depends on the slot bitmap, the xarray index and
freeq_page_cursor from the same series.

Should this be folded into that commit, or name it as the commit being
followed up, instead of carrying a Fixes tag for 4d5ae32f5e1e?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 87c537ae83e7a..a0186cc4b9436 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -762,6 +763,35 @@ static int geth_freeq_alloc_slot(struct gemini_ethernet *geth)
>  	return slot;
>  }
>  
> +static int geth_freeq_recycle_slot(struct gemini_ethernet *geth)
> +{
> +	unsigned int slot = geth->freeq_page_cursor;
> +	unsigned int scanned;
> +
> +	lockdep_assert_held(&geth->freeq_lock);
> +
> +	if (!geth->freeq_recycle_pending)
> +		return -ENOSPC;
> +
> +	for (scanned = 0; scanned < geth->num_freeq_pages; scanned++) {
> +		struct gmac_queue_page *gpage = &geth->freeq_pages[slot];
> +
> +		if (gpage->page && !gpage->fragments &&
> +		    page_ref_count(gpage->page) == 1) {

[Severity: Medium]
How long can hard IRQs stay off in this loop? It walks all
num_freeq_pages entries and reads the refcount of every fully claimed
page. It runs under spin_lock_irqsave(&geth->freeq_lock), called from
geth_fill_freeq() in both the gmac_napi_poll() and
gemini_port_irq_thread() refill paths.

The only early exit is the freeq_recycle_pending check. geth_freeq_claim()
increments that counter while skbs still reference the page, and only a
successful pick decrements it.

If even one page stays in a socket queue, reassembly queue or qdisc
backlog, would every geth_fill_freeq() call that runs out of recyclable
pages do a full walk and find nothing?

num_freeq_pages is 2048 with defaults. With "ethtool -G rx 32768" it can
reach 16384 + 2 * 32768 = 81920. Before this patch, the IRQs-off part of
geth_fill_freeq() was one find_next_zero_bit() over the bitmap plus
posting a single page.

This cost also adds up with the SWFQ_EMPTY retrigger described in the
geth_freeq_claim() comment below.

[ ... ]

> @@ -794,16 +823,8 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
>  				      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_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);
> -		put_page(page);
> -	}
> +	if (!--gpage->fragments)
> +		geth->freeq_recycle_pending++;

[Severity: High]
Can this let the free queue run out of buffers and stop RX on both GMAC
ports?

A fully claimed page now keeps gpage->page and its bit in
freeq_page_bitmap. The slot is reused only when geth_freeq_recycle_slot()
sees page_ref_count() == 1. The bit is cleared only on the
geth_freeq_add_page() error path.

geth_freeq_page_slots() sizes the slot pool for pages the hardware can
hold:

	unsigned int slots = 1 << (order - fpp_order);

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

That leaves no room for pages that skbs still reference after the claim.
Each fresh page from the fallback path takes a slot until it is recycled.

Once every slot holds a retained page that the stack still references,
the refill path looks like this:

geth_fill_freeq()
  geth_freeq_recycle_page()
    geth_freeq_recycle_slot()   returns -ENOSPC, refcount > 1
  geth_freeq_map_page()         new page allocated and mapped
  geth_freeq_add_page()
    geth_freeq_alloc_slot()     returns -ENOSPC, no zero bit left
  dma_unmap_single(); put_page(); break;

After that the write pointer in GLOBAL_SWFQ_RWPTR_REG stops advancing.
The shared software free queue then drains for both ports, including
traffic for sockets unrelated to the held pages.

The spare headroom is roughly 1024 pages with defaults. The smaller
rxq_order defaults on 32/64 MB systems from the later "Scale Gemini RX
queues to system memory" patch make it smaller.

gmac_rx() adds only frag_len to skb->truesize, so each small packet holds
a 2 KB half-page. Unread UDP sockets, TCP out-of-order queues, qdisc
backlogs or IP fragment reassembly queues could hold that many pages, and
a remote sender can fill the reassembly and socket queues.

Once RX has stopped, NAPI refill no longer runs. gemini_port_irq_thread()
then ACKs and re-enables SWFQ_EMPTY even when geth_fill_freeq() posted
nothing.

Depending on how the hardware asserts SWFQ_EMPTY, could this become
either an interrupt storm or a stall that remains after the stack
releases the pages?

The commit message says "Fall back to allocating a new page while old
fragments remain in the stack". Does that fallback only work until each
slot has been used once?

None of the later patches in the series ("Validate RX fragment lengths",
"Release partial RX frames on stop", "Scale Gemini RX queues to system
memory") changes the slot accounting.

[ ... ]

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

Re: [PATCH net-next v3 10/12] net: ethernet: cortina: Validate RX fragment lengths

[netdev-bot+sashiko]

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

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

- [High] Inconsistent descriptor lengths can still produce an skb shorter
  than an Ethernet header, and handing it to GRO causes a NULL…
- [High] net: gemini: valid perfectly aligned packets are dropped on
  0-length EOF
- [Low] The commit message does not say how the issue was discovered or
  validated, or on what hardware the fix was tested.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but gmac_rx()
  adds only the payload length to skb->truesize (`skb->truesize +=…

--

net: ethernet: cortina: Validate RX fragment lengths

This commit adds length checks to gmac_rx() so that RX descriptor
lengths cannot place a fragment outside the posted free queue fragment
or the backing page. Short initial fragments, frame length underflow and
zero-length fragments are now dropped instead of being added to the skb.

> Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
> Assisted-by: LLM
> Signed-off-by: Linus Walleij <linusw@kernel.org>

[Severity: Low]
Could the commit message say how this issue was found and how the fix was
tested? Documentation/process/maintainer-netdev.rst asks for this in the
bug fixes section:

  All bug fixes must state how the issue was discovered and validated,
  and how the fix was tested.

For hardware drivers, the same section asks for the hardware the fix was
tested on.

The new checks depend on how the hardware fills buffer_size and
byte_count. Was this tested on Gemini/SL3516 hardware?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index a0186cc4b9436..6b333eb81f9c4 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1771,6 +1772,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_drop;
> +
>  			page_offs += NET_IP_ALIGN;
>  			frag_len -= NET_IP_ALIGN;
>  			frag_nr = 0;
> @@ -1779,15 +1783,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_drop;
>  			frag_len = frame_len - skb->len;
> +		}

[Severity: High]
Can inconsistent descriptors still produce an skb shorter than an
Ethernet header here?

The ETH_ZLEN minimum in gmac_skb_if_good_frame() only applies to the SOF
descriptor's byte_count. On the EOF descriptor, frame_len comes from that
descriptor's own word1.bits.byte_count. The only new check on it is
frame_len < skb->len.

For example:

  SOF descriptor (no EOF bit): buffer_size = 3, byte_count = 60
    passes gmac_skb_if_good_frame() and frag_len >= NET_IP_ALIGN,
    appends 1 byte
  EOF descriptor: byte_count between 2 and 13
    passes frame_len >= skb->len, the range check and the zero-length
    check, appends the rest

napi_gro_frags() then receives an skb of 2 to 13 bytes.

napi_frags_skb() cannot pull the 14 byte ethhdr, so it reuses the skb and
returns NULL:

net/core/gro.c:napi_frags_skb() {
    ...
	if (unlikely(!skb_gro_may_pull(skb, hlen))) {
		eth = skb_gro_header_slow(skb, hlen, 0);
		if (unlikely(!eth)) {
			...
			napi_reuse_skb(napi, skb);
			return NULL;
		}
    ...
}

napi_gro_frags() then passes that NULL straight on:

net/core/gro.c:napi_gro_frags() {
	...
	struct sk_buff *skb = napi_frags_skb(napi);
	...
	ret = napi_frags_finish(napi, skb, dev_gro_receive(&napi->gro, skb));
	...
}

dev_gro_receive() calls skb_get_hash_raw(skb) on entry. Wouldn't that
be a NULL dereference in NAPI softirq context?

This crash path existed before this patch. However, the commit message
says these descriptor inconsistencies are now dropped. Should gmac_rx()
check that skb->len is at least ETH_HLEN (or ETH_ZLEN) before calling
napi_gro_frags()? Alternatively, it could check that the EOF frame_len
matches the SOF frame_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_drop;
>  
> -		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_drop;
> +		}
>  
>  		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);
>  		skb->len += frag_len;

[Severity: High]
Does this drop complete frames when the EOF descriptor carries no
payload?

On an EOF descriptor frag_len is recomputed as frame_len - skb->len.
The earlier descriptors of the chain may already have delivered all
frame_len bytes. One case is a frame that ends exactly on a free queue
fragment boundary while the hardware still closes the chain with a
separate EOF descriptor. Then frag_len is 0 here even though the skb
already holds the whole frame.

The new goto err_drop then calls napi_free_frags() on that skb and
counts it in rx_dropped, instead of handing it to napi_gro_frags().

Before this patch, this case only printed the ratelimited "Received
fragment with len = 0" message, and the frame was still delivered.
That message suggests the condition has been seen on real hardware.
If the hardware can emit such descriptors, frames of particular
lengths would now be lost every time, which can be triggered by remote
traffic.

Would it be better to treat a zero-length EOF fragment as having
nothing to append? gmac_rx() could release the page with put_page(),
skip skb_fill_page_desc(), and still complete the skb with
napi_gro_frags(). The zero-length check would then need to come before
the frag_nr == MAX_SKB_FRAGS check. Otherwise a complete frame that
uses all MAX_SKB_FRAGS slots and is followed by an empty EOF
descriptor is still dropped.

Was the zero-length case checked against the behaviour of the
Gemini/SL3516 hardware before turning it into a drop?

[Severity: Medium]
This isn't a bug introduced by this patch, but the line just after this
in gmac_rx() adds only the payload length to truesize:

		skb->truesize += frag_len;

Each fragment pins a whole DMA buffer of 1 << freeq_frag_order bytes,
with its own page reference taken in geth_freeq_post_page():

	frag_len = 1 << geth->freeq_frag_order; /* Usually 2048 */

Doesn't this understate the pinned memory by up to about 2 KiB per
fragment for socket rmem and GRO accounting? Would adding freeq_frag_len
here be more accurate?

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

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

[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

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 u64_stats writer added to gmac_stop()
  (drivers/net/ethernet/cortina/gemini.c) runs in preemptible process…
- [Low] The commit message has no statement of how the bug was found, how
  it was confirmed, or how the fix was tested.

--

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

This patch frees a partially assembled RX frag skb in gmac_stop() after
napi_disable(). The pages already attached to port->rx_skb are then no
longer leaked when the interface goes down. The discarded frame is
counted in rx_dropped under rx_stats_syncp.

> net: ethernet: cortina: Release partial RX frames on stop
>
> 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.

[ ... ]

> Fixes: 06937db21ee3 ("net: ethernet: cortina: Make RX SKB per-port")
> Assisted-by: LLM
> Signed-off-by: Linus Walleij <linusw@kernel.org>

[Severity: Low]
This isn't a bug, but could the commit message say how the leak was found
and confirmed, and how the fix was tested?

The bug fixes section of Documentation/process/maintainer-netdev.rst
asks for this:

    All bug fixes must state how the issue was discovered and validated,
    and how the fix was tested.

For hardware drivers, it also asks for the hardware the fix was tested on.

For example, was an ifdown during a partly received multi-descriptor
(jumbo) frame reproduced on Gemini hardware?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 6b333eb81f9c4..2ebfbbe84ebaf 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2173,6 +2173,12 @@ 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);
> +		u64_stats_update_begin(&port->rx_stats_syncp);
> +		port->stats.rx_dropped++;
> +		u64_stats_update_end(&port->rx_stats_syncp);

[Severity: Medium]
Is it safe to call u64_stats_update_begin() here with preemption enabled?

gmac_stop() is the ndo_stop callback. It runs in process context and
holds only RTNL. Gemini is a 32-bit ARM SoC, so rx_stats_syncp contains a
real seqcount.

On !PREEMPT_RT, __u64_stats_update_begin() calls
preempt_disable_nested(), which only runs
lockdep_assert_preemption_disabled(). The write_seqcount_begin() that
follows on a plain seqcount_t also only asserts. Neither one disables
preemption.

With CONFIG_PROVE_LOCKING, would this hit the WARN_ON_ONCE() in
lockdep_assert_preemption_disabled() on every ifdown that finds a pending
partial frame?

On a preemptible kernel the writer could also be preempted while the
sequence count is odd. Could a reader on the same CPU then spin in the
retry loop in gmac_get_stats64() or gmac_get_ethtool_stats()?

gmac_get_stats64() {
    ...
	do {
		start = u64_stats_fetch_begin(&port->rx_stats_syncp);
		...
	} while (u64_stats_fetch_retry(&port->rx_stats_syncp, start));
    ...
}

Before this patch, the only writer to rx_stats_syncp was gmac_napi_poll()
in softirq context. The other process-context writer on this stop path,
gmac_update_hw_stats(), disables interrupts first:

gmac_update_hw_stats() {
    ...
	spin_lock_irqsave(&geth->irq_lock, flags);
	u64_stats_update_begin(&port->ir_stats_syncp);
    ...
}

Does this block need similar protection, for example
u64_stats_update_begin_irqsave() or a local_bh_disable() section?

gmac_stop() still contains the same block at the end of the series, in
"net: ethernet: cortina: Scale Gemini RX queues to system memory".

[ ... ]

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