Re: [net-next,v2,5/8] net: macb: add XDP support for gem
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-02-24 00:08:51
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.
quoted hunk ↗ jump to hunk
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>
[ ... ]
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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); +}
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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().
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.