* [PATCH net-next v6 01/15] net: ethernet: cortina: Keep shared free queue parent-owned
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-10 1:16 ` Jakub Kicinski
2026-11-03 17:51 ` [PATCH net-next v6 02/15] net: ethernet: cortina: Keep PHY-less port bound for shared IRQ Linus Walleij
` (13 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, Linus Walleij
The software free queue is shared by both Ethernet ports, but teardown is
not ordered around that ownership. On parent unbind or module unload, the
parent remove callback runs before devres depopulates the child devices. It
therefore destroys the queue while both child netdevs are still registered
and can be doing DMA. Devres then removes each child, and both child remove
callbacks destroy the same queue again, resulting in up to three cleanup
attempts.
Removal before the first interface has been opened is broken as well. The
shared block initialization assigns a nonzero queue order, but allocation
is deferred until open. Cleanup consequently derives a nonzero page count
and dereferences the NULL queue ring.
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")
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next v6 01/15] net: ethernet: cortina: Keep shared free queue parent-owned
2026-11-03 17:51 ` [PATCH net-next v6 01/15] net: ethernet: cortina: Keep shared free queue parent-owned Linus Walleij
@ 2026-11-10 1:16 ` Jakub Kicinski
2026-11-10 10:08 ` Linus Walleij
0 siblings, 1 reply; 24+ messages in thread
From: Jakub Kicinski @ 2026-11-10 1:16 UTC (permalink / raw)
To: linusw
Cc: Jakub Kicinski, ulli.kroll, andrew+netdev, davem, pabeni,
mirq-linux, mhun512, edumazet, netdev
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -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;
> }
>
[Severity: High]
devm_of_platform_depopulate() unbinds both port devices synchronously,
and gemini_port_remove() ends with clk_disable_unprepare(port->pclk).
By the time we get here both GMAC gate clocks are therefore off. That
includes this writel() to GLOBAL_INTERRUPT_ENABLE_4_REG and the
readw()/writew()/writel() of GLOBAL_SWFQ_RWPTR_REG and
GLOBAL_SW_FREEQ_BASE_SIZE_REG in geth_cleanup_freeq(). All of them
access geth->base while the block is unclocked.
The parent cortina,gemini-ethernet device does not hold a clock of its
own, and the global registers need at least one port clocked. On ARM
that means one of two things. The writes may be silently lost, which
leaves SWFQ_EMPTY_INT_BIT enabled and the old free queue base
programmed. Or the access takes an external abort on unbind or module
unload.
The child path has a similar window. The clock is gated in .remove,
but devres frees the devm threaded IRQ (gemini_port_irq() and
gemini_port_irq_thread()) only after .remove returns.
SWFQ_EMPTY_INT_BIT is still enabled at that point. The thread can
therefore still run geth_fill_freeq() and rewrite
GLOBAL_INTERRUPT_ENABLE_4_REG on an unclocked block.
Could the free queue interrupt be masked, and the queue torn down,
while at least one port clock is still running? Another option is for
the parent to hold its own enable on a port clock across remove. Which
ordering did you have in mind, given that port 1 owns the refill IRQ?
--
pw-bot: cr
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next v6 01/15] net: ethernet: cortina: Keep shared free queue parent-owned
2026-11-10 1:16 ` Jakub Kicinski
@ 2026-11-10 10:08 ` Linus Walleij
0 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2026-11-10 10:08 UTC (permalink / raw)
To: Jakub Kicinski
Cc: ulli.kroll, andrew+netdev, davem, pabeni, mirq-linux, mhun512,
edumazet, netdev
On Tue, Nov 10, 2026 at 2:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
> [Severity: High]
> devm_of_platform_depopulate() unbinds both port devices synchronously,
> and gemini_port_remove() ends with clk_disable_unprepare(port->pclk).
Hmmmm the bots nitpicks in that we need devm_of_platform_depopulate()
then find that this causes new hard to predict semantic
bugs .... OK rinse repeat I guess.
I'll get to it.
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next v6 02/15] net: ethernet: cortina: Keep PHY-less port bound for shared IRQ
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 01/15] net: ethernet: cortina: Keep shared free queue parent-owned Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-10 1:16 ` Jakub Kicinski
2026-11-03 17:51 ` [PATCH net-next v6 03/15] net: ethernet: cortina: Drain free queue IRQ before resize Linus Walleij
` (12 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, 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 with RTNL. Refuse shared queue setup
until both ports have completed initialization.
Keep shared hardware initialization parent-owned across individual child
removal so recreating one child cannot reset a live sibling. Mask
firmware interrupt state before child IRQ handlers are registered, and
use the allocated ring rather than an order sentinel to decide whether
the queue still needs initial setup.
Fixes: 3e813d61401a ("net: gemini: Clean up phy registration")
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
drivers/net/ethernet/cortina/gemini.c | 110 +++++++++++++++++++++++++---------
1 file changed, 82 insertions(+), 28 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index bf7652b04f1b..79c046364292 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,
@@ -1076,7 +1084,7 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
new_order = min(15, ilog2(new_size - 1) + 1);
dev_dbg(geth->dev, "set shared queue to size %d order %d\n",
new_size, new_order);
- if (geth->freeq_order == new_order)
+ if (geth->freeq_ring && geth->freeq_order == new_order)
return 0;
spin_lock_irqsave(&geth->irq_lock, flags);
@@ -2387,8 +2395,35 @@ 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;
+}
+
+static void gemini_ethernet_disable_irqs(struct gemini_ethernet *geth)
+{
+ writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_0_REG);
+ writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_1_REG);
+ writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_2_REG);
+ writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_3_REG);
+ writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
+}
+
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,19 +2433,15 @@ 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);
- writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_1_REG);
- writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_2_REG);
- writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_3_REG);
- writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
+ gemini_ethernet_disable_irqs(geth);
/* Interrupt config:
*
@@ -2442,10 +2473,20 @@ 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;
+}
+
+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 +2582,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 +2632,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 +2649,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;
@@ -2675,6 +2723,9 @@ static int gemini_ethernet_probe(struct platform_device *pdev)
spin_lock_init(&geth->irq_lock);
spin_lock_init(&geth->freeq_lock);
+ /* Quiesce firmware state before child IRQ handlers are registered. */
+ gemini_ethernet_disable_irqs(geth);
+
/* The children will use this */
platform_set_drvdata(pdev, geth);
@@ -2686,10 +2737,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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next v6 02/15] net: ethernet: cortina: Keep PHY-less port bound for shared IRQ
2026-11-03 17:51 ` [PATCH net-next v6 02/15] net: ethernet: cortina: Keep PHY-less port bound for shared IRQ Linus Walleij
@ 2026-11-10 1:16 ` Jakub Kicinski
0 siblings, 0 replies; 24+ messages in thread
From: Jakub Kicinski @ 2026-11-10 1:16 UTC (permalink / raw)
To: linusw
Cc: Jakub Kicinski, ulli.kroll, andrew+netdev, davem, pabeni,
mirq-linux, mhun512, edumazet, netdev
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: cortina: Keep PHY-less port bound for shared IRQ
This patch keeps a Gemini port child bound as an IRQ-only port when its
DT node has neither phy-handle nor fixed-link. That way the threaded
software free queue handler on port 1 stays registered. Ports are now
published to the parent under RTNL only after their IRQ and optional
netdev are ready, and geth_resize_freeq() refuses to run until both ports
are published.
> 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.
[Severity: Low]
Does this description understate what happens on the parent tree?
Before this patch, gemini_ethernet_port_probe() published the port before
PHY setup:
if (!id)
geth->port0 = port;
else
geth->port1 = port;
gemini_ethernet_init(geth);
...
ret = gmac_setup_phy(netdev);
port is embedded in netdev_priv() of a devm_alloc_etherdev_mqs() netdev.
On dns-313, nas4210b, nas4220b, rut1xx, wbd111 and dir-685,
ethernet-port@1 has no phy-mode or phy-handle. gmac_setup_phy() therefore
fails, and devres frees the netdev while geth->port1 still points into it.
The next gmac_open() on port 0 then reaches geth_resize_freeq(), which
used to do:
other_netdev = geth->port1->netdev;
if (other_netdev && netif_running(other_netdev))
return -EBUSY;
Isn't that a use-after-free of the port 1 private data?
The commit message also doesn't mention the new phy_disconnect() on the
register_netdevice() failure path. Before this patch, a register_netdev()
failure jumped to unprepare with the PHY still attached to a netdev that
devres was about to free.
Could the commit message describe the dangling geth->port1 pointer and
the phy_disconnect() fix?
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index bf7652b04f1ba..79c0463642928 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
[ ... ]
> @@ -2600,6 +2632,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 +2649,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;
> + }
[Severity: Medium]
Does this fully cover a port 1 that describes a PHY but fails to set it
up?
Only a node with neither phy-handle nor fixed-link takes the IRQ-only
publish path. Port 1 can instead have phy-handle (wbd222, ssi1328) or
fixed-link (sl93512r, sq201). If gmac_setup_phy() then fails, probe still
does goto unprepare without publishing the port. The register_netdevice()
failure path above does the same.
gmac_setup_phy() returns -ENODEV, not -EPROBE_DEFER, whenever
of_phy_get_and_connect() returns NULL:
phy = of_phy_get_and_connect(netdev,
dev->of_node,
gmac_adjust_link);
if (!phy)
return -ENODEV;
That covers a PHY that doesn't answer at its MDIO address, a PHY device
that was never registered, and an unsupported phy-mode.
In that case devres releases port 1's threaded software free queue
handler, and geth->port1 stays NULL. The new check in geth_resize_freeq()
then fails permanently:
if (!geth->initialized || !geth->port0 || !geth->port1)
return -ENODEV;
gmac_open() only tolerates -EBUSY:
err = geth_resize_freeq(port);
...
if (err && (err != -EBUSY)) {
netdev_err(netdev, "could not resize freeq\n");
goto err_stop_phy;
}
Wouldn't every open of the working port 0 then fail with "could not
resize freeq"?
This looks like the same port 0 breakage that 3e813d61401a introduced.
Before that commit, a PHY failure was deferred to ifup and port 1 stayed
bound.
The gmac_setup_phy() failure path also appears to be unchanged in the
final revision of the series.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next v6 03/15] net: ethernet: cortina: Drain free queue IRQ before resize
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 01/15] net: ethernet: cortina: Keep shared free queue parent-owned Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 02/15] net: ethernet: cortina: Keep PHY-less port bound for shared IRQ Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 04/15] net: ethernet: cortina: Normalize free queue pointers on rebuild Linus Walleij
` (11 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, 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")
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
drivers/net/ethernet/cortina/gemini.c | 53 +++++++++++++++++++++--------------
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 79c046364292..003108270d06 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();
@@ -1087,13 +1104,8 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
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);
--
2.56.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next v6 04/15] net: ethernet: cortina: Normalize free queue pointers on rebuild
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (2 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 03/15] net: ethernet: cortina: Drain free queue IRQ before resize Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-10 1:16 ` Jakub Kicinski
2026-11-03 17:51 ` [PATCH net-next v6 05/15] net: ethernet: cortina: Correct free queue DMA mappings Linus Walleij
` (10 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, Linus Walleij
The free queue read pointer is owned by hardware and can retain a
position 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. A replacement ring which does not contain
the raw retained pointer can also let hardware address past the new
allocation.
Mask software page indices to the active queue order. Keep the
replacement ring large enough for the retained hardware pointer and
initialize the software-owned write pointer from it before filling the
ring. This represents an empty queue without trying to write the
read-only pointer.
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
drivers/net/ethernet/cortina/gemini.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 003108270d06..7ebd62f8a3a4 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;
@@ -936,6 +937,7 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
union dma_skb_size skbsz;
unsigned int filled;
unsigned int pn;
+ u16 rptr;
geth->freeq_ring = dma_alloc_coherent(geth->dev,
sizeof(*geth->freeq_ring) << geth->freeq_order,
@@ -958,6 +960,10 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
if (!geth_freeq_alloc_map_page(geth, pn))
goto err_freeq_alloc;
+ /* Match the software pointer to the retained hardware pointer. */
+ rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
+ SET_WPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG, rptr & (len - 1));
+
filled = geth_fill_freeq(geth, false);
if (!filled)
goto err_freeq_alloc;
@@ -1068,6 +1074,7 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
struct net_device *other_netdev;
unsigned int new_size = 0;
unsigned int new_order;
+ u16 rptr;
int ret;
ASSERT_RTNL();
@@ -1099,6 +1106,10 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
}
new_order = min(15, ilog2(new_size - 1) + 1);
+ rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
+ if (rptr >= BIT(15))
+ return -EIO;
+ new_order = max_t(unsigned int, new_order, fls(rptr));
dev_dbg(geth->dev, "set shared queue to size %d order %d\n",
new_size, new_order);
if (geth->freeq_ring && geth->freeq_order == new_order)
--
2.56.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next v6 04/15] net: ethernet: cortina: Normalize free queue pointers on rebuild
2026-11-03 17:51 ` [PATCH net-next v6 04/15] net: ethernet: cortina: Normalize free queue pointers on rebuild Linus Walleij
@ 2026-11-10 1:16 ` Jakub Kicinski
0 siblings, 0 replies; 24+ messages in thread
From: Jakub Kicinski @ 2026-11-10 1:16 UTC (permalink / raw)
To: linusw
Cc: Jakub Kicinski, ulli.kroll, andrew+netdev, davem, pabeni,
mirq-linux, mhun512, edumazet, netdev
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: cortina: Normalize free queue pointers on rebuild
This commit masks the software free queue page indices to the active
queue order. When the free queue is rebuilt, it raises the order so the
new ring covers the read pointer the hardware kept. It also seeds the
software write pointer from that read pointer before filling the ring.
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 003108270d060..7ebd62f8a3a46 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;
[Severity: Low]
This fixes an out-of-bounds access. Should it have a Fixes: tag?
Before this change, geth_fill_freeq() used the unmasked pn taken from the
retained hardware rptr to compute &geth->freeq_pages[pn]. After a rebuild
at a smaller order, the first loop iteration could index past the newly
allocated pages array. page_ref_count() and page_ref_add() would then run
on a garbage struct page pointer:
gmac_set_ringparam() / gmac_open()
geth_resize_freeq()
geth_setup_freeq()
geth_fill_freeq(geth, false)
The unmasked code and geth_resize_freeq() both come from:
Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
The other fixes in this series have that tag. Later patches such as
"Correct free queue DMA mappings" and "Preserve in-flight free queue
pages" rely on this patch keeping rptr inside the ring.
Without the tag, could stable pick up those patches without this one?
[ ... ]
> @@ -958,6 +960,10 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
> if (!geth_freeq_alloc_map_page(geth, pn))
> goto err_freeq_alloc;
>
> + /* Match the software pointer to the retained hardware pointer. */
> + rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
> + SET_WPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG, rptr & (len - 1));
> +
[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Do the
TX queues have the same retained rptr problem this patch fixes for the
free queue?
gmac_set_ringparam() lets txq_order shrink while the interface is down.
On the next open, gmac_setup_txqs() allocates skb_tab and desc_ring with
1 << txq_order entries. It then seeds wptr and cptr from the hardware
rptr without masking it:
drivers/net/ethernet/cortina/gemini.c:gmac_setup_txqs() {
...
r = readw(rwptr_reg);
rwptr_reg += 2;
writew(r, rwptr_reg);
rwptr_reg += 2;
txq->cptr = r;
...
}
The TX rptr uses the same union dma_rwptr layout, where rptr is
read-only, and gmac_cleanup_txqs() only sets wptr = rptr. Take the
default order 8 shrunk with "ethtool -G ethX tx 16": any retained
rptr >= 16 is then outside the new ring.
gmac_start_xmit() reads the unmasked wptr, and the space check masks only
the difference:
w = rw.bits.wptr;
d = txq->cptr - w - 1;
d &= m;
gmac_map_tx_bufs() then uses w before masking it:
txq->skb[w] = skb;
...
txd = txq->ring + w;
Can this write past skb_tab and past the coherent TX descriptor ring, and
also give the hardware a pointer outside its ring?
gmac_clean_txq() also starts from the unmasked txq->cptr and dereferences
txq->ring + c and txq->skb[c].
This is still present at the end of the series.
[ ... ]
> @@ -1099,6 +1106,10 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
> }
>
> new_order = min(15, ilog2(new_size - 1) + 1);
> + rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
> + if (rptr >= BIT(15))
> + return -EIO;
> + new_order = max_t(unsigned int, new_order, fls(rptr));
> dev_dbg(geth->dev, "set shared queue to size %d order %d\n",
> new_size, new_order);
> if (geth->freeq_ring && geth->freeq_order == new_order)
[Severity: Medium]
Does this let leftover hardware state, not the configured RX ring sizes,
decide the size of the shared free queue?
gemini.h marks rptr as read-only:
unsigned int rptr : 16; /* Read Ptr, RO */
so the writel(0, GLOBAL_SWFQ_RWPTR_REG) in geth_global_init() only
clears wptr in practice. After a ring of order N has been used, rptr can
be anywhere in [0, 2^N). Whenever rptr >= 2^(N-1), fls(rptr) == N.
In that case, lowering rx_pending through gmac_set_ringparam() hits:
if (geth->freeq_ring && geth->freeq_order == new_order)
return 0;
ethtool -G then reports success, but the free queue keeps its old
descriptor ring. It also keeps its (1 << order) >> fpp_order pages,
which were allocated with GFP_ATOMIC.
Both callers resize only while RX DMA is idle. gmac_set_ringparam()
requires !netif_running(), and gmac_open() resizes before
gmac_start_dma(). So rptr cannot move between attempts.
Suppose a rebuild at the forced order fails in dma_alloc_coherent() or in
the GFP_ATOMIC page allocations in geth_setup_freeq(). Won't every later
gmac_open() compute the same order and fail the same way? gmac_open()
treats that as fatal:
err = geth_resize_freeq(port);
if (err && (err != -EBUSY)) {
netdev_err(netdev, "could not resize freeq\n");
goto err_stop_phy;
Asking for a smaller ring no longer helps in that state.
A stale rptr left by a bootloader, or by a kexec'd kernel with larger
rings, would set the same floor at first open. That seems to work against
the later "Scale Gemini RX queues to system memory" patch, which shrinks
the free queue on 32/64 MiB systems.
The commit message only says the replacement ring is kept "large enough
for the retained hardware pointer". Could it mention that requested
shrinks may be skipped? Is there a fallback when allocation at the
forced order fails?
This behaviour is unchanged at the end of the series.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next v6 05/15] net: ethernet: cortina: Correct free queue DMA mappings
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (3 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 04/15] net: ethernet: cortina: Normalize free queue pointers on rebuild Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 06/15] net: ethernet: cortina: Index free queue fragments with XArray Linus Walleij
` (9 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, 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.
The same reassignment leaves replacement metadata holding the new page
base while gmac_rx() still adds PAGE_SIZE before lookup. Once a page is
replaced under RX load, lookup therefore normally fails with "could not
find mapping" and drops the frame. If the resulting key matches another
page, RX can instead attach that page to the skb and drop the wrong page
reference.
Keep the page DMA base separate from the fragment iterator and remove the
invalid replacement unmap. The replaced mapping cannot be released until
its in-flight fragments have been claimed, which is handled with their
lifetime later in the series. Use the saved DMA base and PAGE_SIZE when
unwinding setup and tearing down the current queue.
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")
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
drivers/net/ethernet/cortina/gemini.c | 112 ++++++++++++++++++----------------
1 file changed, 58 insertions(+), 54 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 7ebd62f8a3a4..006362a03395 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;
@@ -982,12 +995,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);
}
@@ -1009,7 +1021,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;
@@ -1023,12 +1034,10 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
for (pn = 0; pn < pages; pn++) {
struct gmac_queue_page *gpage;
- dma_addr_t mapping;
-
- mapping = geth->freeq_ring[pn << fpp_order].word2.buf_adr;
- dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);
gpage = &geth->freeq_pages[pn];
+ dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
+ DMA_FROM_DEVICE);
while (page_ref_count(gpage->page) > 0)
put_page(gpage->page);
}
@@ -1486,7 +1495,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;
@@ -1523,8 +1531,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);
@@ -1543,14 +1549,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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next v6 06/15] net: ethernet: cortina: Index free queue fragments with XArray
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (4 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 05/15] net: ethernet: cortina: Correct free queue DMA mappings Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-10 1:16 ` Jakub Kicinski
2026-11-03 17:51 ` [PATCH net-next v6 07/15] net: ethernet: cortina: Preserve in-flight free queue pages Linus Walleij
` (8 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, 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.
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 006362a03395..c1c42e1f93f4 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;
u16 rptr;
@@ -962,16 +1002,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;
+ }
/* Match the software pointer to the retained hardware pointer. */
rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
@@ -1002,6 +1046,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;
@@ -2748,6 +2794,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);
/* Quiesce firmware state before child IRQ handlers are registered. */
gemini_ethernet_disable_irqs(geth);
--
2.56.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next v6 06/15] net: ethernet: cortina: Index free queue fragments with XArray
2026-11-03 17:51 ` [PATCH net-next v6 06/15] net: ethernet: cortina: Index free queue fragments with XArray Linus Walleij
@ 2026-11-10 1:16 ` Jakub Kicinski
0 siblings, 0 replies; 24+ messages in thread
From: Jakub Kicinski @ 2026-11-10 1:16 UTC (permalink / raw)
To: linusw
Cc: Jakub Kicinski, ulli.kroll, andrew+netdev, davem, pabeni,
mirq-linux, mhun512, edumazet, netdev
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: cortina: Index free queue fragments with XArray
This commit replaces the linear scan over freeq_pages in
geth_freeq_lookup() with an XArray lookup. The XArray is indexed by
fragment DMA address in fragment units. geth_freeq_alloc_map_page()
inserts the entries under freeq_lock and erases them when a page is
replaced.
> 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...
[Severity: Low]
This isn't a bug, but should the commit message say that later fixes in
this series depend on this patch?
The patch is described only as a lookup speed-up and has no Fixes: tag.
Later patches in the series do carry Fixes: tags, and they build on the
per-fragment index added here.
"Correct free queue DMA mappings" (Fixes: 4d5ae32f5e1e) drops the
replacement unmap. It says the lifetime of the replaced mapping "is
handled ... later in the series".
"Preserve in-flight free queue pages" (Fixes: 4d5ae32f5e1e) says "Keep
each exact fragment DMA address in the XArray until software claims its
descriptor". Its geth_freeq_claim() calls xa_erase() on one fragment at a
time.
If those fixes go to net or stable, they won't apply or build without
freeq_mappings, geth_freeq_mapping_index() and the per-fragment
xa_insert()/xa_erase() calls from this patch. Could the dependency be
noted here, so this patch isn't mistaken for net-next material?
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 006362a033952..c1c42e1f93f42 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
[ ... ]
> @@ -726,43 +728,47 @@ static int gmac_setup_rxq(struct net_device *netdev)
[ ... ]
> -unlock:
> + spin_lock_irqsave(&geth->freeq_lock, flags);
> + gpage = xa_load(&geth->freeq_mappings, index);
> + if (!gpage || !gpage->page)
> + goto err_unlock;
> +
> + page = gpage->page;
> + page_mapping = gpage->mapping;
> + valid = mapping >= page_mapping &&
> + mapping - page_mapping <= PAGE_SIZE - frag_len &&
> + !((mapping - page_mapping) & (frag_len - 1));
> + if (!valid)
> + goto err_unlock;
> +
> + *page_offs = mapping - page_mapping;
> spin_unlock_irqrestore(&geth->freeq_lock, flags);
> return page;
[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
is a CPU sync missing before the fragment is handed back to gmac_rx()?
geth_freeq_lookup() returns a fragment that the device has written,
without calling dma_sync_single_range_for_cpu().
In the other direction, geth_fill_freeq() gives a page with refcount 1
back to the hardware without calling dma_sync_single_for_device():
geth_fill_freeq() {
...
/* Add one reference per fragment in the page */
page_ref_add(page, 1 << fpp_order);
...
}
There are no dma_sync_* calls anywhere in gemini.c at this commit. With
non-coherent DMA, could the CPU read stale cache lines for RX data?
Later patches in this series appear to fix this.
geth_freeq_claim() calls dma_sync_single_range_for_cpu() for each
claimed fragment. geth_freeq_post_page() calls
dma_sync_single_for_device() when recycling a page.
geth_freeq_release_pages() syncs the fragments that hardware still owns.
The commit message of "Preserve in-flight free queue pages" also notes
that the CPU sync is currently a no-op on FA526.
[ ... ]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next v6 07/15] net: ethernet: cortina: Preserve in-flight free queue pages
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (5 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 06/15] net: ethernet: cortina: Index free queue fragments with XArray Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-10 1:16 ` Jakub Kicinski
2026-11-03 17:51 ` [PATCH net-next v6 08/15] net: ethernet: cortina: Rotate free queue page allocation Linus Walleij
` (7 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, 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.
Keep free queue positions in descriptor units when posting pages. The
retained hardware pointer need not be aligned to the number of fragments
per page; rounding it down would place one fragment behind the hardware
read pointer and retain its mapping and reference until teardown. Wrap
individual descriptor entries instead.
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.
Size the metadata pool for the shared free queue and both registered RX
rings. Include that capacity in resize decisions, reuse an oversized queue
only while the peer is running, and roll back rejected RX ring changes.
This prevents same-order ring growth from exhausting page slots and lets
downsizing release surplus pages when both ports are idle.
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")
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
drivers/net/ethernet/cortina/gemini.c | 379 +++++++++++++++++++++-------------
1 file changed, 237 insertions(+), 142 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index c1c42e1f93f4..9602429388b1 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>
@@ -21,6 +22,7 @@
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/dma-mapping.h>
+#include <linux/circ_buf.h>
#include <linux/cache.h>
#include <linux/interrupt.h>
#include <linux/reset.h>
@@ -89,11 +91,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 +174,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 +740,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 +773,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 +784,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 +845,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 +858,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 +881,35 @@ 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 wptr,
+ struct page *page, dma_addr_t page_mapping)
+{
+ struct gmac_rxdesc *freeq_entry;
+ struct gmac_queue_page *gpage;
+ unsigned int ring_mask;
+ 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
@@ -866,8 +919,11 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
frag_len = 1 << geth->freeq_frag_order; /* Usually 2048 */
fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
fragments = 1 << fpp_order;
+ ring_mask = (1 << geth->freeq_order) - 1;
- 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,33 +932,22 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
if (ret)
goto err_mappings;
}
+ gpage->fragments = fragments;
+ page_ref_add(page, fragments);
- 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;
-
- 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);
+ dev_dbg(geth->dev,
+ "post page at entry %u fragment length %u fragments per page %u\n",
+ wptr, frag_len, fragments);
mapping = page_mapping;
- for (i = (1 << fpp_order); i > 0; i--) {
+ for (i = 0; i < fragments; i++) {
+ freeq_entry = geth->freeq_ring + ((wptr + i) & ring_mask);
freeq_entry->word2.buf_adr = mapping;
- freeq_entry++;
mapping += frag_len;
}
- dev_dbg(geth->dev, "page %d, DMA addr: %pad, page %p\n",
- pn, &page_mapping, page);
+ dev_dbg(geth->dev, "DMA addr: %pad, page %p\n",
+ &page_mapping, page);
- return page;
+ return 0;
err_mappings:
while (i--) {
@@ -910,88 +955,146 @@ 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 fragments = 1 << fpp_order;
+ unsigned int len = 1 << geth->freeq_order;
+ unsigned int ring_mask = len - 1;
unsigned int count = 0;
- unsigned int pn, epn;
+ unsigned int rptr, wptr;
unsigned long flags;
union dma_rwptr rw;
- unsigned int m_pn;
+ bool space;
- /* Mask for page */
- m_pn = (1 << (geth->freeq_order - fpp_order)) - 1;
+ /* Loop over the freeq ring buffer entries */
+ for (;;) {
+ dma_addr_t page_mapping;
+ struct page *page;
+ int ret;
- spin_lock_irqsave(&geth->freeq_lock, flags);
+ spin_lock_irqsave(&geth->freeq_lock, flags);
+ rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
+ rptr = rw.bits.rptr & ring_mask;
+ wptr = rw.bits.wptr & ring_mask;
+ space = CIRC_SPACE(wptr, rptr, len) >= fragments;
+ spin_unlock_irqrestore(&geth->freeq_lock, flags);
+ if (!space)
+ break;
- 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;
+ ret = geth_freeq_map_page(geth, &page, &page_mapping);
+ if (ret)
+ break;
- /* Loop over the freeq ring buffer entries */
- while (pn != epn) {
+ spin_lock_irqsave(&geth->freeq_lock, flags);
+
+ rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
+ rptr = rw.bits.rptr & ring_mask;
+ wptr = rw.bits.wptr & ring_mask;
+ if (CIRC_SPACE(wptr, rptr, len) < fragments) {
+ ret = -ENOSPC;
+ } else {
+ ret = geth_freeq_add_page(geth, wptr, page,
+ page_mapping);
+ if (!ret) {
+ count += fragments;
+ wptr = (wptr + fragments) & ring_mask;
+ SET_WPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG, wptr);
+ }
+ }
+
+ 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;
+ }
+ }
+
+ return count;
+}
+
+static void geth_freeq_release_pages(struct gemini_ethernet *geth)
+{
+ unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
+ unsigned int frag_len = 1 << geth->freeq_frag_order;
+ unsigned int pn;
+
+ for (pn = 0; pn < geth->num_freeq_pages; pn++) {
struct gmac_queue_page *gpage;
+ dma_addr_t mapping;
struct page *page;
+ unsigned int i;
gpage = &geth->freeq_pages[pn];
- 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);
-
- if (page_ref_count(page) > 1) {
- unsigned int fl = (pn - epn) & m_pn;
+ if (!gpage->page)
+ continue;
- if (fl > 64 >> fpp_order)
- break;
+ 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;
- page = geth_freeq_alloc_map_page(geth, pn);
- if (!page)
- break;
+ dma_sync_single_range_for_cpu(geth->dev, gpage->mapping,
+ i * frag_len, frag_len,
+ DMA_FROM_DEVICE);
}
-
- /* Add one reference per fragment in the page */
- page_ref_add(page, 1 << fpp_order);
- count += 1 << fpp_order;
- pn++;
- pn &= m_pn;
+ 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);
+}
- writew(pn << fpp_order, geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
+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);
- spin_unlock_irqrestore(&geth->freeq_lock, flags);
+ 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 count;
+ 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;
+ unsigned int expected;
union queue_threshold qt;
union dma_skb_size skbsz;
- unsigned long flags;
- struct page *page;
unsigned int filled;
- unsigned int pn;
u16 rptr;
+ 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,
&geth->freeq_dma_base, GFP_KERNEL);
@@ -1003,27 +1106,22 @@ 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;
/* Match the software pointer to the retained hardware pointer. */
rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
SET_WPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG, rptr & (len - 1));
- 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;
@@ -1036,18 +1134,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;
@@ -1065,11 +1158,6 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
*/
static void geth_cleanup_freeq(struct gemini_ethernet *geth)
{
- unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
- unsigned int len = 1 << geth->freeq_order;
- unsigned int pages = len >> fpp_order;
- unsigned int pn;
-
if (!geth->freeq_ring)
return;
@@ -1077,17 +1165,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;
-
- gpage = &geth->freeq_pages[pn];
- dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
- DMA_FROM_DEVICE);
- while (page_ref_count(gpage->page) > 0)
- put_page(gpage->page);
- }
- xa_destroy(&geth->freeq_mappings);
+ geth_freeq_release_pages(geth);
+ bitmap_free(geth->freeq_page_bitmap);
+ geth->freeq_page_bitmap = NULL;
kfree(geth->freeq_pages);
geth->freeq_pages = NULL;
geth->num_freeq_pages = 0;
@@ -1127,7 +1208,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;
u16 rptr;
int ret;
@@ -1143,9 +1226,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",
@@ -1167,8 +1248,19 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
new_order = max_t(unsigned int, new_order, fls(rptr));
dev_dbg(geth->dev, "set shared queue to size %d order %d\n",
new_size, new_order);
- if (geth->freeq_ring && geth->freeq_order == new_order)
- return 0;
+ if (new_order <= PAGE_SHIFT - geth->freeq_frag_order)
+ return -EINVAL;
+
+ 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);
@@ -1595,7 +1687,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");
@@ -1692,7 +1784,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 &&
@@ -1947,10 +2039,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;
}
@@ -2309,14 +2398,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);
@@ -2422,7 +2517,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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next v6 07/15] net: ethernet: cortina: Preserve in-flight free queue pages
2026-11-03 17:51 ` [PATCH net-next v6 07/15] net: ethernet: cortina: Preserve in-flight free queue pages Linus Walleij
@ 2026-11-10 1:16 ` Jakub Kicinski
0 siblings, 0 replies; 24+ messages in thread
From: Jakub Kicinski @ 2026-11-10 1:16 UTC (permalink / raw)
To: linusw
Cc: Jakub Kicinski, ulli.kroll, andrew+netdev, davem, pabeni,
mirq-linux, mhun512, edumazet, netdev
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: cortina: Preserve in-flight free queue pages
This patch stops indexing free queue page metadata by ring position.
Metadata slots now come from a pool tracked by a bitmap, and each fragment's
DMA address stays in the XArray until software claims it. Each posted
fragment holds its own page reference, the page is unmapped once both
fragments are claimed, and free queue sizing now accounts for both RX rings.
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index c1c42e1f93f42..9602429388b16 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
[ ... ]
> @@ -910,88 +955,146 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
[ ... ]
> -static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
> +static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
> {
[ ... ]
> + /* Loop over the freeq ring buffer entries */
> + for (;;) {
> + dma_addr_t page_mapping;
> + struct page *page;
> + int ret;
>
> - spin_lock_irqsave(&geth->freeq_lock, flags);
> + spin_lock_irqsave(&geth->freeq_lock, flags);
> + rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
> + rptr = rw.bits.rptr & ring_mask;
> + wptr = rw.bits.wptr & ring_mask;
> + space = CIRC_SPACE(wptr, rptr, len) >= fragments;
> + spin_unlock_irqrestore(&geth->freeq_lock, flags);
> + if (!space)
> + break;
>
> - 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;
> + ret = geth_freeq_map_page(geth, &page, &page_mapping);
> + if (ret)
> + break;
[Severity: Low]
This isn't a functional bug, but since this patch has a Fixes: tag, I have
a couple of questions about how it is meant to go to stable.
geth_freeq_claim() depends on freeq_mappings and
geth_freeq_mapping_index(). Both come from the earlier "Index free queue
fragments with XArray" patch, which has no Fixes: tag. Would this patch
apply and build on its own in a stable tree without that prerequisite?
This patch also removes position-based recycling. Every refill now maps a
fresh page through geth_freeq_map_page(), and geth_freeq_claim() unmaps
and frees each page once its last fragment is claimed.
The commit message says "A later patch restores recycling independently of
queue position". The later "Recycle claimed free queue pages" patch in this
series does that, and it is also tagged Fixes: 4d5ae32f5e1e. Should the
two patches be kept together for backports, so stable doesn't end up with
only the allocate-on-every-refill state?
[ ... ]
> @@ -1595,7 +1687,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]
Can free queue pages leak if the hardware uses a posted buffer but the
driver never claims it?
With this patch, three things are released together: the page's slot in
freeq_page_bitmap, its streaming DMA mapping and its per-fragment
references. That happens only when geth_freeq_claim() sees every
fragment's exact DMA address, or when geth_freeq_release_pages() tears
down the whole free queue:
geth_freeq_claim() {
...
if (!--gpage->fragments) {
...
__clear_bit(slot, geth->freeq_page_bitmap);
put_page(page);
}
...
}
gmac_rx() has two paths that drop a descriptor without claiming anything.
One is the "could not find mapping" case above. The other is the zero
address case just before it:
if (!mapping) {
if (net_ratelimit())
netdev_err(netdev,
"rxq[%u]: HW BUG: zero DMA desc\n",
r);
goto err_drop;
}
If the hardware used a free queue fragment for one of these descriptors,
that fragment is never claimed.
gmac_cleanup_rxq() only walks the RX ring between rptr and wptr. A buffer
the DMA engine took from the shared free queue but had not yet written into
the ring before gmac_stop() would also stay unclaimed.
These slots come back only when the free queue is rebuilt. That doesn't
happen while the peer port is running, or on a reopen with the same order,
because geth_resize_freeq() returns 0 early in both cases.
geth_freeq_page_slots() sizes the pool as the free queue pages plus both RX
ring sizes, which leaves little slack. Once enough slots are pinned,
geth_freeq_alloc_slot() returns -ENOSPC:
slot = find_first_zero_bit(geth->freeq_page_bitmap,
geth->num_freeq_pages);
if (slot == geth->num_freeq_pages)
return -ENOSPC;
and geth_fill_freeq() stops refilling the shared queue. Could RX on both
ports eventually starve this way?
The old position-indexed scheme got the metadata slot back the next time
that position was refilled.
The code doesn't make it clear whether the hardware really uses a posted
buffer in these error cases, so this may not be reachable in practice. The
same behaviour seems to remain at the end of the series, where slots are
still only released through a claim followed by recycle or discard.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next v6 08/15] net: ethernet: cortina: Rotate free queue page allocation
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (6 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 07/15] net: ethernet: cortina: Preserve in-flight free queue pages Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 09/15] net: ethernet: cortina: Recycle claimed free queue pages Linus Walleij
` (6 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, 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.
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 9602429388b1..1befa8dd13a6 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -175,6 +175,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 */
};
@@ -746,12 +747,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;
}
@@ -1113,6 +1122,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;
/* Match the software pointer to the retained hardware pointer. */
rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
@@ -1144,6 +1154,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,
@@ -1172,6 +1183,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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next v6 09/15] net: ethernet: cortina: Recycle claimed free queue pages
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (7 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 08/15] net: ethernet: cortina: Rotate free queue page allocation Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-10 1:16 ` Jakub Kicinski
2026-11-03 17:51 ` [PATCH net-next v6 10/15] net: ethernet: cortina: Validate RX fragment lengths Linus Walleij
` (5 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, 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.
Recheck the recycle bitmap on each refill iteration and again after mapping
a candidate page. If another RX path retires a page concurrently, release
the candidate and return to the recycler before consuming another metadata
slot. Retry recycling immediately when a retained page is discarded so
other pending entries are examined before allocating a replacement. This
avoids stopping refill while recyclable slots are pending.
Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
drivers/net/ethernet/cortina/gemini.c | 152 +++++++++++++++++++++++++++++-----
1 file changed, 130 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 1befa8dd13a6..25121325f219 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -175,7 +175,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 */
};
@@ -765,6 +767,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)
@@ -774,8 +804,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);
@@ -799,13 +829,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;
@@ -899,26 +924,23 @@ static int geth_freeq_map_page(struct gemini_ethernet *geth,
return 0;
}
-static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int wptr,
- struct page *page, dma_addr_t page_mapping)
+static int geth_freeq_post_page(struct gemini_ethernet *geth,
+ unsigned int wptr,
+ struct gmac_queue_page *gpage, bool recycle)
{
struct gmac_rxdesc *freeq_entry;
- struct gmac_queue_page *gpage;
unsigned int ring_mask;
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
@@ -930,9 +952,6 @@ static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int wptr,
fragments = 1 << fpp_order;
ring_mask = (1 << geth->freeq_order) - 1;
- 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,
@@ -941,6 +960,9 @@ static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int wptr,
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);
@@ -964,9 +986,69 @@ static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int wptr,
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 wptr,
+ 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, wptr, 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 wptr)
+{
+ 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, wptr, gpage, true);
+ if (ret) {
+ __set_bit(slot, geth->freeq_recycle_bitmap);
+ geth->freeq_recycle_pending++;
+ }
+
return ret;
}
@@ -1000,9 +1082,22 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
rptr = rw.bits.rptr & ring_mask;
wptr = rw.bits.wptr & ring_mask;
space = CIRC_SPACE(wptr, rptr, len) >= fragments;
+ ret = -ENOSPC;
+ if (space && geth->freeq_recycle_pending) {
+ ret = geth_freeq_recycle_page(geth, wptr);
+ if (!ret) {
+ count += fragments;
+ wptr = (wptr + fragments) & ring_mask;
+ SET_WPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG, wptr);
+ }
+ }
spin_unlock_irqrestore(&geth->freeq_lock, flags);
if (!space)
break;
+ if (!ret || ret == -EAGAIN)
+ continue;
+ if (ret != -ENOSPC)
+ break;
ret = geth_freeq_map_page(geth, &page, &page_mapping);
if (ret)
@@ -1015,6 +1110,8 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
wptr = rw.bits.wptr & ring_mask;
if (CIRC_SPACE(wptr, rptr, len) < fragments) {
ret = -ENOSPC;
+ } else if (geth->freeq_recycle_pending) {
+ ret = -EAGAIN;
} else {
ret = geth_freeq_add_page(geth, wptr, page,
page_mapping);
@@ -1031,6 +1128,8 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
DMA_FROM_DEVICE);
put_page(page);
+ if (ret == -EAGAIN)
+ continue;
break;
}
}
@@ -1075,6 +1174,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
@@ -1121,6 +1221,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;
@@ -1148,6 +1251,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:
@@ -1178,6 +1284,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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next v6 09/15] net: ethernet: cortina: Recycle claimed free queue pages
2026-11-03 17:51 ` [PATCH net-next v6 09/15] net: ethernet: cortina: Recycle claimed free queue pages Linus Walleij
@ 2026-11-10 1:16 ` Jakub Kicinski
0 siblings, 0 replies; 24+ messages in thread
From: Jakub Kicinski @ 2026-11-10 1:16 UTC (permalink / raw)
To: linusw
Cc: Jakub Kicinski, ulli.kroll, andrew+netdev, davem, pabeni,
mirq-linux, mhun512, edumazet, netdev
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -964,9 +986,69 @@ static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int wptr,
> 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 wptr,
> + 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, wptr, 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 wptr)
> +{
> + 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, wptr, gpage, true);
> + if (ret) {
> + __set_bit(slot, geth->freeq_recycle_bitmap);
> + geth->freeq_recycle_pending++;
> + }
> +
> return ret;
> }
>
[Severity: Medium]
geth_freeq_recycle_page() only checks page_ref_count() before handing
the page back to geth_freeq_post_page(). The pages come from
alloc_page(GFP_ATOMIC) in geth_freeq_map_page(). Under memory pressure
that allocation can return a page from the PFMEMALLOC reserves.
When gmac_rx() attaches a fragment with skb_fill_page_desc(), the skb
inherits skb->pfmemalloc. The core then drops it for any socket that
does not have SOCK_MEMALLOC set. That drop happens inside the NAPI
poll, so by the time geth_fill_freeq() runs the refcount is back to
one and the page is reposted to the free queue.
Nothing ever returns that page to the allocator. The reserve page stays
pinned, and every frame that lands in it is dropped, long after the
pressure has gone away.
Before this patch the base reference was dropped on the last claim in
geth_freeq_claim(), so a reserve page was freed once the stack let go
of it. The problem is new with recycling.
Other drivers that recycle RX pages gate reuse on
dev_page_is_reusable(), which also rejects pages from a remote NUMA
node. Could the discard branch here also be taken when
dev_page_is_reusable(gpage->page) is false? Pfmemalloc pages would then
go through the dma_unmap_single_attrs() and put_page() path instead of
being reposted.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next v6 10/15] net: ethernet: cortina: Validate RX fragment lengths
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (8 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 09/15] net: ethernet: cortina: Recycle claimed free queue pages Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 11/15] net: ethernet: cortina: Account RX buffer size in truesize Linus Walleij
` (4 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, 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. Validate the
final fragment length derived from the total frame byte count, rejecting
underflow and ranges extending beyond either the DMA fragment or the page.
Drop zero-length non-final descriptors. If an EOF descriptor carries no new
data after earlier descriptors completed the frame, release its unused page
reference and pass the assembled skb to GRO. Check this case before the
fragment limit so an empty EOF can terminate a full fragment array.
Correct the copied buffer_size comment to match the Gemini manual, which
defines it as the transfer buffer size associated with the descriptor.
Require the final frame length to cover an Ethernet frame so a malformed
chain cannot pass a short skb to GRO. Account 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.
Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
drivers/net/ethernet/cortina/gemini.c | 42 +++++++++++++++++++++++++++--------
drivers/net/ethernet/cortina/gemini.h | 2 +-
2 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 25121325f219..972119080c2e 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1745,6 +1745,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;
@@ -1819,6 +1820,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;
@@ -1827,15 +1831,33 @@ 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)
+ goto err_drop;
frag_len = frame_len - skb->len;
+ }
+
+ if (!frag_len) {
+ if (word3.bits32 & EOF_BIT) {
+ put_page(page);
+ page = NULL;
+ goto complete_frame;
+ }
+
+ if (net_ratelimit())
+ netdev_err(netdev,
+ "Received fragment with len = 0\n");
+ goto err_drop;
+ }
/* append page frag to skb */
if (frag_nr == MAX_SKB_FRAGS)
goto err_drop;
-
- if (frag_len == 0 && net_ratelimit())
- netdev_err(netdev, "Received fragment with len = 0\n");
+ if (frag_len > freeq_frag_len -
+ (page_offs & (freeq_frag_len - 1)) ||
+ frag_len > PAGE_SIZE - page_offs)
+ goto err_drop;
skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);
skb->len += frag_len;
@@ -1843,11 +1865,13 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
skb->truesize += frag_len;
frag_nr++;
- if (word3.bits32 & EOF_BIT) {
- napi_gro_frags(&port->napi);
- skb = NULL;
- frag_nr = 0;
- }
+ if (!(word3.bits32 & EOF_BIT))
+ goto next_desc;
+
+complete_frame:
+ napi_gro_frags(&port->napi);
+ skb = NULL;
+ frag_nr = 0;
goto next_desc;
err_drop:
diff --git a/drivers/net/ethernet/cortina/gemini.h b/drivers/net/ethernet/cortina/gemini.h
index d359c633c5de..43006fe44c9f 100644
--- a/drivers/net/ethernet/cortina/gemini.h
+++ b/drivers/net/ethernet/cortina/gemini.h
@@ -519,7 +519,7 @@ struct gmac_txdesc {
union gmac_rxdesc_0 {
unsigned int bits32;
struct bit_8060 {
- /* bit 15:0 number of descriptors used for the current frame */
+ /* bit 15:0 transfer buffer size for this descriptor */
unsigned int buffer_size:16;
/* bit 21:16 number of descriptors used for the current frame */
unsigned int desc_count:6;
--
2.56.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next v6 11/15] net: ethernet: cortina: Account RX buffer size in truesize
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (9 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 10/15] net: ethernet: cortina: Validate RX fragment lengths Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 12/15] net: ethernet: cortina: Release partial RX frames on stop Linus Walleij
` (3 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, 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.
Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
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 972119080c2e..7569c9935b85 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1862,7 +1862,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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next v6 12/15] net: ethernet: cortina: Release partial RX frames on stop
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (10 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 11/15] net: ethernet: cortina: Account RX buffer size in truesize Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 13/15] net: ethernet: cortina: Scale Gemini RX queues to system memory Linus Walleij
` (2 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, Linus Walleij
A NAPI poll can retain a partially assembled frame across invocations. The
stop path clears port->rx_skb without freeing the skb retained in
napi->skb. If the interface is reopened, napi_get_frags() returns that
stale skb. The next frame overwrites its fragment array from index zero
while len and data_len still include the partial frame.
This can lose page references belonging to the old fragments or pass an
skb with inconsistent fragment accounting to GRO. If the interface is
removed without reopening, NAPI teardown eventually frees the stale skb.
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.
Fixes: 06937db21ee3 ("net: ethernet: cortina: Make RX SKB per-port")
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 7569c9935b85..be2b9702718f 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -2224,12 +2224,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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next v6 13/15] net: ethernet: cortina: Scale Gemini RX queues to system memory
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (11 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 12/15] net: ethernet: cortina: Release partial RX frames on stop Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 14/15] net: ethernet: cortina: Scope RX statistics updates Linus Walleij
2026-11-03 17:51 ` [PATCH net-next v6 15/15] net: ethernet: cortina: Drain RX coalescing timer on stop Linus Walleij
14 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, 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.
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 be2b9702718f..5e89057bb2fb 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>
@@ -37,6 +38,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>
@@ -475,6 +477,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);
@@ -543,7 +556,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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next v6 14/15] net: ethernet: cortina: Scope RX statistics updates
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (12 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 13/15] net: ethernet: cortina: Scale Gemini RX queues to system memory Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
2026-11-10 1:16 ` Jakub Kicinski
2026-11-03 17:51 ` [PATCH net-next v6 15/15] net: ethernet: cortina: Drain RX coalescing timer on stop Linus Walleij
14 siblings, 1 reply; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, Linus Walleij
On 32-bit PREEMPT_RT 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. spinlock_t can sleep on PREEMPT_RT 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.
Initialize the dynamically allocated statistics synchronization objects
so their seqcount lockdep keys are valid on 32-bit debug kernels.
The issue was found by inspecting the u64_stats_sync writer scope. The
series was tested on a D-Link DIR-685 with media playback and ksmbd
transfers under ping load. PREEMPT_RT was not runtime-tested.
Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
drivers/net/ethernet/cortina/gemini.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 5e89057bb2fb..c32e9574c8c9 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1720,12 +1720,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))
@@ -1736,6 +1736,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;
}
@@ -1747,8 +1748,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;
}
@@ -1806,7 +1811,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;
}
@@ -1898,7 +1905,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;
}
@@ -1930,13 +1939,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) {
@@ -2863,6 +2872,9 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
}
port = netdev_priv(netdev);
+ u64_stats_init(&port->tx_stats_syncp);
+ u64_stats_init(&port->rx_stats_syncp);
+ u64_stats_init(&port->ir_stats_syncp);
SET_NETDEV_DEV(netdev, dev);
port->netdev = netdev;
port->id = id;
--
2.56.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next v6 14/15] net: ethernet: cortina: Scope RX statistics updates
2026-11-03 17:51 ` [PATCH net-next v6 14/15] net: ethernet: cortina: Scope RX statistics updates Linus Walleij
@ 2026-11-10 1:16 ` Jakub Kicinski
0 siblings, 0 replies; 24+ messages in thread
From: Jakub Kicinski @ 2026-11-10 1:16 UTC (permalink / raw)
To: linusw
Cc: Jakub Kicinski, ulli.kroll, andrew+netdev, davem, pabeni,
mirq-linux, mhun512, edumazet, netdev
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: cortina: Scope RX statistics updates
This patch narrows the rx_stats_syncp writer sections in gmac_napi_poll()
so they no longer span gmac_rx(). The writer sections now wrap only the
individual counter updates in gmac_skb_if_good_frame() and gmac_rx(). It
also adds u64_stats_init() calls for the tx, rx and ir syncp objects in
gemini_ethernet_port_probe().
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 5e89057bb2fb6..c32e9574c8c95 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
[ ... ]
> @@ -2863,6 +2872,9 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
> }
>
> port = netdev_priv(netdev);
> + u64_stats_init(&port->tx_stats_syncp);
> + u64_stats_init(&port->rx_stats_syncp);
> + u64_stats_init(&port->ir_stats_syncp);
[Severity: High]
This patch didn't introduce this, but since it touches tx_stats_syncp:
can the TX reader loop in gmac_get_ethtool_stats() write past the end of
the ethtool stats buffer?
drivers/net/ethernet/cortina/gemini.c:gmac_get_ethtool_stats() {
...
/* Racing with TX start_xmit */
do {
p = values;
start = u64_stats_fetch_begin(&port->tx_stats_syncp);
for (i = 0; i < TX_MAX_FRAGS; i++) {
*values++ = port->tx_frag_stats[i];
port->tx_frag_stats[i] = 0;
}
*values++ = port->tx_frags_linearized;
*values++ = port->tx_hw_csummed;
} while (u64_stats_fetch_retry(&port->tx_stats_syncp, start));
}
The loop resets p at the top of each pass, but the body writes through
values++ and never uses p. After the first pass, values points at the end
of the GMAC_STATS_NUM u64 array.
ethtool_get_stats() allocates that array with
vzalloc(n_stats * sizeof(u64)). If u64_stats_fetch_retry() forces a
retry, the next pass writes another TX_MAX_FRAGS + 2 u64 values (144
bytes) past the end. Each further retry writes another 144 bytes, and
nothing bounds the number of retries.
Gemini is 32-bit, so the seqcount is real there. A transmit that runs
during the read can bump tx_stats_syncp and trigger the retry, for
example:
gmac_start_xmit()->gmac_clean_txq()
u64_stats_update_begin(&port->tx_stats_syncp);
port->tx_frag_stats[nfrags]++;
u64_stats_update_end(&port->tx_stats_syncp);
ETHTOOL_GSTATS is one of the commands that does not need privileges.
This loop has two more problems. port->tx_hw_csummed is read under
tx_stats_syncp, but gmac_clean_txq() writes it under ir_stats_syncp:
u64_stats_update_begin(&port->ir_stats_syncp);
...
port->tx_hw_csummed += hwchksum;
u64_stats_update_end(&port->ir_stats_syncp);
So this reader has no protection against torn reads of that counter.
Also, the reader clears port->tx_frag_stats[i] = 0 outside any writer
section. That races with the tx_frag_stats[nfrags]++ in gmac_clean_txq()
and can lose increments. It also resets the counters on every read.
Should the loop write through *p++ like the RX and MIB loops above it,
read tx_hw_csummed under ir_stats_syncp, and stop zeroing tx_frag_stats
from the reader?
> SET_NETDEV_DEV(netdev, dev);
> port->netdev = netdev;
> port->id = id;
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next v6 15/15] net: ethernet: cortina: Drain RX coalescing timer on stop
2026-11-03 17:51 [PATCH net-next v6 00/15] net: ethernet: cortina: Fix Gemini RX buffer management Linus Walleij
` (13 preceding siblings ...)
2026-11-03 17:51 ` [PATCH net-next v6 14/15] net: ethernet: cortina: Scope RX statistics updates Linus Walleij
@ 2026-11-03 17:51 ` Linus Walleij
14 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2026-11-03 17:51 UTC (permalink / raw)
To: Hans Ulli Kroll, Andrew Lunn, David S. Miller, Jakub Kicinski,
Paolo Abeni, Michał Mirosław, Myeonghun Pak,
Eric Dumazet
Cc: netdev, Linus Walleij
gmac_stop() cancels the RX coalescing timer before disabling and
freeing the IRQ. An RX interrupt racing with that cancellation can
rearm the timer, leaving it queued after the netdev and its private
port state have been freed.
Initialize the timer before requesting the IRQ, and cancel it again
after free_irq() has drained every handler that can rearm it. Keep the
first cancellation to prevent the timer callback from scheduling NAPI
while the poll instance is being disabled.
This was found by code inspection and tested on the D-Link DIR-685.
Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
drivers/net/ethernet/cortina/gemini.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index c32e9574c8c9..d4afe6efb29e 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -2194,6 +2194,10 @@ static int gmac_open(struct net_device *netdev)
struct gemini_ethernet_port *port = netdev_priv(netdev);
int err;
+ hrtimer_setup(&port->rx_coalesce_timer,
+ &gmac_coalesce_delay_expired,
+ CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+
err = request_irq(netdev->irq, gmac_irq,
IRQF_SHARED, netdev->name, netdev);
if (err) {
@@ -2230,9 +2234,6 @@ static int gmac_open(struct net_device *netdev)
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);
-
netdev_dbg(netdev, "opened\n");
return 0;
@@ -2269,6 +2270,7 @@ static int gmac_stop(struct net_device *netdev)
phy_stop(netdev->phydev);
free_irq(netdev->irq, netdev);
+ hrtimer_cancel(&port->rx_coalesce_timer);
gmac_update_hw_stats(netdev);
return 0;
--
2.56.0
^ permalink raw reply related [flat|nested] 24+ messages in thread