* [PATCH net-next v2 1/8] net: macb: move Rx buffers alloc from link up to open
2026-02-23 18:26 [PATCH net-next v2 0/8] net: macb: Add XDP support and page pool integration Paolo Valerio
@ 2026-02-23 18:26 ` Paolo Valerio
2026-02-24 0:08 ` [net-next,v2,1/8] " Jakub Kicinski
2026-02-23 18:26 ` [PATCH net-next v2 2/8] net: macb: rename rx_skbuff into rx_buff Paolo Valerio
` (6 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Paolo Valerio @ 2026-02-23 18:26 UTC (permalink / raw)
To: netdev
Cc: Nicolas Ferre, Claudiu Beznea, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lorenzo Bianconi,
Théo Lebrun
From: Théo Lebrun <theo.lebrun@bootlin.com>
mog_alloc_rx_buffers(), getting called at open, does not do rx buffer
alloc on GEM. The bulk of the work is done by gem_rx_refill() filling
up all slots with valid buffers.
gem_rx_refill() is called at link up by
gem_init_rings() == bp->macbgem_ops.mog_init_rings().
Move operation to macb_open(), mostly to allow it to fail early and
loudly rather than init the device with Rx mostly broken.
About `bool fail_early`:
- When called from macb_open(), ring init fails as soon as a queue
cannot be refilled.
- When called from macb_hresp_error_task(), we do our best to reinit
the device: we still iterate over all queues and try refilling all
even if a previous queue failed.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
drivers/net/ethernet/cadence/macb.h | 2 +-
drivers/net/ethernet/cadence/macb_main.c | 36 ++++++++++++++++++------
2 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 87414a2ddf6e..2cb65ec37d44 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1180,7 +1180,7 @@ struct macb_queue;
struct macb_or_gem_ops {
int (*mog_alloc_rx_buffers)(struct macb *bp);
void (*mog_free_rx_buffers)(struct macb *bp);
- void (*mog_init_rings)(struct macb *bp);
+ int (*mog_init_rings)(struct macb *bp, bool fail_early);
int (*mog_rx)(struct macb_queue *queue, struct napi_struct *napi,
int budget);
};
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 43cd013bb70e..8cc16f2b4024 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1247,13 +1247,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
return packets;
}
-static void gem_rx_refill(struct macb_queue *queue)
+static int gem_rx_refill(struct macb_queue *queue)
{
unsigned int entry;
struct sk_buff *skb;
dma_addr_t paddr;
struct macb *bp = queue->bp;
struct macb_dma_desc *desc;
+ int err = 0;
while (CIRC_SPACE(queue->rx_prepared_head, queue->rx_tail,
bp->rx_ring_size) > 0) {
@@ -1270,6 +1271,7 @@ static void gem_rx_refill(struct macb_queue *queue)
if (unlikely(!skb)) {
netdev_err(bp->dev,
"Unable to allocate sk_buff\n");
+ err = -ENOMEM;
break;
}
@@ -1319,6 +1321,7 @@ static void gem_rx_refill(struct macb_queue *queue)
netdev_vdbg(bp->dev, "rx ring: queue: %p, prepared head %d, tail %d\n",
queue, queue->rx_prepared_head, queue->rx_tail);
+ return err;
}
/* Mark DMA descriptors from begin up to and not including end as unused */
@@ -1771,7 +1774,7 @@ static void macb_hresp_error_task(struct work_struct *work)
netif_tx_stop_all_queues(dev);
netif_carrier_off(dev);
- bp->macbgem_ops.mog_init_rings(bp);
+ bp->macbgem_ops.mog_init_rings(bp, false);
/* Initialize TX and RX buffers */
macb_init_buffers(bp);
@@ -2544,8 +2547,6 @@ static int macb_alloc_consistent(struct macb *bp)
if (!queue->tx_skb)
goto out_err;
}
- if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
- goto out_err;
/* Required for tie off descriptor for PM cases */
if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE)) {
@@ -2557,6 +2558,11 @@ static int macb_alloc_consistent(struct macb *bp)
goto out_err;
}
+ if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
+ goto out_err;
+ if (bp->macbgem_ops.mog_init_rings(bp, true))
+ goto out_err;
+
return 0;
out_err:
@@ -2577,11 +2583,13 @@ static void macb_init_tieoff(struct macb *bp)
desc->ctrl = 0;
}
-static void gem_init_rings(struct macb *bp)
+static int gem_init_rings(struct macb *bp, bool fail_early)
{
struct macb_queue *queue;
struct macb_dma_desc *desc = NULL;
+ int last_err = 0;
unsigned int q;
+ int err;
int i;
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
@@ -2597,13 +2605,24 @@ static void gem_init_rings(struct macb *bp)
queue->rx_tail = 0;
queue->rx_prepared_head = 0;
- gem_rx_refill(queue);
+ /* We get called in two cases:
+ * - open: we can propagate alloc errors (so fail early),
+ * - HRESP error: cannot propagate, we attempt to reinit
+ * all queues in case of failure.
+ */
+ err = gem_rx_refill(queue);
+ if (err) {
+ last_err = err;
+ if (fail_early)
+ break;
+ }
}
macb_init_tieoff(bp);
+ return last_err;
}
-static void macb_init_rings(struct macb *bp)
+static int macb_init_rings(struct macb *bp, bool fail_early)
{
int i;
struct macb_dma_desc *desc = NULL;
@@ -2620,6 +2639,7 @@ static void macb_init_rings(struct macb *bp)
desc->ctrl |= MACB_BIT(TX_WRAP);
macb_init_tieoff(bp);
+ return 0;
}
static void macb_reset_hw(struct macb *bp)
@@ -2951,7 +2971,7 @@ static int macb_open(struct net_device *dev)
goto pm_exit;
}
- bp->macbgem_ops.mog_init_rings(bp);
+ bp->macbgem_ops.mog_init_rings(bp, true);
macb_init_buffers(bp);
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
--
2.52.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [net-next,v2,1/8] net: macb: move Rx buffers alloc from link up to open
2026-02-23 18:26 ` [PATCH net-next v2 1/8] net: macb: move Rx buffers alloc from link up to open Paolo Valerio
@ 2026-02-24 0:08 ` Jakub Kicinski
2026-02-25 18:29 ` Paolo Valerio
0 siblings, 1 reply; 20+ messages in thread
From: Jakub Kicinski @ 2026-02-24 0:08 UTC (permalink / raw)
To: pvalerio
Cc: Jakub Kicinski, davem, claudiu.beznea, nicolas.ferre, edumazet,
pabeni, theo.lebrun, netdev, lorenzo, andrew+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.
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: macb: move Rx buffers alloc from link up to open
This commit moves Rx buffer allocation to macb_open() to enable early
error propagation, adding a fail_early parameter to control whether ring
initialization fails fast or continues on error.
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 87414a2ddf6e..2cb65ec37d44 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -1180,7 +1180,7 @@ struct macb_queue;
> struct macb_or_gem_ops {
> int (*mog_alloc_rx_buffers)(struct macb *bp);
> void (*mog_free_rx_buffers)(struct macb *bp);
> - void (*mog_init_rings)(struct macb *bp);
> + int (*mog_init_rings)(struct macb *bp, bool fail_early);
> int (*mog_rx)(struct macb_queue *queue, struct napi_struct *napi,
> int budget);
> };
[ ... ]
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 43cd013bb70e..8cc16f2b4024 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
> @@ -2544,8 +2547,6 @@ static int macb_alloc_consistent(struct macb *bp)
> if (!queue->tx_skb)
> goto out_err;
> }
> - if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
> - goto out_err;
>
> /* Required for tie off descriptor for PM cases */
> if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE)) {
> @@ -2557,6 +2558,11 @@ static int macb_alloc_consistent(struct macb *bp)
> goto out_err;
> }
>
> + if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
> + goto out_err;
> + if (bp->macbgem_ops.mog_init_rings(bp, true))
> + goto out_err;
> +
> return 0;
>
> out_err:
[ ... ]
> @@ -2951,7 +2971,7 @@ static int macb_open(struct net_device *dev)
> goto pm_exit;
> }
>
> - bp->macbgem_ops.mog_init_rings(bp);
> + bp->macbgem_ops.mog_init_rings(bp, true);
> macb_init_buffers(bp);
>
> for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
Is mog_init_rings() being called twice in the macb_open() path? Looking at
the code flow:
macb_open()
macb_alloc_consistent() at line 2967
mog_init_rings(bp, true) at line 2563
mog_init_rings(bp, true) at line 2974
The first call inside macb_alloc_consistent() initializes the rings and
checks the return value. The second call in macb_open() appears to be
redundant: by the time macb_open() reaches line 2974, gem_init_rings() has
already refilled all Rx slots via the earlier call.
When gem_init_rings() runs the second time, CIRC_SPACE returns 0 because
all slots are filled, so gem_rx_refill() effectively does nothing. The
second call reinitializes head/tail pointers that are already set and runs
a no-op refill.
Should this second call be removed to avoid the redundancy?
Additionally, the return value from the second mog_init_rings(bp, true)
call is silently discarded. The stated purpose of this patch is to "fail
early and loudly rather than init the device with Rx mostly broken," but
this call site doesn't check for errors while the call in
macb_alloc_consistent() does.
If the second call is intentional, should its return value be checked?
This was noted in the v2 review by Théo Lebrun (Jan 8, 2026) who pointed
out that commit 99537d5c476c relocated mog_init_rings() from
macb_mac_link_up() to macb_open(), making this second call redundant. Paolo
acknowledged this with "will do" but the duplicate call remains.
--
pw-bot: cr
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next,v2,1/8] net: macb: move Rx buffers alloc from link up to open
2026-02-24 0:08 ` [net-next,v2,1/8] " Jakub Kicinski
@ 2026-02-25 18:29 ` Paolo Valerio
0 siblings, 0 replies; 20+ messages in thread
From: Paolo Valerio @ 2026-02-25 18:29 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jakub Kicinski, davem, claudiu.beznea, nicolas.ferre, edumazet,
pabeni, theo.lebrun, netdev, lorenzo, andrew+netdev
On 23 Feb 2026 at 04:08:32 PM, Jakub Kicinski <kuba@kernel.org> wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> net: macb: move Rx buffers alloc from link up to open
>
> This commit moves Rx buffer allocation to macb_open() to enable early
> error propagation, adding a fail_early parameter to control whether ring
> initialization fails fast or continues on error.
>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 87414a2ddf6e..2cb65ec37d44 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -1180,7 +1180,7 @@ struct macb_queue;
>> struct macb_or_gem_ops {
>> int (*mog_alloc_rx_buffers)(struct macb *bp);
>> void (*mog_free_rx_buffers)(struct macb *bp);
>> - void (*mog_init_rings)(struct macb *bp);
>> + int (*mog_init_rings)(struct macb *bp, bool fail_early);
>> int (*mog_rx)(struct macb_queue *queue, struct napi_struct *napi,
>> int budget);
>> };
>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 43cd013bb70e..8cc16f2b4024 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>
> [ ... ]
>
>> @@ -2544,8 +2547,6 @@ static int macb_alloc_consistent(struct macb *bp)
>> if (!queue->tx_skb)
>> goto out_err;
>> }
>> - if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
>> - goto out_err;
>>
>> /* Required for tie off descriptor for PM cases */
>> if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE)) {
>> @@ -2557,6 +2558,11 @@ static int macb_alloc_consistent(struct macb *bp)
>> goto out_err;
>> }
>>
>> + if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
>> + goto out_err;
>> + if (bp->macbgem_ops.mog_init_rings(bp, true))
>> + goto out_err;
>> +
>> return 0;
>>
>> out_err:
>
> [ ... ]
>
>> @@ -2951,7 +2971,7 @@ static int macb_open(struct net_device *dev)
>> goto pm_exit;
>> }
>>
>> - bp->macbgem_ops.mog_init_rings(bp);
>> + bp->macbgem_ops.mog_init_rings(bp, true);
>> macb_init_buffers(bp);
>>
>> for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>
> Is mog_init_rings() being called twice in the macb_open() path? Looking at
> the code flow:
>
> macb_open()
> macb_alloc_consistent() at line 2967
> mog_init_rings(bp, true) at line 2563
> mog_init_rings(bp, true) at line 2974
>
> The first call inside macb_alloc_consistent() initializes the rings and
> checks the return value. The second call in macb_open() appears to be
> redundant: by the time macb_open() reaches line 2974, gem_init_rings() has
> already refilled all Rx slots via the earlier call.
>
> When gem_init_rings() runs the second time, CIRC_SPACE returns 0 because
> all slots are filled, so gem_rx_refill() effectively does nothing. The
> second call reinitializes head/tail pointers that are already set and runs
> a no-op refill.
>
> Should this second call be removed to avoid the redundancy?
>
> Additionally, the return value from the second mog_init_rings(bp, true)
> call is silently discarded. The stated purpose of this patch is to "fail
> early and loudly rather than init the device with Rx mostly broken," but
> this call site doesn't check for errors while the call in
> macb_alloc_consistent() does.
>
> If the second call is intentional, should its return value be checked?
>
> This was noted in the v2 review by Théo Lebrun (Jan 8, 2026) who pointed
> out that commit 99537d5c476c relocated mog_init_rings() from
> macb_mac_link_up() to macb_open(), making this second call redundant. Paolo
> acknowledged this with "will do" but the duplicate call remains.
yes, this definitely is not intentional. Must have been left/reintroduced
by mistake while rebasing v2, sorry.
> --
> pw-bot: cr
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH net-next v2 2/8] net: macb: rename rx_skbuff into rx_buff
2026-02-23 18:26 [PATCH net-next v2 0/8] net: macb: Add XDP support and page pool integration Paolo Valerio
2026-02-23 18:26 ` [PATCH net-next v2 1/8] net: macb: move Rx buffers alloc from link up to open Paolo Valerio
@ 2026-02-23 18:26 ` Paolo Valerio
2026-02-23 18:26 ` [PATCH net-next v2 3/8] net: macb: Add page pool support handle multi-descriptor frame rx Paolo Valerio
` (5 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Paolo Valerio @ 2026-02-23 18:26 UTC (permalink / raw)
To: netdev
Cc: Nicolas Ferre, Claudiu Beznea, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lorenzo Bianconi,
Théo Lebrun
This is a preparation commit as the field in later patches will no
longer accomomdate skbuffs, but more generic frames.
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
drivers/net/ethernet/cadence/macb.h | 2 +-
drivers/net/ethernet/cadence/macb_main.c | 24 ++++++++++++------------
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 2cb65ec37d44..3b184e9ac771 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1262,7 +1262,7 @@ struct macb_queue {
unsigned int rx_tail;
unsigned int rx_prepared_head;
struct macb_dma_desc *rx_ring;
- struct sk_buff **rx_skbuff;
+ void **rx_buff;
void *rx_buffers;
struct napi_struct napi_rx;
struct queue_stats stats;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 8cc16f2b4024..fef9f3b895dd 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1265,7 +1265,7 @@ static int gem_rx_refill(struct macb_queue *queue)
desc = macb_rx_desc(queue, entry);
- if (!queue->rx_skbuff[entry]) {
+ if (!queue->rx_buff[entry]) {
/* allocate sk_buff for this free entry in ring */
skb = netdev_alloc_skb(bp->dev, bp->rx_buffer_size);
if (unlikely(!skb)) {
@@ -1284,7 +1284,7 @@ static int gem_rx_refill(struct macb_queue *queue)
break;
}
- queue->rx_skbuff[entry] = skb;
+ queue->rx_buff[entry] = skb;
if (entry == bp->rx_ring_size - 1)
paddr |= MACB_BIT(RX_WRAP);
@@ -1387,7 +1387,7 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
queue->stats.rx_dropped++;
break;
}
- skb = queue->rx_skbuff[entry];
+ skb = queue->rx_buff[entry];
if (unlikely(!skb)) {
netdev_err(bp->dev,
"inconsistent Rx descriptor chain\n");
@@ -1396,7 +1396,7 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
break;
}
/* now everything is ready for receiving packet */
- queue->rx_skbuff[entry] = NULL;
+ queue->rx_buff[entry] = NULL;
len = ctrl & bp->rx_frm_len_mask;
netdev_vdbg(bp->dev, "gem_rx %u (len %u)\n", entry, len);
@@ -2395,11 +2395,11 @@ static void gem_free_rx_buffers(struct macb *bp)
int i;
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
- if (!queue->rx_skbuff)
+ if (!queue->rx_buff)
continue;
for (i = 0; i < bp->rx_ring_size; i++) {
- skb = queue->rx_skbuff[i];
+ skb = queue->rx_buff[i];
if (!skb)
continue;
@@ -2413,8 +2413,8 @@ static void gem_free_rx_buffers(struct macb *bp)
skb = NULL;
}
- kfree(queue->rx_skbuff);
- queue->rx_skbuff = NULL;
+ kfree(queue->rx_buff);
+ queue->rx_buff = NULL;
}
}
@@ -2477,13 +2477,13 @@ static int gem_alloc_rx_buffers(struct macb *bp)
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
size = bp->rx_ring_size * sizeof(struct sk_buff *);
- queue->rx_skbuff = kzalloc(size, GFP_KERNEL);
- if (!queue->rx_skbuff)
+ queue->rx_buff = kzalloc(size, GFP_KERNEL);
+ if (!queue->rx_buff)
return -ENOMEM;
else
netdev_dbg(bp->dev,
- "Allocated %d RX struct sk_buff entries at %p\n",
- bp->rx_ring_size, queue->rx_skbuff);
+ "Allocated %d RX buff entries at %p\n",
+ bp->rx_ring_size, queue->rx_buff);
}
return 0;
}
--
2.52.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH net-next v2 3/8] net: macb: Add page pool support handle multi-descriptor frame rx
2026-02-23 18:26 [PATCH net-next v2 0/8] net: macb: Add XDP support and page pool integration Paolo Valerio
2026-02-23 18:26 ` [PATCH net-next v2 1/8] net: macb: move Rx buffers alloc from link up to open Paolo Valerio
2026-02-23 18:26 ` [PATCH net-next v2 2/8] net: macb: rename rx_skbuff into rx_buff Paolo Valerio
@ 2026-02-23 18:26 ` Paolo Valerio
2026-02-23 18:26 ` [PATCH net-next v2 4/8] net: macb: use the current queue number for stats Paolo Valerio
` (4 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Paolo Valerio @ 2026-02-23 18:26 UTC (permalink / raw)
To: netdev
Cc: Nicolas Ferre, Claudiu Beznea, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lorenzo Bianconi,
Théo Lebrun
Use the page pool allocator for the data buffers and enable skb
recycling support, instead of relying on netdev_alloc_skb
allocating the entire skb during the refill.
The patch also add support for receiving network frames that span
multiple DMA descriptors in the Cadence MACB/GEM Ethernet driver.
The patch removes the requirement that limited frame reception to
a single descriptor (RX_SOF && RX_EOF), also avoiding potential
contiguous multi-page allocation for large frames.
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
drivers/net/ethernet/cadence/Kconfig | 1 +
drivers/net/ethernet/cadence/macb.h | 5 +
drivers/net/ethernet/cadence/macb_main.c | 371 +++++++++++++++--------
3 files changed, 254 insertions(+), 123 deletions(-)
diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index 5b2a461dfd28..ae500f717433 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -25,6 +25,7 @@ config MACB
depends on PTP_1588_CLOCK_OPTIONAL
select PHYLINK
select CRC32
+ select PAGE_POOL
help
The Cadence MACB ethernet interface is found on many Atmel AT32 and
AT91 parts. This driver also supports the Cadence GEM (Gigabit
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 3b184e9ac771..a78ad00f53b1 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -14,6 +14,7 @@
#include <linux/interrupt.h>
#include <linux/phy/phy.h>
#include <linux/workqueue.h>
+#include <net/page_pool/helpers.h>
#define MACB_GREGS_NBR 16
#define MACB_GREGS_VERSION 2
@@ -1266,6 +1267,8 @@ struct macb_queue {
void *rx_buffers;
struct napi_struct napi_rx;
struct queue_stats stats;
+ struct page_pool *page_pool;
+ struct sk_buff *skb;
};
struct ethtool_rx_fs_item {
@@ -1289,6 +1292,8 @@ struct macb {
struct macb_dma_desc *rx_ring_tieoff;
dma_addr_t rx_ring_tieoff_dma;
size_t rx_buffer_size;
+ size_t rx_headroom;
+ unsigned int rx_ip_align;
unsigned int rx_ring_size;
unsigned int tx_ring_size;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index fef9f3b895dd..d42a49a25a62 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1247,14 +1247,22 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
return packets;
}
-static int gem_rx_refill(struct macb_queue *queue)
+static unsigned int gem_total_rx_buffer_size(struct macb *bp)
+{
+ return SKB_HEAD_ALIGN(bp->rx_buffer_size + bp->rx_headroom);
+}
+
+static int gem_rx_refill(struct macb_queue *queue, bool napi)
{
- unsigned int entry;
- struct sk_buff *skb;
- dma_addr_t paddr;
struct macb *bp = queue->bp;
struct macb_dma_desc *desc;
+ unsigned int entry;
+ struct page *page;
+ dma_addr_t paddr;
+ gfp_t gfp_alloc;
int err = 0;
+ void *data;
+ int offset;
while (CIRC_SPACE(queue->rx_prepared_head, queue->rx_tail,
bp->rx_ring_size) > 0) {
@@ -1266,25 +1274,26 @@ static int gem_rx_refill(struct macb_queue *queue)
desc = macb_rx_desc(queue, entry);
if (!queue->rx_buff[entry]) {
- /* allocate sk_buff for this free entry in ring */
- skb = netdev_alloc_skb(bp->dev, bp->rx_buffer_size);
- if (unlikely(!skb)) {
+ gfp_alloc = napi ? GFP_ATOMIC : GFP_KERNEL;
+ page = page_pool_alloc_frag(queue->page_pool, &offset,
+ gem_total_rx_buffer_size(bp),
+ gfp_alloc | __GFP_NOWARN);
+ if (!page) {
netdev_err(bp->dev,
- "Unable to allocate sk_buff\n");
+ "Unable to allocate rx buffer\n");
err = -ENOMEM;
break;
}
- /* now fill corresponding descriptor entry */
- paddr = dma_map_single(&bp->pdev->dev, skb->data,
- bp->rx_buffer_size,
- DMA_FROM_DEVICE);
- if (dma_mapping_error(&bp->pdev->dev, paddr)) {
- dev_kfree_skb(skb);
- break;
- }
+ paddr = page_pool_get_dma_addr(page) + NET_SKB_PAD + offset;
- queue->rx_buff[entry] = skb;
+ dma_sync_single_for_device(&bp->pdev->dev,
+ paddr + bp->rx_ip_align,
+ bp->rx_buffer_size,
+ page_pool_get_dma_dir(queue->page_pool));
+
+ data = page_address(page) + offset;
+ queue->rx_buff[entry] = data;
if (entry == bp->rx_ring_size - 1)
paddr |= MACB_BIT(RX_WRAP);
@@ -1294,20 +1303,6 @@ static int gem_rx_refill(struct macb_queue *queue)
*/
dma_wmb();
macb_set_addr(bp, desc, paddr);
-
- /* Properly align Ethernet header.
- *
- * Hardware can add dummy bytes if asked using the RBOF
- * field inside the NCFGR register. That feature isn't
- * available if hardware is RSC capable.
- *
- * We cannot fallback to doing the 2-byte shift before
- * DMA mapping because the address field does not allow
- * setting the low 2/3 bits.
- * It is 3 bits if HW_DMA_CAP_PTP, else 2 bits.
- */
- if (!(bp->caps & MACB_CAPS_RSC))
- skb_reserve(skb, NET_IP_ALIGN);
} else {
desc->ctrl = 0;
dma_wmb();
@@ -1348,17 +1343,21 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
int budget)
{
+ struct skb_shared_info *shinfo;
struct macb *bp = queue->bp;
- unsigned int len;
- unsigned int entry;
- struct sk_buff *skb;
- struct macb_dma_desc *desc;
- int count = 0;
+ struct macb_dma_desc *desc;
+ unsigned int entry;
+ struct page *page;
+ void *buff_head;
+ int count = 0;
+ int data_len;
+ int nr_frags;
+
while (count < budget) {
- u32 ctrl;
+ bool rxused, first_frame, last_frame;
dma_addr_t addr;
- bool rxused;
+ u32 ctrl;
entry = macb_rx_ring_wrap(bp, queue->rx_tail);
desc = macb_rx_desc(queue, entry);
@@ -1380,58 +1379,119 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
queue->rx_tail++;
count++;
- if (!(ctrl & MACB_BIT(RX_SOF) && ctrl & MACB_BIT(RX_EOF))) {
+ buff_head = queue->rx_buff[entry];
+ if (unlikely(!buff_head)) {
netdev_err(bp->dev,
- "not whole frame pointed by descriptor\n");
+ "inconsistent Rx descriptor chain\n");
bp->dev->stats.rx_dropped++;
queue->stats.rx_dropped++;
break;
}
- skb = queue->rx_buff[entry];
- if (unlikely(!skb)) {
+
+ first_frame = ctrl & MACB_BIT(RX_SOF);
+ last_frame = ctrl & MACB_BIT(RX_EOF);
+
+ if (!first_frame && !queue->skb) {
netdev_err(bp->dev,
- "inconsistent Rx descriptor chain\n");
- bp->dev->stats.rx_dropped++;
- queue->stats.rx_dropped++;
- break;
+ "Received non-starting frame while expecting a starting one\n");
+ continue;
}
- /* now everything is ready for receiving packet */
- queue->rx_buff[entry] = NULL;
- len = ctrl & bp->rx_frm_len_mask;
- netdev_vdbg(bp->dev, "gem_rx %u (len %u)\n", entry, len);
+ data_len = last_frame ? (ctrl & bp->rx_frm_len_mask) : bp->rx_buffer_size;
+
+ if (last_frame && !first_frame)
+ data_len -= queue->skb->len;
+
+ dma_sync_single_for_cpu(&bp->pdev->dev, addr + bp->rx_ip_align,
+ data_len,
+ page_pool_get_dma_dir(queue->page_pool));
+
+ if (first_frame) {
+ queue->skb = napi_build_skb(buff_head, gem_total_rx_buffer_size(bp));
+ if (unlikely(!queue->skb)) {
+ netdev_err(bp->dev,
+ "Unable to allocate sk_buff\n");
+ goto free_frags;
+ }
+
+ /* Properly align Ethernet header.
+ *
+ * Hardware can add dummy bytes if asked using the RBOF
+ * field inside the NCFGR register. That feature isn't
+ * available if hardware is RSC capable.
+ *
+ * We cannot fallback to doing the 2-byte shift before
+ * DMA mapping because the address field does not allow
+ * setting the low 2/3 bits.
+ * It is 3 bits if HW_DMA_CAP_PTP, else 2 bits.
+ */
+ skb_reserve(queue->skb, bp->rx_headroom);
+ skb_mark_for_recycle(queue->skb);
+ skb_put(queue->skb, data_len);
+ queue->skb->protocol = eth_type_trans(queue->skb, bp->dev);
+ } else {
+ shinfo = skb_shinfo(queue->skb);
+ page = virt_to_head_page(buff_head);
+ nr_frags = shinfo->nr_frags;
+
+ if (nr_frags >= ARRAY_SIZE(shinfo->frags))
+ goto free_frags;
- skb_put(skb, len);
- dma_unmap_single(&bp->pdev->dev, addr,
- bp->rx_buffer_size, DMA_FROM_DEVICE);
+ skb_add_rx_frag(queue->skb, nr_frags, page,
+ buff_head - page_address(page) + bp->rx_headroom,
+ data_len, gem_total_rx_buffer_size(bp));
+ }
- skb->protocol = eth_type_trans(skb, bp->dev);
- skb_checksum_none_assert(skb);
- if (bp->dev->features & NETIF_F_RXCSUM &&
- !(bp->dev->flags & IFF_PROMISC) &&
- GEM_BFEXT(RX_CSUM, ctrl) & GEM_RX_CSUM_CHECKED_MASK)
- skb->ip_summed = CHECKSUM_UNNECESSARY;
+ /* now everything is ready for receiving packet */
+ queue->rx_buff[entry] = NULL;
- bp->dev->stats.rx_packets++;
- queue->stats.rx_packets++;
- bp->dev->stats.rx_bytes += skb->len;
- queue->stats.rx_bytes += skb->len;
+ netdev_vdbg(bp->dev, "%s %u (len %u)\n", __func__, entry, data_len);
- gem_ptp_do_rxstamp(bp, skb, desc);
+ if (last_frame) {
+ bp->dev->stats.rx_packets++;
+ queue->stats.rx_packets++;
+ bp->dev->stats.rx_bytes += queue->skb->len;
+ queue->stats.rx_bytes += queue->skb->len;
+ skb_checksum_none_assert(queue->skb);
+ if (bp->dev->features & NETIF_F_RXCSUM &&
+ !(bp->dev->flags & IFF_PROMISC) &&
+ GEM_BFEXT(RX_CSUM, ctrl) & GEM_RX_CSUM_CHECKED_MASK)
+ queue->skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+ gem_ptp_do_rxstamp(bp, queue->skb, desc);
#if defined(DEBUG) && defined(VERBOSE_DEBUG)
- netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
- skb->len, skb->csum);
- print_hex_dump(KERN_DEBUG, " mac: ", DUMP_PREFIX_ADDRESS, 16, 1,
- skb_mac_header(skb), 16, true);
- print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_ADDRESS, 16, 1,
- skb->data, 32, true);
+ netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
+ queue->skb->len, queue->skb->csum);
+ print_hex_dump_debug(" mac: ", DUMP_PREFIX_ADDRESS, 16, 1,
+ skb_mac_header(queue->skb), 16, true);
+ print_hex_dump_debug("data: ", DUMP_PREFIX_ADDRESS, 16, 1,
+ queue->skb->data, 32, true);
#endif
- napi_gro_receive(napi, skb);
+ napi_gro_receive(napi, queue->skb);
+ queue->skb = NULL;
+ }
+
+ continue;
+
+free_frags:
+ if (queue->skb) {
+ dev_kfree_skb(queue->skb);
+ queue->skb = NULL;
+ }
+
+ if (buff_head)
+ page_pool_put_full_page(queue->page_pool,
+ virt_to_head_page(buff_head),
+ false);
+
+ bp->dev->stats.rx_dropped++;
+ queue->stats.rx_dropped++;
+ queue->rx_buff[entry] = NULL;
}
- gem_rx_refill(queue);
+ gem_rx_refill(queue, true);
return count;
}
@@ -2365,12 +2425,22 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
return ret;
}
-static void macb_init_rx_buffer_size(struct macb *bp, size_t size)
+static void macb_init_rx_buffer_size(struct macb *bp, unsigned int mtu)
{
+ unsigned int overhead;
+ size_t size;
+
if (!macb_is_gem(bp)) {
bp->rx_buffer_size = MACB_RX_BUFFER_SIZE;
} else {
- bp->rx_buffer_size = size;
+ size = mtu + ETH_HLEN + ETH_FCS_LEN;
+ bp->rx_buffer_size = SKB_DATA_ALIGN(size);
+ if (gem_total_rx_buffer_size(bp) > PAGE_SIZE) {
+ overhead = bp->rx_headroom +
+ SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+ bp->rx_buffer_size = rounddown(PAGE_SIZE - overhead,
+ RX_BUFFER_MULTIPLE);
+ }
if (bp->rx_buffer_size % RX_BUFFER_MULTIPLE) {
netdev_dbg(bp->dev,
@@ -2381,17 +2451,16 @@ static void macb_init_rx_buffer_size(struct macb *bp, size_t size)
}
}
- netdev_dbg(bp->dev, "mtu [%u] rx_buffer_size [%zu]\n",
- bp->dev->mtu, bp->rx_buffer_size);
+ netdev_dbg(bp->dev, "mtu [%u] rx_buffer_size [%zu] rx_headroom [%zu] total [%u]\n",
+ bp->dev->mtu, bp->rx_buffer_size, bp->rx_headroom,
+ gem_total_rx_buffer_size(bp));
}
static void gem_free_rx_buffers(struct macb *bp)
{
- struct sk_buff *skb;
- struct macb_dma_desc *desc;
struct macb_queue *queue;
- dma_addr_t addr;
unsigned int q;
+ void *data;
int i;
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
@@ -2399,22 +2468,25 @@ static void gem_free_rx_buffers(struct macb *bp)
continue;
for (i = 0; i < bp->rx_ring_size; i++) {
- skb = queue->rx_buff[i];
-
- if (!skb)
+ data = queue->rx_buff[i];
+ if (!data)
continue;
- desc = macb_rx_desc(queue, i);
- addr = macb_get_addr(bp, desc);
+ page_pool_put_full_page(queue->page_pool,
+ virt_to_head_page(data),
+ false);
+ queue->rx_buff[i] = NULL;
+ }
- dma_unmap_single(&bp->pdev->dev, addr, bp->rx_buffer_size,
- DMA_FROM_DEVICE);
- dev_kfree_skb_any(skb);
- skb = NULL;
+ if (queue->skb) {
+ dev_kfree_skb(queue->skb);
+ queue->skb = NULL;
}
kfree(queue->rx_buff);
queue->rx_buff = NULL;
+ page_pool_destroy(queue->page_pool);
+ queue->page_pool = NULL;
}
}
@@ -2476,13 +2548,12 @@ static int gem_alloc_rx_buffers(struct macb *bp)
int size;
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
- size = bp->rx_ring_size * sizeof(struct sk_buff *);
+ size = bp->rx_ring_size * sizeof(*queue->rx_buff);
queue->rx_buff = kzalloc(size, GFP_KERNEL);
if (!queue->rx_buff)
return -ENOMEM;
else
- netdev_dbg(bp->dev,
- "Allocated %d RX buff entries at %p\n",
+ netdev_dbg(bp->dev, "Allocated %d RX buff entries at %p\n",
bp->rx_ring_size, queue->rx_buff);
}
return 0;
@@ -2570,6 +2641,40 @@ static int macb_alloc_consistent(struct macb *bp)
return -ENOMEM;
}
+static int gem_create_page_pool(struct macb_queue *queue)
+{
+ struct page_pool_params pp_params = {
+ .order = 0,
+ .flags = PP_FLAG_DMA_MAP,
+ .pool_size = queue->bp->rx_ring_size,
+ .nid = NUMA_NO_NODE,
+ .dma_dir = DMA_FROM_DEVICE,
+ .dev = &queue->bp->pdev->dev,
+ .netdev = queue->bp->dev,
+ .napi = &queue->napi_rx,
+ .max_len = PAGE_SIZE,
+ };
+ struct page_pool *pool;
+ int err = 0;
+
+ /* This can happen in the case of HRESP error.
+ * Do nothing as page pool is already existing.
+ */
+ if (queue->page_pool)
+ return err;
+
+ pool = page_pool_create(&pp_params);
+ if (IS_ERR(pool)) {
+ netdev_err(queue->bp->dev, "cannot create rx page pool\n");
+ err = PTR_ERR(pool);
+ pool = NULL;
+ }
+
+ queue->page_pool = pool;
+
+ return err;
+}
+
static void macb_init_tieoff(struct macb *bp)
{
struct macb_dma_desc *desc = bp->rx_ring_tieoff;
@@ -2605,12 +2710,24 @@ static int gem_init_rings(struct macb *bp, bool fail_early)
queue->rx_tail = 0;
queue->rx_prepared_head = 0;
+ /* This is a hard failure, so the best we can do is try the
+ * next queue in case of HRESP error.
+ */
+ err = gem_create_page_pool(queue);
+ if (err) {
+ last_err = err;
+ if (fail_early)
+ break;
+
+ continue;
+ }
+
/* We get called in two cases:
* - open: we can propagate alloc errors (so fail early),
* - HRESP error: cannot propagate, we attempt to reinit
* all queues in case of failure.
*/
- err = gem_rx_refill(queue);
+ err = gem_rx_refill(queue, false);
if (err) {
last_err = err;
if (fail_early)
@@ -2754,39 +2871,40 @@ static void macb_configure_dma(struct macb *bp)
unsigned int q;
u32 dmacfg;
- buffer_size = bp->rx_buffer_size / RX_BUFFER_MULTIPLE;
- if (macb_is_gem(bp)) {
- dmacfg = gem_readl(bp, DMACFG) & ~GEM_BF(RXBS, -1L);
- for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
- if (q)
- queue_writel(queue, RBQS, buffer_size);
- else
- dmacfg |= GEM_BF(RXBS, buffer_size);
- }
- if (bp->dma_burst_length)
- dmacfg = GEM_BFINS(FBLDO, bp->dma_burst_length, dmacfg);
- dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L);
- dmacfg &= ~GEM_BIT(ENDIA_PKT);
+ if (!macb_is_gem((bp)))
+ return;
- if (bp->native_io)
- dmacfg &= ~GEM_BIT(ENDIA_DESC);
+ buffer_size = bp->rx_buffer_size / RX_BUFFER_MULTIPLE;
+ dmacfg = gem_readl(bp, DMACFG) & ~GEM_BF(RXBS, -1L);
+ for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
+ if (q)
+ queue_writel(queue, RBQS, buffer_size);
else
- dmacfg |= GEM_BIT(ENDIA_DESC); /* CPU in big endian */
+ dmacfg |= GEM_BF(RXBS, buffer_size);
+ }
+ if (bp->dma_burst_length)
+ dmacfg = GEM_BFINS(FBLDO, bp->dma_burst_length, dmacfg);
+ dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L);
+ dmacfg &= ~GEM_BIT(ENDIA_PKT);
- if (bp->dev->features & NETIF_F_HW_CSUM)
- dmacfg |= GEM_BIT(TXCOEN);
- else
- dmacfg &= ~GEM_BIT(TXCOEN);
+ if (bp->native_io)
+ dmacfg &= ~GEM_BIT(ENDIA_DESC);
+ else
+ dmacfg |= GEM_BIT(ENDIA_DESC); /* CPU in big endian */
- dmacfg &= ~GEM_BIT(ADDR64);
- if (macb_dma64(bp))
- dmacfg |= GEM_BIT(ADDR64);
- if (macb_dma_ptp(bp))
- dmacfg |= GEM_BIT(RXEXT) | GEM_BIT(TXEXT);
- netdev_dbg(bp->dev, "Cadence configure DMA with 0x%08x\n",
- dmacfg);
- gem_writel(bp, DMACFG, dmacfg);
- }
+ if (bp->dev->features & NETIF_F_HW_CSUM)
+ dmacfg |= GEM_BIT(TXCOEN);
+ else
+ dmacfg &= ~GEM_BIT(TXCOEN);
+
+ dmacfg &= ~GEM_BIT(ADDR64);
+ if (macb_dma64(bp))
+ dmacfg |= GEM_BIT(ADDR64);
+ if (macb_dma_ptp(bp))
+ dmacfg |= GEM_BIT(RXEXT) | GEM_BIT(TXEXT);
+ netdev_dbg(bp->dev, "Cadence configure DMA with 0x%08x\n",
+ dmacfg);
+ gem_writel(bp, DMACFG, dmacfg);
}
static void macb_init_hw(struct macb *bp)
@@ -2949,7 +3067,6 @@ static void macb_set_rx_mode(struct net_device *dev)
static int macb_open(struct net_device *dev)
{
- size_t bufsz = dev->mtu + ETH_HLEN + ETH_FCS_LEN + NET_IP_ALIGN;
struct macb *bp = netdev_priv(dev);
struct macb_queue *queue;
unsigned int q;
@@ -2962,7 +3079,7 @@ static int macb_open(struct net_device *dev)
return err;
/* RX buffers initialization */
- macb_init_rx_buffer_size(bp, bufsz);
+ macb_init_rx_buffer_size(bp, dev->mtu);
err = macb_alloc_consistent(bp);
if (err) {
@@ -5627,6 +5744,14 @@ static int macb_probe(struct platform_device *pdev)
if (err)
goto err_out_phy_exit;
+ if (macb_is_gem(bp)) {
+ bp->rx_headroom = NET_SKB_PAD;
+ if (!(bp->caps & MACB_CAPS_RSC)) {
+ bp->rx_ip_align = NET_IP_ALIGN;
+ bp->rx_headroom += NET_IP_ALIGN;
+ }
+ }
+
netif_carrier_off(dev);
err = register_netdev(dev);
--
2.52.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH net-next v2 4/8] net: macb: use the current queue number for stats
2026-02-23 18:26 [PATCH net-next v2 0/8] net: macb: Add XDP support and page pool integration Paolo Valerio
` (2 preceding siblings ...)
2026-02-23 18:26 ` [PATCH net-next v2 3/8] net: macb: Add page pool support handle multi-descriptor frame rx Paolo Valerio
@ 2026-02-23 18:26 ` Paolo Valerio
2026-02-23 18:26 ` [PATCH net-next v2 5/8] net: macb: add XDP support for gem Paolo Valerio
` (3 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Paolo Valerio @ 2026-02-23 18:26 UTC (permalink / raw)
To: netdev
Cc: Nicolas Ferre, Claudiu Beznea, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lorenzo Bianconi,
Théo Lebrun
gem_get_ethtool_stats calculates the size of the statistics
data to copy always considering maximum number of queues.
The patch makes sure the statistics are copied only for the
active queues as returned in the string set count op.
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
drivers/net/ethernet/cadence/macb_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d42a49a25a62..708107e47ae3 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3264,7 +3264,7 @@ static void gem_get_ethtool_stats(struct net_device *dev,
spin_lock_irq(&bp->stats_lock);
gem_update_stats(bp);
memcpy(data, &bp->ethtool_stats, sizeof(u64)
- * (GEM_STATS_LEN + QUEUE_STATS_LEN * MACB_MAX_QUEUES));
+ * (GEM_STATS_LEN + QUEUE_STATS_LEN * bp->num_queues));
spin_unlock_irq(&bp->stats_lock);
}
--
2.52.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH net-next v2 5/8] net: macb: add XDP support for gem
2026-02-23 18:26 [PATCH net-next v2 0/8] net: macb: Add XDP support and page pool integration Paolo Valerio
` (3 preceding siblings ...)
2026-02-23 18:26 ` [PATCH net-next v2 4/8] net: macb: use the current queue number for stats Paolo Valerio
@ 2026-02-23 18:26 ` Paolo Valerio
2026-02-23 23:23 ` kernel test robot
2026-02-24 0:08 ` [net-next,v2,5/8] " Jakub Kicinski
2026-02-23 18:26 ` [PATCH net-next v2 6/8] net: macb: make macb_tx_skb generic Paolo Valerio
` (2 subsequent siblings)
7 siblings, 2 replies; 20+ messages in thread
From: Paolo Valerio @ 2026-02-23 18:26 UTC (permalink / raw)
To: netdev
Cc: Nicolas Ferre, Claudiu Beznea, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lorenzo Bianconi,
Théo Lebrun
Introduce basic XDP support for macb/gem with the XDP_PASS,
XDP_DROP, XDP_REDIRECT verdict support.
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
drivers/net/ethernet/cadence/macb.h | 3 +
drivers/net/ethernet/cadence/macb_main.c | 207 ++++++++++++++++++++---
2 files changed, 188 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index a78ad00f53b1..33a963b6dd4d 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -15,6 +15,7 @@
#include <linux/phy/phy.h>
#include <linux/workqueue.h>
#include <net/page_pool/helpers.h>
+#include <net/xdp.h>
#define MACB_GREGS_NBR 16
#define MACB_GREGS_VERSION 2
@@ -1269,6 +1270,7 @@ struct macb_queue {
struct queue_stats stats;
struct page_pool *page_pool;
struct sk_buff *skb;
+ struct xdp_rxq_info xdp_rxq;
};
struct ethtool_rx_fs_item {
@@ -1369,6 +1371,7 @@ struct macb {
struct macb_pm_data pm_data;
const struct macb_usrio_config *usrio;
+ struct bpf_prog __rcu *prog;
};
#ifdef CONFIG_MACB_USE_HWSTAMP
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 708107e47ae3..26b517ed251c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -6,6 +6,7 @@
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/bpf_trace.h>
#include <linux/circ_buf.h>
#include <linux/clk-provider.h>
#include <linux/clk.h>
@@ -1247,9 +1248,27 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
return packets;
}
+static unsigned int gem_rx_headroom(struct macb *bp)
+{
+ if (rcu_access_pointer(bp->prog))
+ return XDP_PACKET_HEADROOM;
+
+ return NET_SKB_PAD;
+}
+
+static unsigned int gem_max_rx_data_size(int base_sz)
+{
+ return SKB_DATA_ALIGN(base_sz + ETH_HLEN + ETH_FCS_LEN);
+}
+
+static unsigned int gem_max_rx_buffer_size(int data_sz, struct macb *bp)
+{
+ return SKB_HEAD_ALIGN(data_sz + bp->rx_headroom);
+}
+
static unsigned int gem_total_rx_buffer_size(struct macb *bp)
{
- return SKB_HEAD_ALIGN(bp->rx_buffer_size + bp->rx_headroom);
+ return gem_max_rx_buffer_size(bp->rx_buffer_size, bp);
}
static int gem_rx_refill(struct macb_queue *queue, bool napi)
@@ -1285,7 +1304,8 @@ static int gem_rx_refill(struct macb_queue *queue, bool napi)
break;
}
- paddr = page_pool_get_dma_addr(page) + NET_SKB_PAD + offset;
+ paddr = page_pool_get_dma_addr(page) +
+ gem_rx_headroom(bp) + offset;
dma_sync_single_for_device(&bp->pdev->dev,
paddr + bp->rx_ip_align,
@@ -1340,12 +1360,61 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
*/
}
+static u32 gem_xdp_run(struct macb_queue *queue, void *buff_head,
+ unsigned int len)
+{
+ struct net_device *dev;
+ struct bpf_prog *prog;
+ struct xdp_buff xdp;
+
+ u32 act = XDP_PASS;
+
+ rcu_read_lock();
+
+ prog = rcu_dereference(queue->bp->prog);
+ if (!prog)
+ goto out;
+
+ xdp_init_buff(&xdp, gem_total_rx_buffer_size(queue->bp), &queue->xdp_rxq);
+ xdp_prepare_buff(&xdp, buff_head, queue->bp->rx_headroom, len, false);
+ xdp_buff_clear_frags_flag(&xdp);
+ dev = queue->bp->dev;
+
+ act = bpf_prog_run_xdp(prog, &xdp);
+ switch (act) {
+ case XDP_PASS:
+ goto out;
+ case XDP_REDIRECT:
+ if (unlikely(xdp_do_redirect(dev, &xdp, prog))) {
+ act = XDP_DROP;
+ break;
+ }
+ goto out;
+ default:
+ bpf_warn_invalid_xdp_action(dev, prog, act);
+ fallthrough;
+ case XDP_ABORTED:
+ trace_xdp_exception(dev, prog, act);
+ fallthrough;
+ case XDP_DROP:
+ break;
+ }
+
+ page_pool_put_full_page(queue->page_pool,
+ virt_to_head_page(xdp.data), true);
+out:
+ rcu_read_unlock();
+
+ return act;
+}
+
static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
int budget)
{
struct skb_shared_info *shinfo;
struct macb *bp = queue->bp;
struct macb_dma_desc *desc;
+ bool xdp_flush = false;
unsigned int entry;
struct page *page;
void *buff_head;
@@ -1353,11 +1422,11 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
int data_len;
int nr_frags;
-
while (count < budget) {
bool rxused, first_frame, last_frame;
dma_addr_t addr;
u32 ctrl;
+ u32 ret;
entry = macb_rx_ring_wrap(bp, queue->rx_tail);
desc = macb_rx_desc(queue, entry);
@@ -1407,6 +1476,15 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
page_pool_get_dma_dir(queue->page_pool));
if (first_frame) {
+ if (last_frame) {
+ ret = gem_xdp_run(queue, buff_head, data_len);
+ if (ret == XDP_REDIRECT)
+ xdp_flush = true;
+
+ if (ret != XDP_PASS)
+ goto next_frame;
+ }
+
queue->skb = napi_build_skb(buff_head, gem_total_rx_buffer_size(bp));
if (unlikely(!queue->skb)) {
netdev_err(bp->dev,
@@ -1443,10 +1521,6 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
}
/* now everything is ready for receiving packet */
- queue->rx_buff[entry] = NULL;
-
- netdev_vdbg(bp->dev, "%s %u (len %u)\n", __func__, entry, data_len);
-
if (last_frame) {
bp->dev->stats.rx_packets++;
queue->stats.rx_packets++;
@@ -1473,6 +1547,8 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
queue->skb = NULL;
}
+next_frame:
+ queue->rx_buff[entry] = NULL;
continue;
free_frags:
@@ -1491,6 +1567,9 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
queue->rx_buff[entry] = NULL;
}
+ if (xdp_flush)
+ xdp_do_flush();
+
gem_rx_refill(queue, true);
return count;
@@ -2428,13 +2507,15 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
static void macb_init_rx_buffer_size(struct macb *bp, unsigned int mtu)
{
unsigned int overhead;
- size_t size;
if (!macb_is_gem(bp)) {
bp->rx_buffer_size = MACB_RX_BUFFER_SIZE;
} else {
- size = mtu + ETH_HLEN + ETH_FCS_LEN;
- bp->rx_buffer_size = SKB_DATA_ALIGN(size);
+ bp->rx_headroom = gem_rx_headroom(bp);
+ bp->rx_ip_align = (!(bp->caps & MACB_CAPS_RSC)) ? NET_IP_ALIGN : 0;
+ bp->rx_headroom += bp->rx_ip_align;
+
+ bp->rx_buffer_size = gem_max_rx_data_size(mtu);
if (gem_total_rx_buffer_size(bp) > PAGE_SIZE) {
overhead = bp->rx_headroom +
SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
@@ -2485,6 +2566,8 @@ static void gem_free_rx_buffers(struct macb *bp)
kfree(queue->rx_buff);
queue->rx_buff = NULL;
+ if (xdp_rxq_info_is_reg(&queue->xdp_rxq))
+ xdp_rxq_info_unreg(&queue->xdp_rxq);
page_pool_destroy(queue->page_pool);
queue->page_pool = NULL;
}
@@ -2641,21 +2724,23 @@ static int macb_alloc_consistent(struct macb *bp)
return -ENOMEM;
}
-static int gem_create_page_pool(struct macb_queue *queue)
+static int gem_create_page_pool(struct macb_queue *queue, int qid)
{
struct page_pool_params pp_params = {
.order = 0,
.flags = PP_FLAG_DMA_MAP,
.pool_size = queue->bp->rx_ring_size,
.nid = NUMA_NO_NODE,
- .dma_dir = DMA_FROM_DEVICE,
+ .dma_dir = rcu_access_pointer(queue->bp->prog)
+ ? DMA_BIDIRECTIONAL
+ : DMA_FROM_DEVICE,
.dev = &queue->bp->pdev->dev,
.netdev = queue->bp->dev,
.napi = &queue->napi_rx,
.max_len = PAGE_SIZE,
};
struct page_pool *pool;
- int err = 0;
+ int err;
/* This can happen in the case of HRESP error.
* Do nothing as page pool is already existing.
@@ -2667,11 +2752,34 @@ static int gem_create_page_pool(struct macb_queue *queue)
if (IS_ERR(pool)) {
netdev_err(queue->bp->dev, "cannot create rx page pool\n");
err = PTR_ERR(pool);
- pool = NULL;
+ goto clear_pool;
}
queue->page_pool = pool;
+ err = xdp_rxq_info_reg(&queue->xdp_rxq, queue->bp->dev, qid,
+ queue->napi_rx.napi_id);
+ if (err < 0) {
+ netdev_err(queue->bp->dev, "xdp: failed to register rxq info\n");
+ goto destroy_pool;
+ }
+
+ err = xdp_rxq_info_reg_mem_model(&queue->xdp_rxq, MEM_TYPE_PAGE_POOL,
+ queue->page_pool);
+ if (err) {
+ netdev_err(queue->bp->dev, "xdp: failed to register rxq memory model\n");
+ goto unreg_info;
+ }
+
+ return 0;
+
+unreg_info:
+ xdp_rxq_info_unreg(&queue->xdp_rxq);
+destroy_pool:
+ page_pool_destroy(pool);
+clear_pool:
+ queue->page_pool = NULL;
+
return err;
}
@@ -2713,7 +2821,7 @@ static int gem_init_rings(struct macb *bp, bool fail_early)
/* This is a hard failure, so the best we can do is try the
* next queue in case of HRESP error.
*/
- err = gem_create_page_pool(queue);
+ err = gem_create_page_pool(queue, q);
if (err) {
last_err = err;
if (fail_early)
@@ -3167,11 +3275,27 @@ static int macb_close(struct net_device *dev)
return 0;
}
+static bool gem_xdp_valid_mtu(struct macb *bp, int mtu)
+{
+ int max_frame_size;
+
+ max_frame_size = gem_max_rx_buffer_size(gem_max_rx_data_size(mtu), bp);
+
+ return max_frame_size <= PAGE_SIZE;
+}
+
static int macb_change_mtu(struct net_device *dev, int new_mtu)
{
+ struct macb *bp = netdev_priv(dev);
+
if (netif_running(dev))
return -EBUSY;
+ if (rcu_access_pointer(bp->prog) && !gem_xdp_valid_mtu(bp, new_mtu)) {
+ netdev_err(dev, "MTU %d too large for XDP", new_mtu);
+ return -EINVAL;
+ }
+
WRITE_ONCE(dev->mtu, new_mtu);
return 0;
@@ -3189,6 +3313,48 @@ static int macb_set_mac_addr(struct net_device *dev, void *addr)
return 0;
}
+static int gem_xdp_setup(struct net_device *dev, struct bpf_prog *prog,
+ struct netlink_ext_ack *extack)
+{
+ struct macb *bp = netdev_priv(dev);
+ struct bpf_prog *old_prog;
+ bool need_update, running;
+
+ if (prog && !gem_xdp_valid_mtu(bp, dev->mtu)) {
+ NL_SET_ERR_MSG_MOD(extack, "MTU too large for XDP");
+ return -EOPNOTSUPP;
+ }
+
+ running = netif_running(dev);
+ need_update = !!bp->prog != !!prog;
+ if (running && need_update)
+ macb_close(dev);
+
+ old_prog = rcu_replace_pointer(bp->prog, prog, lockdep_rtnl_is_held());
+ if (old_prog)
+ bpf_prog_put(old_prog);
+
+ if (running && need_update)
+ return macb_open(dev);
+
+ return 0;
+}
+
+static int gem_xdp(struct net_device *dev, struct netdev_bpf *xdp)
+{
+ struct macb *bp = netdev_priv(dev);
+
+ if (!macb_is_gem(bp))
+ return -EOPNOTSUPP;
+
+ switch (xdp->command) {
+ case XDP_SETUP_PROG:
+ return gem_xdp_setup(dev, xdp->prog, xdp->extack);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
static void gem_update_stats(struct macb *bp)
{
struct macb_queue *queue;
@@ -4447,6 +4613,7 @@ static const struct net_device_ops macb_netdev_ops = {
.ndo_hwtstamp_set = macb_hwtstamp_set,
.ndo_hwtstamp_get = macb_hwtstamp_get,
.ndo_setup_tc = macb_setup_tc,
+ .ndo_bpf = gem_xdp,
};
/* Configure peripheral capabilities according to device tree
@@ -5744,13 +5911,9 @@ static int macb_probe(struct platform_device *pdev)
if (err)
goto err_out_phy_exit;
- if (macb_is_gem(bp)) {
- bp->rx_headroom = NET_SKB_PAD;
- if (!(bp->caps & MACB_CAPS_RSC)) {
- bp->rx_ip_align = NET_IP_ALIGN;
- bp->rx_headroom += NET_IP_ALIGN;
- }
- }
+ if (macb_is_gem(bp))
+ dev->xdp_features = NETDEV_XDP_ACT_BASIC |
+ NETDEV_XDP_ACT_REDIRECT;
netif_carrier_off(dev);
--
2.52.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH net-next v2 5/8] net: macb: add XDP support for gem
2026-02-23 18:26 ` [PATCH net-next v2 5/8] net: macb: add XDP support for gem Paolo Valerio
@ 2026-02-23 23:23 ` kernel test robot
2026-02-24 0:08 ` [net-next,v2,5/8] " Jakub Kicinski
1 sibling, 0 replies; 20+ messages in thread
From: kernel test robot @ 2026-02-23 23:23 UTC (permalink / raw)
To: Paolo Valerio, netdev
Cc: llvm, oe-kbuild-all, Nicolas Ferre, Claudiu Beznea, Andrew Lunn,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lorenzo Bianconi,
Théo Lebrun
Hi Paolo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Paolo-Valerio/net-macb-move-Rx-buffers-alloc-from-link-up-to-open/20260224-023220
base: net-next/main
patch link: https://lore.kernel.org/r/20260223182632.1681809-6-pvalerio%40redhat.com
patch subject: [PATCH net-next v2 5/8] net: macb: add XDP support for gem
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20260224/202602240713.bQWzr8aD-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260224/202602240713.bQWzr8aD-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602240713.bQWzr8aD-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/net/ethernet/cadence/macb_main.c:2749:10: warning: variable 'err' is uninitialized when used here [-Wuninitialized]
2749 | return err;
| ^~~
drivers/net/ethernet/cadence/macb_main.c:2743:9: note: initialize the variable 'err' to silence this warning
2743 | int err;
| ^
| = 0
1 warning generated.
vim +/err +2749 drivers/net/ethernet/cadence/macb_main.c
89e5785fc8a6b9 drivers/net/macb.c Haavard Skinnemoen 2006-11-09 2726
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2727 static int gem_create_page_pool(struct macb_queue *queue, int qid)
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2728 {
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2729 struct page_pool_params pp_params = {
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2730 .order = 0,
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2731 .flags = PP_FLAG_DMA_MAP,
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2732 .pool_size = queue->bp->rx_ring_size,
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2733 .nid = NUMA_NO_NODE,
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2734 .dma_dir = rcu_access_pointer(queue->bp->prog)
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2735 ? DMA_BIDIRECTIONAL
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2736 : DMA_FROM_DEVICE,
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2737 .dev = &queue->bp->pdev->dev,
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2738 .netdev = queue->bp->dev,
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2739 .napi = &queue->napi_rx,
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2740 .max_len = PAGE_SIZE,
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2741 };
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2742 struct page_pool *pool;
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2743 int err;
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2744
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2745 /* This can happen in the case of HRESP error.
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2746 * Do nothing as page pool is already existing.
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2747 */
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2748 if (queue->page_pool)
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 @2749 return err;
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2750
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2751 pool = page_pool_create(&pp_params);
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2752 if (IS_ERR(pool)) {
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2753 netdev_err(queue->bp->dev, "cannot create rx page pool\n");
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2754 err = PTR_ERR(pool);
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2755 goto clear_pool;
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2756 }
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2757
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2758 queue->page_pool = pool;
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2759
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2760 err = xdp_rxq_info_reg(&queue->xdp_rxq, queue->bp->dev, qid,
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2761 queue->napi_rx.napi_id);
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2762 if (err < 0) {
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2763 netdev_err(queue->bp->dev, "xdp: failed to register rxq info\n");
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2764 goto destroy_pool;
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2765 }
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2766
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2767 err = xdp_rxq_info_reg_mem_model(&queue->xdp_rxq, MEM_TYPE_PAGE_POOL,
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2768 queue->page_pool);
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2769 if (err) {
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2770 netdev_err(queue->bp->dev, "xdp: failed to register rxq memory model\n");
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2771 goto unreg_info;
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2772 }
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2773
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2774 return 0;
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2775
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2776 unreg_info:
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2777 xdp_rxq_info_unreg(&queue->xdp_rxq);
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2778 destroy_pool:
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2779 page_pool_destroy(pool);
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2780 clear_pool:
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2781 queue->page_pool = NULL;
fa62bb4def147c drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2782
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2783 return err;
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2784 }
65104588f8cde9 drivers/net/ethernet/cadence/macb_main.c Paolo Valerio 2026-02-23 2785
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next,v2,5/8] net: macb: add XDP support for gem
2026-02-23 18:26 ` [PATCH net-next v2 5/8] net: macb: add XDP support for gem Paolo Valerio
2026-02-23 23:23 ` kernel test robot
@ 2026-02-24 0:08 ` Jakub Kicinski
2026-02-25 18:30 ` Paolo Valerio
1 sibling, 1 reply; 20+ messages in thread
From: Jakub Kicinski @ 2026-02-24 0:08 UTC (permalink / raw)
To: pvalerio
Cc: Jakub Kicinski, davem, claudiu.beznea, nicolas.ferre, edumazet,
pabeni, theo.lebrun, netdev, lorenzo, andrew+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.
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: macb: add XDP support for gem
This patch introduces basic XDP support for the macb/gem driver with
XDP_PASS, XDP_DROP, and XDP_REDIRECT verdict support.
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index a78ad00f53b1..33a963b6dd4d 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -15,6 +15,7 @@
> #include <linux/phy/phy.h>
> #include <linux/workqueue.h>
> #include <net/page_pool/helpers.h>
> +#include <net/xdp.h>
[ ... ]
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 708107e47ae3..26b517ed251c 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
> @@ -1247,9 +1248,27 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
> return packets;
> }
>
> +static unsigned int gem_rx_headroom(struct macb *bp)
> +{
> + if (rcu_access_pointer(bp->prog))
> + return XDP_PACKET_HEADROOM;
> +
> + return NET_SKB_PAD;
> +}
> +
> +static unsigned int gem_max_rx_data_size(int base_sz)
> +{
> + return SKB_DATA_ALIGN(base_sz + ETH_HLEN + ETH_FCS_LEN);
> +}
> +
> +static unsigned int gem_max_rx_buffer_size(int data_sz, struct macb *bp)
> +{
> + return SKB_HEAD_ALIGN(data_sz + bp->rx_headroom);
> +}
[ ... ]
> @@ -1340,12 +1360,61 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
> */
> }
>
> +static u32 gem_xdp_run(struct macb_queue *queue, void *buff_head,
> + unsigned int len)
> +{
> + struct net_device *dev;
> + struct bpf_prog *prog;
> + struct xdp_buff xdp;
> +
> + u32 act = XDP_PASS;
> +
> + rcu_read_lock();
> +
> + prog = rcu_dereference(queue->bp->prog);
> + if (!prog)
> + goto out;
> +
> + xdp_init_buff(&xdp, gem_total_rx_buffer_size(queue->bp), &queue->xdp_rxq);
> + xdp_prepare_buff(&xdp, buff_head, queue->bp->rx_headroom, len, false);
> + xdp_buff_clear_frags_flag(&xdp);
> + dev = queue->bp->dev;
> +
> + act = bpf_prog_run_xdp(prog, &xdp);
> + switch (act) {
> + case XDP_PASS:
> + goto out;
> + case XDP_REDIRECT:
> + if (unlikely(xdp_do_redirect(dev, &xdp, prog))) {
> + act = XDP_DROP;
> + break;
> + }
> + goto out;
> + default:
> + bpf_warn_invalid_xdp_action(dev, prog, act);
> + fallthrough;
> + case XDP_ABORTED:
> + trace_xdp_exception(dev, prog, act);
> + fallthrough;
> + case XDP_DROP:
> + break;
> + }
Does this function need to handle XDP_TX? The driver advertises
NETDEV_XDP_ACT_BASIC (set in macb_probe() below), which declares support
for XDP_ABORTED, XDP_DROP, XDP_PASS, and XDP_TX per the UAPI definition
in include/uapi/linux/netdev.h. An XDP program returning XDP_TX will fall
through to the default case, call bpf_warn_invalid_xdp_action(), and
treat it as XDP_DROP. The packet won't be transmitted; it will be dropped
with a warning instead.
[ ... ]
> @@ -2641,21 +2724,23 @@ static int macb_alloc_consistent(struct macb *bp)
> return -ENOMEM;
> }
>
> -static int gem_create_page_pool(struct macb_queue *queue)
> +static int gem_create_page_pool(struct macb_queue *queue, int qid)
> {
> struct page_pool_params pp_params = {
> .order = 0,
> .flags = PP_FLAG_DMA_MAP,
> .pool_size = queue->bp->rx_ring_size,
> .nid = NUMA_NO_NODE,
> - .dma_dir = DMA_FROM_DEVICE,
> + .dma_dir = rcu_access_pointer(queue->bp->prog)
> + ? DMA_BIDIRECTIONAL
> + : DMA_FROM_DEVICE,
> .dev = &queue->bp->pdev->dev,
> .netdev = queue->bp->dev,
> .napi = &queue->napi_rx,
> .max_len = PAGE_SIZE,
> };
> struct page_pool *pool;
> - int err = 0;
> + int err;
>
> /* This can happen in the case of HRESP error.
> * Do nothing as page pool is already existing.
> */
> if (queue->page_pool)
> return err;
^^^
Does this return an uninitialized value? The patch changed 'int err = 0'
to 'int err' without initialization. The early-return path for HRESP error
recovery returns err when the page pool already exists.
gem_init_rings() calls this function during HRESP error recovery:
drivers/net/ethernet/cadence/macb_main.c:gem_init_rings()
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
...
err = gem_create_page_pool(queue, q);
if (err) {
last_err = err;
if (fail_early)
return err;
continue;
}
When err is non-zero, gem_init_rings() skips gem_rx_refill() for that
queue via continue, potentially leaving the RX queue without buffers.
The unmodified macb_hresp_error_task() calls mog_init_rings(bp, false)
which calls gem_init_rings() to recover from HRESP errors. During
recovery, page pools already exist from the initial open, so the
early-return is taken. If err happens to be non-zero, gem_init_rings()
skips gem_rx_refill(), breaking packet reception on affected queues after
HRESP error recovery.
[ ... ]
> @@ -3167,11 +3275,27 @@ static int macb_close(struct net_device *dev)
> return 0;
> }
>
> +static bool gem_xdp_valid_mtu(struct macb *bp, int mtu)
> +{
> + int max_frame_size;
> +
> + max_frame_size = gem_max_rx_buffer_size(gem_max_rx_data_size(mtu), bp);
> +
> + return max_frame_size <= PAGE_SIZE;
> +}
Looking at the XDP setup flow, is this MTU validation using the correct
headroom value?
When gem_xdp_setup() calls gem_xdp_valid_mtu():
drivers/net/ethernet/cadence/macb_main.c:gem_xdp_setup()
if (prog && !gem_xdp_valid_mtu(bp, dev->mtu)) {
NL_SET_ERR_MSG_MOD(extack, "MTU too large for XDP");
return -EOPNOTSUPP;
}
running = netif_running(dev);
need_update = !!bp->prog != !!prog;
if (running && need_update)
macb_close(dev);
old_prog = rcu_replace_pointer(bp->prog, prog, lockdep_rtnl_is_held());
The validation happens before bp->prog is updated. gem_max_rx_buffer_size()
uses bp->rx_headroom, which is cached during macb_init_rx_buffer_size().
At validation time, bp->rx_headroom still holds the non-XDP value
(NET_SKB_PAD, typically 64 bytes) rather than the XDP value
(XDP_PACKET_HEADROOM = 256 bytes). This makes the validation 192 bytes too
permissive.
An MTU close to the PAGE_SIZE limit could pass validation but then cause
gem_total_rx_buffer_size() to exceed PAGE_SIZE once the correct XDP
headroom is applied in macb_open()->macb_init_rx_buffer_size().
[ ... ]
> @@ -5744,13 +5911,9 @@ static int macb_probe(struct platform_device *pdev)
> if (err)
> goto err_out_phy_exit;
>
> - if (macb_is_gem(bp)) {
> - bp->rx_headroom = NET_SKB_PAD;
> - if (!(bp->caps & MACB_CAPS_RSC)) {
> - bp->rx_ip_align = NET_IP_ALIGN;
> - bp->rx_headroom += NET_IP_ALIGN;
> - }
> - }
> + if (macb_is_gem(bp))
> + dev->xdp_features = NETDEV_XDP_ACT_BASIC |
> + NETDEV_XDP_ACT_REDIRECT;
Is NETDEV_XDP_ACT_BASIC accurate here? Looking at gem_xdp_run() above,
XDP_TX falls through to the default case and is treated as XDP_DROP with
a warning. NETDEV_XDP_ACT_BASIC includes XDP_TX support per the UAPI
definition, but the driver doesn't implement it.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next,v2,5/8] net: macb: add XDP support for gem
2026-02-24 0:08 ` [net-next,v2,5/8] " Jakub Kicinski
@ 2026-02-25 18:30 ` Paolo Valerio
2026-02-27 10:52 ` Théo Lebrun
0 siblings, 1 reply; 20+ messages in thread
From: Paolo Valerio @ 2026-02-25 18:30 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jakub Kicinski, davem, claudiu.beznea, nicolas.ferre, edumazet,
pabeni, theo.lebrun, netdev, lorenzo, andrew+netdev
On 23 Feb 2026 at 04:08:44 PM, Jakub Kicinski <kuba@kernel.org> wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> net: macb: add XDP support for gem
>
> This patch introduces basic XDP support for the macb/gem driver with
> XDP_PASS, XDP_DROP, and XDP_REDIRECT verdict support.
>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index a78ad00f53b1..33a963b6dd4d 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -15,6 +15,7 @@
>> #include <linux/phy/phy.h>
>> #include <linux/workqueue.h>
>> #include <net/page_pool/helpers.h>
>> +#include <net/xdp.h>
>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 708107e47ae3..26b517ed251c 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>
> [ ... ]
>
>> @@ -1247,9 +1248,27 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>> return packets;
>> }
>>
>> +static unsigned int gem_rx_headroom(struct macb *bp)
>> +{
>> + if (rcu_access_pointer(bp->prog))
>> + return XDP_PACKET_HEADROOM;
>> +
>> + return NET_SKB_PAD;
>> +}
>> +
>> +static unsigned int gem_max_rx_data_size(int base_sz)
>> +{
>> + return SKB_DATA_ALIGN(base_sz + ETH_HLEN + ETH_FCS_LEN);
>> +}
>> +
>> +static unsigned int gem_max_rx_buffer_size(int data_sz, struct macb *bp)
>> +{
>> + return SKB_HEAD_ALIGN(data_sz + bp->rx_headroom);
>> +}
>
> [ ... ]
>
>> @@ -1340,12 +1360,61 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
>> */
>> }
>>
>> +static u32 gem_xdp_run(struct macb_queue *queue, void *buff_head,
>> + unsigned int len)
>> +{
>> + struct net_device *dev;
>> + struct bpf_prog *prog;
>> + struct xdp_buff xdp;
>> +
>> + u32 act = XDP_PASS;
>> +
>> + rcu_read_lock();
>> +
>> + prog = rcu_dereference(queue->bp->prog);
>> + if (!prog)
>> + goto out;
>> +
>> + xdp_init_buff(&xdp, gem_total_rx_buffer_size(queue->bp), &queue->xdp_rxq);
>> + xdp_prepare_buff(&xdp, buff_head, queue->bp->rx_headroom, len, false);
>> + xdp_buff_clear_frags_flag(&xdp);
>> + dev = queue->bp->dev;
>> +
>> + act = bpf_prog_run_xdp(prog, &xdp);
>> + switch (act) {
>> + case XDP_PASS:
>> + goto out;
>> + case XDP_REDIRECT:
>> + if (unlikely(xdp_do_redirect(dev, &xdp, prog))) {
>> + act = XDP_DROP;
>> + break;
>> + }
>> + goto out;
>> + default:
>> + bpf_warn_invalid_xdp_action(dev, prog, act);
>> + fallthrough;
>> + case XDP_ABORTED:
>> + trace_xdp_exception(dev, prog, act);
>> + fallthrough;
>> + case XDP_DROP:
>> + break;
>> + }
>
> Does this function need to handle XDP_TX? The driver advertises
> NETDEV_XDP_ACT_BASIC (set in macb_probe() below), which declares support
> for XDP_ABORTED, XDP_DROP, XDP_PASS, and XDP_TX per the UAPI definition
> in include/uapi/linux/netdev.h. An XDP program returning XDP_TX will fall
> through to the default case, call bpf_warn_invalid_xdp_action(), and
> treat it as XDP_DROP. The packet won't be transmitted; it will be dropped
> with a warning instead.
>
> [ ... ]
>
>> @@ -2641,21 +2724,23 @@ static int macb_alloc_consistent(struct macb *bp)
>> return -ENOMEM;
>> }
>>
>> -static int gem_create_page_pool(struct macb_queue *queue)
>> +static int gem_create_page_pool(struct macb_queue *queue, int qid)
>> {
>> struct page_pool_params pp_params = {
>> .order = 0,
>> .flags = PP_FLAG_DMA_MAP,
>> .pool_size = queue->bp->rx_ring_size,
>> .nid = NUMA_NO_NODE,
>> - .dma_dir = DMA_FROM_DEVICE,
>> + .dma_dir = rcu_access_pointer(queue->bp->prog)
>> + ? DMA_BIDIRECTIONAL
>> + : DMA_FROM_DEVICE,
>> .dev = &queue->bp->pdev->dev,
>> .netdev = queue->bp->dev,
>> .napi = &queue->napi_rx,
>> .max_len = PAGE_SIZE,
>> };
>> struct page_pool *pool;
>> - int err = 0;
>> + int err;
>>
>> /* This can happen in the case of HRESP error.
>> * Do nothing as page pool is already existing.
>> */
>> if (queue->page_pool)
>> return err;
> ^^^
>
> Does this return an uninitialized value? The patch changed 'int err = 0'
> to 'int err' without initialization. The early-return path for HRESP error
> recovery returns err when the page pool already exists.
>
argh, I missed this patch removing the initialization while rebasing.
> gem_init_rings() calls this function during HRESP error recovery:
>
> drivers/net/ethernet/cadence/macb_main.c:gem_init_rings()
> for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> ...
> err = gem_create_page_pool(queue, q);
> if (err) {
> last_err = err;
> if (fail_early)
> return err;
> continue;
> }
>
> When err is non-zero, gem_init_rings() skips gem_rx_refill() for that
> queue via continue, potentially leaving the RX queue without buffers.
>
> The unmodified macb_hresp_error_task() calls mog_init_rings(bp, false)
> which calls gem_init_rings() to recover from HRESP errors. During
> recovery, page pools already exist from the initial open, so the
> early-return is taken. If err happens to be non-zero, gem_init_rings()
> skips gem_rx_refill(), breaking packet reception on affected queues after
> HRESP error recovery.
>
> [ ... ]
>
>> @@ -3167,11 +3275,27 @@ static int macb_close(struct net_device *dev)
>> return 0;
>> }
>>
>> +static bool gem_xdp_valid_mtu(struct macb *bp, int mtu)
>> +{
>> + int max_frame_size;
>> +
>> + max_frame_size = gem_max_rx_buffer_size(gem_max_rx_data_size(mtu), bp);
>> +
>> + return max_frame_size <= PAGE_SIZE;
>> +}
>
> Looking at the XDP setup flow, is this MTU validation using the correct
> headroom value?
>
> When gem_xdp_setup() calls gem_xdp_valid_mtu():
>
> drivers/net/ethernet/cadence/macb_main.c:gem_xdp_setup()
> if (prog && !gem_xdp_valid_mtu(bp, dev->mtu)) {
> NL_SET_ERR_MSG_MOD(extack, "MTU too large for XDP");
> return -EOPNOTSUPP;
> }
>
> running = netif_running(dev);
> need_update = !!bp->prog != !!prog;
> if (running && need_update)
> macb_close(dev);
>
> old_prog = rcu_replace_pointer(bp->prog, prog, lockdep_rtnl_is_held());
>
> The validation happens before bp->prog is updated. gem_max_rx_buffer_size()
> uses bp->rx_headroom, which is cached during macb_init_rx_buffer_size().
> At validation time, bp->rx_headroom still holds the non-XDP value
> (NET_SKB_PAD, typically 64 bytes) rather than the XDP value
> (XDP_PACKET_HEADROOM = 256 bytes). This makes the validation 192 bytes too
> permissive.
>
I guess gem_xdp_valid_mtu() can depend only on XDP_PACKET_HEADROOM and
just check against it and drop the dependency with bp->rx_headroom.
> An MTU close to the PAGE_SIZE limit could pass validation but then cause
> gem_total_rx_buffer_size() to exceed PAGE_SIZE once the correct XDP
> headroom is applied in macb_open()->macb_init_rx_buffer_size().
>
> [ ... ]
>
>> @@ -5744,13 +5911,9 @@ static int macb_probe(struct platform_device *pdev)
>> if (err)
>> goto err_out_phy_exit;
>>
>> - if (macb_is_gem(bp)) {
>> - bp->rx_headroom = NET_SKB_PAD;
>> - if (!(bp->caps & MACB_CAPS_RSC)) {
>> - bp->rx_ip_align = NET_IP_ALIGN;
>> - bp->rx_headroom += NET_IP_ALIGN;
>> - }
>> - }
>> + if (macb_is_gem(bp))
>> + dev->xdp_features = NETDEV_XDP_ACT_BASIC |
>> + NETDEV_XDP_ACT_REDIRECT;
>
> Is NETDEV_XDP_ACT_BASIC accurate here? Looking at gem_xdp_run() above,
> XDP_TX falls through to the default case and is treated as XDP_DROP with
> a warning. NETDEV_XDP_ACT_BASIC includes XDP_TX support per the UAPI
> definition, but the driver doesn't implement it.
This makes sense. Will require some rearrangement in the way the series
is split.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next,v2,5/8] net: macb: add XDP support for gem
2026-02-25 18:30 ` Paolo Valerio
@ 2026-02-27 10:52 ` Théo Lebrun
2026-02-28 13:49 ` Claudiu Beznea
0 siblings, 1 reply; 20+ messages in thread
From: Théo Lebrun @ 2026-02-27 10:52 UTC (permalink / raw)
To: Paolo Valerio, Jakub Kicinski
Cc: davem, claudiu.beznea, nicolas.ferre, edumazet, pabeni,
theo.lebrun, netdev, lorenzo, andrew+netdev, Grégory Clement,
Thomas Petazzoni
Hello Paolo,
On Wed Feb 25, 2026 at 7:30 PM CET, Paolo Valerio wrote:
> On 23 Feb 2026 at 04:08:44 PM, Jakub Kicinski <kuba@kernel.org> wrote:
>> This is an AI-generated review of your patch. The human sending this
>> email has considered the AI review valid, or at least plausible.
>>
>> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
>> ---
>> net: macb: add XDP support for gem
>>
>> This patch introduces basic XDP support for the macb/gem driver with
>> XDP_PASS, XDP_DROP, and XDP_REDIRECT verdict support.
>>
>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>>> index 708107e47ae3..26b517ed251c 100644
>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
[...]
>>> @@ -2641,21 +2724,23 @@ static int macb_alloc_consistent(struct macb *bp)
>>> return -ENOMEM;
>>> }
>>>
>>> -static int gem_create_page_pool(struct macb_queue *queue)
>>> +static int gem_create_page_pool(struct macb_queue *queue, int qid)
>>> {
>>> struct page_pool_params pp_params = {
>>> .order = 0,
>>> .flags = PP_FLAG_DMA_MAP,
>>> .pool_size = queue->bp->rx_ring_size,
>>> .nid = NUMA_NO_NODE,
>>> - .dma_dir = DMA_FROM_DEVICE,
>>> + .dma_dir = rcu_access_pointer(queue->bp->prog)
>>> + ? DMA_BIDIRECTIONAL
>>> + : DMA_FROM_DEVICE,
>>> .dev = &queue->bp->pdev->dev,
>>> .netdev = queue->bp->dev,
>>> .napi = &queue->napi_rx,
>>> .max_len = PAGE_SIZE,
>>> };
>>> struct page_pool *pool;
>>> - int err = 0;
>>> + int err;
>>>
>>> /* This can happen in the case of HRESP error.
>>> * Do nothing as page pool is already existing.
>>> */
>>> if (queue->page_pool)
>>> return err;
>> ^^^
>>
>> Does this return an uninitialized value? The patch changed 'int err = 0'
>> to 'int err' without initialization. The early-return path for HRESP error
>> recovery returns err when the page pool already exists.
>>
>
> argh, I missed this patch removing the initialization while rebasing.
I debugged a nasty bug yesterday, which ended up being this typo
combined with the fact that we call gem_init_rings() through the
mog_init_rings function pointer twice at open. Once near the end of
macb_alloc_consistent() and once from macb_open().
This bug is invisible because we supposedly skip the second
gem_create_page_pool() with the queue->page_pool check.
On my system it lead to the "DMA bus error: HRESP not OK" error;
my theory (which I didn't dig into much as the root cause is known):
- undefined behaviour leading to the compiler skipping the
`if (queue->page_pool)` condition in gem_create_page_pool()
- leakage of the old page allocator and the old buffers leading to
buffer lifetime issues somehow
I still don't understand how we all tolerate -Wno-maybe-uninitialized in
the kernel under GCC. But now that it has been here for a long time
many false positives slipped in, making it harder to re-enable.
https://elixir.bootlin.com/linux/v6.19.3/source/scripts/Makefile.warn#L184-L186
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next,v2,5/8] net: macb: add XDP support for gem
2026-02-27 10:52 ` Théo Lebrun
@ 2026-02-28 13:49 ` Claudiu Beznea
0 siblings, 0 replies; 20+ messages in thread
From: Claudiu Beznea @ 2026-02-28 13:49 UTC (permalink / raw)
To: Théo Lebrun, Paolo Valerio, Jakub Kicinski
Cc: davem, nicolas.ferre, edumazet, pabeni, netdev, lorenzo,
andrew+netdev, Grégory Clement, Thomas Petazzoni
Hi,
On 2/27/26 12:52, Théo Lebrun wrote:
> Hello Paolo,
>
> On Wed Feb 25, 2026 at 7:30 PM CET, Paolo Valerio wrote:
>> On 23 Feb 2026 at 04:08:44 PM, Jakub Kicinski <kuba@kernel.org> wrote:
>>> This is an AI-generated review of your patch. The human sending this
>>> email has considered the AI review valid, or at least plausible.
>>>
>>> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
>>> ---
>>> net: macb: add XDP support for gem
>>>
>>> This patch introduces basic XDP support for the macb/gem driver with
>>> XDP_PASS, XDP_DROP, and XDP_REDIRECT verdict support.
>>>
>>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>>>> index 708107e47ae3..26b517ed251c 100644
>>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
> [...]
>>>> @@ -2641,21 +2724,23 @@ static int macb_alloc_consistent(struct macb *bp)
>>>> return -ENOMEM;
>>>> }
>>>>
>>>> -static int gem_create_page_pool(struct macb_queue *queue)
>>>> +static int gem_create_page_pool(struct macb_queue *queue, int qid)
>>>> {
>>>> struct page_pool_params pp_params = {
>>>> .order = 0,
>>>> .flags = PP_FLAG_DMA_MAP,
>>>> .pool_size = queue->bp->rx_ring_size,
>>>> .nid = NUMA_NO_NODE,
>>>> - .dma_dir = DMA_FROM_DEVICE,
>>>> + .dma_dir = rcu_access_pointer(queue->bp->prog)
>>>> + ? DMA_BIDIRECTIONAL
>>>> + : DMA_FROM_DEVICE,
>>>> .dev = &queue->bp->pdev->dev,
>>>> .netdev = queue->bp->dev,
>>>> .napi = &queue->napi_rx,
>>>> .max_len = PAGE_SIZE,
>>>> };
>>>> struct page_pool *pool;
>>>> - int err = 0;
>>>> + int err;
>>>>
>>>> /* This can happen in the case of HRESP error.
>>>> * Do nothing as page pool is already existing.
>>>> */
>>>> if (queue->page_pool)
>>>> return err;
>>> ^^^
>>>
>>> Does this return an uninitialized value? The patch changed 'int err = 0'
>>> to 'int err' without initialization. The early-return path for HRESP error
>>> recovery returns err when the page pool already exists.
>>>
>>
>> argh, I missed this patch removing the initialization while rebasing.
>
> I debugged a nasty bug yesterday, which ended up being this typo
> combined with the fact that we call gem_init_rings() through the
> mog_init_rings function pointer twice at open. Once near the end of
> macb_alloc_consistent() and once from macb_open().
>
> This bug is invisible because we supposedly skip the second
> gem_create_page_pool() with the queue->page_pool check.
>
> On my system it lead to the "DMA bus error: HRESP not OK" error;
This is reproducible also on Microchip SAMA7G5-EK with this series, following
these steps:
# ifconfig eth0 192.168.32.8
# ping 192.168.32.2
PING 192.168.32.2 (192.168.32.2) 56(84) bytes of data.
64 bytes from 192.168.32.2: icmp_seq=1 ttl=64 time=0.589 ms
64 bytes from 192.168.32.2: icmp_seq=2 ttl=64 time=0.629 ms
64 bytes from 192.168.32.2: icmp_seq=3 ttl=64 time=0.550 ms
LDO2: disabling
64 bytes from 192.168.32.2: icmp_seq=4 ttl=64 time=0.579 ms
--- 192.168.32.2 ping statistics ---
4 packets transmitted, 4 received, 0% packet loss, time 3005ms
rtt min/avg/max/mdev = 0.550/0.586/0.629/0.028 ms
#
# uname -r
7.0.0-rc1-next-20260227+
# iperf3 -c 192.168.32.2
Connecting to host 192.168.32.2, port 5201
[ 5] local 192.168.32.8 port 51488 connected to 192.168.32.2 port 5201
macb e2800000.ethernet eth0: DMA bus error: HRESP not OK
macb e2800000.ethernet eth0: inconsistent Rx descriptor chain
macb e2800000.ethernet eth0: inconsistent Rx descriptor chain
[ ID] Interval Transfer Bitrate Retr Cwnd
[ 5] 0.00-1.00 sec 31.2 MBytes 262 Mbits/sec 14 1.41 KBytes
[ 5] 1.00-2.00 sec 0.00 Bytes 0.00 bits/sec 1 1.41 KBytes
[ 5] 2.00-3.00 sec 0.00 Bytes 0.00 bits/sec 1 1.41 KBytes
[ 5] 3.00-4.00 sec 0.00 Bytes 0.00 bits/sec 1 1.41 KBytes
[ 5] 4.00-5.00 sec 0.00 Bytes 0.00 bits/sec 0 1.41 KBytes
[ 5] 5.00-6.00 sec 0.00 Bytes 0.00 bits/sec 0 1.41 KBytes
[ 5] 6.00-7.00 sec 0.00 Bytes 0.00 bits/sec 0 1.41 KBytes
[ 5] 7.00-8.00 sec 0.00 Bytes 0.00 bits/sec 1 1.41 KBytes
[ 5] 8.00-9.00 sec 0.00 Bytes 0.00 bits/sec 0 1.41 KBytes
[ 5] 10.00-25.83 sec 0.00 Bytes 0.00 bits/sec 1 1.41 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bitrate Retr
[ 5] 0.00-25.83 sec 31.2 MBytes 10.1 Mbits/sec 19 sender
[ 5] 0.00-25.83 sec 0.00 Bytes 0.00 bits/sec receiver
iperf3: interrupt - the client has terminated
#
Thank you,
Claudiu
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH net-next v2 6/8] net: macb: make macb_tx_skb generic
2026-02-23 18:26 [PATCH net-next v2 0/8] net: macb: Add XDP support and page pool integration Paolo Valerio
` (4 preceding siblings ...)
2026-02-23 18:26 ` [PATCH net-next v2 5/8] net: macb: add XDP support for gem Paolo Valerio
@ 2026-02-23 18:26 ` Paolo Valerio
2026-02-24 0:08 ` [net-next,v2,6/8] " Jakub Kicinski
2026-02-23 18:26 ` [PATCH net-next v2 7/8] net: macb: make tx path skb agnostic Paolo Valerio
2026-02-23 18:26 ` [PATCH net-next v2 8/8] net: macb: introduce xmit support Paolo Valerio
7 siblings, 1 reply; 20+ messages in thread
From: Paolo Valerio @ 2026-02-23 18:26 UTC (permalink / raw)
To: netdev
Cc: Nicolas Ferre, Claudiu Beznea, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lorenzo Bianconi,
Théo Lebrun
The macb_tx_skb structure is renamed to macb_tx_buff with
no functional changes.
This is a preparatory step for adding xdp xmit support.
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
drivers/net/ethernet/cadence/macb.h | 10 +--
drivers/net/ethernet/cadence/macb_main.c | 96 ++++++++++++------------
2 files changed, 53 insertions(+), 53 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 33a963b6dd4d..4e3cc0e9ea87 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -959,7 +959,7 @@ struct macb_dma_desc_ptp {
/* Scaled PPM fraction */
#define PPM_FRACTION 16
-/* struct macb_tx_skb - data about an skb which is being transmitted
+/* struct macb_tx_buff - data about an skb which is being transmitted
* @skb: skb currently being transmitted, only set for the last buffer
* of the frame
* @mapping: DMA address of the skb's fragment buffer
@@ -967,8 +967,8 @@ struct macb_dma_desc_ptp {
* @mapped_as_page: true when buffer was mapped with skb_frag_dma_map(),
* false when buffer was mapped with dma_map_single()
*/
-struct macb_tx_skb {
- struct sk_buff *skb;
+struct macb_tx_buff {
+ void *skb;
dma_addr_t mapping;
size_t size;
bool mapped_as_page;
@@ -1253,7 +1253,7 @@ struct macb_queue {
spinlock_t tx_ptr_lock;
unsigned int tx_head, tx_tail;
struct macb_dma_desc *tx_ring;
- struct macb_tx_skb *tx_skb;
+ struct macb_tx_buff *tx_buff;
dma_addr_t tx_ring_dma;
struct work_struct tx_error_task;
bool txubr_pending;
@@ -1332,7 +1332,7 @@ struct macb {
phy_interface_t phy_interface;
/* AT91RM9200 transmit queue (1 on wire + 1 queued) */
- struct macb_tx_skb rm9200_txq[2];
+ struct macb_tx_buff rm9200_txq[2];
unsigned int max_tx_length;
u64 ethtool_stats[GEM_STATS_LEN + QUEUE_STATS_LEN * MACB_MAX_QUEUES];
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 26b517ed251c..f65f976123fd 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -157,10 +157,10 @@ static struct macb_dma_desc *macb_tx_desc(struct macb_queue *queue,
return &queue->tx_ring[index];
}
-static struct macb_tx_skb *macb_tx_skb(struct macb_queue *queue,
- unsigned int index)
+static struct macb_tx_buff *macb_tx_buff(struct macb_queue *queue,
+ unsigned int index)
{
- return &queue->tx_skb[macb_tx_ring_wrap(queue->bp, index)];
+ return &queue->tx_buff[macb_tx_ring_wrap(queue->bp, index)];
}
static dma_addr_t macb_tx_dma(struct macb_queue *queue, unsigned int index)
@@ -967,21 +967,21 @@ static int macb_halt_tx(struct macb *bp)
bp, TSR);
}
-static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb, int budget)
+static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff, int budget)
{
- if (tx_skb->mapping) {
- if (tx_skb->mapped_as_page)
- dma_unmap_page(&bp->pdev->dev, tx_skb->mapping,
- tx_skb->size, DMA_TO_DEVICE);
+ if (tx_buff->mapping) {
+ if (tx_buff->mapped_as_page)
+ dma_unmap_page(&bp->pdev->dev, tx_buff->mapping,
+ tx_buff->size, DMA_TO_DEVICE);
else
- dma_unmap_single(&bp->pdev->dev, tx_skb->mapping,
- tx_skb->size, DMA_TO_DEVICE);
- tx_skb->mapping = 0;
+ dma_unmap_single(&bp->pdev->dev, tx_buff->mapping,
+ tx_buff->size, DMA_TO_DEVICE);
+ tx_buff->mapping = 0;
}
- if (tx_skb->skb) {
- napi_consume_skb(tx_skb->skb, budget);
- tx_skb->skb = NULL;
+ if (tx_buff->skb) {
+ napi_consume_skb(tx_buff->skb, budget);
+ tx_buff->skb = NULL;
}
}
@@ -1027,7 +1027,7 @@ static void macb_tx_error_task(struct work_struct *work)
u32 queue_index;
u32 packets = 0;
u32 bytes = 0;
- struct macb_tx_skb *tx_skb;
+ struct macb_tx_buff *tx_buff;
struct macb_dma_desc *desc;
struct sk_buff *skb;
unsigned int tail;
@@ -1067,16 +1067,16 @@ static void macb_tx_error_task(struct work_struct *work)
desc = macb_tx_desc(queue, tail);
ctrl = desc->ctrl;
- tx_skb = macb_tx_skb(queue, tail);
- skb = tx_skb->skb;
+ tx_buff = macb_tx_buff(queue, tail);
+ skb = tx_buff->skb;
if (ctrl & MACB_BIT(TX_USED)) {
/* skb is set for the last buffer of the frame */
while (!skb) {
- macb_tx_unmap(bp, tx_skb, 0);
+ macb_tx_unmap(bp, tx_buff, 0);
tail++;
- tx_skb = macb_tx_skb(queue, tail);
- skb = tx_skb->skb;
+ tx_buff = macb_tx_buff(queue, tail);
+ skb = tx_buff->skb;
}
/* ctrl still refers to the first buffer descriptor
@@ -1105,7 +1105,7 @@ static void macb_tx_error_task(struct work_struct *work)
desc->ctrl = ctrl | MACB_BIT(TX_USED);
}
- macb_tx_unmap(bp, tx_skb, 0);
+ macb_tx_unmap(bp, tx_buff, 0);
}
netdev_tx_completed_queue(netdev_get_tx_queue(bp->dev, queue_index),
@@ -1183,7 +1183,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
spin_lock_irqsave(&queue->tx_ptr_lock, flags);
head = queue->tx_head;
for (tail = queue->tx_tail; tail != head && packets < budget; tail++) {
- struct macb_tx_skb *tx_skb;
+ struct macb_tx_buff *tx_buff;
struct sk_buff *skb;
struct macb_dma_desc *desc;
u32 ctrl;
@@ -1203,8 +1203,8 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
/* Process all buffers of the current transmitted frame */
for (;; tail++) {
- tx_skb = macb_tx_skb(queue, tail);
- skb = tx_skb->skb;
+ tx_buff = macb_tx_buff(queue, tail);
+ skb = tx_buff->skb;
/* First, update TX stats if needed */
if (skb) {
@@ -1224,7 +1224,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
}
/* Now we can safely release resources */
- macb_tx_unmap(bp, tx_skb, budget);
+ macb_tx_unmap(bp, tx_buff, budget);
/* skb is set only for the last buffer of the frame.
* WARNING: at this point skb has been freed by
@@ -2137,8 +2137,8 @@ static unsigned int macb_tx_map(struct macb *bp,
unsigned int f, nr_frags = skb_shinfo(skb)->nr_frags;
unsigned int len, i, tx_head = queue->tx_head;
u32 ctrl, lso_ctrl = 0, seq_ctrl = 0;
+ struct macb_tx_buff *tx_buff = NULL;
unsigned int eof = 1, mss_mfs = 0;
- struct macb_tx_skb *tx_skb = NULL;
struct macb_dma_desc *desc;
unsigned int offset, size;
dma_addr_t mapping;
@@ -2161,7 +2161,7 @@ static unsigned int macb_tx_map(struct macb *bp,
offset = 0;
while (len) {
- tx_skb = macb_tx_skb(queue, tx_head);
+ tx_buff = macb_tx_buff(queue, tx_head);
mapping = dma_map_single(&bp->pdev->dev,
skb->data + offset,
@@ -2170,10 +2170,10 @@ static unsigned int macb_tx_map(struct macb *bp,
goto dma_error;
/* Save info to properly release resources */
- tx_skb->skb = NULL;
- tx_skb->mapping = mapping;
- tx_skb->size = size;
- tx_skb->mapped_as_page = false;
+ tx_buff->skb = NULL;
+ tx_buff->mapping = mapping;
+ tx_buff->size = size;
+ tx_buff->mapped_as_page = false;
len -= size;
offset += size;
@@ -2190,7 +2190,7 @@ static unsigned int macb_tx_map(struct macb *bp,
offset = 0;
while (len) {
size = umin(len, bp->max_tx_length);
- tx_skb = macb_tx_skb(queue, tx_head);
+ tx_buff = macb_tx_buff(queue, tx_head);
mapping = skb_frag_dma_map(&bp->pdev->dev, frag,
offset, size, DMA_TO_DEVICE);
@@ -2198,10 +2198,10 @@ static unsigned int macb_tx_map(struct macb *bp,
goto dma_error;
/* Save info to properly release resources */
- tx_skb->skb = NULL;
- tx_skb->mapping = mapping;
- tx_skb->size = size;
- tx_skb->mapped_as_page = true;
+ tx_buff->skb = NULL;
+ tx_buff->mapping = mapping;
+ tx_buff->size = size;
+ tx_buff->mapped_as_page = true;
len -= size;
offset += size;
@@ -2210,13 +2210,13 @@ static unsigned int macb_tx_map(struct macb *bp,
}
/* Should never happen */
- if (unlikely(!tx_skb)) {
+ if (unlikely(!tx_buff)) {
netdev_err(bp->dev, "BUG! empty skb!\n");
return 0;
}
/* This is the last buffer of the frame: save socket buffer */
- tx_skb->skb = skb;
+ tx_buff->skb = skb;
/* Update TX ring: update buffer descriptors in reverse order
* to avoid race condition
@@ -2247,10 +2247,10 @@ static unsigned int macb_tx_map(struct macb *bp,
do {
i--;
- tx_skb = macb_tx_skb(queue, i);
+ tx_buff = macb_tx_buff(queue, i);
desc = macb_tx_desc(queue, i);
- ctrl = (u32)tx_skb->size;
+ ctrl = (u32)tx_buff->size;
if (eof) {
ctrl |= MACB_BIT(TX_LAST);
eof = 0;
@@ -2273,7 +2273,7 @@ static unsigned int macb_tx_map(struct macb *bp,
ctrl |= MACB_BF(MSS_MFS, mss_mfs);
/* Set TX buffer descriptor */
- macb_set_addr(bp, desc, tx_skb->mapping);
+ macb_set_addr(bp, desc, tx_buff->mapping);
/* desc->addr must be visible to hardware before clearing
* 'TX_USED' bit in desc->ctrl.
*/
@@ -2289,9 +2289,9 @@ static unsigned int macb_tx_map(struct macb *bp,
netdev_err(bp->dev, "TX DMA map failed\n");
for (i = queue->tx_head; i != tx_head; i++) {
- tx_skb = macb_tx_skb(queue, i);
+ tx_buff = macb_tx_buff(queue, i);
- macb_tx_unmap(bp, tx_skb, 0);
+ macb_tx_unmap(bp, tx_buff, 0);
}
return -ENOMEM;
@@ -2617,8 +2617,8 @@ static void macb_free_consistent(struct macb *bp)
dma_free_coherent(dev, size, bp->queues[0].rx_ring, bp->queues[0].rx_ring_dma);
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
- kfree(queue->tx_skb);
- queue->tx_skb = NULL;
+ kfree(queue->tx_buff);
+ queue->tx_buff = NULL;
queue->tx_ring = NULL;
queue->rx_ring = NULL;
}
@@ -2696,9 +2696,9 @@ static int macb_alloc_consistent(struct macb *bp)
queue->rx_ring = rx + macb_rx_ring_size_per_queue(bp) * q;
queue->rx_ring_dma = rx_dma + macb_rx_ring_size_per_queue(bp) * q;
- size = bp->tx_ring_size * sizeof(struct macb_tx_skb);
- queue->tx_skb = kmalloc(size, GFP_KERNEL);
- if (!queue->tx_skb)
+ size = bp->tx_ring_size * sizeof(struct macb_tx_buff);
+ queue->tx_buff = kmalloc(size, GFP_KERNEL);
+ if (!queue->tx_buff)
goto out_err;
}
--
2.52.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [net-next,v2,6/8] net: macb: make macb_tx_skb generic
2026-02-23 18:26 ` [PATCH net-next v2 6/8] net: macb: make macb_tx_skb generic Paolo Valerio
@ 2026-02-24 0:08 ` Jakub Kicinski
0 siblings, 0 replies; 20+ messages in thread
From: Jakub Kicinski @ 2026-02-24 0:08 UTC (permalink / raw)
To: pvalerio
Cc: Jakub Kicinski, davem, claudiu.beznea, nicolas.ferre, edumazet,
pabeni, theo.lebrun, netdev, lorenzo, andrew+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.
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: macb: make macb_tx_skb generic
The macb_tx_skb structure is renamed to macb_tx_buff with no functional
changes. This is a preparatory step for adding xdp xmit support.
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 33a963b6dd4d..4e3cc0e9ea87 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -959,7 +959,7 @@ struct macb_dma_desc_ptp {
> /* Scaled PPM fraction */
> #define PPM_FRACTION 16
>
> -/* struct macb_tx_skb - data about an skb which is being transmitted
> +/* struct macb_tx_buff - data about an skb which is being transmitted
> * @skb: skb currently being transmitted, only set for the last buffer
> * of the frame
The comment documentation still describes this structure exclusively in
terms of sk_buff ('data about an skb which is being transmitted' and 'skb
currently being transmitted'), but the field type is being changed from
'struct sk_buff *' to 'void *' to support future XDP buffers.
Should the documentation be updated to reflect the generic purpose? For
example, indicating that the field can hold either an sk_buff pointer or an
XDP buffer pointer, which aligns with the intent stated in the commit
message about preparing for XDP xmit support.
> * @mapping: DMA address of the skb's fragment buffer
> @@ -967,8 +967,8 @@ struct macb_dma_desc_ptp {
> * @mapped_as_page: true when buffer was mapped with skb_frag_dma_map(),
> * false when buffer was mapped with dma_map_single()
> */
> -struct macb_tx_skb {
> - struct sk_buff *skb;
> +struct macb_tx_buff {
> + void *skb;
> dma_addr_t mapping;
> size_t size;
> bool mapped_as_page;
[ ... ]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH net-next v2 7/8] net: macb: make tx path skb agnostic
2026-02-23 18:26 [PATCH net-next v2 0/8] net: macb: Add XDP support and page pool integration Paolo Valerio
` (5 preceding siblings ...)
2026-02-23 18:26 ` [PATCH net-next v2 6/8] net: macb: make macb_tx_skb generic Paolo Valerio
@ 2026-02-23 18:26 ` Paolo Valerio
2026-02-24 0:09 ` [net-next,v2,7/8] " Jakub Kicinski
2026-02-23 18:26 ` [PATCH net-next v2 8/8] net: macb: introduce xmit support Paolo Valerio
7 siblings, 1 reply; 20+ messages in thread
From: Paolo Valerio @ 2026-02-23 18:26 UTC (permalink / raw)
To: netdev
Cc: Nicolas Ferre, Claudiu Beznea, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lorenzo Bianconi,
Théo Lebrun
Rename macb_tx_buff member skb to ptr and introduce macb_tx_buff_type
to identify the buffer type macb_tx_buff represents.
This is the last preparatory step for xdp xmit support.
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
drivers/net/ethernet/cadence/macb.h | 23 +++++++++++-----
drivers/net/ethernet/cadence/macb_main.c | 35 ++++++++++++++----------
2 files changed, 37 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 4e3cc0e9ea87..67fef88af2c6 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -959,19 +959,28 @@ struct macb_dma_desc_ptp {
/* Scaled PPM fraction */
#define PPM_FRACTION 16
-/* struct macb_tx_buff - data about an skb which is being transmitted
- * @skb: skb currently being transmitted, only set for the last buffer
- * of the frame
+enum macb_tx_buff_type {
+ MACB_TYPE_SKB,
+ MACB_TYPE_XDP_TX,
+ MACB_TYPE_XDP_NDO,
+};
+
+/* struct macb_tx_buff - data about an skb or xdp frame which is being
+ * transmitted.
+ * @ptr: pointer to skb or xdp frame being transmitted, only set
+ * for the last buffer for sk_buff
* @mapping: DMA address of the skb's fragment buffer
* @size: size of the DMA mapped buffer
* @mapped_as_page: true when buffer was mapped with skb_frag_dma_map(),
* false when buffer was mapped with dma_map_single()
+ * @type: type of buffer (MACB_TYPE_SKB, MACB_TYPE_XDP_TX, MACB_TYPE_XDP_NDO)
*/
struct macb_tx_buff {
- void *skb;
- dma_addr_t mapping;
- size_t size;
- bool mapped_as_page;
+ void *ptr;
+ dma_addr_t mapping;
+ size_t size;
+ bool mapped_as_page;
+ enum macb_tx_buff_type type;
};
/* Hardware-collected statistics. Used when updating the network
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index f65f976123fd..50646ee90672 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -967,7 +967,8 @@ static int macb_halt_tx(struct macb *bp)
bp, TSR);
}
-static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff, int budget)
+static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff,
+ int budget)
{
if (tx_buff->mapping) {
if (tx_buff->mapped_as_page)
@@ -979,9 +980,9 @@ static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff, int bud
tx_buff->mapping = 0;
}
- if (tx_buff->skb) {
- napi_consume_skb(tx_buff->skb, budget);
- tx_buff->skb = NULL;
+ if (tx_buff->ptr) {
+ napi_consume_skb(tx_buff->ptr, budget);
+ tx_buff->ptr = NULL;
}
}
@@ -1068,7 +1069,7 @@ static void macb_tx_error_task(struct work_struct *work)
desc = macb_tx_desc(queue, tail);
ctrl = desc->ctrl;
tx_buff = macb_tx_buff(queue, tail);
- skb = tx_buff->skb;
+ skb = tx_buff->ptr;
if (ctrl & MACB_BIT(TX_USED)) {
/* skb is set for the last buffer of the frame */
@@ -1076,7 +1077,7 @@ static void macb_tx_error_task(struct work_struct *work)
macb_tx_unmap(bp, tx_buff, 0);
tail++;
tx_buff = macb_tx_buff(queue, tail);
- skb = tx_buff->skb;
+ skb = tx_buff->ptr;
}
/* ctrl still refers to the first buffer descriptor
@@ -1204,7 +1205,9 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
/* Process all buffers of the current transmitted frame */
for (;; tail++) {
tx_buff = macb_tx_buff(queue, tail);
- skb = tx_buff->skb;
+
+ if (tx_buff->type == MACB_TYPE_SKB)
+ skb = tx_buff->ptr;
/* First, update TX stats if needed */
if (skb) {
@@ -2170,7 +2173,8 @@ static unsigned int macb_tx_map(struct macb *bp,
goto dma_error;
/* Save info to properly release resources */
- tx_buff->skb = NULL;
+ tx_buff->ptr = NULL;
+ tx_buff->type = MACB_TYPE_SKB;
tx_buff->mapping = mapping;
tx_buff->size = size;
tx_buff->mapped_as_page = false;
@@ -2198,7 +2202,8 @@ static unsigned int macb_tx_map(struct macb *bp,
goto dma_error;
/* Save info to properly release resources */
- tx_buff->skb = NULL;
+ tx_buff->ptr = NULL;
+ tx_buff->type = MACB_TYPE_SKB;
tx_buff->mapping = mapping;
tx_buff->size = size;
tx_buff->mapped_as_page = true;
@@ -2216,7 +2221,8 @@ static unsigned int macb_tx_map(struct macb *bp,
}
/* This is the last buffer of the frame: save socket buffer */
- tx_buff->skb = skb;
+ tx_buff->ptr = skb;
+ tx_buff->type = MACB_TYPE_SKB;
/* Update TX ring: update buffer descriptors in reverse order
* to avoid race condition
@@ -5136,8 +5142,9 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
netif_stop_queue(dev);
/* Store packet information (to free when Tx completed) */
- lp->rm9200_txq[desc].skb = skb;
+ lp->rm9200_txq[desc].ptr = skb;
lp->rm9200_txq[desc].size = skb->len;
+ lp->rm9200_txq[desc].type = MACB_TYPE_SKB;
lp->rm9200_txq[desc].mapping = dma_map_single(&lp->pdev->dev, skb->data,
skb->len, DMA_TO_DEVICE);
if (dma_mapping_error(&lp->pdev->dev, lp->rm9200_txq[desc].mapping)) {
@@ -5229,9 +5236,9 @@ static irqreturn_t at91ether_interrupt(int irq, void *dev_id)
dev->stats.tx_errors++;
desc = 0;
- if (lp->rm9200_txq[desc].skb) {
- dev_consume_skb_irq(lp->rm9200_txq[desc].skb);
- lp->rm9200_txq[desc].skb = NULL;
+ if (lp->rm9200_txq[desc].ptr) {
+ dev_consume_skb_irq(lp->rm9200_txq[desc].ptr);
+ lp->rm9200_txq[desc].ptr = NULL;
dma_unmap_single(&lp->pdev->dev, lp->rm9200_txq[desc].mapping,
lp->rm9200_txq[desc].size, DMA_TO_DEVICE);
dev->stats.tx_packets++;
--
2.52.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [net-next,v2,7/8] net: macb: make tx path skb agnostic
2026-02-23 18:26 ` [PATCH net-next v2 7/8] net: macb: make tx path skb agnostic Paolo Valerio
@ 2026-02-24 0:09 ` Jakub Kicinski
2026-02-25 18:36 ` Paolo Valerio
0 siblings, 1 reply; 20+ messages in thread
From: Jakub Kicinski @ 2026-02-24 0:09 UTC (permalink / raw)
To: pvalerio
Cc: Jakub Kicinski, davem, claudiu.beznea, nicolas.ferre, edumazet,
pabeni, theo.lebrun, netdev, lorenzo, andrew+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.
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: macb: make tx path skb agnostic
This commit renames the skb member of macb_tx_buff to ptr and introduces
macb_tx_buff_type to identify the buffer type. This is preparatory work
for XDP transmit support.
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 4e3cc0e9ea87..67fef88af2c6 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
[ ... ]
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index f65f976123fd..50646ee90672 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
> @@ -1204,7 +1205,9 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
> /* Process all buffers of the current transmitted frame */
> for (;; tail++) {
> tx_buff = macb_tx_buff(queue, tail);
> - skb = tx_buff->skb;
> +
> + if (tx_buff->type == MACB_TYPE_SKB)
> + skb = tx_buff->ptr;
>
> /* First, update TX stats if needed */
> if (skb) {
In macb_tx_complete(), the local variable skb is declared without
initialization at the start of the outer for loop. The patch changes the
unconditional assignment to a conditional one based on tx_buff->type.
When tx_buff->type is not MACB_TYPE_SKB, skb remains uninitialized with
whatever garbage value was on the stack. The subsequent "if (skb)" check
will likely evaluate as true on this garbage pointer, causing dereferences
of skb->len, skb->data, and skb_shinfo(skb) on invalid memory.
This could lead to a crash or memory corruption. Should skb be initialized
to NULL at declaration?
Note: While all buffers are MACB_TYPE_SKB in this commit, the commit message
states this is the "last preparatory step for xdp xmit support," suggesting
the next commit will introduce non-SKB buffer types that would trigger this
issue.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next,v2,7/8] net: macb: make tx path skb agnostic
2026-02-24 0:09 ` [net-next,v2,7/8] " Jakub Kicinski
@ 2026-02-25 18:36 ` Paolo Valerio
0 siblings, 0 replies; 20+ messages in thread
From: Paolo Valerio @ 2026-02-25 18:36 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jakub Kicinski, davem, claudiu.beznea, nicolas.ferre, edumazet,
pabeni, theo.lebrun, netdev, lorenzo, andrew+netdev
On 23 Feb 2026 at 04:09:00 PM, Jakub Kicinski <kuba@kernel.org> wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> net: macb: make tx path skb agnostic
>
> This commit renames the skb member of macb_tx_buff to ptr and introduces
> macb_tx_buff_type to identify the buffer type. This is preparatory work
> for XDP transmit support.
>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 4e3cc0e9ea87..67fef88af2c6 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index f65f976123fd..50646ee90672 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>
> [ ... ]
>
>> @@ -1204,7 +1205,9 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>> /* Process all buffers of the current transmitted frame */
>> for (;; tail++) {
>> tx_buff = macb_tx_buff(queue, tail);
>> - skb = tx_buff->skb;
>> +
>> + if (tx_buff->type == MACB_TYPE_SKB)
>> + skb = tx_buff->ptr;
>>
>> /* First, update TX stats if needed */
>> if (skb) {
>
> In macb_tx_complete(), the local variable skb is declared without
> initialization at the start of the outer for loop. The patch changes the
> unconditional assignment to a conditional one based on tx_buff->type.
>
> When tx_buff->type is not MACB_TYPE_SKB, skb remains uninitialized with
> whatever garbage value was on the stack. The subsequent "if (skb)" check
> will likely evaluate as true on this garbage pointer, causing dereferences
> of skb->len, skb->data, and skb_shinfo(skb) on invalid memory.
>
> This could lead to a crash or memory corruption. Should skb be initialized
> to NULL at declaration?
>
The branch on MACB_TYPE_SKB is a bit redundant, so I guess we
can either remove that or initialize skb at declaration to improve
clarity. There should be no potential NULL dereference anyways
(for the note below).
Next patch initializes the local variable and should be fine as well.
> Note: While all buffers are MACB_TYPE_SKB in this commit, the commit message
> states this is the "last preparatory step for xdp xmit support," suggesting
> the next commit will introduce non-SKB buffer types that would trigger this
> issue.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH net-next v2 8/8] net: macb: introduce xmit support
2026-02-23 18:26 [PATCH net-next v2 0/8] net: macb: Add XDP support and page pool integration Paolo Valerio
` (6 preceding siblings ...)
2026-02-23 18:26 ` [PATCH net-next v2 7/8] net: macb: make tx path skb agnostic Paolo Valerio
@ 2026-02-23 18:26 ` Paolo Valerio
2026-02-24 0:09 ` [net-next,v2,8/8] " Jakub Kicinski
7 siblings, 1 reply; 20+ messages in thread
From: Paolo Valerio @ 2026-02-23 18:26 UTC (permalink / raw)
To: netdev
Cc: Nicolas Ferre, Claudiu Beznea, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lorenzo Bianconi,
Théo Lebrun
Add XDP_TX verdict support, also introduce ndo_xdp_xmit function for
redirection, and update macb_tx_unmap() to handle both skbs and xdp
frames advertising NETDEV_XDP_ACT_NDO_XMIT capability and the ability
to process XDP_TX verdicts.
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
drivers/net/ethernet/cadence/macb_main.c | 171 +++++++++++++++++++++--
1 file changed, 162 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 50646ee90672..69392ec0065f 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -967,6 +967,17 @@ static int macb_halt_tx(struct macb *bp)
bp, TSR);
}
+static void macb_tx_release_buff(void *buff, enum macb_tx_buff_type type, int budget)
+{
+ if (type == MACB_TYPE_SKB) {
+ napi_consume_skb(buff, budget);
+ } else if (type == MACB_TYPE_XDP_TX) {
+ xdp_return_frame_rx_napi(buff);
+ } else {
+ xdp_return_frame(buff);
+ }
+}
+
static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff,
int budget)
{
@@ -981,7 +992,7 @@ static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff,
}
if (tx_buff->ptr) {
- napi_consume_skb(tx_buff->ptr, budget);
+ macb_tx_release_buff(tx_buff->ptr, tx_buff->type, budget);
tx_buff->ptr = NULL;
}
}
@@ -1069,6 +1080,10 @@ static void macb_tx_error_task(struct work_struct *work)
desc = macb_tx_desc(queue, tail);
ctrl = desc->ctrl;
tx_buff = macb_tx_buff(queue, tail);
+
+ if (tx_buff->type != MACB_TYPE_SKB)
+ goto unmap;
+
skb = tx_buff->ptr;
if (ctrl & MACB_BIT(TX_USED)) {
@@ -1106,6 +1121,7 @@ static void macb_tx_error_task(struct work_struct *work)
desc->ctrl = ctrl | MACB_BIT(TX_USED);
}
+unmap:
macb_tx_unmap(bp, tx_buff, 0);
}
@@ -1184,6 +1200,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
spin_lock_irqsave(&queue->tx_ptr_lock, flags);
head = queue->tx_head;
for (tail = queue->tx_tail; tail != head && packets < budget; tail++) {
+ void *data = NULL;
struct macb_tx_buff *tx_buff;
struct sk_buff *skb;
struct macb_dma_desc *desc;
@@ -1206,11 +1223,16 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
for (;; tail++) {
tx_buff = macb_tx_buff(queue, tail);
- if (tx_buff->type == MACB_TYPE_SKB)
- skb = tx_buff->ptr;
+ if (tx_buff->type != MACB_TYPE_SKB) {
+ data = tx_buff->ptr;
+ goto unmap;
+ }
/* First, update TX stats if needed */
- if (skb) {
+ if (tx_buff->type == MACB_TYPE_SKB && tx_buff->ptr) {
+ data = tx_buff->ptr;
+ skb = tx_buff->ptr;
+
if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
!ptp_one_step_sync(skb))
gem_ptp_do_txstamp(bp, skb, desc);
@@ -1226,6 +1248,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
bytes += skb->len;
}
+unmap:
/* Now we can safely release resources */
macb_tx_unmap(bp, tx_buff, budget);
@@ -1233,7 +1256,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
* WARNING: at this point skb has been freed by
* macb_tx_unmap().
*/
- if (skb)
+ if (data)
break;
}
}
@@ -1363,10 +1386,127 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
*/
}
+static int macb_xdp_submit_frame(struct macb *bp, struct xdp_frame *xdpf,
+ struct net_device *dev, bool dma_map,
+ dma_addr_t addr)
+{
+ enum macb_tx_buff_type buff_type;
+ struct macb_tx_buff *tx_buff;
+ int cpu = smp_processor_id();
+ struct macb_dma_desc *desc;
+ struct macb_queue *queue;
+ unsigned int next_head;
+ unsigned long flags;
+ dma_addr_t mapping;
+ u16 queue_index;
+ int err = 0;
+ u32 ctrl;
+
+ queue_index = cpu % bp->num_queues;
+ queue = &bp->queues[queue_index];
+ buff_type = dma_map ? MACB_TYPE_XDP_NDO : MACB_TYPE_XDP_TX;
+
+ spin_lock_irqsave(&queue->tx_ptr_lock, flags);
+
+ /* This is a hard error, log it. */
+ if (CIRC_SPACE(queue->tx_head, queue->tx_tail, bp->tx_ring_size) < 1) {
+ netif_stop_subqueue(dev, queue_index);
+ netdev_dbg(bp->dev, "tx_head = %u, tx_tail = %u\n",
+ queue->tx_head, queue->tx_tail);
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ if (dma_map) {
+ mapping = dma_map_single(&bp->pdev->dev,
+ xdpf->data,
+ xdpf->len, DMA_TO_DEVICE);
+ if (unlikely(dma_mapping_error(&bp->pdev->dev, mapping))) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+ } else {
+ mapping = addr;
+ dma_sync_single_for_device(&bp->pdev->dev, mapping,
+ xdpf->len, DMA_BIDIRECTIONAL);
+ }
+
+ next_head = queue->tx_head + 1;
+
+ ctrl = MACB_BIT(TX_USED);
+ desc = macb_tx_desc(queue, next_head);
+ desc->ctrl = ctrl;
+
+ desc = macb_tx_desc(queue, queue->tx_head);
+ tx_buff = macb_tx_buff(queue, queue->tx_head);
+ tx_buff->ptr = xdpf;
+ tx_buff->type = buff_type;
+ tx_buff->mapping = dma_map ? mapping : 0;
+ tx_buff->size = xdpf->len;
+ tx_buff->mapped_as_page = false;
+
+ ctrl = (u32)tx_buff->size;
+ ctrl |= MACB_BIT(TX_LAST);
+
+ if (unlikely(macb_tx_ring_wrap(bp, queue->tx_head) == (bp->tx_ring_size - 1)))
+ ctrl |= MACB_BIT(TX_WRAP);
+
+ /* Set TX buffer descriptor */
+ macb_set_addr(bp, desc, mapping);
+ /* desc->addr must be visible to hardware before clearing
+ * 'TX_USED' bit in desc->ctrl.
+ */
+ wmb();
+ desc->ctrl = ctrl;
+ queue->tx_head = next_head;
+
+ /* Make newly initialized descriptor visible to hardware */
+ wmb();
+
+ spin_lock(&bp->lock);
+ macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
+ spin_unlock(&bp->lock);
+
+ if (CIRC_SPACE(queue->tx_head, queue->tx_tail, bp->tx_ring_size) < 1)
+ netif_stop_subqueue(dev, queue_index);
+
+unlock:
+ spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);
+
+ if (err)
+ macb_tx_release_buff(xdpf, buff_type, 0);
+
+ return err;
+}
+
+static int gem_xdp_xmit(struct net_device *dev, int num_frame,
+ struct xdp_frame **frames, u32 flags)
+{
+ struct macb *bp = netdev_priv(dev);
+ u32 xmitted = 0;
+ int i;
+
+ if (!macb_is_gem(bp))
+ return -EOPNOTSUPP;
+
+ if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
+ return -EINVAL;
+
+ for (i = 0; i < num_frame; i++) {
+ if (macb_xdp_submit_frame(bp, frames[i], dev, true, 0))
+ break;
+
+ xmitted++;
+ }
+
+ return xmitted;
+}
+
static u32 gem_xdp_run(struct macb_queue *queue, void *buff_head,
- unsigned int len)
+ unsigned int len, dma_addr_t addr)
{
struct net_device *dev;
+ struct xdp_frame *xdpf;
struct bpf_prog *prog;
struct xdp_buff xdp;
@@ -1393,6 +1533,18 @@ static u32 gem_xdp_run(struct macb_queue *queue, void *buff_head,
break;
}
goto out;
+ case XDP_TX:
+ xdpf = xdp_convert_buff_to_frame(&xdp);
+
+ if (unlikely(!xdpf)) {
+ act = XDP_DROP;
+ break;
+ }
+
+ if (macb_xdp_submit_frame(queue->bp, xdpf, dev, false,
+ addr))
+ act = XDP_DROP;
+ goto out;
default:
bpf_warn_invalid_xdp_action(dev, prog, act);
fallthrough;
@@ -1480,7 +1632,7 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
if (first_frame) {
if (last_frame) {
- ret = gem_xdp_run(queue, buff_head, data_len);
+ ret = gem_xdp_run(queue, buff_head, data_len, addr);
if (ret == XDP_REDIRECT)
xdp_flush = true;
@@ -4620,6 +4772,7 @@ static const struct net_device_ops macb_netdev_ops = {
.ndo_hwtstamp_get = macb_hwtstamp_get,
.ndo_setup_tc = macb_setup_tc,
.ndo_bpf = gem_xdp,
+ .ndo_xdp_xmit = gem_xdp_xmit,
};
/* Configure peripheral capabilities according to device tree
@@ -5920,8 +6073,8 @@ static int macb_probe(struct platform_device *pdev)
if (macb_is_gem(bp))
dev->xdp_features = NETDEV_XDP_ACT_BASIC |
- NETDEV_XDP_ACT_REDIRECT;
-
+ NETDEV_XDP_ACT_REDIRECT |
+ NETDEV_XDP_ACT_NDO_XMIT;
netif_carrier_off(dev);
err = register_netdev(dev);
--
2.52.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [net-next,v2,8/8] net: macb: introduce xmit support
2026-02-23 18:26 ` [PATCH net-next v2 8/8] net: macb: introduce xmit support Paolo Valerio
@ 2026-02-24 0:09 ` Jakub Kicinski
0 siblings, 0 replies; 20+ messages in thread
From: Jakub Kicinski @ 2026-02-24 0:09 UTC (permalink / raw)
To: pvalerio
Cc: Jakub Kicinski, davem, claudiu.beznea, nicolas.ferre, edumazet,
pabeni, theo.lebrun, netdev, lorenzo, andrew+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.
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: macb: introduce xmit support
This commit adds XDP_TX verdict support and introduces ndo_xdp_xmit
function for redirection. It updates macb_tx_unmap() to handle both
skbs and xdp frames advertising NETDEV_XDP_ACT_NDO_XMIT capability.
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 50646ee90672..69392ec0065f 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
> @@ -967,6 +967,17 @@ static int macb_halt_tx(struct macb *bp)
> bp, TSR);
> }
>
> +static void macb_tx_release_buff(void *buff, enum macb_tx_buff_type type, int budget)
> +{
> + if (type == MACB_TYPE_SKB) {
> + napi_consume_skb(buff, budget);
> + } else if (type == MACB_TYPE_XDP_TX) {
> + xdp_return_frame_rx_napi(buff);
^^^^
Can xdp_return_frame_rx_napi() be called in workqueue context?
This function is only safe in NAPI or softirq context because it calls
__xdp_return() with napi_direct=true, which eventually calls
bpf_net_ctx_get_ri() -> bpf_net_ctx_get(), dereferencing
current->bpf_net_context without a NULL check.
In NAPI context, bpf_net_context is set up by bpf_net_ctx_set() in
net_rx_action/napi_poll. In workqueue context, current->bpf_net_context
is NULL.
macb_tx_release_buff() is called from macb_tx_unmap(), which is called
from macb_tx_error_task(). macb_tx_error_task() runs as a workqueue
task, scheduled by:
macb_interrupt() ->
schedule_work(&queue->tx_error_task) ->
macb_tx_error_task() ->
macb_tx_unmap() ->
macb_tx_release_buff(ptr, MACB_TYPE_XDP_TX, 0) ->
xdp_return_frame_rx_napi() ->
__xdp_return(napi_direct=true) ->
xdp_return_frame_no_direct() ->
bpf_net_ctx_get_ri() ->
NULL deref crash
Should this use xdp_return_frame() instead of xdp_return_frame_rx_napi()
when not in NAPI context, similar to how napi_consume_skb() uses the
budget parameter to distinguish contexts (budget=0 means non-NAPI)?
> + } else {
> + xdp_return_frame(buff);
> + }
> +}
> +
[ ... ]
> @@ -1069,6 +1080,10 @@ static void macb_tx_error_task(struct work_struct *work)
> desc = macb_tx_desc(queue, tail);
> ctrl = desc->ctrl;
> tx_buff = macb_tx_buff(queue, tail);
> +
> + if (tx_buff->type != MACB_TYPE_SKB)
> + goto unmap;
> +
> skb = tx_buff->ptr;
Does macb_tx_error_task() hold the necessary locks when accessing the TX
ring?
macb_tx_error_task accesses queue->tx_head (loop boundary in line 1077)
and queue->tx_tail without holding queue->tx_ptr_lock. It only holds
bp->lock.
The new macb_xdp_submit_frame modifies queue->tx_head (line 1461) under
tx_ptr_lock but BEFORE acquiring the nested bp->lock (line 1464).
macb_tx_error_task only calls napi_disable(&queue->napi_tx), so RX NAPI
(which calls gem_xdp_run -> macb_xdp_submit_frame for XDP_TX) remains
active. netif_tx_stop_all_queues does not stop ndo_xdp_xmit (gem_xdp_xmit
-> macb_xdp_submit_frame).
The race can occur:
CPU 0: macb_tx_error_task
- holds bp->lock
- iterates ring using tx_head as boundary
CPU 1: macb_xdp_submit_frame
- acquires tx_ptr_lock
- submits a frame
- updates tx_head
- blocks on bp->lock
CPU 0:
- sees updated tx_head
- iterates into newly submitted frame
- frees its buffer via macb_tx_unmap
- resets tx_head=0, tx_tail=0
CPU 1:
- acquires bp->lock
- kicks TSTART on reset ring
This results in a freed DMA buffer in an active ring, and inconsistent
tx_head/tx_tail state.
[ ... ]
> @@ -1184,6 +1200,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
> spin_lock_irqsave(&queue->tx_ptr_lock, flags);
> head = queue->tx_head;
> for (tail = queue->tx_tail; tail != head && packets < budget; tail++) {
> + void *data = NULL;
> struct macb_tx_buff *tx_buff;
> struct sk_buff *skb;
> struct macb_dma_desc *desc;
[ ... ]
> @@ -1206,11 +1223,16 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
> for (;; tail++) {
> tx_buff = macb_tx_buff(queue, tail);
>
> - if (tx_buff->type == MACB_TYPE_SKB)
> - skb = tx_buff->ptr;
> + if (tx_buff->type != MACB_TYPE_SKB) {
> + data = tx_buff->ptr;
> + goto unmap;
> + }
>
> /* First, update TX stats if needed */
> - if (skb) {
> + if (tx_buff->type == MACB_TYPE_SKB && tx_buff->ptr) {
^^^^
This isn't a bug, but could the type check here be simplified?
At line 1226, non-SKB types are filtered out with 'if (tx_buff->type !=
MACB_TYPE_SKB) goto unmap'. At line 1232, the condition checks the type
again with 'if (tx_buff->type == MACB_TYPE_SKB && tx_buff->ptr)', but at
this point the type is always MACB_TYPE_SKB.
The condition could be simplified to 'if (tx_buff->ptr)' for clarity.
> + data = tx_buff->ptr;
> + skb = tx_buff->ptr;
> +
[ ... ]
^ permalink raw reply [flat|nested] 20+ messages in thread