[PATCH net-next 0/3] net: ethernet: cortina: Use page_pool for Gemini RX buffers

[PATCH net-next 0/3] net: ethernet: cortina: Use page_pool for Gemini RX buffers

[Linus Walleij]

The Gemini ethernet has this really convoluted queue mechanism using
a custom queue.

Move the Gemini RX buffer management to a parent-owned page_pool.

This series fixes DMA mapping and synchronization, fragment recycling,
and teardown while retaining the hardware free queue shared by both ports.

We need to track fragments by the exact DMA address returned by
hardware, since RX software IDs originate in the classification hash
table rather than the free queue.

Scale the default RX rings and shared free queue on Gemini systems
with 32 or 64 MiB of RAM, as the RAM usage may be a bit heavy on
those.

Assisted-by: LLM
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
Linus Walleij (3):
      net: ethernet: cortina: Keep shared free queue parent-owned
      net: ethernet: cortina: Manage RX buffers with page_pool
      net: ethernet: cortina: Scale Gemini RX queues to system memory

 drivers/net/ethernet/cortina/Kconfig  |   1 +
 drivers/net/ethernet/cortina/gemini.c | 389 ++++++++++++++++++++++------------
 2 files changed, 252 insertions(+), 138 deletions(-)
---
base-commit: 2ff4ca33660706f4092866b220eb0c94940f7e0c
change-id: 20260919-gemini-ethernet-fixes-3-f0403653f23a

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

[PATCH net-next 1/3] 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.

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

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

-- 
2.55.0

Re: [PATCH net-next 1/3] net: ethernet: cortina: Keep shared free queue parent-owned

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Claim that after removing the queue teardown from
  gemini_port_remove(), unbinding only the eth1 port device leaves the…

Pre-existing issues:
- [High] gemini_port_remove() (the function this patch edits) never
  clears geth->port0 / geth->port1, and geth_resize_freeq() dereferences…
- [High] geth_resize_freeq() re-arms SWFQ_EMPTY_INT_BIT in
  GLOBAL_INTERRUPT_ENABLE_4_REG unconditionally, ignoring…
- [Medium] geth_cleanup_freeq() drops every reference on each queue page
  (`while (page_ref_count(gpage->page) > 0) put_page(gpage->page);`),…

--

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

The shared software free queue was torn down by every ethernet-port child
removal and then once more by the parent removal. This patch moves the
teardown to the parent only, depopulates the child devices first, masks the
free-queue interrupt after the managed handlers are gone, and makes
geth_cleanup_freeq() a no-op when no queue was allocated.

Resolves: 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 2fe7fd0202d2..31bcd41c17fa 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1004,6 +1004,9 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  	unsigned int pages = len >> fpp_order;
>  	unsigned int pn;
>  
> +	if (!geth->freeq_ring)
> +		return;
> +
>  	writew(readw(geth->base + GLOBAL_SWFQ_RWPTR_REG),
>  	       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
>  	writel(0, geth->base + GLOBAL_SW_FREEQ_BASE_SIZE_REG);

[Severity: High]
This is a pre-existing issue and not introduced here, but since the new
guard keys on geth->freeq_ring it may be worth looking at what the rest of
the free-queue state looks like when that pointer is NULL.

geth_setup_freeq() clears only freeq_ring on the error paths:

	kfree(geth->freeq_pages);
err_freeq:
	dma_free_coherent(geth->dev,
			  sizeof(*geth->freeq_ring) << geth->freeq_order,
			  geth->freeq_ring, geth->freeq_dma_base);
	geth->freeq_ring = NULL;
	return -ENOMEM;

geth->freeq_pages keeps pointing at the kfree()d array, and
geth_resize_freeq() re-arms the free-queue interrupt without looking at the
return value:

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

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

	return ret;

So after a failing gmac_open() -> geth_resize_freeq() (dma_alloc_coherent(),
kzalloc_objs(), geth_freeq_alloc_map_page() or a zero return from
geth_fill_freeq()), GLOBAL_SW_FREEQ_BASE_SIZE_REG has just been written to 0
by geth_cleanup_freeq() while SWFQ_EMPTY_INT_BIT stays enabled, and nothing
in the gmac_open() unwind masks it again.

When the free-queue-empty status then asserts, gemini_port_irq() sees
val & en & SWFQ_EMPTY_INT_BIT and returns IRQ_WAKE_THREAD, and
gemini_port_irq_thread() -> geth_fill_freeq(geth, true) walks the freed
array:

		gpage = &geth->freeq_pages[pn];
		page = gpage->page;
		...
		if (page_ref_count(page) > 1) {

Should the interrupt only be re-enabled when geth_setup_freeq() succeeded,
and should the error paths also set geth->freeq_pages to NULL?

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

[Severity: High]
This isn't a bug introduced by this patch, but since this is now the whole
of the child teardown, is anything supposed to clear the parent's
back-pointer to the port?

geth->port0 / geth->port1 are only ever assigned, in
gemini_ethernet_port_probe():

	/* Assign pointer in the main state container */
	if (!id)
		geth->port0 = port;
	else
		geth->port1 = port;

The port object is the netdev private area from
devm_alloc_etherdev_mqs(), so devres frees it right after
gemini_port_remove() returns, while geth->portN still points at it.

geth_resize_freeq() then dereferences the sibling slot before any NULL
test:

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

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

Can unbinding one ethernet-port child and then bringing the surviving port
up (gmac_open() -> geth_resize_freeq(), or ethtool -G ->
gmac_set_ringparam()) read the freed netdev private area here and pass the
resulting garbage to netif_running()?

The same statement also dereferences NULL if the sibling port never reached
the geth->portN assignment, for example on a probe failure or
-EPROBE_DEFER, since the check below only guards other_netdev and not the
port pointer.

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

[Severity: Medium]
This is a pre-existing issue rather than something added here, but this
call site is now the only path into the forced reference drop in
geth_cleanup_freeq():

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

gmac_rx() hands free-queue pages to the stack as skb fragments:

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

If a socket still holds those skbs across unregister_netdev(), does this
loop return pages to the allocator while the queued fragments still
reference them, and does skb_release_data() then underflow the refcount?

For what it is worth, the following patch in the series, "net: ethernet:
cortina: Manage RX buffers with page_pool", drops this loop and returns
only the fragments the driver still owns, with page_pool_destroy()
deferring the final teardown, so the behaviour does not survive to the end
of the series.

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

[PATCH net-next 2/3] net: ethernet: cortina: Manage RX buffers with page_pool

[Linus Walleij]

The Gemini software free queue supplies page fragments to the RX queues
of both Ethernet ports. Its current buffer lifecycle does not obey the
streaming DMA API.

Each page is mapped for PAGE_SIZE, but replacement overwrites the free
queue entry before reading its old DMA address. It consequently unmaps
the new page for only one fragment and leaks the old mapping. The RX path
also hands device-written data to the stack without a sync for the CPU,
and teardown drops page references that can still belong to skbs.

Create one page_pool owned by the parent Ethernet device. Let it map and
sync whole pages for the device, and split each page into free queue
fragments with the page_pool fragment API.

The RX descriptor software ID comes from the classification hash table,
not the free queue. Index every fragment by its DMA address in fragment
units so RX can recover the page and fragment offset from the buffer
address returned by hardware without relying on DMA address masking.
Validate the complete DMA address before removing the indexed fragment.

Sync each received fragment for the CPU before attaching it to an skb,
mark completed skbs for page_pool recycling, and return dropped or
unclaimed fragments through the pool. Free a partial skb when stopping a
port and let page_pool defer final destruction while delivered skbs still
hold fragments.

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/Kconfig  |   1 +
 drivers/net/ethernet/cortina/gemini.c | 368 +++++++++++++++++++++-------------
 2 files changed, 233 insertions(+), 136 deletions(-)

diff --git a/drivers/net/ethernet/cortina/Kconfig b/drivers/net/ethernet/cortina/Kconfig
index aaf9e294b70b..e69140fe3187 100644
--- a/drivers/net/ethernet/cortina/Kconfig
+++ b/drivers/net/ethernet/cortina/Kconfig
@@ -17,6 +17,7 @@ config GEMINI_ETHERNET
 	depends on HAS_IOMEM
 	select PHYLIB
 	select CRC32
+	select PAGE_POOL
 	help
 	  This driver supports StorLink SL351x (Gemini) dual Gigabit Ethernet.
 
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 31bcd41c17fa..6d5f7dacf12a 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>
@@ -37,11 +38,13 @@
 #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>
 #include <linux/ipv6.h>
 #include <net/gro.h>
+#include <net/page_pool/helpers.h>
 
 #include "gemini.h"
 
@@ -87,11 +90,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 {
@@ -164,8 +169,12 @@ struct gemini_ethernet {
 	unsigned int	freeq_frag_order;
 	struct gmac_rxdesc *freeq_ring;
 	dma_addr_t	freeq_dma_base;
+	struct page_pool *freeq_pool;
 	struct gmac_queue_page	*freeq_pages;
+	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 */
 };
 
@@ -724,31 +733,91 @@ 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 int geth_freeq_alloc_slot(struct gemini_ethernet *geth)
 {
-	struct gmac_queue_page *gpage;
-	dma_addr_t mapping;
-	int i;
+	unsigned int slot;
 
-	/* Only look for even pages */
-	mapping = addr & PAGE_MASK;
+	lockdep_assert_held(&geth->freeq_lock);
 
-	if (!geth->freeq_pages) {
-		dev_err_ratelimited(geth->dev,
-				    "try to get page with no page list\n");
-		return NULL;
+	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;
 	}
 
-	/* 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;
+	__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;
+}
+
+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_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;
+
+	index = geth_freeq_mapping_index(geth, mapping);
+
+	spin_lock_irqsave(&geth->freeq_lock, flags);
+	gpage = xa_load(&geth->freeq_mappings, index);
+	if (!gpage)
+		goto err_unlock;
+
+	page = gpage->page;
+	if (!page || !gpage->fragments)
+		goto err_unlock;
+
+	page_mapping = page_pool_get_dma_addr(page);
+	valid = page_mapping == gpage->mapping &&
+		mapping >= page_mapping &&
+		mapping - page_mapping <= PAGE_SIZE - frag_len &&
+		!((mapping - page_mapping) & (frag_len - 1));
+	if (!valid)
+		goto err_invalid;
+
+	xa_erase(&geth->freeq_mappings, index);
+	if (!--gpage->fragments) {
+		slot = gpage - geth->freeq_pages;
+		gpage->page = NULL;
+		gpage->mapping = 0;
+		__clear_bit(slot, geth->freeq_page_bitmap);
 	}
+	spin_unlock_irqrestore(&geth->freeq_lock, flags);
+
+	*page_offs = mapping - page_mapping;
+	return page;
+
+err_invalid:
+	spin_unlock_irqrestore(&geth->freeq_lock, flags);
+	dev_err_ratelimited(geth->dev, "invalid freeq mapping %pad\n",
+			    &mapping);
+	return NULL;
 
+err_unlock:
+	spin_unlock_irqrestore(&geth->freeq_lock, flags);
+	dev_err_ratelimited(geth->dev, "untracked freeq mapping %pad\n",
+			    &mapping);
 	return NULL;
 }
 
@@ -757,11 +826,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;
 
@@ -782,84 +852,100 @@ static void gmac_cleanup_rxq(struct net_device *netdev)
 	 */
 	while (r != w) {
 		mapping = rxd[r].word2.buf_adr;
+		if (mapping) {
+			page = geth_freeq_claim(geth, mapping,
+						&page_offs);
+			if (page)
+				page_pool_put_full_page(geth->freeq_pool,
+							page, false);
+		}
+
 		r++;
 		r &= ((1 << port->rxq_order) - 1);
-
-		if (!mapping)
-			continue;
-
-		/* Freeq pointers are one page off */
-		gpage = gmac_get_queue_page(geth, port, mapping + PAGE_SIZE);
-		if (!gpage) {
-			dev_err(geth->dev, "could not find page\n");
-			continue;
-		}
-		/* Release the RX queue reference to the page */
-		put_page(gpage->page);
 	}
 
 	dma_free_coherent(geth->dev, sizeof(*port->rxq_ring) << port->rxq_order,
 			  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_alloc_page(struct gemini_ethernet *geth, unsigned int pn)
 {
 	struct gmac_rxdesc *freeq_entry;
 	struct gmac_queue_page *gpage;
+	unsigned int fragments;
 	unsigned int fpp_order;
 	unsigned int frag_len;
+	dma_addr_t page_mapping;
 	dma_addr_t mapping;
 	struct page *page;
-	int i;
+	int ret;
+	int slot;
+	unsigned int i;
 
-	/* First allocate and DMA map a single page */
-	page = alloc_page(GFP_ATOMIC);
+	page = page_pool_dev_alloc_pages(geth->freeq_pool);
 	if (!page)
-		return NULL;
+		return -ENOMEM;
 
-	mapping = dma_map_single(geth->dev, page_address(page),
-				 PAGE_SIZE, DMA_FROM_DEVICE);
-	if (dma_mapping_error(geth->dev, mapping)) {
-		put_page(page);
-		return NULL;
+	slot = geth_freeq_alloc_slot(geth);
+	if (slot < 0) {
+		page_pool_put_full_page(geth->freeq_pool, page, false);
+		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
-	 * bytes, 2k) so fpp_order (fragments per page order) is default 1. Thus
-	 * each page normally needs two entries in the queue.
+	/* PAGE_SHIFT is 12 on Gemini, while the default fragment order is 11,
+	 * so each page normally supplies two free queue entries.
 	 */
 	frag_len = 1 << geth->freeq_frag_order; /* Usually 2048 */
 	fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
+	fragments = 1 << fpp_order;
 	freeq_entry = geth->freeq_ring + (pn << fpp_order);
+	page_mapping = page_pool_get_dma_addr(page);
+	if (page_mapping > U32_MAX - (PAGE_SIZE - 1)) {
+		dev_err_ratelimited(geth->dev,
+				    "freeq DMA mapping exceeds 32 bits\n");
+		ret = -EOVERFLOW;
+		goto err_slot;
+	}
+
+	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,
+				geth_freeq_mapping_index(geth, mapping),
+				gpage, GFP_ATOMIC);
+		if (ret)
+			goto err_mappings;
+	}
+
+	gpage->fragments = fragments;
+	page_pool_fragment_page(page, fragments);
+
 	dev_dbg(geth->dev, "allocate page %d fragment length %d fragments per page %d, freeq entry %p\n",
-		 pn, frag_len, (1 << fpp_order), freeq_entry);
-	for (i = (1 << fpp_order); i > 0; i--) {
-		freeq_entry->word2.buf_adr = mapping;
+		 pn, frag_len, fragments, freeq_entry);
+	mapping = page_mapping;
+	for (i = 0; i < fragments; i++) {
+		freeq_entry->word1.bits32 = 0;
+		freeq_entry->word2.buf_adr = lower_32_bits(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;
+	return 0;
 
-	return page;
+err_mappings:
+	while (i--) {
+		mapping = page_mapping + i * frag_len;
+		xa_erase(&geth->freeq_mappings,
+			 geth_freeq_mapping_index(geth, mapping));
+	}
+	gpage->page = NULL;
+	gpage->mapping = 0;
+err_slot:
+	__clear_bit(slot, geth->freeq_page_bitmap);
+	page_pool_put_full_page(geth->freeq_pool, page, false);
+	return ret;
 }
 
 /**
@@ -890,28 +976,9 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
 
 	/* Loop over the freeq ring buffer entries */
 	while (pn != epn) {
-		struct gmac_queue_page *gpage;
-		struct page *page;
-
-		gpage = &geth->freeq_pages[pn];
-		page = gpage->page;
+		if (geth_freeq_alloc_page(geth, pn))
+			break;
 
-		dev_dbg(geth->dev, "fill entry %d page ref count %d add %d refs\n",
-			pn, page_ref_count(page), 1 << fpp_order);
-
-		if (page_ref_count(page) > 1) {
-			unsigned int fl = (pn - epn) & m_pn;
-
-			if (fl > 64 >> fpp_order)
-				break;
-
-			page = geth_freeq_alloc_map_page(geth, pn);
-			if (!page)
-				break;
-		}
-
-		/* Add one reference per fragment in the page */
-		page_ref_add(page, 1 << fpp_order);
 		count += 1 << fpp_order;
 		pn++;
 		pn &= m_pn;
@@ -926,14 +993,28 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
 
 static int geth_setup_freeq(struct gemini_ethernet *geth)
 {
+	struct page_pool_params pp_params = {
+		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
+		.order = 0,
+		.nid = NUMA_NO_NODE,
+		.dev = geth->dev,
+		.dma_dir = DMA_FROM_DEVICE,
+		.max_len = PAGE_SIZE,
+	};
 	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 page_slots = pages;
 	union queue_threshold qt;
 	union dma_skb_size skbsz;
 	unsigned int filled;
-	unsigned int pn;
+	int ret = -ENOMEM;
+
+	if (geth->port0)
+		page_slots += 1 << geth->port0->rxq_order;
+	if (geth->port1)
+		page_slots += 1 << geth->port1->rxq_order;
+	pp_params.pool_size = pages;
 
 	geth->freeq_ring = dma_alloc_coherent(geth->dev,
 		sizeof(*geth->freeq_ring) << geth->freeq_order,
@@ -946,19 +1027,25 @@ 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++)
-		if (!geth_freeq_alloc_map_page(geth, pn))
-			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;
+	geth->freeq_page_cursor = 0;
+
+	geth->freeq_pool = page_pool_create(&pp_params);
+	if (IS_ERR(geth->freeq_pool)) {
+		ret = PTR_ERR(geth->freeq_pool);
+		geth->freeq_pool = NULL;
+		goto err_freeq_bitmap;
+	}
 
 	filled = geth_fill_freeq(geth, false);
 	if (!filled)
-		goto err_freeq_alloc;
+		goto err_freeq_pool;
 
 	qt.bits32 = readl(geth->base + GLOBAL_QUEUE_THRESHOLD_REG);
 	qt.bits.swfq_empty = 32;
@@ -971,25 +1058,23 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 
 	return 0;
 
-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];
-		put_page(gpage->page);
-	}
-
+err_freeq_pool:
+	xa_destroy(&geth->freeq_mappings);
+	page_pool_destroy(geth->freeq_pool);
+	geth->freeq_pool = NULL;
+err_freeq_bitmap:
+	bitmap_free(geth->freeq_page_bitmap);
+	geth->freeq_page_bitmap = NULL;
+err_freeq_pages:
 	kfree(geth->freeq_pages);
+	geth->freeq_pages = NULL;
+	geth->num_freeq_pages = 0;
 err_freeq:
 	dma_free_coherent(geth->dev,
 			  sizeof(*geth->freeq_ring) << geth->freeq_order,
 			  geth->freeq_ring, geth->freeq_dma_base);
 	geth->freeq_ring = NULL;
-	return -ENOMEM;
+	return ret;
 }
 
 /**
@@ -998,10 +1083,7 @@ 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;
+	struct page_pool *pool;
 	unsigned int pn;
 
 	if (!geth->freeq_ring)
@@ -1011,23 +1093,30 @@ 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++) {
+	pool = geth->freeq_pool;
+	for (pn = 0; pn < geth->num_freeq_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);
+		while (gpage->fragments) {
+			page_pool_put_full_page(pool, gpage->page, false);
+			gpage->fragments--;
+		}
 	}
+	xa_destroy(&geth->freeq_mappings);
 
+	bitmap_free(geth->freeq_page_bitmap);
+	geth->freeq_page_bitmap = NULL;
 	kfree(geth->freeq_pages);
+	geth->freeq_pages = NULL;
+	geth->num_freeq_pages = 0;
 
 	dma_free_coherent(geth->dev,
 			  sizeof(*geth->freeq_ring) << geth->freeq_order,
 			  geth->freeq_ring, geth->freeq_dma_base);
+	geth->freeq_ring = NULL;
+	geth->freeq_pool = NULL;
+	page_pool_destroy(pool);
 }
 
 /**
@@ -1435,6 +1524,7 @@ static struct sk_buff *gmac_skb_if_good_frame(struct gemini_ethernet_port *port,
 	skb = napi_get_frags(&port->napi);
 	if (!skb)
 		goto update_exit;
+	skb_mark_for_recycle(skb);
 
 	if (rx_csum == RX_CHKSUM_IP_UDP_TCP_OK)
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -1459,7 +1549,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;
@@ -1496,8 +1585,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);
@@ -1516,20 +1603,18 @@ 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) {
-			dev_err_ratelimited(geth->dev,
-					    "could not find mapping\n");
+		page = geth_freeq_claim(geth, mapping, &page_offs);
+		if (!page)
 			goto err_drop;
-		}
-		page = gpage->page;
 
 		if (word3.bits32 & SOF_BIT) {
 			skb = gmac_skb_if_good_frame(port, word0, frame_len);
 			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;
@@ -1544,10 +1629,18 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 		/* append page frag to skb */
 		if (frag_nr == MAX_SKB_FRAGS)
 			goto err_drop;
+		if (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;
+		}
 
+		page_pool_dma_sync_for_cpu(geth->freeq_pool, page, page_offs,
+					   frag_len);
 		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);
 		skb->len += frag_len;
 		skb->data_len += frag_len;
@@ -1569,7 +1662,7 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 		}
 
 		if (page)
-			put_page(page);
+			page_pool_put_full_page(geth->freeq_pool, page, false);
 
 		if (!dropping) {
 			port->stats.rx_dropped++;
@@ -1920,6 +2013,8 @@ static int gmac_stop(struct net_device *netdev)
 	gmac_disable_tx_rx(netdev);
 	gmac_stop_dma(port);
 	napi_disable(&port->napi);
+	if (port->rx_skb)
+		napi_free_frags(&port->napi);
 	port->rx_skb = NULL;
 	port->rx_frag_nr = 0;
 	port->rx_dropping = false;
@@ -2673,6 +2768,7 @@ static int gemini_ethernet_probe(struct platform_device *pdev)
 
 	spin_lock_init(&geth->irq_lock);
 	spin_lock_init(&geth->freeq_lock);
+	xa_init(&geth->freeq_mappings);
 
 	/* The children will use this */
 	platform_set_drvdata(pdev, geth);

-- 
2.55.0

Re: [PATCH net-next 2/3] net: ethernet: cortina: Manage RX buffers with page_pool

[netdev-bot+sashiko]

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

Critical: 0 · High: 3 (2) · Medium: 3 · 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] page_pool operations that manipulate BH state are performed
  while geth->freeq_lock is held with hardirqs disabled.
- [High] A failed geth_setup_freeq() leaves geth->freeq_pool,
  geth->freeq_ring, geth->freeq_pages and geth->freeq_page_bitmap NULL…
- [High] net: cortina: valid packets dropped on 0-length EOF descriptors
- [Medium] The newly added RX length guard in gmac_rx() bounds the
  fragment against the page ('if (frag_len > PAGE_SIZE - page_offs) goto…
- [Medium] geth_setup_freeq() now sizes geth->freeq_pages,
  geth->freeq_page_bitmap and geth->num_freeq_pages from…
- [Medium] The rewritten geth_fill_freeq() drops the throttle that
  bounded per-refill allocation work and moves all of it under an IRQ-…

Pre-existing issues:
- [High] geth_cleanup_freeq() destroys the page_pool, frees/NULLs
  geth->freeq_page_bitmap, frees geth->freeq_pages, zeroes…
- [High] geth_resize_freeq() dereferences the sibling port pointer before
  checking it: 'if (netdev->dev_id == 0) other_netdev =…

--

net: ethernet: cortina: Manage RX buffers with page_pool

This converts the Gemini shared software free queue from hand-rolled
alloc_page()/dma_map_single() buffers to a single page_pool owned by the
parent Ethernet device, indexes each fragment by DMA address in an xarray,
and adds sync-for-CPU plus page_pool recycling on the RX path.

A few questions below about the new locking context, the failure paths and
the fragment length validation.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 31bcd41c17fa..6d5f7dacf12a 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -782,84 +852,100 @@ static void gmac_cleanup_rxq(struct net_device *netdev)

[ ... ]

> -static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
> -					      int pn)
> +static int geth_freeq_alloc_page(struct gemini_ethernet *geth, unsigned int pn)
>  {
>  	struct gmac_rxdesc *freeq_entry;
>  	struct gmac_queue_page *gpage;
> +	unsigned int fragments;
>  	unsigned int fpp_order;
>  	unsigned int frag_len;
> +	dma_addr_t page_mapping;
>  	dma_addr_t mapping;
>  	struct page *page;
> -	int i;
> +	int ret;
> +	int slot;
> +	unsigned int i;
>
> -	/* First allocate and DMA map a single page */
> -	page = alloc_page(GFP_ATOMIC);
> +	page = page_pool_dev_alloc_pages(geth->freeq_pool);
>  	if (!page)
> -		return NULL;
> +		return -ENOMEM;
>
> -	mapping = dma_map_single(geth->dev, page_address(page),
> -				 PAGE_SIZE, DMA_FROM_DEVICE);
> -	if (dma_mapping_error(geth->dev, mapping)) {
> -		put_page(page);
> -		return NULL;
> +	slot = geth_freeq_alloc_slot(geth);
> +	if (slot < 0) {
> +		page_pool_put_full_page(geth->freeq_pool, page, false);
> +		return slot;
>  	}

[Severity: High]
Is it safe to call into page_pool from here?  This function is only ever
called from geth_fill_freeq(), which holds geth->freeq_lock taken with
spin_lock_irqsave(), so hardirqs are disabled for the whole call.

page_pool takes and releases BH when the caller is not in softirq context.
The fresh-mapping path does:

net/core/page_pool.c:page_pool_register_dma_index() {
	if (in_softirq())
		err = xa_alloc(&pool->dma_mapped, ...);
	else
		err = xa_alloc_bh(&pool->dma_mapped, ...);

and the put path does:

net/core/page_pool.c:page_pool_producer_lock() {
	bool in_softirq = in_softirq();

	if (in_softirq)
		spin_lock(&pool->ring.producer_lock);
	else
		spin_lock_bh(&pool->ring.producer_lock);

Two of the callers of geth_fill_freeq() are not softirq context:
gmac_open() -> geth_resize_freeq() -> geth_setup_freeq(), and the threaded
handler gemini_port_irq_thread().  In those the matching xa_unlock_bh() /
spin_unlock_bh() reaches:

kernel/softirq.c:__local_bh_enable_ip() {
	WARN_ON_ONCE(in_hardirq());
	lockdep_assert_irqs_enabled();
	...
	if (unlikely(!in_interrupt() && local_softirq_pending())) {
		... do_softirq ...

so the lockdep assert fires with IRQs disabled, and when softirqs are
pending do_softirq() can run NET_RX_SOFTIRQ on the same CPU:

gmac_napi_poll() -> gmac_rx() -> geth_freeq_claim()
	-> spin_lock_irqsave(&geth->freeq_lock)

Can that self-deadlock on freeq_lock, which geth_fill_freeq() is already
holding?  Before this patch the same region only used put_page() and
page_ref_add(), neither of which touches BH state.

Would moving the page_pool allocation and the put outside the freeq_lock
critical section, or using allow_direct/softirq-safe context, address this?

>
> -	/* 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
> -	 * bytes, 2k) so fpp_order (fragments per page order) is default 1. Thus
> -	 * each page normally needs two entries in the queue.
> +	/* PAGE_SHIFT is 12 on Gemini, while the default fragment order is 11,
> +	 * so each page normally supplies two free queue entries.
>  	 */
>  	frag_len = 1 << geth->freeq_frag_order; /* Usually 2048 */
>  	fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
> +	fragments = 1 << fpp_order;
>  	freeq_entry = geth->freeq_ring + (pn << fpp_order);
> +	page_mapping = page_pool_get_dma_addr(page);
> +	if (page_mapping > U32_MAX - (PAGE_SIZE - 1)) {
> +		dev_err_ratelimited(geth->dev,
> +				    "freeq DMA mapping exceeds 32 bits\n");
> +		ret = -EOVERFLOW;
> +		goto err_slot;
> +	}

[ ... ]

> +err_mappings:
> +	while (i--) {
> +		mapping = page_mapping + i * frag_len;
> +		xa_erase(&geth->freeq_mappings,
> +			 geth_freeq_mapping_index(geth, mapping));
> +	}
> +	gpage->page = NULL;
> +	gpage->mapping = 0;
> +err_slot:
> +	__clear_bit(slot, geth->freeq_page_bitmap);
> +	page_pool_put_full_page(geth->freeq_pool, page, false);
> +	return ret;
>  }

> @@ -890,28 +976,9 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
>
>  	/* Loop over the freeq ring buffer entries */
>  	while (pn != epn) {
> -		struct gmac_queue_page *gpage;
> -		struct page *page;
> -
> -		gpage = &geth->freeq_pages[pn];
> -		page = gpage->page;
> +		if (geth_freeq_alloc_page(geth, pn))
> +			break;
>
> -		dev_dbg(geth->dev, "fill entry %d page ref count %d add %d refs\n",
> -			pn, page_ref_count(page), 1 << fpp_order);
> -
> -		if (page_ref_count(page) > 1) {
> -			unsigned int fl = (pn - epn) & m_pn;
> -
> -			if (fl > 64 >> fpp_order)
> -				break;
> -
> -			page = geth_freeq_alloc_map_page(geth, pn);
> -			if (!page)
> -				break;
> -		}
> -
> -		/* Add one reference per fragment in the page */
> -		page_ref_add(page, 1 << fpp_order);
>  		count += 1 << fpp_order;
>  		pn++;
>  		pn &= m_pn;

[Severity: Medium]
Should the removal of the "fl > 64 >> fpp_order" bound be mentioned in the
commit message?

The old loop reused the existing page with a cheap page_ref_add() whenever
page_ref_count(page) == 1, and when a replacement was actually needed it
stopped after roughly 32 replacements.  The initial full fill was a separate
loop in geth_setup_freeq() that ran outside any lock.

The new loop calls geth_freeq_alloc_page() for every page slot between wptr
and rptr-1 with no cap, and each iteration does a page_pool allocation plus
one xa_insert(GFP_ATOMIC) per fragment, all inside
spin_lock_irqsave(&geth->freeq_lock).  With the default freeq_order 11 and
fpp_order 1 that is up to 1024 allocations and 2048 inserts with hardirqs
off; at the advertised maximum freeq_order 15 it is up to 16384 allocations
and 32768 inserts.  The same path is the SWFQ-empty threaded handler and the
NAPI poll refill.

The commit message lists the lifecycle changes but not the removal of the
refill work bound.  Could it say something about the new per-refill cost?

>  }
>
>  static int geth_setup_freeq(struct gemini_ethernet *geth)
>  {
> +	struct page_pool_params pp_params = {
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> +		.order = 0,
> +		.nid = NUMA_NO_NODE,
> +		.dev = geth->dev,
> +		.dma_dir = DMA_FROM_DEVICE,
> +		.max_len = PAGE_SIZE,
> +	};
>  	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 page_slots = pages;
>  	union queue_threshold qt;
>  	union dma_skb_size skbsz;
>  	unsigned int filled;
> -	unsigned int pn;
> +	int ret = -ENOMEM;
> +
> +	if (geth->port0)
> +		page_slots += 1 << geth->port0->rxq_order;
> +	if (geth->port1)
> +		page_slots += 1 << geth->port1->rxq_order;
> +	pp_params.pool_size = pages;

[Severity: Medium]
Can num_freeq_pages go stale relative to the ports' rxq_order?

This is the only place that sizes freeq_pages, freeq_page_bitmap and
num_freeq_pages, and it reads both ports' rxq_order.  The site that mutates
those inputs is gmac_set_ringparam():

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

rxq_order is written unconditionally, and geth_resize_freeq() returns early:

	new_order = min(15, ilog2(new_size - 1) + 1);
	...
	if (geth->freeq_order == new_order)
		return 0;

Because new_order saturates at 15, different rxq_order pairs can map to the
same freeq_order.  Starting from the default rxq_order 9 on both ports
(freeq_order 11), "ethtool -G eth0 rx 32768" gives rxq_order 15 and
new_order 15, so a resize runs and page_slots becomes
16384 + 32768 + 512 = 49664.  A following "ethtool -G eth1 rx 32768" also
computes new_order 15, which equals geth->freeq_order, so geth_setup_freeq()
never re-runs while eth1's rxq_order is now 15 too.  num_freeq_pages stays
49664 while the two RX rings plus the free queue can need up to
16384 + 32768 + 32768 = 81920 slots.

geth_freeq_alloc_slot() then returns -ENOSPC and geth_fill_freeq() breaks
out early, silently under-filling the free queue.  Both ring sizes are
inside the range gmac_get_ringparam() advertises.

[Severity: High]
This isn't a bug introduced by this patch, but since these port pointers are
now read here as well: geth_resize_freeq() dereferences the sibling port
before checking it for NULL.

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

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

gemini_ethernet_port_probe() only assigns geth->port0 / geth->port1 after
the clock and reset steps, and it registers each netdev independently.  If
one port's probe defers or fails early (devm_clk_get() returning
-EPROBE_DEFER, clk_prepare_enable() or devm_reset_control_get_exclusive()
failing), the surviving port's netdev is registered while the sibling
pointer is still NULL.  Bringing that interface up then runs gmac_open() ->
geth_resize_freeq() and dereferences NULL.  Should the NULL check come
before the dereference?

>
>  	geth->freeq_ring = dma_alloc_coherent(geth->dev,
>  		sizeof(*geth->freeq_ring) << geth->freeq_order,
> @@ -946,19 +1027,25 @@ 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++)
> -		if (!geth_freeq_alloc_map_page(geth, pn))
> -			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;
> +	geth->freeq_page_cursor = 0;
> +
> +	geth->freeq_pool = page_pool_create(&pp_params);
> +	if (IS_ERR(geth->freeq_pool)) {
> +		ret = PTR_ERR(geth->freeq_pool);
> +		geth->freeq_pool = NULL;
> +		goto err_freeq_bitmap;
> +	}
>
>  	filled = geth_fill_freeq(geth, false);
>  	if (!filled)
> -		goto err_freeq_alloc;
> +		goto err_freeq_pool;

[ ... ]

> -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];
> -		put_page(gpage->page);
> -	}
> -
> +err_freeq_pool:
> +	xa_destroy(&geth->freeq_mappings);
> +	page_pool_destroy(geth->freeq_pool);
> +	geth->freeq_pool = NULL;
> +err_freeq_bitmap:
> +	bitmap_free(geth->freeq_page_bitmap);
> +	geth->freeq_page_bitmap = NULL;
> +err_freeq_pages:
>  	kfree(geth->freeq_pages);
> +	geth->freeq_pages = NULL;
> +	geth->num_freeq_pages = 0;
>  err_freeq:
>  	dma_free_coherent(geth->dev,
>  			  sizeof(*geth->freeq_ring) << geth->freeq_order,
>  			  geth->freeq_ring, geth->freeq_dma_base);
>  	geth->freeq_ring = NULL;
> -	return -ENOMEM;
> +	return ret;
>  }

[Severity: High]
After these labels run, geth->freeq_pool is NULL and num_freeq_pages is 0,
but the refill path does not check either:

geth_freeq_alloc_page() {
	page = page_pool_dev_alloc_pages(geth->freeq_pool);
	if (!page)
		return -ENOMEM;

Can this oops?  geth_resize_freeq() latches the new order and arms the
free-queue-empty interrupt regardless of what geth_setup_freeq() returned:

	geth->freeq_order = new_order;
	ret = geth_setup_freeq(geth);
	...
	en |= SWFQ_EMPTY_INT_BIT;
	writel(en, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);

so after a bitmap_zalloc(), page_pool_create() or "filled == 0" failure the
threaded handler gemini_port_irq_thread() -> geth_fill_freeq(geth, true)
runs with pn != epn and calls page_pool_dev_alloc_pages(NULL).  The -ENOSPC
guard in geth_freeq_alloc_slot() cannot help because the pool allocation
happens first.

There is a second effect from the latched order: on the next attempt

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

reports success even though nothing was set up, so gmac_open() proceeds to
napi_enable(), gmac_start_dma() and gmac_enable_irq() with no free queue and
no pool.  The order-before-success assignment predates this patch, but the
NULL pool is new.  Would checking geth->freeq_pool in geth_fill_freeq(), and
only arming SWFQ_EMPTY_INT_BIT on success, cover both?

>
>  /**
> @@ -1011,23 +1093,30 @@ static int geth_setup_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++) {
> +	pool = geth->freeq_pool;
> +	for (pn = 0; pn < geth->num_freeq_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);
> +		while (gpage->fragments) {
> +			page_pool_put_full_page(pool, gpage->page, false);
> +			gpage->fragments--;
> +		}
>  	}
> +	xa_destroy(&geth->freeq_mappings);
>
> +	bitmap_free(geth->freeq_page_bitmap);
> +	geth->freeq_page_bitmap = NULL;
>  	kfree(geth->freeq_pages);
> +	geth->freeq_pages = NULL;
> +	geth->num_freeq_pages = 0;
>
>  	dma_free_coherent(geth->dev,
>  			  sizeof(*geth->freeq_ring) << geth->freeq_order,
>  			  geth->freeq_ring, geth->freeq_dma_base);
> +	geth->freeq_ring = NULL;
> +	geth->freeq_pool = NULL;
> +	page_pool_destroy(pool);
>  }

[Severity: High]
This is a pre-existing teardown race, but this patch adds new objects to the
unsynchronized window, so it may be worth handling here.

None of these frees is done under geth->freeq_lock, while the readers of the
very same fields do hold it:

geth_freeq_alloc_page()	-> find_next_zero_bit(geth->freeq_page_bitmap, ...)
			-> page_pool_dev_alloc_pages(geth->freeq_pool)
geth_freeq_claim()	-> xa_load(&geth->freeq_mappings, ...)

geth_resize_freeq() only masks the interrupt source before calling in:

	en &= ~SWFQ_EMPTY_INT_BIT;
	writel(en, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
	...
	/* Drop the old queue */
	if (geth->freeq_ring)
		geth_cleanup_freeq(geth);

There is no disable_irq()/synchronize_irq(), so a gemini_port_irq_thread()
that was already woken by IRQ_WAKE_THREAD can run concurrently with, or
preempt, the resize and call geth_fill_freeq(geth, true).  Can that use the
freed freeq_page_bitmap and freeq_pages, the destroyed xarray, and the
destroyed or NULL page_pool?  page_pool_destroy() also expects no concurrent
producers.  The thread re-enables SWFQ_EMPTY_INT_BIT on exit, which undoes
the mask.

>
>  /**

[ ... ]

> @@ -1544,10 +1629,18 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  		/* append page frag to skb */
>  		if (frag_nr == MAX_SKB_FRAGS)
>  			goto err_drop;
> +		if (frag_len > PAGE_SIZE - page_offs)
> +			goto err_drop;

[Severity: Medium]
Should this be bounded by the fragment size rather than by the page?

The device is told how much it may write per free queue entry in
geth_setup_freeq():

	skbsz.bits.sw_skb_size = 1 << geth->freeq_frag_order;
	writel(skbsz.bits32, geth->base + GLOBAL_DMA_SKB_SIZE_REG);

and geth_freeq_claim() returns a fragment-aligned page_offs inside
[0, PAGE_SIZE - frag_len]:

	mapping - page_mapping <= PAGE_SIZE - frag_len &&
	!((mapping - page_mapping) & (frag_len - 1));
	...
	*page_offs = mapping - page_mapping;

So with the default freeq_frag_order 11 the first fragment of a page passes
this check with up to PAGE_SIZE - 2 bytes, that is up to 2046 bytes past the
2048-byte buffer the device was allowed to write into.  frag_len comes
straight from hardware (word0.bits.buffer_size, and on EOF descriptors it is
replaced by "frame_len - skb->len" from word1.bits.byte_count), and neither
is cross-checked against the claimed fragment size.  Multi-descriptor chains
are reachable from the wire since gmac_pick_rx_max_len() selects
CONFIG0_MAXLEN_9k for the driver's max_mtu.

> -		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;
> +		}
>
> +		page_pool_dma_sync_for_cpu(geth->freeq_pool, page, page_offs,
> +					   frag_len);
>  		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);

[Severity: High]
Turning the len == 0 warning into a drop looks like it changes
behaviour for frames that are actually fine.

A few lines above, EOF descriptors get their length recomputed from
the frame length rather than from the descriptor:

	if (word3.bits32 & EOF_BIT)
		frag_len = frame_len - skb->len;

so whenever the payload has already been fully accumulated by the
preceding fragments, the closing EOF descriptor legitimately
contributes zero bytes and frag_len becomes 0. With the default
freeq_frag_order 11 the first fragment carries 2048 - NET_IP_ALIGN
bytes and the following ones 2048, so the sizes where the remainder
lands exactly on 0 are ordinary on-wire frame lengths, not a hardware
fault.

Before this patch that case only emitted the ratelimited
"Received fragment with len = 0" message and then fell through to the
EOF handling, which pushed the completed skb into napi_gro_frags().
Now it takes err_drop, which calls napi_free_frags() and throws away
the whole chain including every fragment already collected, and
returns the claimed page to the pool. The frame was complete, so this
is a silent loss of good traffic driven purely by packet size, and it
also leaves the counters inconsistent: gmac_skb_if_good_frame() has
already done

	port->stats.rx_bytes += frame_len;
	port->stats.rx_packets++;

for this frame, and err_drop then adds to rx_dropped as well.

Should the zero-length case be split by descriptor type, i.e. treat
an EOF descriptor with a zero remainder as "frame already complete"
and go straight to the napi_gro_frags() path (returning the unused
fragment to the pool), and keep the drop only for a non-EOF
descriptor that reports no data? Or is there a reason the hardware
cannot produce a zero-remainder EOF descriptor on this part, in which
case it would help to say so in the commit message, since the change
from warn-and-continue to drop is not mentioned there.

Continuing the previous question: with the page-granular bound, can this
sync-for-CPU and this skb_fill_page_desc() cover the sibling fragment, which
may still be queued in the free queue and owned by the device, possibly
being written for the other port?  That would both invalidate cache lines
for a device-owned range and hand the sibling fragment's contents to the
stack.  Would a bound like

	if (frag_len > (1 << geth->freeq_frag_order) -
		       (page_offs & ((1 << geth->freeq_frag_order) - 1)))
		goto err_drop;

match what the hardware was programmed with?

>  		skb->len += frag_len;
>  		skb->data_len += frag_len;

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

[PATCH net-next 3/3] 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 6d5f7dacf12a..a14209ec2fe6 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>
@@ -472,6 +474,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);
@@ -540,7 +553,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 0/3] net: ethernet: cortina: Use page_pool for Gemini RX buffers

[Jakub Kicinski]

On Sun, 20 Sep 2026 22:48:50 +0200 Linus Walleij wrote:
> The Gemini ethernet has this really convoluted queue mechanism using
> a custom queue.
> 
> Move the Gemini RX buffer management to a parent-owned page_pool.

TBH sounds like weird piece of HW. IDK if we want to burden the generic
page pool code with weird, 20+ yo drivers+HW :/ Well, I know that 
I definitely don't, I don't know if others agree :)

Looks like AI has found plenty o'bugs.

Also, please never put Fixes tags from Linus's tree on net-next patches.
Either net or not a fix.
-- 
pw-bot: cr

Re: [PATCH net-next 0/3] net: ethernet: cortina: Use page_pool for Gemini RX buffers

[Linus Walleij]

Hi Jakub,

On Thu, Sep 24, 2026 at 3:28 AM Jakub Kicinski <kuba@kernel.org> wrote:

> > Move the Gemini RX buffer management to a parent-owned page_pool.
>
> TBH sounds like weird piece of HW. IDK if we want to burden the generic
> page pool code with weird, 20+ yo drivers+HW :/ Well, I know that
> I definitely don't, I don't know if others agree :)

The current AI reviews actively encourages people to go and fix
really old bugs,  or code that hasn't seen a lot of love in recent
years, pre-existing issues you know.

That's how the patch came about that this patch is a "let's do it
properly in that case" reply to:
https://lore.kernel.org/netdev/20260917192835.58126-1-mhun512@gmail.com/

We have had, for years, a debug print in this driver:

                /* Freeq pointers are one page off */
                gpage = gmac_get_queue_page(geth, port, mapping + PAGE_SIZE);
                if (!gpage) {
                        dev_err(geth->dev, "could not find page\n");
                        continue;

and it seems this patch set actually finally fixes that bug that was
bugging me (heh) for years and years.

So there is a bit of incentive to actually fix it.

(I also have actual users of the code, it's not therapeutic coding.)

> Also, please never put Fixes tags from Linus's tree on net-next patches.
> Either net or not a fix.

OK I get it.

Yours,
Linus Walleij

Re: [PATCH net-next 0/3] net: ethernet: cortina: Use page_pool for Gemini RX buffers

[Jakub Kicinski]

On Thu, 24 Sep 2026 09:40:24 +0200 Linus Walleij wrote:
> > > Move the Gemini RX buffer management to a parent-owned page_pool.  
> >
> > TBH sounds like weird piece of HW. IDK if we want to burden the generic
> > page pool code with weird, 20+ yo drivers+HW :/ Well, I know that
> > I definitely don't, I don't know if others agree :)  
> 
> The current AI reviews actively encourages people to go and fix
> really old bugs,  or code that hasn't seen a lot of love in recent
> years, pre-existing issues you know.
> 
> That's how the patch came about that this patch is a "let's do it
> properly in that case" reply to:
> https://lore.kernel.org/netdev/20260917192835.58126-1-mhun512@gmail.com/
> 
> We have had, for years, a debug print in this driver:
> 
>                 /* Freeq pointers are one page off */
>                 gpage = gmac_get_queue_page(geth, port, mapping + PAGE_SIZE);
>                 if (!gpage) {
>                         dev_err(geth->dev, "could not find page\n");
>                         continue;
> 
> and it seems this patch set actually finally fixes that bug that was
> bugging me (heh) for years and years.
> 
> So there is a bit of incentive to actually fix it.
> 
> (I also have actual users of the code, it's not therapeutic coding.)

Understood, but none of this really necessitate the page pool.
Supporting weird drivers makes it much harder for us to create
reasonable uAPI and protect these things with fine-granularity
locks.