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

[PATCH net-next v4 00/15] 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 v4:
- Protect stop-path RX statistics updates against preemption on 32-bit systems.
- Validate final RX lengths against the Ethernet minimum and descriptor
  transfer size so malformed chains cannot pass short skbs to GRO.
- Charge the full posted RX fragment size in skb truesize accounting.
- Reject free queue geometries that cannot post one complete page and
  document the changed refill and recycling behavior.
- Reset the free queue allocation cursor with its bitmap during teardown.
- Bound recycler IRQ latency and release retained mappings for stack-owned
  pages before allocating replacements.
- Rebuild oversized free queues while both ports are down so RX ring
  downsizing releases the surplus pages.
- Scope RX statistics writer sections to counter updates for PREEMPT_RT.
- Restrict shared free queue handling to port 1 so IRQ draining covers
  every registered handler path.
- Reset and mask free queue pointers when rebuilding the queue.
- Reorder the shared queue ownership fix before the port lifetime fix.
- Publish ports only after setup, serialize their lifetime with RTNL and
  suppress independent child unbind while port 1 owns the shared IRQ.
- Move child-unbind suppression and PHY teardown ordering into the
  parent-ownership patch.
- Link to v3: https://patch.msgid.link/20261002-gemini-ethernet-fixes-3-v3-0-3e1f25890ae8@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 (15):
      net: ethernet: cortina: Keep shared free queue parent-owned
      net: ethernet: cortina: Keep PHY-less port bound for shared IRQ
      net: ethernet: cortina: Drain free queue IRQ before resize
      net: ethernet: cortina: Normalize free queue pointers on rebuild
      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: Account RX buffer size in truesize
      net: ethernet: cortina: Release partial RX frames on stop
      net: ethernet: cortina: Scale Gemini RX queues to system memory
      net: ethernet: cortina: Scope RX statistics updates

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

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

[PATCH net-next v4 01/15] 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 paths where no queue was allocated.

Port 1 owns the shared refill interrupt, so prevent the children from
being unbound independently. Unregister each netdev before detaching its
PHY so ndo_stop can still access netdev->phydev.

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 | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 2fe7fd0202d2..bf7652b04f1b 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);
@@ -2387,11 +2390,10 @@ static irqreturn_t gemini_port_irq(int irq, void *data)
 static void gemini_port_remove(struct gemini_ethernet_port *port)
 {
 	if (port->netdev) {
-		phy_disconnect(port->netdev->phydev);
 		unregister_netdev(port->netdev);
+		phy_disconnect(port->netdev->phydev);
 	}
 	clk_disable_unprepare(port->pclk);
-	geth_cleanup_freeq(port->geth);
 }
 
 static void gemini_ethernet_init(struct gemini_ethernet *geth)
@@ -2635,6 +2637,7 @@ static struct platform_driver gemini_ethernet_port_driver = {
 	.driver = {
 		.name = "gemini-ethernet-port",
 		.of_match_table = gemini_ethernet_port_of_match,
+		.suppress_bind_attrs = true,
 	},
 	.probe = gemini_ethernet_port_probe,
 	.remove = gemini_ethernet_port_remove,
@@ -2683,6 +2686,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.56.0

[PATCH net-next v4 02/15] 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 and releases the threaded
handler needed by the shared queue.

Keep children without a PHY description bound as IRQ-only ports.
Publish a port to the parent only after its IRQ and optional netdev are
ready, and protect port lifetime and initialization state with RTNL.
Refuse shared queue setup until both ports have completed
initialization.

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 | 88 ++++++++++++++++++++++++++++-------
 1 file changed, 70 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index bf7652b04f1b..ec2e47688337 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -25,6 +25,7 @@
 #include <linux/interrupt.h>
 #include <linux/reset.h>
 #include <linux/clk.h>
+#include <linux/rtnetlink.h>
 #include <linux/of.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
@@ -155,6 +156,7 @@ struct gemini_ethernet_port {
 struct gemini_ethernet {
 	struct device *dev;
 	void __iomem *base;
+	/* Protected by RTNL. */
 	struct gemini_ethernet_port *port0;
 	struct gemini_ethernet_port *port1;
 	bool initialized;
@@ -1051,10 +1053,17 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	u32 en;
 	int ret;
 
+	ASSERT_RTNL();
+
+	/* The software free queue interrupt is routed through port 1. */
+	if (!geth->initialized || !geth->port0 || !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;
@@ -1065,7 +1074,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,
@@ -2387,8 +2395,28 @@ 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;
+
+	ASSERT_RTNL();
+
+	if (!port->id && geth->port0 == port)
+		geth->port0 = NULL;
+	else if (port->id && geth->port1 == port)
+		geth->port1 = NULL;
+	else
+		return;
+
+	geth->initialized = false;
+}
+
 static void gemini_port_remove(struct gemini_ethernet_port *port)
 {
+	rtnl_lock();
+	gemini_port_clear(port);
+	rtnl_unlock();
+
 	if (port->netdev) {
 		unregister_netdev(port->netdev);
 		phy_disconnect(port->netdev->phydev);
@@ -2398,12 +2426,12 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
 
 static void gemini_ethernet_init(struct gemini_ethernet *geth)
 {
+	ASSERT_RTNL();
+
 	/* Only do this once both ports are online */
 	if (geth->initialized)
 		return;
-	if (geth->port0 && geth->port1)
-		geth->initialized = true;
-	else
+	if (!geth->port0 || !geth->port1)
 		return;
 
 	writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_0_REG);
@@ -2446,6 +2474,20 @@ static void gemini_ethernet_init(struct gemini_ethernet *geth)
 	 * set up and enable the queue IRQ. FIXME: fragile.
 	 */
 	geth->freeq_order = 1;
+	geth->initialized = true;
+}
+
+static void gemini_port_publish(struct gemini_ethernet_port *port)
+{
+	struct gemini_ethernet *geth = port->geth;
+
+	ASSERT_RTNL();
+
+	if (!port->id)
+		geth->port0 = port;
+	else
+		geth->port1 = port;
+	gemini_ethernet_init(geth);
 }
 
 static void gemini_port_save_mac_addr(struct gemini_ethernet_port *port)
@@ -2541,15 +2583,6 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
 	reset_control_reset(port->reset);
 	usleep_range(100, 500);
 
-	/* Assign pointer in the main state container */
-	if (!id)
-		geth->port0 = port;
-	else
-		geth->port1 = port;
-
-	/* This will just be done once both ports are up and reset */
-	gemini_ethernet_init(geth);
-
 	platform_set_drvdata(pdev, port);
 
 	/* Set up and register the netdev */
@@ -2600,6 +2633,16 @@ 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;
+		rtnl_lock();
+		gemini_port_publish(port);
+		rtnl_unlock();
+		return 0;
+	}
+
 	ret = gmac_setup_phy(netdev);
 	if (ret) {
 		netdev_err(netdev,
@@ -2607,9 +2650,15 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
 		goto unprepare;
 	}
 
-	ret = register_netdev(netdev);
-	if (ret)
+	rtnl_lock();
+	ret = register_netdevice(netdev);
+	if (!ret)
+		gemini_port_publish(port);
+	rtnl_unlock();
+	if (ret) {
+		phy_disconnect(netdev->phydev);
 		goto unprepare;
+	}
 
 	return 0;
 
@@ -2686,10 +2735,13 @@ static void gemini_ethernet_remove(struct platform_device *pdev)
 {
 	struct gemini_ethernet *geth = platform_get_drvdata(pdev);
 
+	rtnl_lock();
+	geth->initialized = false;
+	rtnl_unlock();
+
 	devm_of_platform_depopulate(&pdev->dev);
 	writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
 	geth_cleanup_freeq(geth);
-	geth->initialized = false;
 }
 
 static const struct of_device_id gemini_ethernet_of_match[] = {

-- 
2.56.0

[PATCH net-next v4 03/15] 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, but both port IRQs
register the global handler. Ignore the source on port 0, then disable the
port 1 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 | 59 ++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index ec2e47688337..c03541deed49 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -986,6 +986,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,
@@ -1026,20 +1028,37 @@ 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);
 }
 
 /**
  * geth_resize_freeq() - resize the software queue depth
  * @port: the port requesting the change
  *
- * This gets called at least once during probe() so the device queue gets
- * "resized" from the hardware defaults. Since both ports/net devices share
- * the same hardware queue, some synchronization between the ports is
- * needed.
+ * This is called when a port is opened or its RX ring size changes. Since
+ * both ports/net devices share the same hardware queue, some synchronization
+ * between the ports is needed.
  */
 static int geth_resize_freeq(struct gemini_ethernet_port *port)
 {
@@ -1049,8 +1068,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;
 
 	ASSERT_RTNL();
@@ -1084,16 +1101,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,14 +1115,10 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	geth->freeq_order = new_order;
 	ret = geth_setup_freeq(geth);
 
-	/* Restart the interrupts - NOTE if this is the first resize
-	 * 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);
+	/* Enable the queue interrupt only after successful setup. */
+	if (!ret)
+		geth_set_freeq_irq(geth, true);
+	enable_irq(geth->port1->irq);
 
 	return ret;
 }
@@ -2373,6 +2381,9 @@ static irqreturn_t gemini_port_irq(int irq, void *data)
 	irqreturn_t ret = IRQ_NONE;
 	u32 val, en;
 
+	if (!port->id)
+		return IRQ_NONE;
+
 	geth = port->geth;
 	spin_lock(&geth->irq_lock);
 
@@ -2470,10 +2481,6 @@ static void gemini_ethernet_init(struct gemini_ethernet *geth)
 	writel(0, geth->base + GLOBAL_HWFQ_RWPTR_REG);
 
 	geth->freeq_frag_order = DEFAULT_RX_BUF_ORDER;
-	/* This makes the queue resize on probe() so that we
-	 * set up and enable the queue IRQ. FIXME: fragile.
-	 */
-	geth->freeq_order = 1;
 	geth->initialized = true;
 }
 

-- 
2.56.0

[PATCH net-next v4 04/15] net: ethernet: cortina: Normalize free queue pointers on rebuild

[Linus Walleij]

The free queue pointers can retain positions from the previous ring when
the queue is rebuilt at a smaller order. Using an unmasked pointer to
index the replacement page table can then access it out of bounds.

Reset the pointer register before initially filling a rebuilt queue and
mask the page index to the active queue order. The mask also keeps the
index safe if hardware retains its read pointer while the queue is
disabled.

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

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index c03541deed49..c3d1abdbebf6 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -887,6 +887,7 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
 
 	rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
 	pn = (refill ? rw.bits.wptr : rw.bits.rptr) >> fpp_order;
+	pn &= m_pn;
 	epn = (rw.bits.rptr >> fpp_order) - 1;
 	epn &= m_pn;
 
@@ -958,6 +959,9 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 		if (!geth_freeq_alloc_map_page(geth, pn))
 			goto err_freeq_alloc;
 
+	/* Discard pointer state belonging to the previous queue geometry. */
+	writel(0, geth->base + GLOBAL_SWFQ_RWPTR_REG);
+
 	filled = geth_fill_freeq(geth, false);
 	if (!filled)
 		goto err_freeq_alloc;

-- 
2.56.0

[PATCH net-next v4 05/15] 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 c3d1abdbebf6..3b915a4c8900 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -726,32 +726,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)
@@ -759,11 +770,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;
 
@@ -790,14 +802,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,
@@ -811,6 +822,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;
@@ -820,9 +832,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;
 	}
@@ -835,31 +855,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;
 }
@@ -930,7 +944,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;
@@ -980,12 +993,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);
 	}
 
@@ -1025,7 +1037,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);
@@ -1479,7 +1490,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;
@@ -1516,8 +1526,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);
@@ -1536,14 +1544,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.56.0

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

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

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 3b915a4c8900..dc82c896fa00 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -38,6 +38,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>
@@ -167,6 +168,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 */
 };
@@ -726,43 +728,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)
@@ -821,12 +827,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)
@@ -855,9 +865,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;
@@ -876,6 +903,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;
 }
 
 /**
@@ -948,6 +986,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;
 
@@ -961,16 +1001,20 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 		goto err_freeq;
 	}
 
-	/* Allocate a mapping to page look-up index */
+	/* Allocate page metadata referenced by the mapping index */
 	geth->freeq_pages = kzalloc_objs(*geth->freeq_pages, pages);
 	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))
+	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;
+	}
 
 	/* Discard pointer state belonging to the previous queue geometry. */
 	writel(0, geth->base + GLOBAL_SWFQ_RWPTR_REG);
@@ -1000,6 +1044,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;
@@ -1041,6 +1086,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;
@@ -2740,6 +2786,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.56.0

[PATCH net-next v4 07/15] 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 each candidate page before taking the free queue
lock, then re-read the hardware pointers and publish one complete page at
a time. The old refill path reused pages by queue position and only
allocated replacements once the queued fill fell to 64 descriptors or
fewer. This change instead allocates replacements until the ring is full.
A later patch restores recycling independently of queue position.

Reject queue geometries that cannot post one complete page before tearing
down the existing queue. 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 | 320 +++++++++++++++++++++-------------
 1 file changed, 196 insertions(+), 124 deletions(-)

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index dc82c896fa00..284823bdfd50 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>
@@ -89,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 {
@@ -170,6 +173,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 */
 };
 
@@ -735,15 +739,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;
 
@@ -751,7 +772,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;
@@ -762,6 +783,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;
@@ -808,7 +844,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;
@@ -821,32 +857,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,
@@ -854,9 +880,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
@@ -867,7 +918,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,
@@ -876,19 +929,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",
@@ -902,7 +944,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--) {
@@ -910,20 +952,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;
@@ -935,48 +977,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;
-	pn &= m_pn;
-	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) & m_pn;
+		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) & m_pn;
+		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)
@@ -984,12 +1073,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,
@@ -1002,26 +1095,21 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 	}
 
 	/* Allocate page metadata referenced by the mapping index */
-	geth->freeq_pages = kzalloc_objs(*geth->freeq_pages, pages);
+	geth->freeq_pages = kzalloc_objs(*geth->freeq_pages, page_slots);
 	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;
 
 	/* Discard pointer state belonging to the previous queue geometry. */
 	writel(0, geth->base + GLOBAL_SWFQ_RWPTR_REG);
 
-	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;
@@ -1034,18 +1122,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;
@@ -1063,12 +1146,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;
 
@@ -1076,18 +1153,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;
@@ -1162,6 +1231,9 @@ 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 (new_order <= PAGE_SHIFT - geth->freeq_frag_order)
+		return -EINVAL;
+
 	if (geth->freeq_ring && geth->freeq_order == new_order)
 		return 0;
 
@@ -1590,7 +1662,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");
@@ -1687,7 +1759,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 &&
@@ -2417,7 +2489,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.56.0

[PATCH net-next v4 08/15] 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. Reuse an oversized queue only while another port
is running; otherwise rebuild it at the requested order so downsizing
releases the surplus pages. Propagate resize failures from open and
restore the old RX order after a failed ethtool request.

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

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 284823bdfd50..cd5ded7384a5 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1068,21 +1068,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,
@@ -1196,7 +1206,9 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	struct net_device *netdev = port->netdev;
 	struct gemini_ethernet_port *other_port;
 	struct net_device *other_netdev;
+	bool other_running;
 	unsigned int new_size = 0;
+	unsigned int page_slots;
 	unsigned int new_order;
 	int ret;
 
@@ -1211,9 +1223,7 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	else
 		other_port = geth->port0;
 	other_netdev = other_port ? other_port->netdev : NULL;
-
-	if (other_netdev && netif_running(other_netdev))
-		return -EBUSY;
+	other_running = other_netdev && netif_running(other_netdev);
 
 	new_size = 1 << (port->rxq_order + 1);
 	netdev_dbg(netdev, "port %d size: %d order %d\n",
@@ -1234,8 +1244,16 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
 	if (new_order <= PAGE_SHIFT - geth->freeq_frag_order)
 		return -EINVAL;
 
-	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->num_freeq_pages >= page_slots &&
+		    (geth->freeq_order == new_order ||
+		     (other_running && geth->freeq_order > new_order)))
+			return 0;
+	}
+
+	if (other_running)
+		return -EBUSY;
 
 	disable_irq(geth->port1->irq);
 	geth_set_freeq_irq(geth, false);
@@ -2014,10 +2032,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;
 	}
@@ -2376,14 +2391,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.56.0

[PATCH net-next v4 09/15] 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. Reset the cursor when the bitmap is released so
the two remain consistent across failed setup and teardown. This keeps
allocation cost distributed across the metadata table.

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

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index cd5ded7384a5..3bb96c51e0b7 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -174,6 +174,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 */
 };
 
@@ -745,12 +746,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;
 }
@@ -1112,6 +1121,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;
 
 	/* Discard pointer state belonging to the previous queue geometry. */
 	writel(0, geth->base + GLOBAL_SWFQ_RWPTR_REG);
@@ -1142,6 +1152,7 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 	kfree(geth->freeq_pages);
 	geth->freeq_pages = NULL;
 	geth->num_freeq_pages = 0;
+	geth->freeq_page_cursor = 0;
 err_freeq:
 	dma_free_coherent(geth->dev,
 			  sizeof(*geth->freeq_ring) << geth->freeq_order,
@@ -1170,6 +1181,7 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 	kfree(geth->freeq_pages);
 	geth->freeq_pages = NULL;
 	geth->num_freeq_pages = 0;
+	geth->freeq_page_cursor = 0;
 
 	dma_free_coherent(geth->dev,
 			  sizeof(*geth->freeq_ring) << geth->freeq_order,

-- 
2.56.0

[PATCH net-next v4 10/15] net: ethernet: cortina: Recycle claimed free queue pages

[Linus Walleij]

The preceding "Preserve in-flight free queue pages" 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. Track fully claimed slots in a separate bitmap and
inspect one at a time during refill. Recycle a page once its reference
count returns to one. If the stack still owns fragments, unmap the page
and relinquish the base reference before allocating its replacement.

This prevents retained pages from exhausting the finite metadata table
and bounds the IRQ-disabled search to a bitmap lookup and one reference
count check. It retains the existing free queue allocator without
introducing a page pool.

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

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 3bb96c51e0b7..943c31c02674 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -174,7 +174,9 @@ struct gemini_ethernet {
 	struct xarray	freeq_mappings;
 	unsigned int	num_freeq_pages;
 	unsigned long	*freeq_page_bitmap;
+	unsigned long	*freeq_recycle_bitmap;
 	unsigned int	freeq_page_cursor;
+	unsigned int	freeq_recycle_pending;
 	spinlock_t	freeq_lock; /* Locks queue from reentrance */
 };
 
@@ -764,6 +766,34 @@ 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;
+
+	lockdep_assert_held(&geth->freeq_lock);
+
+	if (!geth->freeq_recycle_pending)
+		return -ENOSPC;
+
+	slot = find_next_bit(geth->freeq_recycle_bitmap,
+			     geth->num_freeq_pages,
+			     geth->freeq_page_cursor);
+	if (slot == geth->num_freeq_pages) {
+		slot = find_first_bit(geth->freeq_recycle_bitmap,
+				      geth->freeq_page_cursor);
+		if (slot == geth->freeq_page_cursor)
+			return -ENOSPC;
+	}
+
+	__clear_bit(slot, geth->freeq_recycle_bitmap);
+	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;
+}
+
 static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
 				     dma_addr_t mapping,
 				     unsigned int *page_offs)
@@ -773,8 +803,8 @@ 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;
+	unsigned int slot;
 	bool valid;
 
 	index = geth_freeq_mapping_index(geth, mapping);
@@ -798,13 +828,8 @@ static struct page *geth_freeq_claim(struct gemini_ethernet *geth,
 	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);
+		__set_bit(slot, geth->freeq_recycle_bitmap);
+		geth->freeq_recycle_pending++;
 	}
 
 	*page_offs = mapping - page_mapping;
@@ -898,25 +923,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
@@ -927,9 +948,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,
@@ -938,11 +956,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--) {
@@ -961,9 +982,68 @@ 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)
+{
+	struct gmac_queue_page *gpage;
+	struct page *page;
+	int ret;
+	int slot;
+
+	lockdep_assert_held(&geth->freeq_lock);
+
+	slot = geth_freeq_recycle_slot(geth);
+	if (slot < 0)
+		return slot;
+
+	gpage = &geth->freeq_pages[slot];
+	if (page_ref_count(gpage->page) > 1) {
+		page = gpage->page;
+		dma_unmap_single_attrs(geth->dev, gpage->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);
+		return -EAGAIN;
+	}
+
+	ret = geth_freeq_post_page(geth, pn, gpage, true);
+	if (ret) {
+		__set_bit(slot, geth->freeq_recycle_bitmap);
+		geth->freeq_recycle_pending++;
+	}
+
 	return ret;
 }
 
@@ -982,6 +1062,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;
@@ -997,9 +1078,26 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
 		pn = (rw.bits.wptr >> fpp_order) & m_pn;
 		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 && ret != -EAGAIN)
+			break;
 
 		ret = geth_freeq_map_page(geth, &page, &page_mapping);
 		if (ret)
@@ -1075,6 +1173,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
@@ -1120,6 +1219,9 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 	geth->freeq_page_bitmap = bitmap_zalloc(page_slots, GFP_KERNEL);
 	if (!geth->freeq_page_bitmap)
 		goto err_freeq_pages;
+	geth->freeq_recycle_bitmap = bitmap_zalloc(page_slots, GFP_KERNEL);
+	if (!geth->freeq_recycle_bitmap)
+		goto err_freeq_page_bitmap;
 	geth->num_freeq_pages = page_slots;
 	geth->freeq_page_cursor = 0;
 
@@ -1146,6 +1248,9 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
 	writew(readw(geth->base + GLOBAL_SWFQ_RWPTR_REG),
 	       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
 	geth_freeq_release_pages(geth);
+	bitmap_free(geth->freeq_recycle_bitmap);
+	geth->freeq_recycle_bitmap = NULL;
+err_freeq_page_bitmap:
 	bitmap_free(geth->freeq_page_bitmap);
 	geth->freeq_page_bitmap = NULL;
 err_freeq_pages:
@@ -1176,6 +1281,8 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
 
 	geth_freeq_release_pages(geth);
 
+	bitmap_free(geth->freeq_recycle_bitmap);
+	geth->freeq_recycle_bitmap = NULL;
 	bitmap_free(geth->freeq_page_bitmap);
 	geth->freeq_page_bitmap = NULL;
 	kfree(geth->freeq_pages);

-- 
2.56.0

[PATCH net-next v4 11/15] 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.
Require the final frame length to cover an Ethernet frame and fit in the
current descriptor so a malformed chain cannot pass a short skb to GRO.
Account these descriptor inconsistencies as drops rather than wire length
errors.

The issue was found by inspecting the descriptor arithmetic and validated
against the Gemini receive descriptor definitions. The series was tested on
a D-Link DIR-685 with media playback and ksmbd transfers under ping load.

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

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 943c31c02674..d8bafcdf4d5f 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1737,6 +1737,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;
@@ -1811,6 +1812,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;
@@ -1819,15 +1823,28 @@ 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 < ETH_ZLEN ||
+			    frame_len < skb->len ||
+			    frame_len - skb->len > frag_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.56.0

[PATCH net-next v4 12/15] net: ethernet: cortina: Account RX buffer size in truesize

[Linus Walleij]

gmac_rx() charges only the received payload length for each page fragment
attached to an skb. Each fragment pins a complete free queue DMA buffer, so
short packets understate their memory use to GRO and socket accounting.

Charge the full posted fragment size for every attached fragment. The skb
already accounts for its own head separately.

The issue was found by reviewing the RX fragment accounting. The series was
tested on a D-Link DIR-685 with media playback and ksmbd transfers under
ping load.

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

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index d8bafcdf4d5f..43fd8fd773e3 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1849,7 +1849,7 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);
 		skb->len += frag_len;
 		skb->data_len += frag_len;
-		skb->truesize += frag_len;
+		skb->truesize += freeq_frag_len;
 		frag_nr++;
 
 		if (word3.bits32 & EOF_BIT) {

-- 
2.56.0

[PATCH net-next v4 13/15] 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. Use the irqsave update helpers because
ndo_stop runs in preemptible process context while 32-bit statistics
writers must not be preempted.

The issue was found by inspecting the partial frame lifetime and statistics
writer contexts. The series was tested on a D-Link DIR-685 with media
playback and ksmbd transfers under ping load.

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

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 43fd8fd773e3..32291d4c3390 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -2209,12 +2209,19 @@ static int gmac_open(struct net_device *netdev)
 static int gmac_stop(struct net_device *netdev)
 {
 	struct gemini_ethernet_port *port = netdev_priv(netdev);
+	unsigned long flags;
 
 	hrtimer_cancel(&port->rx_coalesce_timer);
 	netif_tx_stop_all_queues(netdev);
 	gmac_disable_tx_rx(netdev);
 	gmac_stop_dma(port);
 	napi_disable(&port->napi);
+	if (port->rx_skb) {
+		napi_free_frags(&port->napi);
+		flags = u64_stats_update_begin_irqsave(&port->rx_stats_syncp);
+		port->stats.rx_dropped++;
+		u64_stats_update_end_irqrestore(&port->rx_stats_syncp, flags);
+	}
 	port->rx_skb = NULL;
 	port->rx_frag_nr = 0;
 	port->rx_dropping = false;

-- 
2.56.0

[PATCH net-next v4 14/15] 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 32291d4c3390..a61bace61ea4 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>
@@ -36,6 +37,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>
@@ -474,6 +476,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);
@@ -542,7 +555,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.56.0

[PATCH net-next v4 15/15] net: ethernet: cortina: Scope RX statistics updates

[Linus Walleij]

On 32-bit systems u64_stats_update_begin() disables preemption. The RX
poll currently holds that writer section across gmac_rx(), which takes
spinlock_t locks and calls into GRO and fragment handling. On PREEMPT_RT,
spinlock_t can sleep and must not be acquired with preemption disabled.

Keep the writer sections around the counter updates only. NAPI ownership
continues to serialize normal RX writers, while the stop path updates its
counter after napi_disable(). This preserves the existing accounting
without covering potentially sleeping operations.

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

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index a61bace61ea4..9f7946876d58 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1712,12 +1712,12 @@ static struct sk_buff *gmac_skb_if_good_frame(struct gemini_ethernet_port *port,
 	unsigned int rx_status = word0.bits.status;
 	struct sk_buff *skb = NULL;
 
-	port->rx_stats[rx_status]++;
-	port->rx_csum_stats[rx_csum]++;
-
 	if (word0.bits.derr || word0.bits.perr ||
 	    rx_status || frame_len < ETH_ZLEN ||
 	    rx_csum >= RX_CHKSUM_IP_ERR_UNKNOWN) {
+		u64_stats_update_begin(&port->rx_stats_syncp);
+		port->rx_stats[rx_status]++;
+		port->rx_csum_stats[rx_csum]++;
 		port->stats.rx_errors++;
 
 		if (frame_len < ETH_ZLEN || RX_ERROR_LENGTH(rx_status))
@@ -1728,6 +1728,7 @@ static struct sk_buff *gmac_skb_if_good_frame(struct gemini_ethernet_port *port,
 			port->stats.rx_crc_errors++;
 		if (RX_ERROR_FRAME(rx_status))
 			port->stats.rx_frame_errors++;
+		u64_stats_update_end(&port->rx_stats_syncp);
 		return NULL;
 	}
 
@@ -1739,8 +1740,12 @@ static struct sk_buff *gmac_skb_if_good_frame(struct gemini_ethernet_port *port,
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 update_exit:
+	u64_stats_update_begin(&port->rx_stats_syncp);
+	port->rx_stats[rx_status]++;
+	port->rx_csum_stats[rx_csum]++;
 	port->stats.rx_bytes += frame_len;
 	port->stats.rx_packets++;
+	u64_stats_update_end(&port->rx_stats_syncp);
 	return skb;
 }
 
@@ -1798,7 +1803,9 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 		if (word3.bits32 & SOF_BIT) {
 			if (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);
 				skb = NULL;
 				frag_nr = 0;
 			}
@@ -1883,7 +1890,9 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
 			put_page(page);
 
 		if (!dropping) {
+			u64_stats_update_begin(&port->rx_stats_syncp);
 			port->stats.rx_dropped++;
+			u64_stats_update_end(&port->rx_stats_syncp);
 			dropping = true;
 		}
 
@@ -1915,13 +1924,13 @@ static int gmac_napi_poll(struct napi_struct *napi, int budget)
 	bool reschedule;
 
 	freeq_threshold = 1 << (geth->freeq_order - 1);
-	u64_stats_update_begin(&port->rx_stats_syncp);
 
 	received = gmac_rx(napi->dev, budget, &freeq_consumed, &reschedule);
-	if (!reschedule && received < budget)
+	if (!reschedule && received < budget) {
+		u64_stats_update_begin(&port->rx_stats_syncp);
 		++port->rx_napi_exits;
-
-	u64_stats_update_end(&port->rx_stats_syncp);
+		u64_stats_update_end(&port->rx_stats_syncp);
+	}
 
 	port->freeq_refill += freeq_consumed;
 	if (port->freeq_refill > freeq_threshold) {

-- 
2.56.0

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

[netdev-bot+sashiko]

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

Low priority / nit picks for your consideration:

- [Low] The commit message gets the old teardown order backwards for the
  common path.

Pre-existing issues:
- [High] The RX coalescing hrtimer can be re-armed after gmac_stop()
  cancels it, and nothing cancels it again.
- [High] Discarded because a later patch in the series fixes it.
- [High] Discarded because a later patch in the series fixes it.
- [High] Discarded because a later patch in the series fixes it.

--

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

This commit moves teardown of the shared software free queue out of the
per-port removal path and into the parent's remove. That teardown now runs
after both port devices are depopulated and the free queue interrupt is
masked. It also adds a NULL guard to geth_cleanup_freeq(), suppresses bind
attributes on the port driver, and unregisters each netdev before
disconnecting its PHY.

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

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

[Severity: Low]
Is the old teardown order described correctly here? The parent driver is
unregistered first in gemini_ethernet_module_exit():

    platform_driver_unregister(&gemini_ethernet_driver);
    platform_driver_unregister(&gemini_ethernet_port_driver);

The children were created with devm_of_platform_populate(), and devres
actions run only after .remove() returns. On module unload or parent unbind,
the old gemini_ethernet_remove() therefore called geth_cleanup_freeq()
first. Both child netdevs were still registered at that point and could
still be doing DMA. Each child's gemini_port_remove() then called it again,
so the queue was freed three times.

The old cleanup also looks like it oopses when no interface was ever opened.
gemini_ethernet_init() sets freeq_order = 1, so
pages = (1 << 1) >> (12 - 11) = 1. geth_cleanup_freeq() then dereferences
geth->freeq_ring[0] while freeq_ring is NULL. The message mentions this
only as "Guard cleanup for paths where no queue was allocated".

Could the message describe these two cases, for the benefit of stable
backporters?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 2fe7fd0202d26..bf7652b04f1ba 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 isn't a bug introduced by this patch, but does the put_page() loop
later in geth_cleanup_freeq() also drop references owned by RX skbs that
are still in flight?

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

Pages passed up the stack with skb_fill_page_desc() hold their own
references. This loop can free pages that socket-queued skbs still use, and
it can underflow the page refcount. A CAP_NET_ADMIN user can reach it by
taking the interface down and then resizing the ring or reopening.

A later patch in the series, "net: ethernet: cortina: Preserve in-flight
free queue pages", appears to fix this. It replaces the loop with
geth_freeq_release_pages(), which drops only gpage->fragments plus the
driver's base reference.

[Severity: High]
This is also a pre-existing issue, but can a failed geth_setup_freeq()
inside geth_resize_freeq() leave the next open running without a queue?

geth_resize_freeq() commits the new order before the setup that can fail.
It also re-enables SWFQ_EMPTY whatever ret is:

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

After a failure, freeq_pages still points at the array that
geth_cleanup_freeq() freed. On the next open, this shortcut returns 0:

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

gmac_open() then carries on, and geth_fill_freeq() indexes the freed array:

    gpage = &geth->freeq_pages[pn];

This appears to be fixed later in the series by "net: ethernet: cortina:
Drain free queue IRQ before resize" and "net: ethernet: cortina: Rebuild
free queue metadata for RX ring changes". Those patches do the following:

  - NULL out freeq_pages, freeq_ring and the bitmaps on cleanup and on
    setup errors
  - take the fast path only when freeq_ring is non-NULL
  - re-enable the IRQ source only on success
  - propagate a resize failure from open

[ ... ]

> @@ -2387,11 +2390,10 @@ static irqreturn_t gemini_port_irq(int irq, void *data)
>  static void gemini_port_remove(struct gemini_ethernet_port *port)
>  {
>  	if (port->netdev) {
> -		phy_disconnect(port->netdev->phydev);
>  		unregister_netdev(port->netdev);
> +		phy_disconnect(port->netdev->phydev);
>  	}
>  	clk_disable_unprepare(port->pclk);
> -	geth_cleanup_freeq(port->geth);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but unregister_netdev() here
ends up in gmac_stop(). Can the RX coalescing hrtimer still be pending
when gmac_stop() returns?

gmac_stop() cancels the timer first. It masks the port IRQs and frees the
IRQ only later:

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

In that window, a NAPI poll that is already running can finish and
re-enable the RX IRQ through gmac_napi_poll()->gmac_enable_rx_irq(). When
rx_coalesce_nsecs is non-zero, gmac_irq() then re-arms the timer:

    hrtimer_start(&port->rx_coalesce_timer, ktime,
                  HRTIMER_MODE_REL);

Nothing cancels the timer again after free_irq().

If the interface is reopened, gmac_open() calls hrtimer_setup() on a timer
that may still be queued. If the device is removed instead, freeing the
netdev also frees the port that embeds the timer. When the timer fires,
gmac_coalesce_delay_expired() calls napi_schedule(&port->napi) on freed
memory.

Triggering this needs a non-zero rx-usecs set with ethtool -C and RX
traffic arriving during ifdown. Would a final hrtimer_cancel() after
free_irq() close the window?

[Severity: High]
This is a pre-existing issue as well, but gemini_port_remove() still does
not clear geth->port0 or geth->port1. Can geth_resize_freeq() then
dereference a stale or NULL sibling pointer?

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

gemini_ethernet_port_probe() also publishes the pointer before steps that
can still fail, and no error path clears it:

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

This looks reachable under RTNL from gmac_open() or the set_ringparam path,
either during depopulate or after a failed probe. The suppress_bind_attrs
change in this patch narrows the exposure.

A later patch in the series, "net: ethernet: cortina: Keep PHY-less port
bound for shared IRQ", appears to fix this. It adds
gemini_port_publish()/gemini_port_clear(), and geth_resize_freeq() returns
-ENODEV unless both ports are present and geth->initialized is set.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

Re: [PATCH net-next v4 02/15] 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: 0 (1) · Medium: 3 · 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 patch moves gemini_port_publish()/gemini_ethernet_init()
  after devm_request_threaded_irq().
- [Medium] With the new IRQ-only peer (port->netdev == NULL),
  geth_resize_freeq() sizes the queue from one port only.
- [Medium] gemini_port_clear(), new in this patch, is called from every
  gemini_port_remove() and unconditionally sets geth->initialized =…

Pre-existing issues:
- [High] In this commit, geth_resize_freeq() masks SWFQ_EMPTY_INT_BIT
  under irq_lock and then calls geth_cleanup_freeq(), which does…
- [Low] In this commit, geth_resize_freeq()'s kernel-doc says it 'gets
  called at least once during probe()', and the comment above…

--

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

The software free queue interrupt is routed through port 1. This commit
keeps a Gemini child port bound as an IRQ-only port when it has no PHY
description. A port is now published to the parent only after its IRQ
and optional netdev are ready, and RTNL protects the port pointers and
the initialized flag.

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

[ ... ]

> @@ -1051,10 +1053,17 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	u32 en;
>  	int ret;
>  
> +	ASSERT_RTNL();
> +
> +	/* The software free queue interrupt is routed through port 1. */
> +	if (!geth->initialized || !geth->port0 || !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;

[Severity: Medium]
If the peer is IRQ-only, other_netdev is now NULL here, so the shared
queue size comes from this port alone. Can the new order then equal the
freeq_order = 1 that gemini_ethernet_init() sets before any queue is
allocated?

Suppose the interface is down and someone runs "ethtool -G ethX rx 1".
gmac_set_ringparam() does:

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

This gives rxq_order = 0, new_size = 2 and new_order = 1. That matches
the sentinel, so this shortcut returns success:

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

geth_setup_freeq() is never called and SWFQ_EMPTY is never enabled. A
later gmac_open() takes the same shortcut. The port then comes up with
GLOBAL_SW_FREEQ_BASE_SIZE_REG still zero.

Can RX work in that state?

Later in the series, "net: ethernet: cortina: Drain free queue IRQ
before resize" removes the sentinel. It also changes the shortcut to
require geth->freeq_ring && geth->freeq_order == new_order. Then "net:
ethernet: cortina: Preserve in-flight free queue pages" rejects
new_order <= fpp_order with -EINVAL.

>  
>  	if (other_netdev && netif_running(other_netdev))
>  		return -EBUSY;
> @@ -1065,7 +1074,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,

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Further
down in geth_resize_freeq(), the SWFQ interrupt is masked and the queue
is torn down straight after:

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

    /* Drop the old queue */
    if (geth->freeq_ring)
        geth_cleanup_freeq(geth);

Nothing here waits for a gemini_port_irq_thread() that is already
awake. That thread calls geth_fill_freeq(), which dereferences
geth->freeq_pages[pn] under freeq_lock. geth_cleanup_freeq() calls
kfree(geth->freeq_pages) without taking freeq_lock.

Can the thread touch freeq_pages after it has been freed?

After the refill, the thread also does:

    irqmask |= readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
    writel(irqmask, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);

This could re-enable SWFQ_EMPTY while geth_setup_freeq() is only half
done.

The next patch in the series, "net: ethernet: cortina: Drain free queue
IRQ before resize", fixes this. It calls disable_irq(geth->port1->irq)
before the teardown and enable_irq() afterwards.

[ ... ]

> +static void gemini_port_clear(struct gemini_ethernet_port *port)
> +{
> +	struct gemini_ethernet *geth = port->geth;
> +
> +	ASSERT_RTNL();
> +
> +	if (!port->id && geth->port0 == port)
> +		geth->port0 = NULL;
> +	else if (port->id && geth->port1 == port)
> +		geth->port1 = NULL;
> +	else
> +		return;
> +
> +	geth->initialized = false;

[Severity: Medium]
Before this patch, geth->initialized was only cleared in
gemini_ethernet_remove(). Now removing a single child clears it, even
though the sibling port and the shared queue are still in use.

Suppose that child is re-created while the parent and sibling stay
bound. gemini_port_publish()->gemini_ethernet_init() then sees
!initialized with both ports set, and runs the cold init again:

    writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_0_REG);
    ...
    writel(0, geth->base + GLOBAL_SW_FREEQ_BASE_SIZE_REG);
    ...
    geth->freeq_order = 1;
    geth->initialized = true;

Would this mask the surviving port's interrupts and deprogram the free
queue while that port may have DMA running?

In this commit, it also resets geth->freeq_order to 1 while freeq_ring
and freeq_pages are still allocated at the old order. The next
geth_resize_freeq()->geth_cleanup_freeq() call derives both the page
count and the dma_free_coherent() size from freeq_order:

    unsigned int len = 1 << geth->freeq_order;
    unsigned int pages = len >> fpp_order;
    ...
    dma_free_coherent(geth->dev,
                      sizeof(*geth->freeq_ring) << geth->freeq_order,

Does that leak the remaining pages and DMA mappings, and free the
coherent ring with the wrong size?

OF dynamic reconfiguration looks like one way to reach this.
of_platform_notify() destroys a populated child on
OF_RECONFIG_CHANGE_REMOVE (for example, an overlay that sets status =
"disabled") and re-creates it on revert. The child driver sets
suppress_bind_attrs, so sysfs unbind does not trigger it.

Later in the series the freeq_order overwrite goes away, but the
register reset stays. After the reset, the fast path in
geth_resize_freeq() can return success without reprogramming the
zeroed GLOBAL_SW_FREEQ_BASE_SIZE_REG.

> +}
> +

[ ... ]

> @@ -2446,6 +2474,20 @@ static void gemini_ethernet_init(struct gemini_ethernet *geth)
>  	 * set up and enable the queue IRQ. FIXME: fragile.
>  	 */
>  	geth->freeq_order = 1;
> +	geth->initialized = true;
> +}

[Severity: Low]
This isn't a bug introduced by this patch, but two comments say the
queue gets resized during probe(): this one, and the kernel-doc of
geth_resize_freeq():

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

geth_resize_freeq() is only called from gmac_open() and
gmac_set_ringparam(). No probe path calls it.

This patch adds ASSERT_RTNL() and a new -ENODEV return to
geth_resize_freeq(). Could the kernel-doc be updated at the same time?

Later in the series, "net: ethernet: cortina: Drain free queue IRQ
before resize" rewrites the kernel-doc to say it is called when a port
is opened or its RX ring size changes. That patch also removes the
freeq_order = 1 sentinel and this comment.

[ ... ]

> @@ -2541,15 +2583,6 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
>  	reset_control_reset(port->reset);
>  	usleep_range(100, 500);
>  
> -	/* Assign pointer in the main state container */
> -	if (!id)
> -		geth->port0 = port;
> -	else
> -		geth->port1 = port;
> -
> -	/* This will just be done once both ports are up and reset */
> -	gemini_ethernet_init(geth);
> -
>  	platform_set_drvdata(pdev, port);

[Severity: Medium]
Moving gemini_ethernet_init() from here changes when it runs relative
to the IRQ request. Before, the second port to probe (normally port 1,
whose line carries the SWFQ interrupt) ran it right after its reset. So
GLOBAL_INTERRUPT_ENABLE_0..4 were zeroed, STATUS_0..4 acked and
SELECT_0..4 programmed before that port's handler was installed.

Now the handler is installed first:

    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_publish()->gemini_ethernet_init() runs only afterwards. For
a port with a PHY, that is after gmac_setup_phy() and
register_netdevice().

During that window the interrupt registers still hold bootloader or
reset state. freeq_order, freeq_frag_order and freeq_pages are still
zero from devm_kzalloc().

If SWFQ_EMPTY_INT_BIT is enabled and pending at that point,
gemini_port_irq() only checks:

    if (val & en & SWFQ_EMPTY_INT_BIT) {

and wakes the thread. gemini_port_irq_thread()->geth_fill_freeq() then
computes:

    m_pn = (1 << (geth->freeq_order - fpp_order)) - 1;

With freeq_order == 0, that is a shift by a huge unsigned count. It
then dereferences:

    gpage = &geth->freeq_pages[pn];

with freeq_pages == NULL. Could this oops during boot?

If the thread runs after init instead, it ORs SWFQ_EMPTY_INT_BIT back
into GLOBAL_INTERRUPT_ENABLE_4_REG for a queue with no base programmed.

This needs the bootloader or reset state to leave SWFQ_EMPTY enabled,
pending and routed to int1, so not every board may hit it. The ordering
is still the same at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

Re: [PATCH net-next v4 03/15] net: ethernet: cortina: Drain free queue IRQ before resize

[netdev-bot+sashiko]

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

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

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

- [Medium] If a single port device is removed and re-created while the
  parent stays bound, the shared software free queue is left…

Pre-existing issues:
- [High] At this commit, geth_cleanup_freeq() only copies rptr into wptr,
  and geth_fill_freeq(geth, false) starts at `pn = rw.bits.rptr >>…
- [High] At this commit, geth_cleanup_freeq() runs `while
  (page_ref_count(gpage->page) > 0) put_page(gpage->page);` for every…
- [Medium] At this commit, geth_freeq_alloc_map_page() maps each free
  queue page with dma_map_single(..., PAGE_SIZE, DMA_FROM_DEVICE), but…

--

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

This patch disables the port 1 Linux IRQ for the whole of
geth_resize_freeq(). That drains any threaded handler that is already
running before the software free queue is torn down. SWFQ_EMPTY_INT_BIT
is now re-enabled only when setup succeeds. The patch also makes the
port 0 handler ignore the shared source, clears freed queue state, and
removes the forced freeq_order = 1 from gemini_ethernet_init().

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index ec2e476883376..c03541deed495 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -986,6 +986,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 is a pre-existing issue, not one introduced by this patch. Does
the err_freeq_alloc unwind just above unmap with the wrong size?

geth_freeq_alloc_map_page() maps the whole page:

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

But this unwind in geth_setup_freeq() and the loop in
geth_cleanup_freeq() both unmap with frag_len, which is usually 2048:

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

CONFIG_DMA_API_DEBUG would report this as a size mismatch. Swiotlb or
IOMMU backends could also leave part of the mapping in place or skip
cache maintenance.

Two later patches in this series, "net: ethernet: cortina: Correct free
queue DMA mappings" and "net: ethernet: cortina: Preserve in-flight free
queue pages", appear to switch all of these unmaps to PAGE_SIZE.

[ ... ]

> @@ -1026,20 +1028,37 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  	}
>  
>  	kfree(geth->freeq_pages);
> +	geth->freeq_pages = NULL;
> +	geth->num_freeq_pages = 0;

[Severity: High]
This isn't a bug introduced by this patch. Still, can the loop that
ends here in geth_cleanup_freeq() free pages that delivered skbs still
reference?

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

gmac_rx() attaches free queue pages to skbs and passes them up the
stack. A packet can still be unread in a socket queue when both ports
are brought down and geth_resize_freeq() rebuilds the queue, on open or
ethtool -G.

Dropping every reference here would then free a page that the skb
still points to. That is a use-after-free, and the contents of the
reallocated page could be exposed.

The later patch "net: ethernet: cortina: Preserve in-flight free queue
pages" in this series seems to fix this. It drops only the unclaimed
gpage->fragments references plus the driver's own reference.

[ ... ]

> @@ -1084,16 +1101,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: Medium]
Could this early return leave the hardware free queue unprogrammed?
The case is a port that is removed and re-added while the parent device
stays bound.

gemini_port_clear() sets geth->initialized = false, but it keeps
geth->freeq_ring, geth->freeq_pages and geth->freeq_order. When the port
is published again, gemini_port_publish()->gemini_ethernet_init() clears
the queue registers and interrupt enables:

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

Before this patch, gemini_ethernet_init() also set geth->freeq_order = 1,
which forced the next geth_resize_freeq() to rebuild the queue. That
line is now gone and freeq_ring is still non-NULL. So if the RX ring
orders have not changed, the next open returns 0 here. It never calls
geth_setup_freeq() or geth_set_freeq_irq(geth, true).

gmac_open() would succeed, but GLOBAL_SW_FREEQ_BASE_SIZE_REG stays 0 and
SWFQ_EMPTY_INT_BIT stays masked, so the interface would not receive.

One way to reach this is a CONFIG_OF_DYNAMIC change that removes and
re-creates a single ethernet-port child. of_platform_notify() handles
that change, and suppress_bind_attrs does not block it.

The rest of the series keeps the same pattern. At the end, the fast
path still checks only geth->freeq_ring, num_freeq_pages and the order.
gemini_ethernet_init() still clears the queue registers without
invalidating freeq_ring.

Should gemini_ethernet_init() or gemini_port_clear() mark the queue as
needing a rebuild?

[ ... ]

> @@ -1103,14 +1115,10 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	geth->freeq_order = new_order;
>  	ret = geth_setup_freeq(geth);

[Severity: High]
This is a pre-existing issue. What happens to the hardware rptr when the
queue is rebuilt here at a smaller order?

geth_cleanup_freeq() only copies rptr into wptr:

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

geth_setup_freeq()->geth_fill_freeq(geth, false) then starts from that
stale rptr without masking it with m_pn:

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

Suppose the RX rings were shrunk, by ethtool -G or by a later open with
smaller rings. Could pn then point past the end of the new
freeq_pages[] array, so that page_ref_count() and page_ref_add() run on
a garbage struct page pointer?

The hardware documentation does not say whether it wraps an rptr that
is out of range for a newly programmed ring.

The later patch "net: ethernet: cortina: Normalize free queue pointers
on rebuild" in this series fixes this. It resets GLOBAL_SWFQ_RWPTR_REG
to 0 in geth_setup_freeq(), and geth_fill_freeq() masks both pn and epn
with m_pn.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

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

[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 new `writel(0, geth->base + GLOBAL_SWFQ_RWPTR_REG)` in
  geth_setup_freeq() comes with the comment "Discard pointer state…
- [Low] The patch fixes a real out-of-bounds access but has no Fixes:
  tag.

--

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

This commit masks the free queue page index in geth_fill_freeq() to the
active queue order. Before the initial fill, it also writes 0 to
GLOBAL_SWFQ_RWPTR_REG in geth_setup_freeq(). Together these are meant to
stop out-of-bounds access to the replacement page table when the free
queue is rebuilt at a smaller order.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index c03541deed495..c3d1abdbebf61 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -887,6 +887,7 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
>  
>  	rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
>  	pn = (refill ? rw.bits.wptr : rw.bits.rptr) >> fpp_order;
> +	pn &= m_pn;

[Severity: Low]
Should this patch carry a Fixes: tag?

The unmasked pn computation in geth_fill_freeq() comes from commit
4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet").
If rptr is left over from a larger ring, the old code indexed
geth->freeq_pages[pn] past the end of the newly allocated table. It then
called page_ref_count()/page_ref_add() on whatever pointer was there.

A user with CAP_NET_ADMIN can reach this. Running ethtool -G on both ports
while they are down shrinks the free queue through:

gmac_set_ringparam()->geth_resize_freeq()->geth_cleanup_freeq()->
    geth_setup_freeq()->geth_fill_freeq(false)

The other free queue lifecycle fixes in this series all carry
Fixes: 4d5ae32f5e1e:

  "Keep shared free queue parent-owned"
  "Drain free queue IRQ before resize"
  "Correct free queue DMA mappings"
  "Preserve in-flight free queue pages"

The mask does not depend on any other patch in the series, so it should
backport on its own.

>  	epn = (rw.bits.rptr >> fpp_order) - 1;
>  	epn &= m_pn;
>  
> @@ -958,6 +959,9 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
>  		if (!geth_freeq_alloc_map_page(geth, pn))
>  			goto err_freeq_alloc;
>  
> +	/* Discard pointer state belonging to the previous queue geometry. */
> +	writel(0, geth->base + GLOBAL_SWFQ_RWPTR_REG);

[Severity: Medium]
Does this writel() actually reset the read pointer? gemini.h defines the
register like this:

union dma_rwptr {
	unsigned int bits32;
	struct bit_000c {
		unsigned int rptr	: 16;	/* Read Ptr, RO */
		unsigned int wptr	: 16;	/* Write Ptr, RW */
	} bits;
};

geth_cleanup_freeq() follows that layout and writes only the wptr half:

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

If rptr is read-only, the 32-bit write of 0 clears only wptr. The hardware
rptr then keeps its position from the old, larger ring.

In that case the new comment, and this part of the commit message, would
not match what the hardware does:

  Reset the pointer register before initially filling a rebuilt queue

geth_fill_freeq(false) masks the stale rptr only for the CPU-side page
index. It then publishes a masked wptr:

	writew(pn << fpp_order, geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);

For example, take a rebuild from order 10 with rptr at 768 to order 8
(256 entries). Software posts entries 0..253 and sets wptr to 254, but the
hardware rptr is still 768.

Suppose the free queue engine computes descriptor addresses from the raw
rptr (freeq_dma_base + rptr * 16). Could it then read descriptors past the
end of the new dma_alloc_coherent() ring and DMA received frames to
whatever buf_adr values it finds there?

Nothing in the driver shows the hardware reducing rptr modulo the new
queue size. The commit message also hedges with "if hardware retains its
read pointer while the queue is disabled".

Later in the series, geth_setup_freeq() still has the same writel(0) and
comment. geth_fill_freeq() then starts at wptr and stops at
((rptr >> fpp_order) - 1) & m_pn. If rptr is retained, a rebuild after
traffic would fill fewer entries than expected. Wouldn't
geth_setup_freeq() then fail with -ENOMEM?

> +
>  	filled = geth_fill_freeq(geth, false);
>  	if (!filled)
>  		goto err_freeq_alloc;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

Re: [PATCH net-next v4 05/15] net: ethernet: cortina: Correct free queue DMA mappings

[netdev-bot+sashiko]

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

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

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

- [Medium] In geth_cleanup_freeq()
  (drivers/net/ethernet/cortina/gemini.c:1038-1039), teardown still…
- [Medium] When geth_fill_freeq() replaces a slot whose page still has
  outstanding references (page_ref_count(page) > 1, lines 919-927),…

Pre-existing issues:
- [High] geth_fill_freeq() (lines 919-927) replaces freeq_pages[pn]
  whenever page_ref_count(page) > 1.
- [High] geth_cleanup_freeq() (lines 1041-1042) runs 'while
  (page_ref_count(gpage->page) > 0) put_page(gpage->page);'.
- [Medium] geth_freeq_lookup() checks the fragment start offset, but
  gmac_rx() never bounds the length it passes to skb_fill_page_desc()…

--

net: ethernet: cortina: Correct free queue DMA mappings

This patch keeps the free queue page DMA base separate from the fragment
iterator and removes the replacement unmap, which was unmapping the new
buffer. It also uses PAGE_SIZE in the setup unwind path and rejects
mappings above 32 bits. It replaces gmac_get_queue_page() with
geth_freeq_lookup(), which derives fragment offsets from the saved DMA
base under freeq_lock.

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

[ ... ]

> @@ -835,31 +855,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);

[ ... ]

> -
> -	/* 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);
> -	}

[Severity: Medium]
With the replacement unmap removed, what releases the old page's DMA
mapping here?

When geth_fill_freeq() sees page_ref_count(page) > 1 and calls
geth_freeq_alloc_map_page(), the old page is put and gpage->mapping is
overwritten with the new page_mapping. The old PAGE_SIZE DMA_FROM_DEVICE
mapping is never unmapped. Its handle is not recorded anywhere else, so
geth_cleanup_freeq() can't release it either.

The old code unmapped the wrong buffer, but every mapping was at least
partly released. After this change, each refill replacement under RX
load leaves one more streaming mapping live.

The commit message says "Releasing mappings with in-flight fragments is
handled together with their lifetime later in the series". The later
commits "net: ethernet: cortina: Preserve in-flight free queue pages" and
"net: ethernet: cortina: Recycle claimed free queue pages" do unmap
gpage->mapping with PAGE_SIZE.

This patch has a Fixes: tag. Would a standalone stable backport pick up
this leaking intermediate state?

[ ... ]

> @@ -1025,7 +1037,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];

[Severity: Medium]
Should this unmap also use gpage->mapping and PAGE_SIZE?

The page is mapped in geth_freeq_alloc_map_page() with:

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

This patch changes the geth_setup_freeq() error unwind to:

    dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
                     DMA_FROM_DEVICE);

However, geth_cleanup_freeq() still unmaps only frag_len bytes, at the
address read back from the descriptor. This path runs from
geth_resize_freeq() on open and on ethtool set_ringparam, and from the
remove path. CONFIG_DMA_API_DEBUG would flag the size mismatch.

The later commit "net: ethernet: cortina: Preserve in-flight free queue
pages" replaces this loop with geth_freeq_release_pages(), which unmaps
gpage->mapping with PAGE_SIZE. The end state of the series looks
correct.

>  		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 free pages
that skbs still reference?

gmac_rx() passes fragments of these pages to the stack through
skb_fill_page_desc() and napi_gro_frags(). Suppose one of those skbs is
still queued (on an unread socket, for example) when this runs:

    geth_resize_freeq()
      geth_cleanup_freeq()
        while (page_ref_count(gpage->page) > 0)
            put_page(gpage->page);

The page goes back to the allocator while the skb still points to it.
The later kfree_skb() would then put a page that is already free.

This can be reached by bringing the ports down and up with a different
ring order, by ethtool set_ringparam, or by removing the device.

The later commit "net: ethernet: cortina: Preserve in-flight free queue
pages" replaces this loop with geth_freeq_release_pages(). That function
drops only the gpage->fragments references plus the base reference.

[ ... ]

> @@ -1516,8 +1526,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) {

[Severity: Medium]
This isn't a bug introduced by this patch, but is frag_len checked
anywhere before it reaches skb_fill_page_desc()?

geth_freeq_lookup() now checks the fragment start offset. The length,
though, still comes straight from the descriptor in gmac_rx():

    frag_len = word0.bits.buffer_size;
    ...
        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);

The NET_IP_ALIGN subtraction has no lower bound, and frame_len - skb->len
can wrap. If the MAC reports inconsistent lengths, could the page frag
cover memory past the 2 KiB fragment or past the end of the page?

The later commit "net: ethernet: cortina: Validate RX fragment lengths"
appears to add these checks.

[ ... ]

> @@ -1536,14 +1544,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;
>  		}

[Severity: High]
This is a pre-existing issue, but what happens here if a refill has
already replaced the fragment's page?

geth_fill_freeq() decides to replace freeq_pages[pn] from the refcount
alone:

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

That count also includes fragments the hardware has already written into
RX descriptors that gmac_rx() hasn't processed yet. Once gpage->page and
gpage->mapping are overwritten, geth_freeq_lookup() finds no match for
the old buf_adr and returns NULL. The frame is then dropped with "could
not find mapping", and err_drop skips the put:

    if (page)
        put_page(page);

Doesn't that leak the reference page_ref_add() took for this fragment,
and so the old page itself? gmac_cleanup_rxq() hits the same lookup
failure and also skips put_page().

The new locking in geth_freeq_lookup() only covers the window between
lookup and use. The later commit "net: ethernet: cortina: Preserve
in-flight free queue pages" tracks each fragment address in an XArray
until geth_freeq_claim(). That looks like it resolves this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

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

[netdev-bot+sashiko]

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

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

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

- [Medium] At this patch, geth_setup_freeq() sizes the slot pool as (len
  >> fpp_order) + 2^port0->rxq_order + 2^port1->rxq_order.
- [Medium] This patch changes where the initial free-queue fill starts.

Pre-existing issues:
- [Medium] gmac_stop() (line 2067 at this commit) sets `port->rx_skb =
  NULL` after napi_disable() without calling napi_free_frags().

--

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

This commit stops indexing gemini free queue page metadata by free queue
ring position. Metadata now lives in slots tracked by a bitmap, each
fragment DMA address stays in the XArray until it is claimed, and each
posted fragment holds its own page reference. The refill path maps pages
outside the lock and publishes one page at a time. Setup now requires a
full initial fill.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index dc82c896fa007..284823bdfd50c 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -935,48 +977,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;
> -	pn &= m_pn;
> -	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) & m_pn;
> +		epn = (rw.bits.rptr >> fpp_order) - 1;
> +		epn &= m_pn;
> +		spin_unlock_irqrestore(&geth->freeq_lock, flags);
> +		if (pn == epn)
> +			break;

[Severity: Medium]
Before this patch, the initial fill from geth_setup_freeq() started at the
hardware read pointer:

    pn = (refill ? rw.bits.wptr : rw.bits.rptr) >> fpp_order;

Now geth_fill_freeq() always starts at wptr and stops one page before
rptr. What happens if the hardware read pointer is not reset by the
writel(0, geth->base + GLOBAL_SWFQ_RWPTR_REG) in geth_setup_freeq()?

gemini.h documents the field as read-only:

    unsigned int rptr	: 16;	/* Read Ptr, RO */

The earlier commit "net: ethernet: cortina: Normalize free queue pointers
on rebuild" also allows for hardware that keeps its read pointer while the
queue is disabled. geth_cleanup_freeq() only copies rptr into wptr and
clears GLOBAL_SW_FREEQ_BASE_SIZE_REG.

Suppose rptr keeps a nonzero value R across a rebuild. This loop would
then post only ((R >> fpp_order) - 1) pages starting at entry 0, and
geth_setup_freeq() would fail here:

    expected = len - (1 << fpp_order);
    filled = geth_fill_freeq(geth);
    if (filled != expected)
        goto err_freeq_bitmap;

Would every ethtool -G resize, or every reopen that rebuilds the queue
after traffic, then fail with -ENOMEM? The old queue has already been torn
down by that point. The old code handled a retained rptr because it filled
relative to rptr.

This seems to still be present at the end of the series.
geth_fill_freeq() still derives pn from wptr, and gmac_open() propagates
the error, so the interface would not come up.

[ ... ]

> @@ -984,12 +1073,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,

[ ... ]

> @@ -1162,6 +1231,9 @@ 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 (new_order <= PAGE_SHIFT - geth->freeq_frag_order)
> +		return -EINVAL;
> +
>  	if (geth->freeq_ring && geth->freeq_order == new_order)
>  		return 0;

[Severity: Medium]
geth_setup_freeq() now sizes the slot pool using both rxq_order values.
This shortcut, however, only compares freeq_order. Can the pool end up
too small if an rxq_order grows while freeq_order stays the same?

Take rxq orders 9 and 8: new_size is 1536, new_order is 11 and
page_slots is 1024 + 512 + 256 = 1792. If ethtool -G moves port1 to
order 9, new_size becomes 2048, which is still order 11. This path then
returns 0, and the pool stays at 1792 slots while worst-case demand is
about 2048.

There is a related path in gmac_set_ringparam(). It updates
port->rxq_order before calling geth_resize_freeq(), and keeps the new
order even when the resize returns -EBUSY because the other port is
running:

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

When the slots run out, geth_fill_freeq() maps a page and gets -ENOSPC
from geth_freeq_add_page(). It then unmaps and frees the page and stops
refilling. Could this leave the shared free queue underfilled and starve
RX under load?

This looks to be resolved later in the series by "net: ethernet: cortina:
Rebuild free queue metadata for RX ring changes". That commit makes
geth_resize_freeq() compare num_freeq_pages against
geth_freeq_page_slots(), makes gmac_set_ringparam() restore old_rxq_order
on failure, and makes gmac_open() propagate resize errors.

[ ... ]

> @@ -1590,7 +1662,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");

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. Still,
what happens to fragments already claimed here for a partly assembled
frame when the port is stopped? gmac_stop() drops the pointer without
freeing the skb:

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

napi->skb still holds references to the claimed fragment pages. On the
next open, napi_get_frags() returns that same skb. gmac_rx() then
overwrites frag 0 through skb_fill_page_desc(), and the old len, data_len
and truesize carry over into the new frame.

Does this leak the fragment pages each time the port is stopped mid-frame?

This is fixed later in the series by "net: ethernet: cortina: Release
partial RX frames on stop", which makes gmac_stop() call
napi_free_frags(&port->napi) when port->rx_skb is set.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

Re: [PATCH net-next v4 08/15] net: ethernet: cortina: Rebuild free queue metadata for RX ring changes

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 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 patch fixes a regression from an earlier patch in the same
  series, but it has no Fixes: tag and does not say where that…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  gmac_set_ringparam() rounds rx_pending/tx_pending up to a power of two…

--

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

This commit adds geth_freeq_page_slots(), which computes how much free
queue metadata is needed. The geth_resize_freeq() fast path now uses it,
so an RX ring change that keeps the same free queue order no longer keeps
an undersized metadata pool. gmac_open() now fails when geth_resize_freeq()
returns any error, and gmac_set_ringparam() restores the old rxq_order if
the resize fails.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 284823bdfd50c..cd5ded7384a51 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1234,8 +1244,16 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	if (new_order <= PAGE_SHIFT - geth->freeq_frag_order)
>  		return -EINVAL;
>  
> -	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->num_freeq_pages >= page_slots &&
> +		    (geth->freeq_order == new_order ||
> +		     (other_running && geth->freeq_order > new_order)))
> +			return 0;
> +	}
> +
> +	if (other_running)
> +		return -EBUSY;

[Severity: Medium]
The code change looks right to me. Should this patch carry Fixes: tags,
though, or should the fast path part go into the earlier patch?

The undersized pool comes from the previous patch in this series, "net:
ethernet: cortina: Preserve in-flight free queue pages". That patch sizes
freeq_pages and the bitmaps as the free queue pages plus
(1 << port0->rxq_order) plus (1 << port1->rxq_order). But it keeps the
fast path that only checks the order:

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

With only that patch applied, raising an RX ring without changing the
computed freeq order keeps the old, smaller pool. For example, take a port
whose rxq_order goes from 8 to 9 while the other port stays at 10. new_size
goes from 2560 to 3072, but new_order is 12 both times. The same thing
happens once new_order hits the cap of 15.

geth_freeq_alloc_slot() then returns -ENOSPC and geth_fill_freeq() stops
refilling early.

"Preserve in-flight free queue pages" carries a Fixes: 4d5ae32f5e1e tag,
which makes it a stable candidate. Could a stable kernel take it without
this untagged follow-up and end up with the undersized pool after
ethtool -G rx N? It also leaves a bisect window between the two patches.

The other two changes here fix behavior that goes back to the original
driver (4d5ae32f5e1e): gmac_set_ringparam() leaving the new rxq_order
installed after a failed resize, and gmac_open() accepting -EBUSY.

Could the fast path change be folded into "Preserve in-flight free queue
pages"? If not, could this patch get Fixes: tags so backporters keep the
two together?

[ ... ]

> @@ -2376,14 +2391,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);

[Severity: Low]
This isn't a bug introduced by this patch, but gmac_set_ringparam() rounds
rx_pending up to a power of two and never writes the applied size back
into rp. include/linux/ethtool.h says:

 * @set_ringparam: Set ring sizes. The &struct ethtool_ringparam argument is
 *	also an output; drivers which normalize requested sizes must update it
 *	with the applied sizes.

After the callback returns, ethnl_set_rings() in net/ethtool/rings.c picks
up the driver's adjustments:

	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);

So a request for rx 300 stores 300 in cfg_pending, while the driver
actually uses 512 (rxq_order 9).

Should rp->rx_pending be set to 1 << port->rxq_order once the resize
succeeds? tx_pending is rounded the same way further down, so it could be
updated too.

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

Re: [PATCH net-next v4 10/15] net: ethernet: cortina: Recycle claimed free queue pages

[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] geth_fill_freeq() (drivers/net/ethernet/cortina/gemini.c) sets
  its local `recycle` flag to false the first time…

--

net: ethernet: cortina: Recycle claimed free queue pages

With this patch, a free queue page keeps its base reference and streaming
mapping after its last fragment is claimed. The slot is marked in a new
recycle bitmap. During refill, geth_fill_freeq() reuses the page if its
refcount has dropped back to one, and otherwise unmaps and releases it
before allocating a replacement.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 3bb96c51e0b71..943c31c026743 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -997,9 +1078,26 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
>  		pn = (rw.bits.wptr >> fpp_order) & m_pn;
>  		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;

[Severity: Low]

Once geth_freeq_recycle_page() returns -ENOSPC because freeq_recycle_pending
was 0, recycle stays false for the rest of this geth_fill_freeq() call.
Can new recyclable slots show up after that point?

freeq_lock is dropped at the end of each iteration and around
geth_freeq_map_page(). In that window gmac_rx()->geth_freeq_claim() can
retire pages. One example is the NAPI softirq running at hardirq exit while
gemini_port_irq_thread() is inside geth_fill_freeq():

geth_freeq_claim()
    if (!--gpage->fragments) {
        slot = gpage - geth->freeq_pages;
        __set_bit(slot, geth->freeq_recycle_bitmap);
        geth->freeq_recycle_pending++;
    }

These retired slots keep their freeq_page_bitmap bit set. Even so, the
remaining iterations still allocate and map a fresh page. Each one takes a
new metadata slot through geth_freeq_add_page()->geth_freeq_alloc_slot().

Can slot usage then exceed what geth_freeq_page_slots() sizes for (ring
pages plus rxq entries)? Retired pages now count on top of posted pages and
pages held by unclaimed RX descriptors.

If the table fills up, geth_freeq_alloc_slot() returns -ENOSPC.
geth_fill_freeq() then throws away the page it just mapped and stops:

    if (ret) {
        dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
                         DMA_FROM_DEVICE);
        put_page(page);
        break;
    }

This leaves the free queue under-filled while recyclable slots are still
pending.

The next geth_fill_freeq() call starts with recycle = true and drains them,
so this recovers on its own. It does still cause extra fresh page
allocations and mappings during the call, which the commit message is
trying to avoid.

Could the latch be dropped, or freeq_recycle_pending re-checked under
freeq_lock, before falling back to a fresh allocation?

> +		}
>  		spin_unlock_irqrestore(&geth->freeq_lock, flags);
>  		if (pn == epn)
>  			break;
> +		if (!ret)
> +			continue;
> +		if (ret != -ENOSPC && ret != -EAGAIN)
> +			break;
>  
>  		ret = geth_freeq_map_page(geth, &page, &page_mapping);
>  		if (ret)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

Re: [PATCH net-next v4 11/15] 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: 1 · Medium: 0 · 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).

- [High] net: gmac_rx() drops entire valid packet on zero-length EOF
  fragment
- [Low] The commit describes itself as a bug fix and has no Fixes: tag.
- [Low] The patch makes RX word0.bits.buffer_size a strict byte-length
  bound on every EOF descriptor.

--

net: ethernet: cortina: Validate RX fragment lengths

This patch adds bounds checks on RX descriptor lengths in gmac_rx(). It
rejects short initial fragments, frame length underflow and fragments that
extend past the DMA fragment or the page. Zero-length fragments are now
dropped instead of being appended to the skb.

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

[Severity: Low]
Should this patch carry a Fixes: tag?

Three of the issues addressed here appear to date back to the original
driver:

  - the unchecked frag_len -= NET_IP_ALIGN
  - the unsigned underflow in frag_len = frame_len - skb->len
  - zero-length fragments that are logged but still appended

That would make the tag:

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

Other patches in the same series, such as "Keep shared free queue
parent-owned" and "Correct free queue DMA mappings", already carry this
tag.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 943c31c026743..d8bafcdf4d5f1 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1811,6 +1812,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;
> @@ -1819,15 +1823,28 @@ 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 < ETH_ZLEN ||
> +			    frame_len < skb->len ||
> +			    frame_len - skb->len > frag_len)
> +				goto err_drop;
>  			frag_len = frame_len - skb->len;
> +		}

[Severity: High]
Does this drop more than the commit message says it does?

The message says zero-length fragments are dropped instead of being
added to the skb. The goto err_drop does not skip just that fragment.
It calls napi_free_frags() on the whole skb assembled so far, puts the
page and increments rx_dropped.

Consider a multi-descriptor frame where the earlier descriptors
already carry all of the data. One example is a frame that exactly
fills the first free queue fragment after the NET_IP_ALIGN offset,
which is 2046 bytes with the default 2 KiB fragment.

If the hardware then closes the chain with an EOF descriptor that
carries no new data, frame_len equals skb->len. The new EOF checks
all pass and frag_len becomes frame_len - skb->len == 0. gmac_rx()
then lands in the !frag_len branch.

Before this patch that frame was handed to napi_gro_frags() with an
empty trailing fragment. After it, a complete and valid frame is
discarded and counted as rx_dropped. Nothing later in the series
seems to change this.

Can the Gemini DMA engine emit such a trailing descriptor at the end
of a chain?

If it can, the EOF case with nothing left to append could be handled
separately: put_page() the unused fragment and still pass the skb to
napi_gro_frags(). err_drop would then be kept for zero-length
descriptors in the middle of a chain. Note that the
frag_nr == MAX_SKB_FRAGS check runs first, so an empty EOF arriving
after a full frags array would also discard the frame unless that
case is handled before it.

[Severity: Low]
Before this patch, gmac_rx() never used word0.bits.buffer_size on an EOF
descriptor, because frag_len was always overwritten with
frame_len - skb->len. Now buffer_size on every EOF descriptor is a strict
byte bound on whether the frame gets delivered.

For a single-descriptor (SOF|EOF) frame, the NET_IP_ALIGN subtraction plus
this check now require:

    buffer_size >= frame_len + NET_IP_ALIGN

What guarantees that the hardware reports buffer_size this way? The driver
never writes buffer_size into free queue entries. geth_freeq_post_page()
and geth_setup_freeq() only set word2.buf_adr, so the hardware supplies the
value.

The only in-tree definition, in gemini.h, describes the field as a
descriptor count:

union gmac_rxdesc_0 {
	...
		/* bit 15:0 number of descriptors used for the current frame */
		unsigned int buffer_size:16;
		/* bit 21:16 number of descriptors used for the current frame */
		unsigned int desc_count:6;

That comment looks copied from desc_count. It doesn't say whether the value
includes the bytes inserted via rd_insert_bytes = NET_IP_ALIGN in
gmac_start_dma(). It also doesn't say how the field is filled on an EOF
descriptor that is not the SOF.

If buffer_size excluded the inserted bytes, wouldn't every
single-descriptor frame be dropped?

There is a second case with an MTU above 1530, where frames span multiple
descriptors. If buffer_size is filled differently on the last descriptor,
could good jumbo frames be silently counted as rx_dropped?

The DIR-685 testing at standard MTU suggests the SOF|EOF case works. The
commit message also says:

  "validated against the Gemini receive descriptor definitions"

The in-tree definitions don't seem to support that. Could the buffer_size
comment in gemini.h be corrected to describe the byte-length semantics this
patch now relies on, including whether the NET_IP_ALIGN bytes are counted?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

Re: [PATCH net-next v4 12/15] net: ethernet: cortina: Account RX buffer size in truesize

[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] Commit "net: ethernet: cortina: Account RX buffer size in
  truesize" fixes a real bug but has no Fixes: trailer.

--

net: ethernet: cortina: Account RX buffer size in truesize

This changes gmac_rx() so that each page fragment attached to an skb adds
the full posted free queue fragment size (freeq_frag_len) to skb->truesize.
Before this change it added only the received payload length (frag_len).

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index d8bafcdf4d5f1..43fd8fd773e3c 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1849,7 +1849,7 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);
>  		skb->len += frag_len;
>  		skb->data_len += frag_len;
> -		skb->truesize += frag_len;
> +		skb->truesize += freeq_frag_len;

[Severity: Low]
The code change looks fine, but should this commit have a Fixes: tag?

The commit message describes a functional problem:

  "short packets understate their memory use to GRO and socket
  accounting."

The old skb->truesize += frag_len line in gmac_rx() has been there since
the original driver commit:

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

Other patches in this series already carry this tag, for example "Keep
shared free queue parent-owned" and "Correct free queue DMA mappings".
Without it, stable tooling won't pick up this fix.

The freeq_frag_len local in gmac_rx() is also missing from the tree
before this series. An earlier patch in the series adds it, so a backport
of this fix needs that patch too. Could that dependency be noted for
backporters?

>  		frag_nr++;
>  
>  		if (word3.bits32 & EOF_BIT) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

Re: [PATCH net-next v4 13/15] net: ethernet: cortina: Release partial RX frames on stop

[netdev-bot+sashiko]

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

Low priority / nit picks for your consideration:

- [Low] The commit fixes a real leak in gmac_stop() but has no Fixes:
  trailer (and no Cc: stable).

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the
  rx_coalesce_timer lifecycle in gmac_open()/gmac_stop() is out of order…
- [Low] This problem wasn't introduced by this patch, but
  port->rx_stats_syncp (and tx_stats_syncp/ir_stats_syncp) is never…

--

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

This patch frees any partially assembled RX frag skb in gmac_stop() after
napi_disable() and before the saved RX state is cleared. The discarded
frame is counted in rx_dropped, using the irqsave u64_stats helpers.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 43fd8fd773e3c..32291d4c33904 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2209,12 +2209,19 @@ static int gmac_open(struct net_device *netdev)
>  static int gmac_stop(struct net_device *netdev)
>  {
>  	struct gemini_ethernet_port *port = netdev_priv(netdev);
> +	unsigned long flags;
>  
>  	hrtimer_cancel(&port->rx_coalesce_timer);

[Severity: High]
This isn't a bug introduced by this patch, but is rx_coalesce_timer
cancelled too early here?

hrtimer_cancel() runs before gmac_enable_irq(netdev, 0) masks the RX
interrupt, and before free_irq(). While NAPI is idle the RX queue interrupt
is enabled, because gmac_napi_poll() calls gmac_enable_rx_irq(napi->dev, 1)
when it completes. If rx_coalesce_nsecs is non-zero, an RX interrupt in
that window re-arms the timer from gmac_irq():

drivers/net/ethernet/cortina/gemini.c:gmac_irq() {
    ...
	if (val & (DEFAULT_Q0_INT_BIT << netdev->dev_id)) {
		gmac_enable_rx_irq(netdev, 0);

		if (!port->rx_coalesce_nsecs) {
			napi_schedule(&port->napi);
		} else {
			...
			hrtimer_start(&port->rx_coalesce_timer, ktime,
				      HRTIMER_MODE_REL);
		}
	}
    ...
}

Can the timer then still be pending after gmac_stop() returns? On device
removal, the netdev private area that embeds the hrtimer could be freed
while the timer is enqueued. gmac_coalesce_delay_expired() would then run
on freed memory.

On a quick reopen, gmac_open() calls hrtimer_setup(). Its
__hrtimer_setup() does a memset() on a timer that may still be linked into
the hrtimer base rbtree.

gmac_open() has a similar ordering problem, because interrupts are enabled
before the timer is set up:

drivers/net/ethernet/cortina/gemini.c:gmac_open() {
    ...
	gmac_start_dma(port);
	gmac_enable_irq(netdev, 1);
	gmac_enable_tx_rx(netdev);
	netif_tx_start_all_queues(netdev);

	hrtimer_setup(&port->rx_coalesce_timer, &gmac_coalesce_delay_expired, CLOCK_MONOTONIC,
		      HRTIMER_MODE_REL);
    ...
}

gmac_set_coalesce() accepts a non-zero rx-usecs while the interface is
down. On the first open after that, an RX interrupt in this window would
call hrtimer_start() on a zeroed hrtimer. lock_hrtimer_base() would then
dereference a NULL timer->base in hardirq context.

On later opens, an interrupt in the same window can arm the old timer just
before hrtimer_setup() clears it.

Would it make sense to set up the timer once at probe time, and to cancel
it in gmac_stop() only after the RX interrupt is masked?

>  	netif_tx_stop_all_queues(netdev);
>  	gmac_disable_tx_rx(netdev);
>  	gmac_stop_dma(port);
>  	napi_disable(&port->napi);
> +	if (port->rx_skb) {
> +		napi_free_frags(&port->napi);
> +		flags = u64_stats_update_begin_irqsave(&port->rx_stats_syncp);

[Severity: Low]
This is a pre-existing issue, but is rx_stats_syncp ever initialized with
u64_stats_init()?

The port is allocated with devm_alloc_etherdev_mqs() in
gemini_ethernet_port_probe(). The driver has no u64_stats_init() call for
rx_stats_syncp, tx_stats_syncp or ir_stats_syncp.

On 32-bit builds with CONFIG_DEBUG_LOCK_ALLOC, write_seqcount_begin()
calls lock_acquire() on a dep_map that has no key. Lockdep then prints
"trying to register non-static key" and turns itself off.

The existing writer in gmac_napi_poll() and the readers already run into
this. The new call in gmac_stop() is one more user.

> +		port->stats.rx_dropped++;
> +		u64_stats_update_end_irqrestore(&port->rx_stats_syncp, flags);
> +	}
>  	port->rx_skb = NULL;

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

The leak seems to come from commit 06937db21ee3 ("net: ethernet: cortina:
Make RX SKB per-port"). That commit added the "port->rx_skb = NULL;" line
in gmac_stop() without freeing napi->skb.

Before 06937db21ee3, the static skb in gmac_rx() kept its value across
stop and open. The start-of-frame handling in gmac_rx() would then free the
stale partial frame:

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

napi_disable() and napi_enable() do not release napi->skb, and
napi_get_frags() returns the existing napi->skb. After a reopen, the stale
skb and its old frags are reused, and the page references they hold are
leaked.

06937db21ee3 carries its own Fixes: tag and is headed for stable. Would it
help to add the following, so this fix reaches the same trees?

Fixes: 06937db21ee3 ("net: ethernet: cortina: Make RX SKB per-port")

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org

Re: [PATCH net-next v4 15/15] net: ethernet: cortina: Scope RX statistics updates

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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] This commit fixes a real runtime bug but has no Fixes: trailer
  and no statement of how the bug was found, how it was confirmed, or…
- [Low] The commit message starts with "On 32-bit systems
  u64_stats_update_begin() disables preemption." That is wrong for…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but none of
  port->rx_stats_syncp, tx_stats_syncp or ir_stats_syncp is ever…

--

net: ethernet: cortina: Scope RX statistics updates

This commit narrows the rx_stats_syncp writer sections in the Gemini RX
path so they cover only the counter updates. Before this, one writer
section in gmac_napi_poll() spanned all of gmac_rx(). The stated reason is
that on 32-bit PREEMPT_RT the writer section disables preemption while
gmac_rx() takes spinlock_t locks and calls into GRO.

> On 32-bit systems u64_stats_update_begin() disables preemption. The RX
> poll currently holds that writer section across gmac_rx(), which takes
> spinlock_t locks and calls into GRO and fragment handling. On PREEMPT_RT,
> spinlock_t can sleep and must not be acquired with preemption disabled.

[Severity: Low]
Is the first sentence accurate for non-RT kernels? On 32-bit,
__u64_stats_update_begin() calls preempt_disable_nested(). That helper
only disables preemption when PREEMPT_RT is enabled:

include/linux/preempt.h:preempt_disable_nested() {
	if (IS_ENABLED(CONFIG_PREEMPT_RT))
		preempt_disable();
	else
		lockdep_assert_preemption_disabled();
}

So the affected configuration looks like 32-bit with PREEMPT_RT only.
Could the opening sentence say that?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index a61bace61ea4b..9f7946876d585 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1712,12 +1712,12 @@ static struct sk_buff *gmac_skb_if_good_frame(struct gemini_ethernet_port *port,
>  	unsigned int rx_status = word0.bits.status;
>  	struct sk_buff *skb = NULL;
>  
> -	port->rx_stats[rx_status]++;
> -	port->rx_csum_stats[rx_csum]++;
> -
>  	if (word0.bits.derr || word0.bits.perr ||
>  	    rx_status || frame_len < ETH_ZLEN ||
>  	    rx_csum >= RX_CHKSUM_IP_ERR_UNKNOWN) {
> +		u64_stats_update_begin(&port->rx_stats_syncp);

[Severity: Medium]
This isn't a bug introduced by this patch, but is rx_stats_syncp ever
initialized?

Nothing in drivers/net/ethernet/cortina/ calls u64_stats_init() for
rx_stats_syncp, tx_stats_syncp or ir_stats_syncp. The port struct is
zeroed memory from devm_alloc_etherdev_mqs(). gemini_ethernet_port_probe()
initializes the config lock:

	spin_lock_init(&port->config_lock);

but none of the three syncp fields.

On 32-bit kernels with CONFIG_DEBUG_LOCK_ALLOC, the seqcount's lockdep_map
has no key. The first writer would therefore go through:

gmac_skb_if_good_frame()
  u64_stats_update_begin()
    __u64_stats_update_begin()
      write_seqcount_begin()
        seqcount_acquire()
          assign_lock_key()

This prints "INFO: trying to register non-static key." and turns lockdep
off for the rest of the boot.

Would it make sense to add u64_stats_init() calls for the three syncp
fields in gemini_ethernet_port_probe(), perhaps as a separate patch?

> +		port->rx_stats[rx_status]++;
> +		port->rx_csum_stats[rx_csum]++;
>  		port->stats.rx_errors++;

[ ... ]

> @@ -1915,13 +1924,13 @@ static int gmac_napi_poll(struct napi_struct *napi, int budget)
>  	bool reschedule;
>  
>  	freeq_threshold = 1 << (geth->freeq_order - 1);
> -	u64_stats_update_begin(&port->rx_stats_syncp);
>  
>  	received = gmac_rx(napi->dev, budget, &freeq_consumed, &reschedule);

[Severity: Medium]
The u64_stats_update_begin() removed here, placed before gmac_rx(), dates
back to the original driver commit. On 32-bit PREEMPT_RT the path is:

gmac_napi_poll()
  u64_stats_update_begin()
    __u64_stats_update_begin()
      preempt_disable_nested()    <- preempt_disable() on PREEMPT_RT
  gmac_rx()
    spin_lock_irqsave(&geth->irq_lock, flags)

gmac_rx() also takes the free queue lock in geth_freeq_claim() and calls
napi_gro_frags() while preemption is disabled.

This patch fixes that "sleeping function called from invalid context"
problem. Should it carry this tag?

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

Documentation/process/maintainer-netdev.rst also asks bug fixes to say how
the issue was found and confirmed, and how the fix was tested. For
hardware drivers it asks for the hardware the fix was tested on. The
commit message has none of these.

Other patches in this series carry Fixes: 4d5ae32f5e1e, for example "Keep
shared free queue parent-owned" and "Drain free queue IRQ before resize".
Others include a testing statement, for example "Account RX buffer size in
truesize", which mentions testing on a D-Link DIR-685. Could this patch get
the same tag and testing statement?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org