[PATCH net-next v2 0/5] fbnic: Support larger RX pages

[PATCH net-next v2 0/5] fbnic: Support larger RX pages

[Björn Töpel]

Hi!

The v1 was out end-of-May! I hope you didn't hold your breath! ;-)
Here goes...

This series adds support for large io_uring zero-copy RX buffers to
fbnic.

fbnic hardware consumes receive memory in 4 KiB device pages, while
io_uring may expose a larger software buffer, such as 256 KiB. Track
the device-page geometry per BDQ, represent each large buffer with
consecutive descriptors, and decode completions to the corresponding
offset within the buffer. HPQ keeps its existing PAGE_SIZE geometry.

Larger buffers also reduce pressure on the memory provider. One
provider allocation supplies multiple 4 KiB device pages, so the same
amount of receive memory needs fewer net_iov objects and fewer
allocation, refill, and recycle operations. Hardware descriptor use
remains unchanged.

Ring depth and buffer size must be validated together. An ethtool ring
change can otherwise make an active memory-provider configuration
invalid. Store accepted ring depths in netdev_config, stage proposed
values in cfg_pending, and validate the default and each RX queue
before committing the change. Both netlink and ioctl use the same
transaction helpers.

The selftest requests a buffer larger than twice the MTU and requires
a receive CQE to end past the first MTU-sized region. Drivers which
reject the size are skipped. Drivers which accept it but only use that
first region fail.

Changes in v2
=============

- Add generic netdev_config lifecycle helpers, and use
  netdev_reconfig_start() and __netdev_free_config() from both the
  netlink and ioctl paths.
- Store ring parameters in netdev_config and revalidate the default and
  each RX queue before committing a ring change.
- Preserve the distinction between accepted and applied HDS state:
  unrelated SET operations retain an accepted auto value, while
  RINGS_GET reports the state applied by the driver.
- Track the fbnic device-page geometry per BDQ and use it when posting
  buffers and decoding completions.
- Validate fbnic RX page size, payload fragment count, and the number of
  software entries available in the payload queue.
- Replace the manual-only offset check with a portable check run by the
  Python large-buffer test, and include the received byte count on
  timeout.
- Rebase on net-next.

v1: https://lore.kernel.org/netdev/20260522113225.241337-1-bjorn@kernel.org/

Björn Töpel (4):
  fbnic: Track BDQ device-page geometry per ring
  net: Revalidate queue config for ringparam changes
  fbnic: Support larger memory-provider RX pages
  selftests: drv-net: Test large zcrx buffers

Jakub Kicinski (1):
  net: Add netdev_config helpers

 drivers/net/ethernet/meta/fbnic/fbnic_csr.h   |  34 +---
 .../net/ethernet/meta/fbnic/fbnic_debugfs.c   |   5 +-
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |   8 +-
 .../net/ethernet/meta/fbnic/fbnic_netdev.c    |   5 +
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c  | 165 ++++++++++++++----
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.h  |  12 ++
 include/net/netdev_queues.h                   |  48 ++++-
 net/core/dev.c                                |   7 +-
 net/core/dev.h                                |   7 +
 net/core/netdev_config.c                      |  75 +++++++-
 net/ethtool/common.c                          |   9 +
 net/ethtool/common.h                          |   2 +
 net/ethtool/ioctl.c                           |  24 ++-
 net/ethtool/netlink.c                         |  15 +-
 net/ethtool/rings.c                           |  13 +-
 .../selftests/drivers/net/hw/iou-zcrx.c       |  33 +++-
 .../selftests/drivers/net/hw/iou-zcrx.py      |   6 +-
 17 files changed, 374 insertions(+), 94 deletions(-)


base-commit: 0abe8777490ebc008635ecb20297761d1b317697
-- 
2.55.0

[PATCH net-next v2 1/5] net: Add netdev_config helpers

[Björn Töpel]

From: Jakub Kicinski <kuba@kernel.org>

netdev_config manipulation will become slightly more complicated
soon and will be used by both ethtool and the queue API.
Encapsulate the logic in helper functions.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 net/core/dev.c           |  7 ++-----
 net/core/dev.h           |  5 +++++
 net/core/netdev_config.c | 37 +++++++++++++++++++++++++++++++++++++
 net/ethtool/netlink.c    | 15 +++++++--------
 4 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 290e0f099e6b..4a7c5a5e48e5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -12195,10 +12195,8 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	if (!dev->ethtool)
 		goto free_all;
 
-	dev->cfg = kzalloc_obj(*dev->cfg, GFP_KERNEL_ACCOUNT);
-	if (!dev->cfg)
+	if (netdev_alloc_config(dev))
 		goto free_all;
-	dev->cfg_pending = dev->cfg;
 
 	dev->num_napi_configs = maxqs;
 	napi_config_sz = array_size(maxqs, sizeof(*dev->napi_config));
@@ -12270,8 +12268,7 @@ void free_netdev(struct net_device *dev)
 		return;
 	}
 
-	WARN_ON(dev->cfg != dev->cfg_pending);
-	kfree(dev->cfg);
+	netdev_free_config(dev);
 	kfree(dev->ethtool);
 	netif_free_tx_queues(dev);
 	netif_free_rx_queues(dev);
diff --git a/net/core/dev.h b/net/core/dev.h
index b757faead4d1..4b52ff779cba 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -102,6 +102,11 @@ extern struct rw_semaphore dev_addr_sem;
 extern struct list_head net_todo_list;
 void netdev_run_todo(void);
 
+int netdev_alloc_config(struct net_device *dev);
+void __netdev_free_config(struct netdev_config *cfg);
+void netdev_free_config(struct net_device *dev);
+int netdev_reconfig_start(struct net_device *dev);
+
 int netdev_queue_config_validate(struct net_device *dev, int rxq_idx,
 				 struct netdev_queue_config *qcfg,
 				 struct netlink_ext_ack *extack);
diff --git a/net/core/netdev_config.c b/net/core/netdev_config.c
index f14af365d5cd..b101341e3251 100644
--- a/net/core/netdev_config.c
+++ b/net/core/netdev_config.c
@@ -6,6 +6,43 @@
 
 #include "dev.h"
 
+int netdev_alloc_config(struct net_device *dev)
+{
+	struct netdev_config *cfg;
+
+	cfg = kzalloc_obj(*dev->cfg, GFP_KERNEL_ACCOUNT);
+	if (!cfg)
+		return -ENOMEM;
+
+	dev->cfg = cfg;
+	dev->cfg_pending = cfg;
+	return 0;
+}
+
+void __netdev_free_config(struct netdev_config *cfg)
+{
+	kfree(cfg);
+}
+
+void netdev_free_config(struct net_device *dev)
+{
+	WARN_ON(dev->cfg != dev->cfg_pending);
+	__netdev_free_config(dev->cfg);
+}
+
+int netdev_reconfig_start(struct net_device *dev)
+{
+	struct netdev_config *cfg;
+
+	WARN_ON(dev->cfg != dev->cfg_pending);
+	cfg = kmemdup(dev->cfg, sizeof(*dev->cfg), GFP_KERNEL_ACCOUNT);
+	if (!cfg)
+		return -ENOMEM;
+
+	dev->cfg_pending = cfg;
+	return 0;
+}
+
 static int netdev_nop_validate_qcfg(struct net_device *dev,
 				    struct netdev_queue_config *qcfg,
 				    struct netlink_ext_ack *extack)
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 1af395b54330..383e911f50f7 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -11,6 +11,8 @@
 #include "module_fw.h"
 #include "netlink.h"
 
+#include "../core/dev.h"
+
 static struct genl_family ethtool_genl_family;
 
 static bool ethnl_ok __read_mostly;
@@ -934,12 +936,9 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	if (need_rtnl)
 		rtnl_lock();
 	netdev_lock_ops(dev);
-	dev->cfg_pending = kmemdup(dev->cfg, sizeof(*dev->cfg),
-				   GFP_KERNEL_ACCOUNT);
-	if (!dev->cfg_pending) {
-		ret = -ENOMEM;
-		goto out_tie_cfg;
-	}
+	ret = netdev_reconfig_start(dev);
+	if (ret)
+		goto out_unlock;
 
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
@@ -958,9 +957,9 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 out_ops:
 	ethnl_ops_complete(dev);
 out_free_cfg:
-	kfree(dev->cfg_pending);
-out_tie_cfg:
+	__netdev_free_config(dev->cfg_pending);
 	dev->cfg_pending = dev->cfg;
+out_unlock:
 	netdev_unlock_ops(dev);
 	if (need_rtnl)
 		rtnl_unlock();
-- 
2.55.0

Re: [PATCH net-next v2 1/5] net: Add netdev_config helpers

[Breno Leitao]

On Thu, Sep 10, 2026 at 08:09:01PM +0200, Björn Töpel wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 
> netdev_config manipulation will become slightly more complicated
> soon and will be used by both ethtool and the queue API.
> Encapsulate the logic in helper functions.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Björn Töpel <bjorn@kernel.org>

Reviewed-by: Breno Leitao <leitao@debian.org>

> +void __netdev_free_config(struct netdev_config *cfg)
> +{
> +	kfree(cfg);
> +}

When I read this one, I was expecting __netdev_free_config() to "become
more complicated", but, it continues to alway be a single kfree(). How
is this helper useful?

--breno

Re: [PATCH net-next v2 1/5] net: Add netdev_config helpers

[Jakub Kicinski]

On Fri, 11 Sep 2026 02:20:35 -0700 Breno Leitao wrote:
> > +void __netdev_free_config(struct netdev_config *cfg)
> > +{
> > +	kfree(cfg);
> > +}  
> 
> When I read this one, I was expecting __netdev_free_config() to "become
> more complicated", but, it continues to alway be a single kfree(). How
> is this helper useful?

It was taken out of a series which had per-queue config so there was
also an allocation of a variable size array. I asked Bjorn to add this
patch, anyway, otherwise we'd need to open code netdev_reconfig_start()
in a second place. The per-queue config will come sooner or later.

[PATCH net-next v2 2/5] fbnic: Track BDQ device-page geometry per ring

[Björn Töpel]

fbnic derives the BDQ buffer layout from PAGE_SIZE. That decides at
build time how a posted page is split into 4 KiB device pages, and
uses the same completion decoding for HPQ and PPQ. That is wrong once
the queues use different posted-page sizes; completions must be
decoded with the geometry of the queue that produced them.

Keep PAGE_SIZE as the posted-page size for both queues in this change,
so the descriptor format and runtime behavior stay unchanged.

Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 drivers/net/ethernet/meta/fbnic/fbnic_csr.h   | 34 ++------
 .../net/ethernet/meta/fbnic/fbnic_debugfs.c   |  5 +-
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c  | 83 ++++++++++++-------
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.h  |  6 ++
 4 files changed, 72 insertions(+), 56 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
index 64b958df7774..2b9f8644b132 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
@@ -109,34 +109,19 @@ enum {
 
 /* Rx Buffer Descriptor Format
  *
- * The layout of this can vary depending on the page size of the system.
+ * Buffer descriptors describe 4 KiB device pages. A posted page larger than
+ * 4 KiB is represented by consecutive device-page descriptors.
  *
- * If the page size is 4K then the layout will simply consist of ID for
- * the 16 most significant bits, and the lower 46 are essentially the page
- * address with the lowest 12 bits being reserved 0 due to the fact that
- * a page will be aligned.
- *
- * If the page size is larger than 4K then the lower n bits of the ID and
- * page address will be reserved for the fragment ID. This fragment will
- * be 4K in size and will be used to index both the DMA address and the ID
- * by the same amount.
+ * The address field stores the 4 KiB-aligned DMA address. The ID field stores
+ * the software page ID, with the low n bits used as the device-page ID when a
+ * posted page spans multiple device pages. The driver increments both the
+ * address and ID by one device page for each descriptor belonging to a posted
+ * page.
  */
 #define FBNIC_BD_DESC_ADDR_MASK			DESC_GENMASK(45, 12)
 #define FBNIC_BD_DESC_ID_MASK			DESC_GENMASK(63, 48)
-#define FBNIC_BD_FRAG_SIZE \
+#define FBNIC_BD_PAGE_SIZE \
 	(FBNIC_BD_DESC_ADDR_MASK & ~(FBNIC_BD_DESC_ADDR_MASK - 1))
-#define FBNIC_BD_FRAG_COUNT \
-	(PAGE_SIZE / FBNIC_BD_FRAG_SIZE)
-#define FBNIC_BD_FRAG_ADDR_MASK \
-	(FBNIC_BD_DESC_ADDR_MASK & \
-	 ~(FBNIC_BD_DESC_ADDR_MASK * FBNIC_BD_FRAG_COUNT))
-#define FBNIC_BD_FRAG_ID_MASK \
-	(FBNIC_BD_DESC_ID_MASK & \
-	 ~(FBNIC_BD_DESC_ID_MASK * FBNIC_BD_FRAG_COUNT))
-#define FBNIC_BD_PAGE_ADDR_MASK \
-	(FBNIC_BD_DESC_ADDR_MASK & ~FBNIC_BD_FRAG_ADDR_MASK)
-#define FBNIC_BD_PAGE_ID_MASK \
-	(FBNIC_BD_DESC_ID_MASK & ~FBNIC_BD_FRAG_ID_MASK)
 
 /* Rx Completion Queue Descriptors */
 #define FBNIC_RCD_TYPE_MASK			DESC_GENMASK(62, 61)
@@ -151,9 +136,6 @@ enum {
 
 /* Address/Length Completion Descriptors */
 #define FBNIC_RCD_AL_BUFF_ID_MASK		DESC_GENMASK(15, 0)
-#define FBNIC_RCD_AL_BUFF_FRAG_MASK		(FBNIC_BD_FRAG_COUNT - 1)
-#define FBNIC_RCD_AL_BUFF_PAGE_MASK \
-	(FBNIC_RCD_AL_BUFF_ID_MASK & ~FBNIC_RCD_AL_BUFF_FRAG_MASK)
 #define FBNIC_RCD_AL_BUFF_LEN_MASK		DESC_GENMASK(28, 16)
 #define FBNIC_RCD_AL_BUFF_OFF_MASK		DESC_GENMASK(43, 32)
 #define FBNIC_RCD_AL_PAGE_FIN			DESC_BIT(60)
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c b/drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c
index 3c4563c8f403..7f23a0f97e63 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c
@@ -181,8 +181,8 @@ static int fbnic_dbg_tcq_desc_seq_show(struct seq_file *s, void *v)
 static int fbnic_dbg_bdq_desc_seq_show(struct seq_file *s, void *v)
 {
 	struct fbnic_ring *ring = s->private;
+	unsigned int i, desc_count;
 	char hdr[80];
-	int i;
 
 	/* Generate header on first entry */
 	fbnic_dbg_ring_show(s);
@@ -197,7 +197,8 @@ static int fbnic_dbg_bdq_desc_seq_show(struct seq_file *s, void *v)
 		return 0;
 	}
 
-	for (i = 0; i < (ring->size_mask + 1) * FBNIC_BD_FRAG_COUNT; i++) {
+	desc_count = (ring->size_mask + 1) * fbnic_bd_page_count(ring);
+	for (i = 0; i < desc_count; i++) {
 		u64 bd = le64_to_cpu(ring->desc[i]);
 
 		seq_printf(s, "%04x %#04llx %#014llx\n", i,
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 401f8b8ae1ca..28b962307c08 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -871,19 +871,31 @@ static void fbnic_clean_bdq(struct fbnic_ring *ring, unsigned int hw_head,
 	ring->head = head;
 }
 
+static u16 fbnic_rcd_bd_idx(const struct fbnic_ring *bdq, u64 rcd)
+{
+	return FIELD_GET(FBNIC_RCD_AL_BUFF_ID_MASK, rcd) >> bdq->bd_page_shift;
+}
+
+static unsigned int fbnic_rcd_bd_page_offset(const struct fbnic_ring *bdq,
+					     u64 rcd)
+{
+	u16 id = FIELD_GET(FBNIC_RCD_AL_BUFF_ID_MASK, rcd);
+	u16 page_id = id & (fbnic_bd_page_count(bdq) - 1);
+
+	return page_id * FBNIC_BD_PAGE_SIZE;
+}
+
 static void fbnic_bd_prep(struct fbnic_ring *bdq, u16 id, netmem_ref netmem)
 {
-	__le64 *bdq_desc = &bdq->desc[id * FBNIC_BD_FRAG_COUNT];
+	__le64 *bdq_desc = &bdq->desc[id * fbnic_bd_page_count(bdq)];
 	dma_addr_t dma = page_pool_get_dma_addr_netmem(netmem);
-	u64 bd, i = FBNIC_BD_FRAG_COUNT;
+	u64 bd, i = fbnic_bd_page_count(bdq);
 
-	bd = (FBNIC_BD_PAGE_ADDR_MASK & dma) |
-	     FIELD_PREP(FBNIC_BD_PAGE_ID_MASK, id);
+	bd = (FBNIC_BD_DESC_ADDR_MASK & dma) |
+	     FIELD_PREP(FBNIC_BD_DESC_ID_MASK, (u64)id << bdq->bd_page_shift);
 
-	/* In the case that a page size is larger than 4K we will map a
-	 * single page to multiple fragments. The fragments will be
-	 * FBNIC_BD_FRAG_COUNT in size and the lower n bits will be use
-	 * to indicate the individual fragment IDs.
+	/* Posted pages larger than 4 KiB use consecutive device-page IDs in
+	 * the low bits of the software page ID.
 	 */
 	do {
 		*bdq_desc = cpu_to_le64(bd);
@@ -928,7 +940,7 @@ static void fbnic_fill_bdq(struct fbnic_ring *bdq)
 		/* Force DMA writes to flush before writing to tail */
 		dma_wmb();
 
-		writel(i * FBNIC_BD_FRAG_COUNT, bdq->doorbell);
+		writel(i * fbnic_bd_page_count(bdq), bdq->doorbell);
 	}
 }
 
@@ -959,26 +971,27 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
 			      struct fbnic_pkt_buff *pkt,
 			      struct fbnic_q_triad *qt)
 {
-	unsigned int hdr_pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
 	unsigned int hdr_pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
-	struct page *page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
 	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
+	unsigned int hdr_pg_idx = fbnic_rcd_bd_idx(&qt->sub0, rcd);
 	unsigned int frame_sz, hdr_pg_start, hdr_pg_end, headroom;
 	unsigned char *hdr_start;
+	struct page *page;
 
 	/* data_hard_start should always be NULL when this is called */
 	WARN_ON_ONCE(pkt->buff.data_hard_start);
 
+	page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
+
 	/* Short-cut the end calculation if we know page is fully consumed */
 	hdr_pg_end = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
-		     FBNIC_BD_FRAG_SIZE : fbnic_hdr_pg_end(hdr_pg_off, len);
+		     FBNIC_BD_PAGE_SIZE : fbnic_hdr_pg_end(hdr_pg_off, len);
 	hdr_pg_start = fbnic_hdr_pg_start(hdr_pg_off);
 
 	headroom = hdr_pg_off - hdr_pg_start + FBNIC_RX_PAD;
 	frame_sz = hdr_pg_end - hdr_pg_start;
 	xdp_init_buff(&pkt->buff, frame_sz, &qt->xdp_rxq);
-	hdr_pg_start += (FBNIC_RCD_AL_BUFF_FRAG_MASK & rcd) *
-			FBNIC_BD_FRAG_SIZE;
+	hdr_pg_start += fbnic_rcd_bd_page_offset(&qt->sub0, rcd);
 
 	/* Sync DMA buffer */
 	dma_sync_single_range_for_cpu(nv->dev, page_pool_get_dma_addr(page),
@@ -999,18 +1012,19 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
 			      struct fbnic_pkt_buff *pkt,
 			      struct fbnic_q_triad *qt)
 {
-	unsigned int pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
 	unsigned int pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
 	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
-	netmem_ref netmem = fbnic_page_pool_get_data(qt, pg_idx);
+	unsigned int pg_idx = fbnic_rcd_bd_idx(&qt->sub1, rcd);
 	unsigned int truesize;
+	netmem_ref netmem;
 	bool added;
 
-	truesize = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
-		   FBNIC_BD_FRAG_SIZE - pg_off : ALIGN(len, 128);
+	netmem = fbnic_page_pool_get_data(qt, pg_idx);
 
-	pg_off += (FBNIC_RCD_AL_BUFF_FRAG_MASK & rcd) *
-		  FBNIC_BD_FRAG_SIZE;
+	truesize = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
+		   FBNIC_BD_PAGE_SIZE - pg_off : ALIGN(len, 128);
+
+	pg_off += fbnic_rcd_bd_page_offset(&qt->sub1, rcd);
 
 	/* Sync DMA buffer */
 	page_pool_dma_sync_netmem_for_cpu(qt->sub1.page_pool, netmem,
@@ -1257,12 +1271,12 @@ static int fbnic_clean_rcq(struct fbnic_napi_vector *nv,
 
 		switch (FIELD_GET(FBNIC_RCD_TYPE_MASK, rcd)) {
 		case FBNIC_RCD_TYPE_HDR_AL:
-			head0 = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
+			head0 = fbnic_rcd_bd_idx(&qt->sub0, rcd);
 			fbnic_pkt_prepare(nv, rcd, pkt, qt);
 
 			break;
 		case FBNIC_RCD_TYPE_PAY_AL:
-			head1 = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
+			head1 = fbnic_rcd_bd_idx(&qt->sub1, rcd);
 			fbnic_add_rx_frag(nv, rcd, pkt, qt);
 
 			break;
@@ -1603,6 +1617,16 @@ fbnic_alloc_qt_page_pools(struct fbnic_net *fbn, struct fbnic_q_triad *qt,
 	return PTR_ERR(pp);
 }
 
+static u8 fbnic_bdq_page_shift(u32 page_size)
+{
+	return ilog2(page_size / FBNIC_BD_PAGE_SIZE);
+}
+
+static void fbnic_bdq_set_page_size(struct fbnic_ring *bdq, u32 page_size)
+{
+	bdq->bd_page_shift = fbnic_bdq_page_shift(page_size);
+}
+
 static void fbnic_ring_init(struct fbnic_ring *ring, u32 __iomem *doorbell,
 			    int q_idx, u8 flags)
 {
@@ -1610,6 +1634,7 @@ static void fbnic_ring_init(struct fbnic_ring *ring, u32 __iomem *doorbell,
 	ring->doorbell = doorbell;
 	ring->q_idx = q_idx;
 	ring->flags = flags;
+	fbnic_bdq_set_page_size(ring, PAGE_SIZE);
 	ring->deferred_head = -1;
 }
 
@@ -1894,12 +1919,12 @@ static int fbnic_alloc_rx_ring_desc(struct fbnic_net *fbn,
 
 	switch (rxr->doorbell - fbnic_ring_csr_base(rxr)) {
 	case FBNIC_QUEUE_BDQ_HPQ_TAIL:
-		rxq_size = fbn->hpq_size / FBNIC_BD_FRAG_COUNT;
-		desc_size *= FBNIC_BD_FRAG_COUNT;
+		rxq_size = fbn->hpq_size / fbnic_bd_page_count(rxr);
+		desc_size *= fbnic_bd_page_count(rxr);
 		break;
 	case FBNIC_QUEUE_BDQ_PPQ_TAIL:
-		rxq_size = fbn->ppq_size / FBNIC_BD_FRAG_COUNT;
-		desc_size *= FBNIC_BD_FRAG_COUNT;
+		rxq_size = fbn->ppq_size / fbnic_bd_page_count(rxr);
+		desc_size *= fbnic_bd_page_count(rxr);
 		break;
 	case FBNIC_QUEUE_RCQ_HEAD:
 		rxq_size = fbn->rcq_size;
@@ -2565,7 +2590,7 @@ static void fbnic_enable_bdq(struct fbnic_ring *hpq, struct fbnic_ring *ppq)
 	hpq->tail = 0;
 	hpq->head = 0;
 
-	log_size = fls(hpq->size_mask) + ilog2(FBNIC_BD_FRAG_COUNT);
+	log_size = fls(hpq->size_mask) + hpq->bd_page_shift;
 
 	/* Store descriptor ring address and size */
 	fbnic_ring_wr32(hpq, FBNIC_QUEUE_BDQ_HPQ_BAL, lower_32_bits(hpq->dma));
@@ -2577,7 +2602,7 @@ static void fbnic_enable_bdq(struct fbnic_ring *hpq, struct fbnic_ring *ppq)
 	if (!ppq->size_mask)
 		goto write_ctl;
 
-	log_size = fls(ppq->size_mask) + ilog2(FBNIC_BD_FRAG_COUNT);
+	log_size = fls(ppq->size_mask) + ppq->bd_page_shift;
 
 	/* Add enabling of PPQ to BDQ control */
 	bdq_ctl |= FBNIC_QUEUE_BDQ_CTL_PPQ_ENABLE;
@@ -2846,8 +2871,10 @@ static int fbnic_queue_mem_alloc(struct net_device *dev,
 
 	fbnic_ring_init(&qt->sub0, real->sub0.doorbell, real->sub0.q_idx,
 			real->sub0.flags);
+	qt->sub0.bd_page_shift = real->sub0.bd_page_shift;
 	fbnic_ring_init(&qt->sub1, real->sub1.doorbell, real->sub1.q_idx,
 			real->sub1.flags);
+	qt->sub1.bd_page_shift = real->sub1.bd_page_shift;
 	fbnic_ring_init(&qt->cmpl, real->cmpl.doorbell, real->cmpl.q_idx,
 			real->cmpl.flags);
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h
index e03c9d2c38dc..626729b1a8b6 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h
@@ -121,6 +121,7 @@ struct fbnic_ring {
 	u16 size_mask;			/* Size of ring in descriptors - 1 */
 	u8 q_idx;			/* Logical netdev ring index */
 	u8 flags;			/* Ring flags (FBNIC_RING_F_*) */
+	u8 bd_page_shift;		/* BDQ: ilog2(page_size / 4096) */
 
 	u32 head, tail;			/* Head/Tail of ring */
 
@@ -162,6 +163,11 @@ struct fbnic_napi_vector {
 
 extern const struct netdev_queue_mgmt_ops fbnic_queue_mgmt_ops;
 
+static inline u16 fbnic_bd_page_count(const struct fbnic_ring *bdq)
+{
+	return 1U << bdq->bd_page_shift;
+}
+
 netdev_tx_t fbnic_xmit_frame(struct sk_buff *skb, struct net_device *dev);
 netdev_features_t
 fbnic_features_check(struct sk_buff *skb, struct net_device *dev,
-- 
2.55.0

[PATCH net-next v2 3/5] net: Revalidate queue config for ringparam changes

[Björn Töpel]

Memory-provider queue configuration is validated when the provider is
bound. A later ethtool ring change may invalidate it because drivers
can size queue memory from both ring depth and RX page size. For
example, fbnic uses multiple PPQ entries for each RX page larger than
PAGE_SIZE, which reduces the usable software ring depth.

Keep accepted ring depths in netdev_config and stage proposed values
in cfg_pending. Validate every RX queue before calling the driver.
Each check validates the device defaults, then the queue's
memory-provider override when present. Commit the values only after
the driver accepts them.

The callback receives a rendered configuration rather than a queue ID.
Validation should depend on the configuration, not queue identity.
Checking defaults also covers the case where every queue has a
memory-provider override.

Use the same transaction for ioctl and netlink. Drivers without
ndo_validate_qcfg skip the new validation.

Link: https://lore.kernel.org/all/20250421222827.283737-14-kuba@kernel.org/
Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 include/net/netdev_queues.h | 48 +++++++++++++++++++++++++++++++++----
 net/core/dev.h              |  2 ++
 net/core/netdev_config.c    | 38 ++++++++++++++++++++++++++---
 net/ethtool/common.c        |  9 +++++++
 net/ethtool/common.h        |  2 ++
 net/ethtool/ioctl.c         | 24 +++++++++++++++++--
 net/ethtool/rings.c         | 13 +++++++++-
 7 files changed, 126 insertions(+), 10 deletions(-)

diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
index 70c9fe9e83cc..31121900aac8 100644
--- a/include/net/netdev_queues.h
+++ b/include/net/netdev_queues.h
@@ -4,18 +4,59 @@
 
 #include <linux/netdevice.h>
 
+/**
+ * struct netdev_ring_config - accepted RX/TX ring depth configuration
+ * @rx_pending:		Size of the regular RX ring.
+ * @rx_mini_pending:	Size of the RX mini ring.
+ * @rx_jumbo_pending:	Size of the RX jumbo ring.
+ * @tx_pending:		Size of the TX ring.
+ *
+ * This stores only persistent configuration values. Capability fields,
+ * such as max ring sizes, are reported by drivers but are not part of the
+ * accepted configuration.
+ *
+ * Note: these values are only used for queue-configuration validation
+ * today. Some drivers update their ring sizes without reflecting the change
+ * in @cfg. Before using the stored values for anything else, those cases
+ * need to be audited, and the core likely needs a driver notification API
+ * similar to ethtool_rxfh_context_lost().
+ */
+struct netdev_ring_config {
+	u32	rx_pending;
+	u32	rx_mini_pending;
+	u32	rx_jumbo_pending;
+	u32	tx_pending;
+};
+
 /**
  * struct netdev_config - queue-related configuration for a netdev
  * @hds_thresh:		HDS Threshold value.
  * @hds_config:		HDS value from userspace.
+ * @rings:		Accepted RX/TX ring depths.
+ *
+ * Direct values, such as @hds_thresh and @rings, hold the current
+ * accepted configuration. Drivers which use them for queue rendering
+ * must initialize them with their defaults.
  */
 struct netdev_config {
 	u32	hds_thresh;
 	u8	hds_config;
+
+	struct netdev_ring_config rings;
 };
 
+/**
+ * struct netdev_queue_config - rendered configuration for an RX queue
+ * @rx_page_size:	Size of one RX page-pool allocation.
+ * @rx_ring_size:	Effective size of the regular RX ring.
+ * @rx_mini_ring_size:	Effective size of the RX mini ring.
+ * @rx_jumbo_ring_size:	Effective size of the RX jumbo ring.
+ */
 struct netdev_queue_config {
 	u32	rx_page_size;
+	u32	rx_ring_size;
+	u32	rx_mini_ring_size;
+	u32	rx_jumbo_ring_size;
 };
 
 /* See the netdev.yaml spec for definition of each statistic */
@@ -145,10 +186,9 @@ enum {
  *
  * @ndo_validate_qcfg: (Optional) Check if queue config is supported.
  *			Called when configuration affecting a queue may be
- *			changing, either due to NIC-wide config, or config
- *			scoped to the queue at a specified index.
- *			When NIC-wide config is changed the callback will
- *			be invoked for all queues.
+ *			changing. When NIC-wide config is changed the
+ *			callback will be invoked for the defaults and all
+ *			queue overrides.
  *
  * @ndo_queue_create:	Create a new RX queue on a virtual device that will
  *			be paired with a physical device's queue via leasing.
diff --git a/net/core/dev.h b/net/core/dev.h
index 4b52ff779cba..567e7b82ef24 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -110,6 +110,8 @@ int netdev_reconfig_start(struct net_device *dev);
 int netdev_queue_config_validate(struct net_device *dev, int rxq_idx,
 				 struct netdev_queue_config *qcfg,
 				 struct netlink_ext_ack *extack);
+int netdev_queue_config_revalidate(struct net_device *dev,
+				   struct netlink_ext_ack *extack);
 
 bool netif_rxq_has_mp(struct net_device *dev, unsigned int rxq_idx);
 bool netif_rxq_is_leased(struct net_device *dev, unsigned int rxq_idx);
diff --git a/net/core/netdev_config.c b/net/core/netdev_config.c
index b101341e3251..1975de42a60d 100644
--- a/net/core/netdev_config.c
+++ b/net/core/netdev_config.c
@@ -50,6 +50,15 @@ static int netdev_nop_validate_qcfg(struct net_device *dev,
 	return 0;
 }
 
+static void netdev_qcfg_apply_dev(struct netdev_queue_config *qcfg,
+				  const struct netdev_config *cfg)
+{
+	/* Device config overrides callback-provided fallbacks. */
+	qcfg->rx_ring_size = cfg->rings.rx_pending;
+	qcfg->rx_mini_ring_size = cfg->rings.rx_mini_pending;
+	qcfg->rx_jumbo_ring_size = cfg->rings.rx_jumbo_pending;
+}
+
 static int __netdev_queue_config(struct net_device *dev, int rxq_idx,
 				 struct netdev_queue_config *qcfg,
 				 struct netlink_ext_ack *extack,
@@ -70,6 +79,7 @@ static int __netdev_queue_config(struct net_device *dev, int rxq_idx,
 	/* Get defaults from the driver, in case user config not set */
 	if (dev->queue_mgmt_ops->ndo_default_qcfg)
 		dev->queue_mgmt_ops->ndo_default_qcfg(dev, qcfg);
+	netdev_qcfg_apply_dev(qcfg, dev->cfg_pending);
 	err = validate_cb(dev, qcfg, extack);
 	if (err)
 		return err;
@@ -91,9 +101,11 @@ static int __netdev_queue_config(struct net_device *dev, int rxq_idx,
  * @rxq_idx:  index of the queue of interest
  * @qcfg: queue configuration struct (output)
  *
- * Render the configuration for a given queue. This helper should be used
- * by drivers which support queue configuration to retrieve config for
- * a particular queue.
+ * Render the configuration for a given queue. During a configuration
+ * transaction this includes the proposed device-wide values in
+ * @dev->cfg_pending; otherwise @dev->cfg_pending points to the accepted
+ * configuration. This helper should be used by drivers which support queue
+ * configuration to retrieve config for a particular queue.
  *
  * @qcfg is an output parameter and is always fully initialized by this
  * function. Some values may not be set by the user, drivers may either
@@ -113,3 +125,23 @@ int netdev_queue_config_validate(struct net_device *dev, int rxq_idx,
 {
 	return __netdev_queue_config(dev, rxq_idx, qcfg, extack, true);
 }
+
+int netdev_queue_config_revalidate(struct net_device *dev,
+				   struct netlink_ext_ack *extack)
+{
+	const struct netdev_queue_mgmt_ops *qops = dev->queue_mgmt_ops;
+	struct netdev_queue_config qcfg;
+	unsigned int i;
+	int err;
+
+	if (!qops || !qops->ndo_validate_qcfg)
+		return 0;
+
+	for (i = 0; i < dev->real_num_rx_queues; i++) {
+		err = netdev_queue_config_validate(dev, i, &qcfg, extack);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 23db40618fed..05ed22fd1f90 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -956,6 +956,15 @@ void ethtool_ringparam_get_cfg(struct net_device *dev,
 	kparam->hds_thresh = dev->cfg->hds_thresh;
 }
 
+void ethtool_ringparam_set_cfg(struct netdev_config *cfg,
+			       const struct ethtool_ringparam *param)
+{
+	cfg->rings.rx_pending = param->rx_pending;
+	cfg->rings.rx_mini_pending = param->rx_mini_pending;
+	cfg->rings.rx_jumbo_pending = param->rx_jumbo_pending;
+	cfg->rings.tx_pending = param->tx_pending;
+}
+
 static void ethtool_init_tsinfo(struct kernel_ethtool_ts_info *info)
 {
 	memset(info, 0, sizeof(*info));
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index 4e5356e26f40..a27944d4cbf8 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -53,6 +53,8 @@ void ethtool_ringparam_get_cfg(struct net_device *dev,
 			       struct ethtool_ringparam *param,
 			       struct kernel_ethtool_ringparam *kparam,
 			       struct netlink_ext_ack *extack);
+void ethtool_ringparam_set_cfg(struct netdev_config *cfg,
+			       const struct ethtool_ringparam *param);
 
 int ethtool_get_rx_ring_count(struct net_device *dev);
 
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 4b0bc503f930..1320289025b2 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -35,6 +35,7 @@
 #include <net/netdev_queues.h>
 
 #include "common.h"
+#include "../core/dev.h"
 
 /* State held across locks and calls for commands which have devlink fallback */
 struct ethtool_devlink_compat {
@@ -2239,10 +2240,29 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
 	    ringparam.tx_pending > max.tx_max_pending)
 		return -EINVAL;
 
+	ret = netdev_reconfig_start(dev);
+	if (ret)
+		return ret;
+
+	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
+
+	ret = netdev_queue_config_revalidate(dev, NULL);
+	if (ret)
+		goto out_free_cfg;
+
 	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
 					      &kernel_ringparam, NULL);
-	if (!ret)
-		ethtool_notify(dev, ETHTOOL_MSG_RINGS_NTF);
+	if (ret)
+		goto out_free_cfg;
+
+	/* The driver may adjust the accepted ring depths. */
+	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
+	swap(dev->cfg, dev->cfg_pending);
+	ethtool_notify(dev, ETHTOOL_MSG_RINGS_NTF);
+
+out_free_cfg:
+	__netdev_free_config(dev->cfg_pending);
+	dev->cfg_pending = dev->cfg;
 	return ret;
 }
 
diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
index 9054c89c5d7b..c04312fc0d06 100644
--- a/net/ethtool/rings.c
+++ b/net/ethtool/rings.c
@@ -4,6 +4,7 @@
 
 #include "common.h"
 #include "netlink.h"
+#include "../core/dev.h"
 
 struct rings_req_info {
 	struct ethnl_req_info		base;
@@ -299,10 +300,20 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
 
 	dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
 	dev->cfg_pending->hds_thresh = kernel_ringparam.hds_thresh;
+	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
+
+	ret = netdev_queue_config_revalidate(dev, info->extack);
+	if (ret)
+		return ret;
 
 	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
 					      &kernel_ringparam, info->extack);
-	return ret < 0 ? ret : 1;
+	if (ret < 0)
+		return ret;
+
+	/* The driver may adjust the accepted ring depths. */
+	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
+	return 1;
 }
 
 const struct ethnl_request_ops ethnl_rings_request_ops = {
-- 
2.55.0

Re: [PATCH net-next v2 3/5] net: Revalidate queue config for ringparam changes

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 4

- [Low] struct netdev_config.rings is zero-allocated by
  netdev_alloc_config() and netdev_qcfg_apply_dev() unconditionally…
- [Low] The commit message is framed as closing a live misconfiguration
  hole (naming fbnic's PPQ-entries-per-page behaviour), but in this…
- [Low] The values stored into dev->cfg->rings after ->set_ringparam()
  returns are the requested values, not necessarily the ones the…
- [Low] Both set paths validate the staged ring depths before calling
  ->set_ringparam(), then re-copy the driver-adjusted depths into…

--

net: Revalidate queue config for ringparam changes

This adds accepted RX/TX ring depths to struct netdev_config, stages
proposed values in dev->cfg_pending, and revalidates every RX queue
through ndo_validate_qcfg() before calling the driver's set_ringparam().
The ioctl and netlink ring-set paths are converted to the same
reconfiguration transaction.

Link: https://lore.kernel.org/all/20250421222827.283737-14-kuba@kernel.org/

A few questions below.

>     Memory-provider queue configuration is validated when the provider is
>     bound. A later ethtool ring change may invalidate it because drivers
>     can size queue memory from both ring depth and RX page size. For
>     example, fbnic uses multiple PPQ entries for each RX page larger than
>     PAGE_SIZE, which reduces the usable software ring depth.
[ ... ]
>     Use the same transaction for ioctl and netlink. Drivers without
>     ndo_validate_qcfg skip the new validation.

[Severity: Low]
The message reads as if it closes a live misconfiguration hole, but at this
commit no in-tree driver implements a ring-depth-aware ndo_validate_qcfg and
no driver reads qcfg->rx_ring_size, so netdev_queue_config_revalidate()
cannot change any validation outcome yet.

The two current ndo_validate_qcfg implementations, bnxt_validate_qcfg() and
mlx5e_queue_validate_qcfg(), only inspect qcfg->rx_page_size. The fbnic
consumer named in the message, fbnic_validate_qcfg() reading
qcfg->rx_jumbo_ring_size, arrives in the following patch of the series.

The last paragraph does disclose the limitation, so this is only about
framing: could the message state that this is core plumbing and that the
fbnic consumer lands in the next patch?

> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> index 70c9fe9e83cc4..31121900aac88 100644
> --- a/include/net/netdev_queues.h
> +++ b/include/net/netdev_queues.h
> @@ -4,18 +4,59 @@
>  
>  #include <linux/netdevice.h>
>  
> +/**
> + * struct netdev_ring_config - accepted RX/TX ring depth configuration
[ ... ]
> + * Note: these values are only used for queue-configuration validation
> + * today. Some drivers update their ring sizes without reflecting the change
> + * in @cfg. Before using the stored values for anything else, those cases
> + * need to be audited, and the core likely needs a driver notification API
> + * similar to ethtool_rxfh_context_lost().
> + */
> +struct netdev_ring_config {
> +	u32	rx_pending;
> +	u32	rx_mini_pending;
> +	u32	rx_jumbo_pending;
> +	u32	tx_pending;
> +};
> +
>  /**
>   * struct netdev_config - queue-related configuration for a netdev
>   * @hds_thresh:		HDS Threshold value.
>   * @hds_config:		HDS value from userspace.
> + * @rings:		Accepted RX/TX ring depths.
> + *
> + * Direct values, such as @hds_thresh and @rings, hold the current
> + * accepted configuration. Drivers which use them for queue rendering
> + * must initialize them with their defaults.
>   */
>  struct netdev_config {
>  	u32	hds_thresh;
>  	u8	hds_config;
> +
> +	struct netdev_ring_config rings;
>  };
>  
[ ... ]

> diff --git a/net/core/netdev_config.c b/net/core/netdev_config.c
> index b101341e32513..1975de42a60d9 100644
> --- a/net/core/netdev_config.c
> +++ b/net/core/netdev_config.c
> @@ -50,6 +50,15 @@ static int netdev_nop_validate_qcfg(struct net_device *dev,
>  	return 0;
>  }
>  
> +static void netdev_qcfg_apply_dev(struct netdev_queue_config *qcfg,
> +				  const struct netdev_config *cfg)
> +{
> +	/* Device config overrides callback-provided fallbacks. */
> +	qcfg->rx_ring_size = cfg->rings.rx_pending;
> +	qcfg->rx_mini_ring_size = cfg->rings.rx_mini_pending;
> +	qcfg->rx_jumbo_ring_size = cfg->rings.rx_jumbo_pending;
> +}
> +

[Severity: Low]
Can these three assignments hand a rendered config full of zeroes to
ndo_validate_qcfg()?

cfg->rings starts out all-zero and the core never seeds it from the driver's
current ring depths:

net/core/netdev_config.c:netdev_alloc_config() {
	cfg = kzalloc_obj(*dev->cfg, GFP_KERNEL_ACCOUNT);
	...
}

The only writer is ethtool_ringparam_set_cfg(), so on a device whose ring
depths were never touched through ethtool, rx_ring_size,
rx_mini_ring_size and rx_jumbo_ring_size all render as 0.

The assignment is also unconditional, unlike the memory-provider override a
few lines below:

	mpp = &__netif_get_rx_queue(dev, rxq_idx)->mp_params;
	if (mpp->rx_page_size)
		qcfg->rx_page_size = mpp->rx_page_size;

so a driver cannot supply ring-depth fallbacks from ndo_default_qcfg() at
all - they are always overwritten. The comment says that is intended, but
should the copy be guarded the same way as rx_page_size, or should
ndo_default_qcfg() not be documented as providing fallbacks for these
fields?

For the record, nothing appears broken in-tree: the only consumer of the new
fields, fbnic, seeds netdev->cfg->rings.{rx,rx_mini,rx_jumbo,tx}_pending in
fbnic_netdev_alloc() in the following patch of the series, and the
"driver initializes dev->cfg" convention already exists there for
hds_thresh. bnxt and mlx5e read only rx_page_size.

>  static int __netdev_queue_config(struct net_device *dev, int rxq_idx,
>  				 struct netdev_queue_config *qcfg,
>  				 struct netlink_ext_ack *extack,
> @@ -70,6 +79,7 @@ static int __netdev_queue_config(struct net_device *dev, int rxq_idx,
>  	/* Get defaults from the driver, in case user config not set */
>  	if (dev->queue_mgmt_ops->ndo_default_qcfg)
>  		dev->queue_mgmt_ops->ndo_default_qcfg(dev, qcfg);
> +	netdev_qcfg_apply_dev(qcfg, dev->cfg_pending);
>  	err = validate_cb(dev, qcfg, extack);
>  	if (err)
>  		return err;
[ ... ]
> @@ -113,3 +125,23 @@ int netdev_queue_config_validate(struct net_device *dev, int rxq_idx,
>  {
>  	return __netdev_queue_config(dev, rxq_idx, qcfg, extack, true);
>  }
> +
> +int netdev_queue_config_revalidate(struct net_device *dev,
> +				   struct netlink_ext_ack *extack)
> +{
> +	const struct netdev_queue_mgmt_ops *qops = dev->queue_mgmt_ops;
> +	struct netdev_queue_config qcfg;
> +	unsigned int i;
> +	int err;
> +
> +	if (!qops || !qops->ndo_validate_qcfg)
> +		return 0;
> +
> +	for (i = 0; i < dev->real_num_rx_queues; i++) {
> +		err = netdev_queue_config_validate(dev, i, &qcfg, extack);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}

> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 23db40618fed1..05ed22fd1f906 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -956,6 +956,15 @@ void ethtool_ringparam_get_cfg(struct net_device *dev,
>  	kparam->hds_thresh = dev->cfg->hds_thresh;
>  }
>  
> +void ethtool_ringparam_set_cfg(struct netdev_config *cfg,
> +			       const struct ethtool_ringparam *param)
> +{
> +	cfg->rings.rx_pending = param->rx_pending;
> +	cfg->rings.rx_mini_pending = param->rx_mini_pending;
> +	cfg->rings.rx_jumbo_pending = param->rx_jumbo_pending;
> +	cfg->rings.tx_pending = param->tx_pending;
> +}
> +
[ ... ]

> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 4b0bc503f9307..1320289025b25 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
[ ... ]
> @@ -2239,10 +2240,29 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
>  	    ringparam.tx_pending > max.tx_max_pending)
>  		return -EINVAL;
>  
> +	ret = netdev_reconfig_start(dev);
> +	if (ret)
> +		return ret;
> +
> +	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
> +
> +	ret = netdev_queue_config_revalidate(dev, NULL);
> +	if (ret)
> +		goto out_free_cfg;
> +
>  	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
>  					      &kernel_ringparam, NULL);
> -	if (!ret)
> -		ethtool_notify(dev, ETHTOOL_MSG_RINGS_NTF);
> +	if (ret)
> +		goto out_free_cfg;
> +
> +	/* The driver may adjust the accepted ring depths. */
> +	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
> +	swap(dev->cfg, dev->cfg_pending);
> +	ethtool_notify(dev, ETHTOOL_MSG_RINGS_NTF);

[Severity: Low]
Is the committed configuration guaranteed to be one that ndo_validate_qcfg()
actually saw?

The sequence here is:

	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);   /* requested */
	netdev_queue_config_revalidate(dev, NULL);                 /* validates */
	dev->ethtool_ops->set_ringparam(...);                      /* may rewrite */
	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);   /* adjusted */
	swap(dev->cfg, dev->cfg_pending);                          /* commits */

If the driver rewrites ringparam in place, the depths that get committed are
not the depths that were validated, and there is no second
netdev_queue_config_revalidate() pass. ethnl_set_rings() has the same
ordering, with the commit done by the framework swap in
ethnl_default_set_doit().

No in-tree case looks reachable today: the only ring-depth-aware validator,
fbnic_validate_qcfg(), runs the same fbnic_ring_size_pow2() helper that
fbnic_set_ringparam() applies, and its ppq_entries >= 4 constraint only gets
easier as the depth is rounded up. Drivers that do rewrite the caller's
ringparam, such as mvpp2_ethtool_set_ringparam() via
mvpp2_check_ringparam_valid(), have no ndo_validate_qcfg, so
netdev_queue_config_revalidate() returns 0 for them.

Would it be worth revalidating after the driver call, or documenting that
->set_ringparam() must not adjust depths in a direction its own
ndo_validate_qcfg() would reject?

> +
> +out_free_cfg:
> +	__netdev_free_config(dev->cfg_pending);
> +	dev->cfg_pending = dev->cfg;
>  	return ret;
>  }
>  

> diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
> index 9054c89c5d7bb..c04312fc0d061 100644
> --- a/net/ethtool/rings.c
> +++ b/net/ethtool/rings.c
[ ... ]
> @@ -299,10 +300,20 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
>  
>  	dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
>  	dev->cfg_pending->hds_thresh = kernel_ringparam.hds_thresh;
> +	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
> +
> +	ret = netdev_queue_config_revalidate(dev, info->extack);
> +	if (ret)
> +		return ret;
>  
>  	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
>  					      &kernel_ringparam, info->extack);
> -	return ret < 0 ? ret : 1;
> +	if (ret < 0)
> +		return ret;
> +
> +	/* The driver may adjust the accepted ring depths. */
> +	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
> +	return 1;
>  }

[Severity: Low]
The comment says the driver may adjust the accepted ring depths, but does
this second copy store the depths the hardware ended up with, or just the
requested ones?

Drivers that normalize through locals never touch the caller's struct
ethtool_ringparam, for example mlx5e_ethtool_set_ringparam() computing
order_base_2() into its own variables, so dev->cfg->rings can end up
describing a depth the device is not running.

This is already acknowledged in the kernel-doc added by this patch ("Some
drivers update their ring sizes without reflecting the change in @cfg"), and
the only consumer of the stored depths, fbnic, does round in place, so
nothing reads a stale value today. Would it be clearer to say the stored
values are the requested depths, adjusted only by drivers that write back
into param?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910180908.1506533-1-bjorn%40kernel.org

[PATCH net-next v2 4/5] fbnic: Support larger memory-provider RX pages

[Björn Töpel]

Memory providers can set the receive page size for a queue through
QCFG_RX_PAGE_SIZE. fbnic creates PAGE_SIZE PPQ page-pool allocations,
so larger rx_buf_len values are not reflected in the PPQ BDQ geometry.

Use the rendered rx_page_size for PPQ allocations and completion
decoding. The NIC still consumes 4 KiB device pages; a larger PPQ page
is represented by consecutive device-page descriptors, and completion
IDs decode to offsets within the same netmem allocation.

Validate rx_page_size against fbnic's fragment-reference budget,
device-page geometry, and rendered PPQ depth. The page size must be a
power-of-two of at least one device page, its worst-case payload
fragment count must fit the page-pool reference bias, and the PPQ must
retain usable software depth after expansion.

Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |  8 +-
 .../net/ethernet/meta/fbnic/fbnic_netdev.c    |  5 ++
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c  | 88 +++++++++++++++++--
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.h  |  6 ++
 4 files changed, 94 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index 0e47088ec44b..2def4c26207d 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -334,10 +334,10 @@ fbnic_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
 	struct fbnic_net *clone;
 	int err;
 
-	ring->rx_pending	= roundup_pow_of_two(ring->rx_pending);
-	ring->rx_mini_pending	= roundup_pow_of_two(ring->rx_mini_pending);
-	ring->rx_jumbo_pending	= roundup_pow_of_two(ring->rx_jumbo_pending);
-	ring->tx_pending	= roundup_pow_of_two(ring->tx_pending);
+	ring->rx_pending	= fbnic_ring_size_pow2(ring->rx_pending);
+	ring->rx_mini_pending	= fbnic_ring_size_pow2(ring->rx_mini_pending);
+	ring->rx_jumbo_pending	= fbnic_ring_size_pow2(ring->rx_jumbo_pending);
+	ring->tx_pending	= fbnic_ring_size_pow2(ring->tx_pending);
 
 	/* These are absolute minimums allowing the device and driver to operate
 	 * but not necessarily guarantee reasonable performance. Settings below
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
index 10bf99be3f24..8bc5e6e5c59e 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
@@ -773,6 +773,11 @@ struct net_device *fbnic_netdev_alloc(struct fbnic_dev *fbd)
 	fbn->ppq_size = FBNIC_PPQ_SIZE_DEFAULT;
 	fbn->rcq_size = FBNIC_RCQ_SIZE_DEFAULT;
 
+	netdev->cfg->rings.rx_pending = fbn->rcq_size;
+	netdev->cfg->rings.rx_mini_pending = fbn->hpq_size;
+	netdev->cfg->rings.rx_jumbo_pending = fbn->ppq_size;
+	netdev->cfg->rings.tx_pending = fbn->txq_size;
+
 	fbn->tx_usecs = FBNIC_TX_USECS_DEFAULT;
 	fbn->rx_usecs = FBNIC_RX_USECS_DEFAULT;
 	fbn->rx_max_frames = FBNIC_RX_FRAMES_DEFAULT;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 28b962307c08..5e22905a6373 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -1022,7 +1022,8 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
 	netmem = fbnic_page_pool_get_data(qt, pg_idx);
 
 	truesize = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
-		   FBNIC_BD_PAGE_SIZE - pg_off : ALIGN(len, 128);
+		   FBNIC_BD_PAGE_SIZE - pg_off :
+		   ALIGN(len, FBNIC_RX_PAYLD_ALIGN);
 
 	pg_off += fbnic_rcd_bd_page_offset(&qt->sub1, rcd);
 
@@ -1030,6 +1031,9 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
 	page_pool_dma_sync_netmem_for_cpu(qt->sub1.page_pool, netmem,
 					  pg_off, truesize);
 
+	/* Consecutive device-page completions from one PPQ page are adjacent
+	 * ranges in the same netmem.
+	 */
 	added = xdp_buff_add_frag(&pkt->buff, netmem, pg_off, len, truesize);
 	if (unlikely(!added)) {
 		pkt->add_frag_failed = true;
@@ -1564,7 +1568,7 @@ void fbnic_free_napi_vectors(struct fbnic_net *fbn)
 
 static int
 fbnic_alloc_qt_page_pools(struct fbnic_net *fbn, struct fbnic_q_triad *qt,
-			  unsigned int rxq_idx)
+			  unsigned int rxq_idx, u32 rx_page_size)
 {
 	struct page_pool_params pp_params = {
 		.order = 0,
@@ -1599,6 +1603,8 @@ fbnic_alloc_qt_page_pools(struct fbnic_net *fbn, struct fbnic_q_triad *qt,
 
 	qt->sub0.page_pool = pp;
 	if (netif_rxq_has_unreadable_mp(fbn->netdev, rxq_idx)) {
+		pp_params.order = get_order(rx_page_size);
+		pp_params.max_len = rx_page_size;
 		pp_params.flags |= PP_FLAG_ALLOW_UNREADABLE_NETMEM;
 		pp_params.dma_dir = DMA_FROM_DEVICE;
 
@@ -2028,15 +2034,18 @@ static int fbnic_alloc_tx_qt_resources(struct fbnic_net *fbn,
 
 static int fbnic_alloc_rx_qt_resources(struct fbnic_net *fbn,
 				       struct fbnic_napi_vector *nv,
-				       struct fbnic_q_triad *qt)
+				       struct fbnic_q_triad *qt,
+				       u32 rx_page_size)
 {
 	struct device *dev = fbn->netdev->dev.parent;
 	int err;
 
-	err = fbnic_alloc_qt_page_pools(fbn, qt, qt->cmpl.q_idx);
+	err = fbnic_alloc_qt_page_pools(fbn, qt, qt->cmpl.q_idx, rx_page_size);
 	if (err)
 		return err;
 
+	fbnic_bdq_set_page_size(&qt->sub1, rx_page_size);
+
 	err = xdp_rxq_info_reg(&qt->xdp_rxq, fbn->netdev, qt->sub0.q_idx,
 			       nv->napi.napi_id);
 	if (err)
@@ -2097,7 +2106,11 @@ static int fbnic_alloc_nv_resources(struct fbnic_net *fbn,
 
 	/* Allocate Rx Resources */
 	for (j = 0; j < nv->rxt_count; j++, i++) {
-		err = fbnic_alloc_rx_qt_resources(fbn, nv, &nv->qt[i]);
+		struct netdev_queue_config qcfg;
+
+		netdev_queue_config(fbn->netdev, nv->qt[i].cmpl.q_idx, &qcfg);
+		err = fbnic_alloc_rx_qt_resources(fbn, nv, &nv->qt[i],
+						  qcfg.rx_page_size);
 		if (err)
 			goto free_qt_resources;
 	}
@@ -2864,21 +2877,75 @@ static int fbnic_queue_mem_alloc(struct net_device *dev,
 	struct fbnic_napi_vector *nv;
 
 	if (!netif_running(dev))
-		return fbnic_alloc_qt_page_pools(fbn, qt, idx);
+		return fbnic_alloc_qt_page_pools(fbn, qt, idx,
+						 qcfg->rx_page_size);
 
 	real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
 	nv = fbn->napi[idx % fbn->num_napi];
 
 	fbnic_ring_init(&qt->sub0, real->sub0.doorbell, real->sub0.q_idx,
 			real->sub0.flags);
-	qt->sub0.bd_page_shift = real->sub0.bd_page_shift;
 	fbnic_ring_init(&qt->sub1, real->sub1.doorbell, real->sub1.q_idx,
 			real->sub1.flags);
-	qt->sub1.bd_page_shift = real->sub1.bd_page_shift;
 	fbnic_ring_init(&qt->cmpl, real->cmpl.doorbell, real->cmpl.q_idx,
 			real->cmpl.flags);
 
-	return fbnic_alloc_rx_qt_resources(fbn, nv, qt);
+	return fbnic_alloc_rx_qt_resources(fbn, nv, qt, qcfg->rx_page_size);
+}
+
+static void fbnic_default_qcfg(struct net_device *dev,
+			       struct netdev_queue_config *qcfg)
+{
+	qcfg->rx_page_size = PAGE_SIZE;
+}
+
+static int fbnic_validate_qcfg(struct net_device *dev,
+			       struct netdev_queue_config *qcfg,
+			       struct netlink_ext_ack *extack)
+{
+	u32 ppq_size = fbnic_ring_size_pow2(qcfg->rx_jumbo_ring_size);
+	u32 bd_page_count, ppq_entries, frag_count;
+	u32 rx_page_size = qcfg->rx_page_size;
+
+	if (!is_power_of_2(rx_page_size)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "rx_page_size must be a power of 2");
+		return -EINVAL;
+	}
+
+	if (rx_page_size < FBNIC_BD_PAGE_SIZE) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "rx_page_size must be at least 4K");
+		return -EINVAL;
+	}
+
+	/* Payload fragments occupy multiples of FBNIC_RX_PAYLD_ALIGN bytes.
+	 * Keep at least one reference in the bias until fbnic_clean_bdq()
+	 * observes a completion from a subsequent allocation.
+	 */
+	frag_count = rx_page_size / FBNIC_RX_PAYLD_ALIGN;
+	if (frag_count >= FBNIC_PAGECNT_BIAS_MAX) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "rx_page_size can produce too many fragments");
+		return -EINVAL;
+	}
+
+	bd_page_count = rx_page_size / FBNIC_BD_PAGE_SIZE;
+	ppq_entries = ppq_size / bd_page_count;
+	/* The PPQ is sized in 4 KiB device pages. One software entry tracks
+	 * each page-pool allocation. In addition to the unused entry for
+	 * empty/full accounting, cleanup retains the current allocation
+	 * until a completion identifies a subsequent allocation. A two-entry
+	 * ring can only post one allocation and cannot make progress.
+	 * Require at least four entries, since ring sizes are powers of two.
+	 */
+	if (ppq_entries < 4) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "rx-jumbo ring size too small for rx_page_size");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static void fbnic_queue_mem_free(struct net_device *dev, void *qmem)
@@ -2980,4 +3047,7 @@ const struct netdev_queue_mgmt_ops fbnic_queue_mgmt_ops = {
 	.ndo_queue_mem_free	= fbnic_queue_mem_free,
 	.ndo_queue_start	= fbnic_queue_start,
 	.ndo_queue_stop		= fbnic_queue_stop,
+	.ndo_default_qcfg	= fbnic_default_qcfg,
+	.ndo_validate_qcfg	= fbnic_validate_qcfg,
+	.supported_params	= QCFG_RX_PAGE_SIZE,
 };
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h
index 626729b1a8b6..12b3a684f461 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h
@@ -49,12 +49,18 @@ struct fbnic_net;
 #define FBNIC_RX_USECS_DEFAULT		30
 #define FBNIC_RX_FRAMES_DEFAULT		0
 
+static inline u32 fbnic_ring_size_pow2(u32 size)
+{
+	return size ? roundup_pow_of_two(size) : 0;
+}
+
 #define FBNIC_RX_TROOM \
 	SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
 #define FBNIC_RX_HROOM_PAD		128
 #define FBNIC_RX_HROOM \
 	(ALIGN(FBNIC_RX_TROOM + FBNIC_RX_HROOM_PAD, 128) - FBNIC_RX_TROOM)
 #define FBNIC_RX_PAD			0
+#define FBNIC_RX_PAYLD_ALIGN		128
 #define FBNIC_RX_PAYLD_OFFSET		0
 #define FBNIC_RX_PAYLD_PG_CL		0
 
-- 
2.55.0

Re: [PATCH net-next v2 4/5] fbnic: Support larger memory-provider RX pages

[netdev-bot+sashiko]

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

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

- [High] fbnic_alloc_rx_qt_resources() programs the PPQ device-page
  geometry unconditionally (`fbnic_bdq_set_page_size(&qt->sub1,…

Pre-existing issues:
- [High] In fbnic_alloc_qt_page_pools()
  (drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:1611-1623) the local…

--

fbnic: Support larger memory-provider RX pages

The patch lets memory providers pick a larger RX page size for a queue via
QCFG_RX_PAGE_SIZE, uses that rendered rx_page_size for the PPQ page-pool
allocations and completion decoding, and adds ndo_default_qcfg /
ndo_validate_qcfg callbacks to sanity check the requested size.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 28b962307c080..5e22905a63739 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c

[ ... ]

> @@ -1599,6 +1603,8 @@ fbnic_alloc_qt_page_pools(struct fbnic_net *fbn, struct fbnic_q_triad *qt,
>  
>  	qt->sub0.page_pool = pp;
>  	if (netif_rxq_has_unreadable_mp(fbn->netdev, rxq_idx)) {
> +		pp_params.order = get_order(rx_page_size);
> +		pp_params.max_len = rx_page_size;
>  		pp_params.flags |= PP_FLAG_ALLOW_UNREADABLE_NETMEM;
>  		pp_params.dma_dir = DMA_FROM_DEVICE;
>  
>  		pp = page_pool_create(&pp_params);
>  		if (IS_ERR(pp))
>  			goto err_destroy_sub0;

[Severity: High]
This isn't a bug introduced by this patch, but the patch widens the set of
inputs that can make this page_pool_create() fail, so it seems worth
mentioning.

At the err_destroy_sub0 label, pp has already been reassigned to the
ERR_PTR returned by the second page_pool_create():

	err_destroy_sub0:
		page_pool_destroy(pp);
		return PTR_ERR(pp);

page_pool_destroy() only short-circuits on NULL:

	net/core/page_pool.c:page_pool_destroy() {
		if (!pool)
			return;

		if (!page_pool_put(pool))
			return;
		...
	}

and page_pool_put() then does refcount_dec_and_test(&pool->user_cnt) on
the error pointer. Should the label destroy qt->sub0.page_pool instead,
and save the error value before overwriting pp?

Along the same path, is qt->sub0.page_pool leaked together with its DMA
mappings? fbnic_alloc_rx_qt_resources() returns the error before reaching
its free_page_pools label, fbnic_alloc_nv_resources()'s "while (i--)"
unwind skips the failing qt, and on the queue-reconfig side
netdev_rx_queue_reconfig() goes to err_free_old_mem (kvfree of the qmem
container only, no ndo_queue_mem_free) when ndo_queue_mem_alloc fails.

The second create can fail for ordinary reasons: page_pool_init() returns
-ENOMEM for the ptr_ring allocation (up to 16384 entries here) and can
also propagate an error from pool->mp_ops->init(pool).

[ ... ]

> @@ -2028,15 +2034,18 @@ static int fbnic_alloc_tx_qt_resources(struct fbnic_net *fbn,
>  
>  static int fbnic_alloc_rx_qt_resources(struct fbnic_net *fbn,
>  				       struct fbnic_napi_vector *nv,
> -				       struct fbnic_q_triad *qt)
> +				       struct fbnic_q_triad *qt,
> +				       u32 rx_page_size)
>  {
>  	struct device *dev = fbn->netdev->dev.parent;
>  	int err;
>  
> -	err = fbnic_alloc_qt_page_pools(fbn, qt, qt->cmpl.q_idx);
> +	err = fbnic_alloc_qt_page_pools(fbn, qt, qt->cmpl.q_idx, rx_page_size);
>  	if (err)
>  		return err;
>  
> +	fbnic_bdq_set_page_size(&qt->sub1, rx_page_size);
> +

[Severity: High]
Can the BDQ geometry here end up describing a page pool that was never
enlarged?

fbnic_bdq_set_page_size(&qt->sub1, rx_page_size) runs unconditionally,
but fbnic_alloc_qt_page_pools() only raises order/max_len when
netif_rxq_has_unreadable_mp() is true; otherwise sub1 just takes another
reference on sub0's order-0, PAGE_SIZE pool. The two predicates are not
the same:

	net/core/netdev_rx_queue.c:netif_rxq_has_unreadable_mp() {
		if (rxq_idx < dev->real_num_rx_queues)
			return __netif_get_rx_queue(dev, rxq_idx)->mp_params.mp_ops;
		return false;
	}

	net/core/netdev_config.c:__netdev_queue_config() {
		mpp = &__netif_get_rx_queue(dev, rxq_idx)->mp_params;
		if (mpp->rx_page_size)
			qcfg->rx_page_size = mpp->rx_page_size;
	}

The config renderer applies the memory provider's rx_page_size with no
real_num_rx_queues bound check, while netif_rxq_has_unreadable_mp()
silently reports false above real_num_rx_queues.

When they disagree, does the device get descriptors past the end of the
mapping? fbnic_bd_prep() posts fbnic_bd_page_count() descriptors from a
single allocation:

	__le64 *bdq_desc = &bdq->desc[id * fbnic_bd_page_count(bdq)];
	dma_addr_t dma = page_pool_get_dma_addr_netmem(netmem);
	u64 bd, i = fbnic_bd_page_count(bdq);

and on the completion side fbnic_rcd_bd_page_offset() decodes offsets up
to rx_page_size - 1, which fbnic_add_rx_frag() then feeds to
page_pool_dma_sync_netmem_for_cpu() and xdp_buff_add_frag() as payload
inside what is really a PAGE_SIZE buffer.

A path that seems to reach this: fbnic only sets real_num_rx_queues in
fbnic_set_netif_queues() at open, so after probe it equals
FBNIC_MAX_RXQS from alloc_etherdev_mq(). A devmem/io_uring bind with
rx-page-size=8K against a high queue index is accepted then, "ip link set
up" shrinks real_num_rx_queues (netif_set_real_num_rx_queues() has no
memory-provider check), and a later "ethtool -L combined 32" makes
fbnic_alloc_nv_resources() allocate that queue while
netif_rxq_has_unreadable_mp() returns false but qcfg.rx_page_size is
still 8192.

Before this patch bd_page_shift was always derived from PAGE_SIZE and so
always matched the order-0 pool. Would it make sense to either derive the
BDQ page size from the pool that was actually created, or reject
rx_page_size != PAGE_SIZE when no large-order pool was allocated?

>  	err = xdp_rxq_info_reg(&qt->xdp_rxq, fbn->netdev, qt->sub0.q_idx,
>  			       nv->napi.napi_id);
>  	if (err)

[ ... ]

> @@ -2097,7 +2106,11 @@ static int fbnic_alloc_nv_resources(struct fbnic_net *fbn,
>  
>  	/* Allocate Rx Resources */
>  	for (j = 0; j < nv->rxt_count; j++, i++) {
> -		err = fbnic_alloc_rx_qt_resources(fbn, nv, &nv->qt[i]);
> +		struct netdev_queue_config qcfg;
> +
> +		netdev_queue_config(fbn->netdev, nv->qt[i].cmpl.q_idx, &qcfg);
> +		err = fbnic_alloc_rx_qt_resources(fbn, nv, &nv->qt[i],
> +						  qcfg.rx_page_size);
>  		if (err)
>  			goto free_qt_resources;
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910180908.1506533-1-bjorn%40kernel.org

[PATCH net-next v2 5/5] selftests: drv-net: Test large zcrx buffers

[Björn Töpel]

A zcrx RX buffer larger than PAGE_SIZE does not prove that the
driver uses space beyond the first MTU-sized part of the buffer.

The test assumes that a driver which accepts a large rx_buf_len uses
space beyond the first MTU-sized region during sustained receive
traffic. Drivers which do not support large buffers are expected to
reject the requested size; the feature probe then skips them. A driver
which accepts the size but only uses the first MTU-sized region fails.

Request a power-of-two RX buffer larger than twice the device MTU and
require one zero-copy receive CQE to end past the first MTU-sized
region.

Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 .../selftests/drivers/net/hw/iou-zcrx.c       | 33 ++++++++++++++++---
 .../selftests/drivers/net/hw/iou-zcrx.py      |  6 +++-
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/iou-zcrx.c b/tools/testing/selftests/drivers/net/hw/iou-zcrx.c
index f6a8fc5fac24..c0d9065b2103 100644
--- a/tools/testing/selftests/drivers/net/hw/iou-zcrx.c
+++ b/tools/testing/selftests/drivers/net/hw/iou-zcrx.c
@@ -84,6 +84,9 @@ static int cfg_oneshot_recvs;
 static int cfg_send_size = SEND_SIZE;
 static struct sockaddr_in6 cfg_addr;
 static unsigned int cfg_rx_buf_len;
+static unsigned int cfg_min_data_end;
+static bool cfg_check_data_end;
+static bool seen_data_end;
 static bool cfg_dry_run;
 
 static char *payload;
@@ -298,6 +301,15 @@ static void process_recvzc(struct io_uring *ring, struct io_uring_cqe *cqe)
 	mask = (1ULL << IORING_ZCRX_AREA_SHIFT) - 1;
 	data = (char *)area_ptr + (rcqe->off & mask);
 
+	if (cfg_check_data_end) {
+		unsigned int rx_buf_len = cfg_rx_buf_len ?: page_size;
+		unsigned int data_end_off;
+
+		data_end_off = (rcqe->off & mask) % rx_buf_len + n;
+		if (data_end_off > cfg_min_data_end)
+			seen_data_end = true;
+	}
+
 	for (i = 0; i < n; i++) {
 		if (*(data + i) != payload[(received + i)])
 			error(1, 0, "payload mismatch at %d", i);
@@ -373,7 +385,10 @@ static void run_server(void)
 		server_loop(&ring);
 
 	if (!stop)
-		error(1, 0, "test failed\n");
+		error(1, 0, "test failed after receiving %zu bytes", received);
+	if (cfg_check_data_end && !seen_data_end)
+		error(1, 0, "no payload CQE ending past offset %u",
+		      cfg_min_data_end);
 }
 
 static void run_client(void)
@@ -406,8 +421,11 @@ static void run_client(void)
 
 static void usage(const char *filepath)
 {
-	error(1, 0, "Usage: %s (-4|-6) (-s|-c) -h<server_ip> -p<port> "
-		    "-l<payload_size> -i<ifname> -q<rxq_id>", filepath);
+	error(1, 0,
+	      "Usage: %s (-4|-6) (-s|-c) -h<server_ip> -p<port>\n"
+	      "\t-l<payload_size> -i<ifname> -q<rxq_id>\n"
+	      "\t[-x<rx_buf_pages>] [-E<min_data_end>] [-d]\n",
+	      filepath);
 }
 
 static void parse_opts(int argc, char **argv)
@@ -425,7 +443,7 @@ static void parse_opts(int argc, char **argv)
 		usage(argv[0]);
 	cfg_payload_len = max_payload_len;
 
-	while ((c = getopt(argc, argv, "sch:p:l:i:q:o:z:x:d")) != -1) {
+	while ((c = getopt(argc, argv, "sch:p:l:i:q:o:z:x:E:d")) != -1) {
 		switch (c) {
 		case 's':
 			if (cfg_client)
@@ -463,6 +481,10 @@ static void parse_opts(int argc, char **argv)
 		case 'x':
 			cfg_rx_buf_len = page_size * strtoul(optarg, NULL, 0);
 			break;
+		case 'E':
+			cfg_check_data_end = true;
+			cfg_min_data_end = strtoul(optarg, NULL, 0);
+			break;
 		case 'd':
 			cfg_dry_run = true;
 			break;
@@ -484,6 +506,9 @@ static void parse_opts(int argc, char **argv)
 
 	if (cfg_payload_len > max_payload_len)
 		error(1, 0, "-l: payload exceeds max (%d)", max_payload_len);
+	if (cfg_check_data_end &&
+	    cfg_min_data_end >= (cfg_rx_buf_len ?: page_size))
+		error(1, 0, "-E: offset outside rx_buf_len");
 }
 
 int main(int argc, char **argv)
diff --git a/tools/testing/selftests/drivers/net/hw/iou-zcrx.py b/tools/testing/selftests/drivers/net/hw/iou-zcrx.py
index b7a225fe4bea..a9f62c6b950b 100755
--- a/tools/testing/selftests/drivers/net/hw/iou-zcrx.py
+++ b/tools/testing/selftests/drivers/net/hw/iou-zcrx.py
@@ -175,9 +175,13 @@ def test_zcrx_large_chunks(cfg) -> None:
 
     single(cfg)
     page_size = resource.getpagesize()
+    mtu = cfg.dev["mtu"]
     nr_pages = 2
+    while nr_pages * page_size <= 2 * mtu:
+        nr_pages *= 2
     rx_buf_len = nr_pages * page_size
-    rx_cmd = f"{cfg.bin_local} -s -p {cfg.port} -i {cfg.ifname} -q {cfg.target} -x {nr_pages}"
+    rx_cmd = (f"{cfg.bin_local} -s -p {cfg.port} -i {cfg.ifname} "
+              f"-q {cfg.target} -x {nr_pages} -E {mtu}")
     tx_cmd = f"{cfg.bin_remote} -c -h {cfg.addr_v['6']} -p {cfg.port} -l 12840"
 
     probe = cmd(rx_cmd + " -d", fail=False)
-- 
2.55.0

Re: [PATCH net-next v2 5/5] selftests: drv-net: Test large zcrx buffers

[netdev-bot+sashiko]

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

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

- [Medium] In `process_recvzc()`
  (tools/testing/selftests/drivers/net/hw/iou-zcrx.c:304-312) the new…
- [Medium] The new hard failure in `run_server()` (iou-zcrx.c:389-391,
  `error(1, 0, "no payload CQE ending past offset %u",…
- [Medium] The test now enforces a placement guarantee that the in-tree
  zcrx UAPI documentation explicitly disclaims.

Pre-existing issues:
- [Low] The reflowed `usage()` string (iou-zcrx.c:422-429) disagrees with
  the option parser in the same file and with the Python driver: it…

--

selftests: drv-net: Test large zcrx buffers

The zcrx selftest now requests an RX buffer larger than twice the device
MTU and requires at least one zero-copy receive CQE to end past the first
MTU-sized region of its chunk. A new -E option carries the threshold, and
the server aborts if no such CQE was observed.

> diff --git a/tools/testing/selftests/drivers/net/hw/iou-zcrx.c b/tools/testing/selftests/drivers/net/hw/iou-zcrx.c
> index f6a8fc5fac241..c0d9065b21030 100644
> --- a/tools/testing/selftests/drivers/net/hw/iou-zcrx.c
> +++ b/tools/testing/selftests/drivers/net/hw/iou-zcrx.c

[ ... ]

> @@ -298,6 +301,15 @@ static void process_recvzc(struct io_uring *ring, struct io_uring_cqe *cqe)
>  	mask = (1ULL << IORING_ZCRX_AREA_SHIFT) - 1;
>  	data = (char *)area_ptr + (rcqe->off & mask);
>  
> +	if (cfg_check_data_end) {
> +		unsigned int rx_buf_len = cfg_rx_buf_len ?: page_size;
> +		unsigned int data_end_off;
> +
> +		data_end_off = (rcqe->off & mask) % rx_buf_len + n;
> +		if (data_end_off > cfg_min_data_end)
> +			seen_data_end = true;
> +	}
> +

[Severity: Medium]
Can this predicate be satisfied by a driver that only uses the first
MTU-sized region?  It keys on the end of the fragment, and rcqe->off is the
payload start offset inside the chunk:

io_uring/zcrx.c:io_zcrx_queue_cqe() {
	...
	offset = off + (net_iov_idx(niov) << ifq->niov_shift);
	rcqe->off = offset + zcrx_area_id_to_token(area->area_id);
	...
}

with off derived from skb_frag_off(frag) in io_zcrx_recv_frag() and
niov_shift == ilog2(rx_buf_len).  So anything the driver places ahead of the
payload within the chunk (page pool headroom pp->p.offset, or per-packet
alignment padding such as the 128-byte payload alignment in fbnic) counts
toward cfg_min_data_end.

Over IPv6+TCP the maximum payload per segment is mtu - 60 (mtu - 72 with TCP
timestamps), so the margin here is only around 60 bytes.  A driver that
stores exactly one full-size segment in a large chunk, but starts the
payload 60 or more bytes into the chunk, would set seen_data_end and pass —
which is the case the commit message says should fail:

    "A driver which accepts the size but only uses the first MTU-sized
    region fails."

I could not point at an in-tree driver that reserves that much payload-buffer
headroom today, so this is about the strength of the check rather than an
observed pass.  Would keying on the payload start offset be placement
independent?

	(rcqe->off & mask) % rx_buf_len >= cfg_min_data_end

Alternatively, should the threshold account for the maximum per-buffer
headroom and alignment padding?

>  	for (i = 0; i < n; i++) {
>  		if (*(data + i) != payload[(received + i)])
>  			error(1, 0, "payload mismatch at %d", i);
> @@ -373,7 +385,10 @@ static void run_server(void)
>  		server_loop(&ring);
>  
>  	if (!stop)
> -		error(1, 0, "test failed\n");
> +		error(1, 0, "test failed after receiving %zu bytes", received);
> +	if (cfg_check_data_end && !seen_data_end)
> +		error(1, 0, "no payload CQE ending past offset %u",
> +		      cfg_min_data_end);
>  }

[Severity: Medium]
Does this turn a runtime configuration property into a driver failure?

On bnxt, one of the drivers advertising QCFG_RX_PAGE_SIZE, the hardware ends
each aggregation buffer at end-of-packet when TPA is off:

drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_init_one_rx_agg_ring_rxbd() {
	...
	if (!(bp->flags & BNXT_FLAG_TPA))
		type |= RX_BD_FLAGS_AGG_EOP;
	...
}

and BNXT_FLAG_TPA comes only from user-settable features:

drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_set_tpa_flags() {
	bp->flags &= ~BNXT_FLAG_TPA;
	...
	if (bp->dev->features & NETIF_F_LRO)
		bp->flags |= BNXT_FLAG_LRO;
	else if (bp->dev->features & NETIF_F_GRO_HW)
		bp->flags |= BNXT_FLAG_GRO;
}

So with ethtool -K <dev> rx-gro-hw off, every packet payload starts at offset
0 of a fresh buffer, the requested rx_buf_len is still fully honored by
bnxt_validate_qcfg() and the -d probe succeeds, but data_end_off never
exceeds the MSS and run_server() aborts.

Separately, the threshold comes from the local interface MTU while the
received segment size is set by the peer and path MSS, so a jumbo-MTU
receiver talking to a 1500-byte peer cannot produce an end offset past the
MTU without hardware coalescing.

Should the test require or check for HW-GRO/LRO being enabled (or check an
offload/queue statistic) before making this a hard failure?

> @@ -406,8 +421,11 @@ static void run_client(void)
>  
>  static void usage(const char *filepath)
>  {
> -	error(1, 0, "Usage: %s (-4|-6) (-s|-c) -h<server_ip> -p<port> "
> -		    "-l<payload_size> -i<ifname> -q<rxq_id>", filepath);
> +	error(1, 0,
> +	      "Usage: %s (-4|-6) (-s|-c) -h<server_ip> -p<port>\n"
> +	      "\t-l<payload_size> -i<ifname> -q<rxq_id>\n"
> +	      "\t[-x<rx_buf_pages>] [-E<min_data_end>] [-d]\n",
> +	      filepath);
>  }

[Severity: Low]
This is a pre-existing mismatch and not introduced by this patch, but since
the string is being reflowed anyway: the usage text still advertises (-4|-6),
which the parser does not accept, and it omits -o<oneshot_recvs> and
-z<send_size>, which it does accept and which iou-zcrx.py uses in
test_zcrx_oneshot (-o 4 and -z 16384).

>  static void parse_opts(int argc, char **argv)
> @@ -425,7 +443,7 @@ static void parse_opts(int argc, char **argv)
>  		usage(argv[0]);
>  	cfg_payload_len = max_payload_len;
>  
> -	while ((c = getopt(argc, argv, "sch:p:l:i:q:o:z:x:d")) != -1) {
> +	while ((c = getopt(argc, argv, "sch:p:l:i:q:o:z:x:E:d")) != -1) {
>  		switch (c) {
>  		case 's':
>  			if (cfg_client)

[ ... ]

> diff --git a/tools/testing/selftests/drivers/net/hw/iou-zcrx.py b/tools/testing/selftests/drivers/net/hw/iou-zcrx.py
> index b7a225fe4beae..a9f62c6b950bb 100755
> --- a/tools/testing/selftests/drivers/net/hw/iou-zcrx.py
> +++ b/tools/testing/selftests/drivers/net/hw/iou-zcrx.py
> @@ -175,9 +175,13 @@ def test_zcrx_large_chunks(cfg) -> None:
>  
>      single(cfg)
>      page_size = resource.getpagesize()
> +    mtu = cfg.dev["mtu"]
>      nr_pages = 2
> +    while nr_pages * page_size <= 2 * mtu:
> +        nr_pages *= 2
>      rx_buf_len = nr_pages * page_size
> -    rx_cmd = f"{cfg.bin_local} -s -p {cfg.port} -i {cfg.ifname} -q {cfg.target} -x {nr_pages}"
> +    rx_cmd = (f"{cfg.bin_local} -s -p {cfg.port} -i {cfg.ifname} "
> +              f"-q {cfg.target} -x {nr_pages} -E {mtu}")
>      tx_cmd = f"{cfg.bin_remote} -c -h {cfg.addr_v['6']} -p {cfg.port} -l 12840"

[Severity: Medium]
Does this contradict the in-tree zcrx documentation, which is not updated by
this change?

Documentation/networking/iou-zcrx.rst, "Area chunking":

    Larger chunks don't give any additional guarantees about buffer sizes
    returned in CQEs, and they can vary depending on many factors like
    traffic pattern, hardware offload, etc.

The commit message states the opposite:

    "A driver which accepts the size but only uses the first MTU-sized
    region fails."

Mechanically seen_data_end is only set when some CQE satisfies
(rcqe->off % rx_buf_len) + res > mtu, so the driver has to pack more than one
fragment per chunk, or return a fragment larger than the L3 MTU via HW-GRO or
LRO.  A driver that allocates one large niov per frame and places one
MSS-sized segment at chunk offset 0 matches the documented contract and still
fails here.

Should the documented contract be tightened in the same change, or should the
assertion be diagnostic, or gated on a hardware coalescing capability?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910180908.1506533-1-bjorn%40kernel.org