* [PATCH net-next 1/8] eth: mpnic: add scaffolding for Meta Platforms NIC
2026-09-23 1:43 [PATCH net-next 0/8] eth: mpnic: initial support for Meta Platforms NIC Daniel Zahka
@ 2026-09-23 1:43 ` Daniel Zahka
2026-09-23 1:43 ` [PATCH net-next 2/8] eth: mpnic: add register init for the device Daniel Zahka
` (6 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Daniel Zahka @ 2026-09-23 1:43 UTC (permalink / raw)
To: Alexander Duyck, Jakub Kicinski, kernel-team, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Dimitri Daskalakis, Mohsin Bashir
Cc: linux-kernel, netdev, bpf
MPNIC is the next network interface controller designed by Meta. Create
a bare PCI driver for it. Subsequent changes will flesh it out.
Registers live in BAR0 and are addressed as an array of 32 bit words,
which is why the CSR indices are a quarter of the byte offset. Most of
the register file is only accessible as 64 bit, so that is the only
accessor added here.
If a read returns all ones, a second read to an unused register with
reset value 0 is performed to determine if the device is gone. If the
device is gone, the mapped BAR0 pointer is cleared, so no further
accesses reach the bus.
For now, if mpnic_mmio_err() clears mpd->uc_addr0, nothing will
reassign it. The user will have to unbind and rebind the driver. Future
series will reset uc_addr0 from pci error handlers.
In addition to being used as a 0 value reference CSR, MPNIC_BDQ_SPARE is
also used to perform write flushes, because it can be read without
producing side effects.
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
Note: not including an mpnic.rst at this point by design. That will be
introduced when the driver actually exposes user-visible statistics or
configuration options.
---
drivers/net/ethernet/meta/Kconfig | 12 +++
drivers/net/ethernet/meta/Makefile | 1 +
drivers/net/ethernet/meta/mpnic/Makefile | 12 +++
drivers/net/ethernet/meta/mpnic/mpnic.h | 40 ++++++++
drivers/net/ethernet/meta/mpnic/mpnic_csr.h | 24 +++++
drivers/net/ethernet/meta/mpnic/mpnic_pci.c | 147 ++++++++++++++++++++++++++++
6 files changed, 236 insertions(+)
diff --git a/drivers/net/ethernet/meta/Kconfig b/drivers/net/ethernet/meta/Kconfig
index ca5c7ac2a5bc..f940048a5e37 100644
--- a/drivers/net/ethernet/meta/Kconfig
+++ b/drivers/net/ethernet/meta/Kconfig
@@ -35,4 +35,16 @@ config FBNIC
To compile this driver as a module, choose M here. The module
will be called fbnic. MSI-X interrupt support is required.
+config MPNIC
+ tristate "Meta Platforms Network Interface Controller"
+ depends on 64BIT || COMPILE_TEST
+ depends on !S390
+ depends on PCI_MSI
+ help
+ This driver supports the Meta Platforms Network Interface
+ Controller.
+
+ To compile this driver as a module, choose M here. The module
+ will be called mpnic.
+
endif # NET_VENDOR_META
diff --git a/drivers/net/ethernet/meta/Makefile b/drivers/net/ethernet/meta/Makefile
index 88804f3de963..633973419c21 100644
--- a/drivers/net/ethernet/meta/Makefile
+++ b/drivers/net/ethernet/meta/Makefile
@@ -4,3 +4,4 @@
#
obj-$(CONFIG_FBNIC) += fbnic/
+obj-$(CONFIG_MPNIC) += mpnic/
diff --git a/drivers/net/ethernet/meta/mpnic/Makefile b/drivers/net/ethernet/meta/mpnic/Makefile
new file mode 100644
index 000000000000..2588f1f1f1d5
--- /dev/null
+++ b/drivers/net/ethernet/meta/mpnic/Makefile
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) Meta Platforms, Inc. and affiliates.
+
+#
+# Makefile for the Meta(R) Platforms Network Interface Controller
+#
+
+obj-$(CONFIG_MPNIC) += mpnic.o
+
+mpnic-y := \
+ mpnic_pci.o \
+# End of mpnic-y
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic.h b/drivers/net/ethernet/meta/mpnic/mpnic.h
new file mode 100644
index 000000000000..6b8bee93033a
--- /dev/null
+++ b/drivers/net/ethernet/meta/mpnic/mpnic.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#ifndef _MPNIC_H_
+#define _MPNIC_H_
+
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/types.h>
+
+#include "mpnic_csr.h"
+
+#define MPNIC_DRV_NAME "mpnic"
+
+struct mpnic_dev {
+ struct device *dev;
+
+ u32 __iomem *uc_addr0;
+};
+
+u64 mpnic_rd64(struct mpnic_dev *mpd, u32 reg);
+
+static inline void mpnic_wr64(struct mpnic_dev *mpd, u32 reg, u64 val)
+{
+ u32 __iomem *csr = READ_ONCE(mpd->uc_addr0);
+
+ if (csr)
+ writeq(val, csr + reg);
+}
+
+static inline void mpnic_wrfl(struct mpnic_dev *mpd)
+{
+ mpnic_rd64(mpd, MPNIC_BDQ_SPARE);
+}
+
+static inline bool mpnic_present(struct mpnic_dev *mpd)
+{
+ return !!READ_ONCE(mpd->uc_addr0);
+}
+
+#endif /* _MPNIC_H_ */
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
new file mode 100644
index 000000000000..473a2e9f5a09
--- /dev/null
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#ifndef _MPNIC_CSR_H_
+#define _MPNIC_CSR_H_
+
+#include <linux/bits.h>
+
+#define CSR_BIT(nr) BIT_ULL(nr)
+#define CSR_GENMASK(h, l) GENMASK_ULL(h, l)
+
+/* Register Definitions
+ *
+ * The register file is addressed as an array of le32, so the byte address of
+ * a register is 4 times the index below. Each register is listed with its
+ * name, index and byte address.
+ *
+ * Name Index Address
+ *****************************************************************************/
+
+/* NIC_CORE_RBP_HP_GLBL */
+#define MPNIC_BDQ_SPARE 0x42013e /* 0x10804f8 */
+
+#endif /* _MPNIC_CSR_H_ */
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
new file mode 100644
index 000000000000..96393e781241
--- /dev/null
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
@@ -0,0 +1,147 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "mpnic.h"
+
+#define PCI_DEVICE_ID_META_MPNIC 0x0014
+
+static void mpnic_mmio_err(struct mpnic_dev *mpd, u32 reg)
+{
+ /* Hardware is giving us all 1's reads, assume it is gone */
+ WRITE_ONCE(mpd->uc_addr0, NULL);
+
+ dev_err(mpd->dev,
+ "Failed read (idx 0x%x AKA addr 0x%x), disabled CSR access, awaiting reset\n",
+ reg, reg << 2);
+}
+
+u64 mpnic_rd64(struct mpnic_dev *mpd, u32 reg)
+{
+ u32 __iomem *csr = READ_ONCE(mpd->uc_addr0);
+ u64 value;
+
+ if (!csr)
+ return ~0ULL;
+
+ value = readq(csr + reg);
+
+ /* If any bits are 0 value should be valid */
+ if (~value)
+ return value;
+
+ /* All ones can be a valid value, so confirm against a register
+ * which never reads that way on a live device.
+ */
+ if (reg != MPNIC_BDQ_SPARE && ~readq(csr + MPNIC_BDQ_SPARE))
+ return value;
+
+ mpnic_mmio_err(mpd, reg);
+
+ return ~0ULL;
+}
+
+static struct mpnic_dev *mpnic_alloc(struct pci_dev *pdev)
+{
+ struct mpnic_dev *mpd;
+
+ mpd = kzalloc_obj(*mpd);
+ if (!mpd)
+ return NULL;
+
+ pci_set_drvdata(pdev, mpd);
+ mpd->dev = &pdev->dev;
+
+ return mpd;
+}
+
+/**
+ * mpnic_probe - Device initialization routine
+ * @pdev: PCI device information struct
+ * @ent: entry in mpnic_pci_tbl
+ *
+ * Return: 0 on success, negative on failure
+ **/
+static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
+ void __iomem *uc_addr0;
+ struct mpnic_dev *mpd;
+ int err;
+
+ if (pdev->error_state != pci_channel_io_normal) {
+ dev_err(&pdev->dev,
+ "PCI device still in an error state. Unable to load...\n");
+ return -EIO;
+ }
+
+ err = pcim_enable_device(pdev);
+ if (err) {
+ dev_err(&pdev->dev, "PCI enable device failed: %d\n", err);
+ return err;
+ }
+
+ err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(46));
+ if (err) {
+ dev_err(&pdev->dev, "DMA configuration failed: %d\n", err);
+ return err;
+ }
+
+ mpd = mpnic_alloc(pdev);
+ if (!mpd)
+ return -ENOMEM;
+
+ uc_addr0 = pcim_iomap_region(pdev, 0, MPNIC_DRV_NAME);
+ if (IS_ERR(uc_addr0)) {
+ err = PTR_ERR(uc_addr0);
+ dev_err(&pdev->dev, "Mapping the register file failed: %d\n",
+ err);
+ goto err_free_mpd;
+ }
+ mpd->uc_addr0 = uc_addr0;
+
+ pci_set_master(pdev);
+ pci_save_state(pdev);
+
+ return 0;
+
+err_free_mpd:
+ kfree(mpd);
+
+ return err;
+}
+
+/**
+ * mpnic_remove - Device removal routine
+ * @pdev: PCI device information struct
+ **/
+static void mpnic_remove(struct pci_dev *pdev)
+{
+ struct mpnic_dev *mpd = pci_get_drvdata(pdev);
+
+ kfree(mpd);
+}
+
+static const struct pci_device_id mpnic_pci_tbl[] = {
+ { PCI_VDEVICE(META, PCI_DEVICE_ID_META_MPNIC) },
+ /* required last entry */
+ {}
+};
+MODULE_DEVICE_TABLE(pci, mpnic_pci_tbl);
+
+static struct pci_driver mpnic_driver = {
+ .name = MPNIC_DRV_NAME,
+ .id_table = mpnic_pci_tbl,
+ .probe = mpnic_probe,
+ .remove = mpnic_remove,
+};
+
+module_pci_driver(mpnic_driver);
+
+MODULE_DESCRIPTION("Meta Platforms Network Interface Controller");
+MODULE_LICENSE("GPL");
--
2.52.0
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH net-next 2/8] eth: mpnic: add register init for the device
2026-09-23 1:43 [PATCH net-next 0/8] eth: mpnic: initial support for Meta Platforms NIC Daniel Zahka
2026-09-23 1:43 ` [PATCH net-next 1/8] eth: mpnic: add scaffolding " Daniel Zahka
@ 2026-09-23 1:43 ` Daniel Zahka
2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-23 1:43 ` [PATCH net-next 3/8] eth: mpnic: allocate MSI-X vectors Daniel Zahka
` (5 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Daniel Zahka @ 2026-09-23 1:43 UTC (permalink / raw)
To: Alexander Duyck, Jakub Kicinski, kernel-team, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Dimitri Daskalakis, Mohsin Bashir
Cc: linux-kernel, netdev, bpf
From: Mohsin Bashir <mohsin.bashr@gmail.com>
Perform one time register initialization during mpnic_probe(). These CSR
settings are either static configs that only need to be set once, or
pulses for zeroizing on chip SRAMs, which also only need to be done
once.
This initialization is idempotent and does not require any matching
deinitialization in the remove path. We can unbind and re-bind the
driver and perform the same initialization without problems.
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
drivers/net/ethernet/meta/mpnic/Makefile | 1 +
drivers/net/ethernet/meta/mpnic/mpnic.h | 9 +
drivers/net/ethernet/meta/mpnic/mpnic_csr.h | 165 ++++++++
drivers/net/ethernet/meta/mpnic/mpnic_init.c | 553 +++++++++++++++++++++++++++
drivers/net/ethernet/meta/mpnic/mpnic_pci.c | 8 +
5 files changed, 736 insertions(+)
diff --git a/drivers/net/ethernet/meta/mpnic/Makefile b/drivers/net/ethernet/meta/mpnic/Makefile
index 2588f1f1f1d5..9dcaa1c72605 100644
--- a/drivers/net/ethernet/meta/mpnic/Makefile
+++ b/drivers/net/ethernet/meta/mpnic/Makefile
@@ -8,5 +8,6 @@
obj-$(CONFIG_MPNIC) += mpnic.o
mpnic-y := \
+ mpnic_init.o \
mpnic_pci.o \
# End of mpnic-y
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic.h b/drivers/net/ethernet/meta/mpnic/mpnic.h
index 6b8bee93033a..67d48b7ce315 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic.h
@@ -11,14 +11,23 @@
#define MPNIC_DRV_NAME "mpnic"
+#define MPNIC_MAX_TXQS 1024u
+#define MPNIC_MAX_RXQS 1024u
+
struct mpnic_dev {
struct device *dev;
u32 __iomem *uc_addr0;
+
+ u32 mps;
+ u32 readrq;
+ u8 relaxed_ord;
};
u64 mpnic_rd64(struct mpnic_dev *mpd, u32 reg);
+int mpnic_dev_init(struct mpnic_dev *mpd);
+
static inline void mpnic_wr64(struct mpnic_dev *mpd, u32 reg, u64 val)
{
u32 __iomem *csr = READ_ONCE(mpd->uc_addr0);
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index 473a2e9f5a09..42eee18de474 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -9,6 +9,17 @@
#define CSR_BIT(nr) BIT_ULL(nr)
#define CSR_GENMASK(h, l) GENMASK_ULL(h, l)
+#define DESC_BIT(nr) BIT_ULL(nr)
+#define DESC_GENMASK(h, l) GENMASK_ULL(h, l)
+
+/* Transmit Work Descriptor Format */
+#define MPNIC_TWD_L2_HLEN DESC_GENMASK(5, 0)
+#define MPNIC_TWD_FLAG_REQ_COMPLETION DESC_BIT(37)
+
+/* Common fields for all DESC_CFG CSRs */
+#define MPNIC_DESC_CFG_NUM_DESCS CSR_GENMASK(2, 0)
+#define MPNIC_DESC_CFG_START_ADDR CSR_GENMASK(19, 8)
+
/* Register Definitions
*
* The register file is addressed as an array of le32, so the byte address of
@@ -19,6 +30,160 @@
*****************************************************************************/
/* NIC_CORE_RBP_HP_GLBL */
+#define MPNIC_BDQ_GLBL_CTL0 0x420080 /* 0x1080200 */
+#define MPNIC_BDQ_GLBL_CTL0_MAX_REQ_SIZE CSR_GENMASK(26, 18)
+#define MPNIC_BDQ_GLBL_CTL0_PREFETCH_SPACE_THRESH \
+ CSR_GENMASK(42, 32)
+#define MPNIC_RDE_CTL 0x420082 /* 0x1080208 */
+#define MPNIC_RDE_CTL_HPQ_DROP_THRESHOLD CSR_GENMASK(10, 0)
+#define MPNIC_RDE_CTL_PPQ_DROP_THRESHOLD CSR_GENMASK(21, 11)
+#define MPNIC_RDE_CTL_HPQ_LOCAL_DROP_THRESHOLD CSR_GENMASK(42, 32)
+#define MPNIC_RDE_CTL_PPQ_LOCAL_DROP_THRESHOLD CSR_GENMASK(53, 43)
+#define MPNIC_BDQ_MEM_INIT_REQ 0x42013a /* 0x10804e8 */
+#define MPNIC_BDQ_MEM_INIT_DONE 0x42013c /* 0x10804f0 */
#define MPNIC_BDQ_SPARE 0x42013e /* 0x10804f8 */
+#define MPNIC_HPQ_DESC_CFG(i) (0x420140 + 2 * (i)) /* 0x1080500 */
+#define MPNIC_PPQ_DESC_CFG(i) (0x420940 + 2 * (i)) /* 0x1082500 */
+
+/* NIC_CORE_RDE_GLBL */
+#define MPNIC_RDE_MEM_INIT_REQ 0x4240e6 /* 0x1090398 */
+#define MPNIC_RDE_MEM_INIT_DONE 0x4240e8 /* 0x10903a0 */
+
+/* NIC_CORE_RCM_GLBL */
+#define MPNIC_RCM_MEM_INIT_REQ 0x42507e /* 0x10941f8 */
+#define MPNIC_RCM_MEM_INIT_DONE 0x425080 /* 0x1094200 */
+
+/* NIC_CORE_RNI_GLBL */
+#define MPNIC_RNI_RBP_CTL 0x427000 /* 0x109c000 */
+#define MPNIC_RNI_RDE_CTL 0x427002 /* 0x109c008 */
+#define MPNIC_RNI_RDE_CTL_MPS CSR_GENMASK(1, 0)
+#define MPNIC_RNI_RDE_CTL_CLS CSR_GENMASK(3, 2)
+#define MPNIC_RNI_RCM_CTL 0x427004 /* 0x109c010 */
+
+/* NIC_CORE_TDF_GLBL */
+#define MPNIC_TWQ_DEF_PRI_TWD 0x428082 /* 0x10a0208 */
+#define MPNIC_TDF_MEM_INIT_REQ 0x42813a /* 0x10a04e8 */
+#define MPNIC_TDF_MEM_INIT_DONE 0x42813c /* 0x10a04f0 */
+#define MPNIC_TDF_DESC_CFG(i) (0x428140 + 2 * (i)) /* 0x10a0500 */
+
+/* NIC_CORE_TQS_GLBL */
+#define MPNIC_TQS_GLBL_CTL0 0x42a000 /* 0x10a8000 */
+#define MPNIC_TQS_GLBL_CTL0_TWD_ERROR_CHECK_EN CSR_BIT(2)
+#define MPNIC_TQS_GLBL_P0_0 0x42a002 /* 0x10a8008 */
+#define MPNIC_TQS_GLBL_P0_0_TXB_MAX_CRDTS_0 CSR_GENMASK(63, 48)
+#define MPNIC_TQS_GLBL_P0_1 0x42a004 /* 0x10a8010 */
+#define MPNIC_TQS_GLBL_BMC 0x42a012 /* 0x10a8048 */
+#define MPNIC_TQS_GLBL_BMC_TXB_MAX_CRDTS CSR_GENMASK(15, 0)
+#define MPNIC_TQS_SLOWDOWN_CTL 0x42a026 /* 0x10a8098 */
+#define MPNIC_TQS_SLOWDOWN_CTL_ENABLE CSR_BIT(6)
+#define MPNIC_TQS_MTU_CTL0 0x42a030 /* 0x10a80c0 */
+#define MPNIC_TQS_MTU_CTL1 0x42a032 /* 0x10a80c8 */
+#define MPNIC_TQS_SET_P0_MAP0(i) (0x42a082 + 2 * (i)) /* 0x10a8208 */
+#define MPNIC_TQS_SET_P0_MAP1(i) (0x42a092 + 2 * (i)) /* 0x10a8248 */
+#define MPNIC_TQS_GLBL_SHAPING 0x42a108 /* 0x10a8420 */
+#define MPNIC_TQS_GLBL_SHAPING_DISABLE CSR_BIT(0)
+#define MPNIC_TQS_ARB_CTL 0x42a122 /* 0x10a8488 */
+#define MPNIC_TQS_ARB_CTL_SET_CRDT_BUCKET_EN CSR_BIT(9)
+#define MPNIC_TQS_ARB_CTL_SET_IMM_DECR_EN CSR_BIT(8)
+#define MPNIC_TQS_ARB_CTL_GROUP_CRDT_BUCKET_EN CSR_BIT(5)
+#define MPNIC_TQS_ARB_CTL_GROUP_IMM_DECR_EN CSR_BIT(4)
+#define MPNIC_TQS_ARB_CTL_QUEUE_CRDT_BUCKET_EN CSR_BIT(1)
+#define MPNIC_TQS_ARB_CTL_QUEUE_IMM_DECR_EN CSR_BIT(0)
+#define MPNIC_TQS_CEV_MIN_SCHED_THRESH_0 \
+ 0x42a124 /* 0x10a8490 */
+#define MPNIC_TQS_CEV_MIN_SCHED_THRESH_0_QUEUE CSR_GENMASK(19, 0)
+#define MPNIC_TQS_CEV_MIN_SCHED_THRESH_0_GROUP CSR_GENMASK(59, 32)
+#define MPNIC_TQS_CEV_MIN_SCHED_THRESH_1 \
+ 0x42a126 /* 0x10a8498 */
+#define MPNIC_TQS_CEV_MIN_SCHED_THRESH_1_SET CSR_GENMASK(27, 0)
+#define MPNIC_TQS_CEV_MIN_SCHED_THRESH_1_PORT CSR_GENMASK(63, 32)
+#define MPNIC_TQS_SRAM_INIT_CTL 0x42a128 /* 0x10a84a0 */
+#define MPNIC_TQS_SRAM_INIT_CTL_QUANTUM CSR_GENMASK(31, 20)
+#define MPNIC_TQS_SRAM_INIT_CTL_INIT CSR_BIT(32)
+#define MPNIC_TQS_GROUP_INIT_CTL 0x42a12a /* 0x10a84a8 */
+#define MPNIC_TQS_GROUP_INIT_CTL_QUANTUM CSR_GENMASK(51, 32)
+#define MPNIC_TQS_GROUP_INIT_CTL_INIT CSR_BIT(52)
+#define MPNIC_TQS_SET_INIT_CTL 0x42a12c /* 0x10a84b0 */
+#define MPNIC_TQS_SET_INIT_CTL_QUANTUM CSR_GENMASK(51, 32)
+#define MPNIC_TQS_SET_INIT_CTL_INIT CSR_BIT(52)
+#define MPNIC_TQS_PORT_INIT_CTL 0x42a12e /* 0x10a84b8 */
+#define MPNIC_TQS_PORT_INIT_CTL_QUANTUM CSR_GENMASK(55, 32)
+#define MPNIC_TQS_PORT_INIT_CTL_INIT CSR_BIT(56)
+#define MPNIC_TQS_SRAM_STS 0x42a130 /* 0x10a84c0 */
+#define MPNIC_TQS_PORT_CTL(i) (0x42a1e4 + 2 * (i)) /* 0x10a8790 */
+
+/* NIC_CORE_TDE_GLBL */
+#define MPNIC_TDE_MEM_INIT_REQ 0x42b1ee /* 0x10ac7b8 */
+#define MPNIC_TDE_MEM_INIT_DONE 0x42b1f0 /* 0x10ac7c0 */
+
+/* NIC_CORE_TCM_GLBL */
+#define MPNIC_TCM_MEM_INIT_REQ 0x42c0be /* 0x10b02f8 */
+#define MPNIC_TCM_MEM_INIT_DONE 0x42c0c0 /* 0x10b0300 */
+
+/* NIC_CORE_TNI_GLBL */
+#define MPNIC_TNI_GLBL_TDF_CTL 0x42e000 /* 0x10b8000 */
+#define MPNIC_TNI_GLBL_TDF_CTL_MRRS CSR_GENMASK(2, 0)
+#define MPNIC_TNI_GLBL_TDF_CTL_CLS CSR_GENMASK(5, 3)
+#define MPNIC_TNI_GLBL_TDE_CTL 0x42e002 /* 0x10b8008 */
+#define MPNIC_TNI_GLBL_TCM_CTL 0x42e004 /* 0x10b8010 */
+
+/* NIC_CORE_TXB */
+#define MPNIC_TXB_PORT_CONFIG 0x600000 /* 0x1800000 */
+#define MPNIC_TXB_PORT_CONFIG_PORT_MODE CSR_GENMASK(15, 13)
+#define MPNIC_TXB_BMC 0x600122 /* 0x1800488 */
+#define MPNIC_TXB_P0(i) (0x600124 + 2 * (i)) /* 0x1800490 */
+#define MPNIC_TXB_P0_CNT 17
+#define MPNIC_TXB_BMC_THRESH 0x60025c /* 0x1800970 */
+#define MPNIC_TXB_P0_THRESH(i) (0x60025e + 2 * (i)) /* 0x1800978 */
+#define MPNIC_TXB_P0_ARB_WEIGHTS(i) (0x600378 + 2 * (i)) /* 0x1800de0 */
+
+/* NIC_CORE_RXB */
+#define MPNIC_RXB_MEM_INIT_REQ 0x620002 /* 0x1880008 */
+#define MPNIC_RXB_MEM_INIT_DONE 0x620004 /* 0x1880010 */
+#define MPNIC_RXB_PORT_CFG(i) (0x620006 + 2 * (i)) /* 0x1880018 */
+#define MPNIC_RXB_PORT_CFG_FCS_STRIP_MODE CSR_GENMASK(22, 22)
+enum {
+ MPNIC_FCS_MODE_KEEP = 0,
+ MPNIC_FCS_MODE_STRIP = 1,
+};
+
+#define MPNIC_RXB_PORT_CLASS_CFG(i) (0x620020 + 2 * (i)) /* 0x1880080 */
+#define MPNIC_RXB_PORT_CLASS_CFG_DEFAULT_L2_ACTION \
+ CSR_GENMASK(0, 0)
+enum {
+ MPNIC_L2_ACTION_DROP = 0,
+ MPNIC_L2_ACTION_PASS = 1,
+};
+
+#define MPNIC_RXB_TC_CRDTS(i) (0x620418 + 2 * (i)) /* 0x1881060 */
+#define MPNIC_RXB_POOL_COMMON_CRDTS(i) (0x620458 + 2 * (i)) /* 0x1881160 */
+#define MPNIC_RXB_COMMON_CRDT_CTRL_TC(i) \
+ (0x620472 + 2 * (i)) /* 0x18811c8 */
+#define MPNIC_RXB_COMMON_CRDT_CTRL_TC_THRESH CSR_GENMASK(15, 0)
+#define MPNIC_RXB_COMMON_CRDT_CTRL_TC_TC_EN CSR_BIT(29)
+#define MPNIC_RXB_COMMON_CRDT_CTRL_TC_MAX_CRDTS CSR_GENMASK(47, 32)
+#define MPNIC_RXB_HOST_DROP_THRESH(i) (0x6205b0 + 2 * (i)) /* 0x18816c0 */
+#define MPNIC_RXB_BMC_CRDTS(i) (0x6205d0 + 2 * (i)) /* 0x1881740 */
+
+/* NIC_CORE_RPC */
+#define MPNIC_RPC_MEM_INIT_REQ 0x780440 /* 0x1e01100 */
+#define MPNIC_RPC_MEM_INIT_DONE 0x780442 /* 0x1e01108 */
+
+/* NIC_CORE_ROF */
+#define MPNIC_RSC_GLOBAL_CONF 0x7e2002 /* 0x1f88008 */
+#define MPNIC_RSC_GLOBAL_CONF_RSC_DISABLE CSR_BIT(0)
+
+/* NIC_CORE_TOF */
+#define MPNIC_TOF_TCAM_DEST_REMAP 0x7e3022 /* 0x1f8c088 */
+
+/* PEMO_WRAPPER */
+#define MPNIC_OB_ATTR_RO CSR_BIT(1)
+#define MPNIC_OB_ATTR_TDE_H 0x9a000e /* 0x2680038 */
+#define MPNIC_OB_ATTR_TDE_P 0x9a0010 /* 0x2680040 */
+#define MPNIC_OB_ATTR_TDF 0x9a0012 /* 0x2680048 */
+#define MPNIC_OB_ATTR_RBP_HPQ 0x9a0014 /* 0x2680050 */
+#define MPNIC_OB_ATTR_RBP_PPQ 0x9a0016 /* 0x2680058 */
+#define MPNIC_OB_ATTR_RDE_H 0x9a0018 /* 0x2680060 */
+#define MPNIC_OB_ATTR_RDE_P 0x9a001a /* 0x2680068 */
#endif /* _MPNIC_CSR_H_ */
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_init.c b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
new file mode 100644
index 000000000000..f8ebb1976673
--- /dev/null
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
@@ -0,0 +1,553 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cache.h>
+#include <linux/if_ether.h>
+#include <linux/iopoll.h>
+#include <linux/log2.h>
+#include <linux/sizes.h>
+
+#include "mpnic.h"
+
+#define MPNIC_MEM_INIT_POLL_US 500
+#define MPNIC_MEM_INIT_TO_US 5000
+
+/* BDQ mem init:
+ * bit 1: fifo_wrptr_mem
+ * bit 0: fifo_rdptr_mem
+ */
+#define MPNIC_MEM_INIT_BDQ_VAL 0x3
+
+/* RCM mem init:
+ * bit 3: cq_base_addr
+ * bit 2: cd_fifo_rptr_stats
+ * bit 1: cd_fifo_wptr_stats
+ * bit 0: cq_head_ptr_stats
+ */
+#define MPNIC_MEM_INIT_RCM_VAL 0xf
+
+/* RDE mem init, bits 0-18. Bits 0-12 are the per-queue packet, error and
+ * drop counters, bits 13-18 the two context memories of each of the HPQ,
+ * PPQ and SPQ descriptor prefetchers.
+ */
+#define MPNIC_MEM_INIT_RDE_VAL 0x7ffff
+
+/* RPC mem init, bit 0 covers the whole classifier. */
+#define MPNIC_MEM_INIT_RPC_VAL 0x1
+
+/* TCM mem init:
+ * bit 3: cq_head_ptr_stats
+ * bit 2: cd_fifo_wptr_stats
+ * bit 1: cd_fifo_rptr_stats
+ * bit 0: cq_base_addr
+ */
+#define MPNIC_MEM_INIT_TCM_VAL 0xf
+
+/* TDE mem init:
+ * bit 1: stats mem
+ * bit 0: dma_head_ptr SRAM
+ */
+#define MPNIC_MEM_INIT_TDE_VAL 0x3
+
+/* TDF mem init, bit 0 covers the descriptor fetch SRAM. */
+#define MPNIC_MEM_INIT_TDF_VAL 0x1
+
+/* RXB mem init, bit 0 covers the DMAC TCAM statistics RAM. */
+#define MPNIC_MEM_INIT_RXB_VAL 0x1
+
+/* TQS arbiter SRAM init done, one bit per DWRR level. */
+#define MPNIC_TQS_ARB_INIT_VAL 0xf
+
+/* On-chip SRAM allocated to each queue for descriptor fetch, in units of
+ * descriptors. Valid values are 64, 128, 256, 512 and 1024.
+ *
+ * The partition sizes are chosen for the maximum number of queues the
+ * device supports, so they do not have to be adjusted when the active
+ * queue count changes:
+ *
+ * BDQ: 1 MiB / (8 B/DESC) / (1024 HPQ + 1024 PPQ) = 64 DESC/QUEUE
+ * TWQ: 2 MiB / (8 B/DESC) / (1024 TXQ * 2 TWQ) = 128 DESC/QUEUE
+ */
+#define MPNIC_BDQ_SRAM_DESCS 64u
+#define MPNIC_TDF_SRAM_DESCS 128u
+
+/* A total of 1 MiB worth of Tx credits is available, in units of 128 B.
+ * The BMC gets a guaranteed share of them whether or not the host is
+ * routing anything its way, everything else goes to MAC TC0.
+ */
+#define MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL 800
+#define MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL \
+ (SZ_1M / 128 - 2 * MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL)
+
+/* The recommended lower bound for the TXB threshold is 80, based on a 10K
+ * MTU. Round up by 20% to stay on the defensive side. The same reasoning
+ * applies to the arbitration weight, which has to exceed the full packet
+ * size.
+ */
+#define MPNIC_TXB_INIT_BMC_THRESH 100
+#define MPNIC_TXB_INIT_P0_THRESH 100
+#define MPNIC_TXB_INIT_P0_ARB_WEIGHTS 0x64
+
+/* RXB host drop threshold in units of 128 B beats. Packets targeting a
+ * queue are dropped when the available credits fall below it. 80 beats is
+ * 10 KB, which is also the largest frame the device is configured for.
+ */
+#define MPNIC_RXB_INIT_HOST_DROP_THRESH 0x50
+
+/* A total of 8 MiB of Rx buffer is available. The recommended per-TC pool
+ * for a 800G configuration is 420 KB with all 8 TCs enabled. Only one TC
+ * is in use, so give it 8 * 420 KB (in units of 128 B) and push the rest
+ * to the common pool. Start drawing from the common pool as soon as the
+ * TC0 credits fall below one max sized frame. The BMC keeps a small
+ * reserve of its own whether or not the host talks to it.
+ */
+#define MPNIC_RXB_INIT_POOL_TC_CRDTS_P0 (0xd20 * 8)
+#define MPNIC_RXB_INIT_BMC_CRDTS 0x20
+#define MPNIC_RXB_INIT_COMMON_CRDT_MAX_THRSH \
+ (SZ_8M / 128 - MPNIC_RXB_INIT_POOL_TC_CRDTS_P0 - \
+ MPNIC_RXB_INIT_BMC_CRDTS)
+#define MPNIC_RXB_INIT_COMMON_CRDT_THRSH 0x50
+
+/* TXB port mode selects the number of active MAC ports for Tx buffer
+ * credit distribution and arbitration. The hardware only supports single,
+ * dual and quad port, encoded as b'001, b'010 and b'100.
+ */
+#define MPNIC_TXB_PORT_MODE_SINGLE 1
+
+/* The MAC traffic classes start at index 8 in the TXB arrays, the BMC
+ * sits above them.
+ */
+#define MPNIC_TXB_TC_IDX_MAC_0 8
+#define MPNIC_TXB_TC_IDX_BMC 16
+
+/* Largest frame the Tx queue scheduler will pass through. Anything above
+ * it gets truncated.
+ */
+#define MPNIC_TQS_MTU_CTL0_MAX 0x2800
+
+/* The unit of the DWRR quantum is 256 B. It has to be large enough for at
+ * least 11 MTUs to be transmitted in one quantum; use 15 for headroom.
+ */
+#define MPNIC_TQS_DWRR_INIT_QUANTUM (15 * MPNIC_TQS_MTU_CTL0_MAX / 256)
+
+/* Lower bound on the credit available to a queue, group, set or port
+ * before the scheduler stops issuing requests for it. 15 MTUs, matching
+ * the DWRR quantum above.
+ */
+#define MPNIC_TQS_DWRR_CEV_MIN_SCHED_THRESH (15 * MPNIC_TQS_MTU_CTL0_MAX)
+
+/* The 1 MiB Tx buffer is partitioned between the MAC and the BMC in units
+ * of 1 KiB. The BMC portion is fixed at 100 KB.
+ */
+#define MPNIC_TQS_GLBL_TXB_CRDT_BMC 100
+#define MPNIC_TQS_GLBL_TXB_CRDT_MAC (SZ_1M / SZ_1K - \
+ MPNIC_TQS_GLBL_TXB_CRDT_BMC)
+
+struct mpnic_init_poll {
+ u64 exp_val;
+ u32 addr;
+};
+
+struct mpnic_poll_state {
+ int poll_idx;
+ u64 val;
+};
+
+static void mpnic_tdf_glbl_init(struct mpnic_dev *mpd)
+{
+ /* Default metadata descriptor, used for frames the driver did not
+ * prepend one to.
+ */
+ mpnic_wr64(mpd, MPNIC_TWQ_DEF_PRI_TWD,
+ FIELD_PREP(MPNIC_TWD_L2_HLEN, ETH_HLEN) |
+ MPNIC_TWD_FLAG_REQ_COMPLETION);
+}
+
+static void mpnic_txb_init(struct mpnic_dev *mpd)
+{
+ int i;
+
+ mpnic_wr64(mpd, MPNIC_TXB_BMC, MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL);
+
+ /* Zero the private credits of every traffic class, then hand the
+ * unreserved ones to MAC TC0.
+ */
+ for (i = 0; i < MPNIC_TXB_P0_CNT; i++)
+ mpnic_wr64(mpd, MPNIC_TXB_P0(i), 0);
+ mpnic_wr64(mpd, MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_MAC_0),
+ MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL);
+ mpnic_wr64(mpd, MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_BMC),
+ MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL);
+
+ mpnic_wr64(mpd, MPNIC_TXB_BMC_THRESH, MPNIC_TXB_INIT_BMC_THRESH);
+
+ mpnic_wr64(mpd, MPNIC_TXB_P0_THRESH(MPNIC_TXB_TC_IDX_MAC_0),
+ MPNIC_TXB_INIT_P0_THRESH);
+ mpnic_wr64(mpd, MPNIC_TXB_P0_ARB_WEIGHTS(MPNIC_TXB_TC_IDX_MAC_0),
+ MPNIC_TXB_INIT_P0_ARB_WEIGHTS);
+ mpnic_wr64(mpd, MPNIC_TXB_PORT_CONFIG,
+ FIELD_PREP(MPNIC_TXB_PORT_CONFIG_PORT_MODE,
+ MPNIC_TXB_PORT_MODE_SINGLE));
+}
+
+static void mpnic_rxb_init(struct mpnic_dev *mpd)
+{
+ /* Accept all packets that miss dmac tcam, until l2 filtering is
+ * implemented.
+ */
+ mpnic_wr64(mpd, MPNIC_RXB_PORT_CLASS_CFG(0),
+ FIELD_PREP(MPNIC_RXB_PORT_CLASS_CFG_DEFAULT_L2_ACTION,
+ MPNIC_L2_ACTION_PASS));
+ mpnic_wr64(mpd, MPNIC_RXB_PORT_CFG(0),
+ FIELD_PREP(MPNIC_RXB_PORT_CFG_FCS_STRIP_MODE,
+ MPNIC_FCS_MODE_STRIP));
+
+ mpnic_wr64(mpd, MPNIC_RXB_HOST_DROP_THRESH(0),
+ MPNIC_RXB_INIT_HOST_DROP_THRESH);
+
+ mpnic_wr64(mpd, MPNIC_RXB_TC_CRDTS(0),
+ MPNIC_RXB_INIT_POOL_TC_CRDTS_P0);
+ mpnic_wr64(mpd, MPNIC_RXB_COMMON_CRDT_CTRL_TC(0),
+ FIELD_PREP(MPNIC_RXB_COMMON_CRDT_CTRL_TC_THRESH,
+ MPNIC_RXB_INIT_COMMON_CRDT_THRSH) |
+ FIELD_PREP(MPNIC_RXB_COMMON_CRDT_CTRL_TC_MAX_CRDTS,
+ MPNIC_RXB_INIT_COMMON_CRDT_MAX_THRSH) |
+ MPNIC_RXB_COMMON_CRDT_CTRL_TC_TC_EN);
+
+ /* Only pool 0 is used, it gets all of the common credits */
+ mpnic_wr64(mpd, MPNIC_RXB_POOL_COMMON_CRDTS(0),
+ MPNIC_RXB_INIT_COMMON_CRDT_MAX_THRSH);
+
+ mpnic_wr64(mpd, MPNIC_RXB_BMC_CRDTS(0), MPNIC_RXB_INIT_BMC_CRDTS);
+
+ mpnic_wr64(mpd, MPNIC_RXB_MEM_INIT_REQ, MPNIC_MEM_INIT_RXB_VAL);
+}
+
+static u64 mpnic_desc_cfg(unsigned int sram_descs, unsigned int q_idx)
+{
+ /* The hardware encodes the partition size as 64 * 2^n descriptors,
+ * and its start address in units of 64 descriptors.
+ */
+ return FIELD_PREP(MPNIC_DESC_CFG_NUM_DESCS, __ffs(sram_descs) - 6) |
+ FIELD_PREP(MPNIC_DESC_CFG_START_ADDR,
+ q_idx * (sram_descs >> 6));
+}
+
+static void mpnic_desc_sram_init(struct mpnic_dev *mpd)
+{
+ int i;
+
+ for (i = 0; i < MPNIC_MAX_TXQS * 2; i++)
+ mpnic_wr64(mpd, MPNIC_TDF_DESC_CFG(i),
+ mpnic_desc_cfg(MPNIC_TDF_SRAM_DESCS, i));
+
+ for (i = 0; i < MPNIC_MAX_RXQS; i++) {
+ mpnic_wr64(mpd, MPNIC_HPQ_DESC_CFG(i),
+ mpnic_desc_cfg(MPNIC_BDQ_SRAM_DESCS, i));
+ mpnic_wr64(mpd, MPNIC_PPQ_DESC_CFG(i),
+ mpnic_desc_cfg(MPNIC_BDQ_SRAM_DESCS,
+ MPNIC_MAX_RXQS + i));
+ }
+}
+
+static void mpnic_rxglb_init(struct mpnic_dev *mpd)
+{
+ /* Descriptor prefetch reads are only issued once 32 descriptors
+ * worth of FIFO space is available, and no more than 64 descriptors
+ * are fetched for one queue at a time so that a single queue cannot
+ * monopolize the bus. Both have to be multiples of 16 to keep the
+ * reads 128 B aligned.
+ */
+ mpnic_wr64(mpd, MPNIC_BDQ_GLBL_CTL0,
+ FIELD_PREP(MPNIC_BDQ_GLBL_CTL0_PREFETCH_SPACE_THRESH, 32) |
+ FIELD_PREP(MPNIC_BDQ_GLBL_CTL0_MAX_REQ_SIZE, 64));
+
+ /* Minimum number of descriptors which has to be available before
+ * the descriptor engine considers a queue usable, globally and in
+ * the per-queue prefetch FIFO.
+ */
+ mpnic_wr64(mpd, MPNIC_RDE_CTL,
+ FIELD_PREP(MPNIC_RDE_CTL_HPQ_DROP_THRESHOLD, 16) |
+ FIELD_PREP(MPNIC_RDE_CTL_PPQ_DROP_THRESHOLD, 16) |
+ FIELD_PREP(MPNIC_RDE_CTL_HPQ_LOCAL_DROP_THRESHOLD, 16) |
+ FIELD_PREP(MPNIC_RDE_CTL_PPQ_LOCAL_DROP_THRESHOLD, 16));
+
+ /* Receive side coalescing is not supported yet */
+ mpnic_wr64(mpd, MPNIC_RSC_GLOBAL_CONF,
+ MPNIC_RSC_GLOBAL_CONF_RSC_DISABLE);
+
+ mpnic_wr64(mpd, MPNIC_BDQ_MEM_INIT_REQ, MPNIC_MEM_INIT_BDQ_VAL);
+ mpnic_wr64(mpd, MPNIC_RCM_MEM_INIT_REQ, MPNIC_MEM_INIT_RCM_VAL);
+ mpnic_wr64(mpd, MPNIC_RDE_MEM_INIT_REQ, MPNIC_MEM_INIT_RDE_VAL);
+ mpnic_wr64(mpd, MPNIC_RPC_MEM_INIT_REQ, MPNIC_MEM_INIT_RPC_VAL);
+}
+
+static void mpnic_txglb_init(struct mpnic_dev *mpd)
+{
+ /* Nothing is redirected to the BMC until the Tx offload TCAM gets
+ * programmed with its addresses.
+ */
+ mpnic_wr64(mpd, MPNIC_TOF_TCAM_DEST_REMAP, 0);
+
+ mpnic_wr64(mpd, MPNIC_TCM_MEM_INIT_REQ, MPNIC_MEM_INIT_TCM_VAL);
+ mpnic_wr64(mpd, MPNIC_TDE_MEM_INIT_REQ, MPNIC_MEM_INIT_TDE_VAL);
+ mpnic_wr64(mpd, MPNIC_TDF_MEM_INIT_REQ, MPNIC_MEM_INIT_TDF_VAL);
+}
+
+/* Fill the DWRR arbiter memories. Setting the INIT bit makes the hardware
+ * write the given credit and quantum into every entry at the queue, group,
+ * set and port level, so nothing has to be programmed per queue.
+ */
+static void mpnic_tqs_sram_init(struct mpnic_dev *mpd)
+{
+ mpnic_wr64(mpd, MPNIC_TQS_GROUP_INIT_CTL,
+ FIELD_PREP(MPNIC_TQS_GROUP_INIT_CTL_QUANTUM,
+ MPNIC_TQS_DWRR_INIT_QUANTUM) |
+ MPNIC_TQS_GROUP_INIT_CTL_INIT);
+ mpnic_wr64(mpd, MPNIC_TQS_SET_INIT_CTL,
+ FIELD_PREP(MPNIC_TQS_SET_INIT_CTL_QUANTUM,
+ MPNIC_TQS_DWRR_INIT_QUANTUM) |
+ MPNIC_TQS_SET_INIT_CTL_INIT);
+ mpnic_wr64(mpd, MPNIC_TQS_PORT_INIT_CTL,
+ FIELD_PREP(MPNIC_TQS_PORT_INIT_CTL_QUANTUM,
+ MPNIC_TQS_DWRR_INIT_QUANTUM) |
+ MPNIC_TQS_PORT_INIT_CTL_INIT);
+ mpnic_wr64(mpd, MPNIC_TQS_SRAM_INIT_CTL,
+ FIELD_PREP(MPNIC_TQS_SRAM_INIT_CTL_QUANTUM,
+ MPNIC_TQS_DWRR_INIT_QUANTUM) |
+ MPNIC_TQS_SRAM_INIT_CTL_INIT);
+}
+
+static void mpnic_tqs_init(struct mpnic_dev *mpd)
+{
+ u64 val;
+
+ /* Initialize to the largest frame we support, the scheduler
+ * truncates anything above it. The BMC gets the same limit.
+ */
+ mpnic_wr64(mpd, MPNIC_TQS_MTU_CTL0, MPNIC_TQS_MTU_CTL0_MAX);
+ mpnic_wr64(mpd, MPNIC_TQS_MTU_CTL1, MPNIC_TQS_MTU_CTL0_MAX);
+
+ mpnic_wr64(mpd, MPNIC_TQS_GLBL_CTL0,
+ MPNIC_TQS_GLBL_CTL0_TWD_ERROR_CHECK_EN);
+
+ mpnic_tqs_sram_init(mpd);
+
+ /* Only port 0 is used. A single traffic class is in use as well, so
+ * give all of the Tx buffer credits to TC0.
+ */
+ mpnic_wr64(mpd, MPNIC_TQS_GLBL_P0_0,
+ FIELD_PREP(MPNIC_TQS_GLBL_P0_0_TXB_MAX_CRDTS_0,
+ MPNIC_TQS_GLBL_TXB_CRDT_MAC));
+ mpnic_wr64(mpd, MPNIC_TQS_GLBL_P0_1, 0);
+ mpnic_wr64(mpd, MPNIC_TQS_GLBL_BMC,
+ FIELD_PREP(MPNIC_TQS_GLBL_BMC_TXB_MAX_CRDTS,
+ MPNIC_TQS_GLBL_TXB_CRDT_BMC));
+
+ mpnic_wr64(mpd, MPNIC_TQS_PORT_CTL(0), 0);
+
+ /* Map all sets to port 0 */
+ mpnic_wr64(mpd, MPNIC_TQS_SET_P0_MAP0(0), ~0ULL);
+ mpnic_wr64(mpd, MPNIC_TQS_SET_P0_MAP1(0), ~0ULL);
+
+ mpnic_wr64(mpd, MPNIC_TQS_CEV_MIN_SCHED_THRESH_0,
+ FIELD_PREP(MPNIC_TQS_CEV_MIN_SCHED_THRESH_0_QUEUE,
+ MPNIC_TQS_DWRR_CEV_MIN_SCHED_THRESH) |
+ FIELD_PREP(MPNIC_TQS_CEV_MIN_SCHED_THRESH_0_GROUP,
+ MPNIC_TQS_DWRR_CEV_MIN_SCHED_THRESH));
+ mpnic_wr64(mpd, MPNIC_TQS_CEV_MIN_SCHED_THRESH_1,
+ FIELD_PREP(MPNIC_TQS_CEV_MIN_SCHED_THRESH_1_SET,
+ MPNIC_TQS_DWRR_CEV_MIN_SCHED_THRESH) |
+ FIELD_PREP(MPNIC_TQS_CEV_MIN_SCHED_THRESH_1_PORT,
+ MPNIC_TQS_DWRR_CEV_MIN_SCHED_THRESH));
+
+ /* The rate limiters are left uninitialized, so shaping has to stay
+ * off or nothing would ever get scheduled.
+ */
+ mpnic_wr64(mpd, MPNIC_TQS_GLBL_SHAPING, MPNIC_TQS_GLBL_SHAPING_DISABLE);
+
+ /* Enable fairness protection (phantom eligibility). Read modify
+ * write so that the reset default slowdown cycle is preserved.
+ */
+ val = mpnic_rd64(mpd, MPNIC_TQS_SLOWDOWN_CTL);
+ val |= MPNIC_TQS_SLOWDOWN_CTL_ENABLE;
+ mpnic_wr64(mpd, MPNIC_TQS_SLOWDOWN_CTL, val);
+
+ /* Use immediate credit decrement at every DWRR level so that the
+ * credit reflects a grant in the same cycle.
+ */
+ val = mpnic_rd64(mpd, MPNIC_TQS_ARB_CTL);
+ val |= MPNIC_TQS_ARB_CTL_SET_CRDT_BUCKET_EN |
+ MPNIC_TQS_ARB_CTL_SET_IMM_DECR_EN |
+ MPNIC_TQS_ARB_CTL_GROUP_CRDT_BUCKET_EN |
+ MPNIC_TQS_ARB_CTL_GROUP_IMM_DECR_EN |
+ MPNIC_TQS_ARB_CTL_QUEUE_CRDT_BUCKET_EN |
+ MPNIC_TQS_ARB_CTL_QUEUE_IMM_DECR_EN;
+ mpnic_wr64(mpd, MPNIC_TQS_ARB_CTL, val);
+}
+
+/* The MPS and CLS fields sit at the same bit positions in every block, so
+ * one set of masks covers both the RNI and the TNI registers.
+ */
+static void mpnic_mps_init(struct mpnic_dev *mpd, u32 reg, unsigned int mps,
+ unsigned int cls)
+{
+ u64 val = mpnic_rd64(mpd, reg);
+
+ val &= ~(MPNIC_RNI_RDE_CTL_MPS | MPNIC_RNI_RDE_CTL_CLS);
+ val |= FIELD_PREP(MPNIC_RNI_RDE_CTL_MPS, mps) |
+ FIELD_PREP(MPNIC_RNI_RDE_CTL_CLS, cls);
+
+ mpnic_wr64(mpd, reg, val);
+}
+
+/* Likewise for the MRRS and CLS fields, which have their own common
+ * layout.
+ */
+static void mpnic_mrrs_init(struct mpnic_dev *mpd, u32 reg, unsigned int mrrs,
+ unsigned int cls)
+{
+ u64 val = mpnic_rd64(mpd, reg);
+
+ val &= ~(MPNIC_TNI_GLBL_TDF_CTL_MRRS | MPNIC_TNI_GLBL_TDF_CTL_CLS);
+ val |= FIELD_PREP(MPNIC_TNI_GLBL_TDF_CTL_MRRS, mrrs) |
+ FIELD_PREP(MPNIC_TNI_GLBL_TDF_CTL_CLS, cls);
+
+ mpnic_wr64(mpd, reg, val);
+}
+
+/**
+ * mpnic_axi_init - Configure AXI bus parameters from host PCIe capabilities
+ * @mpd: Device to configure
+ *
+ * Programs the max read request size, max payload size and cache line size
+ * of the DMA engines. The hardware encodes all three as a power of 2 index,
+ * MRRS and MPS relative to 128 B and CLS relative to 64 B.
+ *
+ * MAX_OT and MAX_OB are left at their hardware defaults.
+ */
+static void mpnic_axi_init(struct mpnic_dev *mpd)
+{
+ int mps, cls, mrrs;
+
+ mps = clamp(ilog2(mpd->mps) - 7, 0, 3);
+ cls = clamp(ilog2(L1_CACHE_BYTES) - 6, 0, 3);
+
+ mpnic_mps_init(mpd, MPNIC_RNI_RDE_CTL, mps, cls);
+ mpnic_mps_init(mpd, MPNIC_RNI_RCM_CTL, mps, cls);
+ mpnic_mps_init(mpd, MPNIC_TNI_GLBL_TCM_CTL, mps, cls);
+
+ mrrs = clamp(ilog2(mpd->readrq) - 7, 0, 3);
+ mpnic_mrrs_init(mpd, MPNIC_RNI_RBP_CTL, mrrs, cls);
+ mpnic_mrrs_init(mpd, MPNIC_TNI_GLBL_TDF_CTL, mrrs, cls);
+
+ /* TDE supports a wider range of MRRS encodings. */
+ mrrs = clamp(ilog2(mpd->readrq) - 7, 0, 5);
+ mpnic_mrrs_init(mpd, MPNIC_TNI_GLBL_TDE_CTL, mrrs, cls);
+}
+
+/**
+ * mpnic_ro_init - Set relaxed ordering on the outbound TLP attributes
+ * @mpd: Device to configure
+ *
+ * Completions must stay ordered so that they are not observed before the
+ * payload DMA they describe has landed, so RCM and TCM are left alone.
+ */
+static void mpnic_ro_init(struct mpnic_dev *mpd)
+{
+ u64 attr = mpd->relaxed_ord ? MPNIC_OB_ATTR_RO : 0;
+
+ mpnic_wr64(mpd, MPNIC_OB_ATTR_TDE_H, attr);
+ mpnic_wr64(mpd, MPNIC_OB_ATTR_TDE_P, attr);
+ mpnic_wr64(mpd, MPNIC_OB_ATTR_TDF, attr);
+ mpnic_wr64(mpd, MPNIC_OB_ATTR_RBP_HPQ, attr);
+ mpnic_wr64(mpd, MPNIC_OB_ATTR_RBP_PPQ, attr);
+ mpnic_wr64(mpd, MPNIC_OB_ATTR_RDE_H, attr);
+ mpnic_wr64(mpd, MPNIC_OB_ATTR_RDE_P, attr);
+}
+
+static bool mpnic_init_status_ready(struct mpnic_dev *mpd,
+ const struct mpnic_init_poll *polls,
+ struct mpnic_poll_state *state)
+{
+ u64 val;
+ int i;
+
+ for (i = state->poll_idx; polls[i].addr; i++) {
+ val = mpnic_rd64(mpd, polls[i].addr);
+
+ if ((val & polls[i].exp_val) != polls[i].exp_val) {
+ state->poll_idx = i;
+ state->val = val;
+ return false;
+ }
+ }
+
+ return true;
+}
+
+/**
+ * mpnic_mem_init_poll - Wait for the memory initializations to complete
+ * @mpd: Device to poll
+ *
+ * The blocks initialize their memories in parallel, so walk the status
+ * registers in order and only go back to sleep on the first one which is
+ * not done yet.
+ *
+ * Return: 0 on success, -ETIMEDOUT if not everything completed in time
+ */
+static int mpnic_mem_init_poll(struct mpnic_dev *mpd)
+{
+ static const struct mpnic_init_poll polls[] = {
+ { MPNIC_TQS_ARB_INIT_VAL, MPNIC_TQS_SRAM_STS },
+ { MPNIC_MEM_INIT_BDQ_VAL, MPNIC_BDQ_MEM_INIT_DONE },
+ { MPNIC_MEM_INIT_RCM_VAL, MPNIC_RCM_MEM_INIT_DONE },
+ { MPNIC_MEM_INIT_RDE_VAL, MPNIC_RDE_MEM_INIT_DONE },
+ { MPNIC_MEM_INIT_RPC_VAL, MPNIC_RPC_MEM_INIT_DONE },
+ { MPNIC_MEM_INIT_TCM_VAL, MPNIC_TCM_MEM_INIT_DONE },
+ { MPNIC_MEM_INIT_TDE_VAL, MPNIC_TDE_MEM_INIT_DONE },
+ { MPNIC_MEM_INIT_TDF_VAL, MPNIC_TDF_MEM_INIT_DONE },
+ { MPNIC_MEM_INIT_RXB_VAL, MPNIC_RXB_MEM_INIT_DONE },
+ { 0 },
+ };
+ struct mpnic_poll_state state = {};
+ bool done;
+ int err;
+
+ err = read_poll_timeout(mpnic_init_status_ready, done, done,
+ MPNIC_MEM_INIT_POLL_US, MPNIC_MEM_INIT_TO_US,
+ false, mpd, polls, &state);
+ if (err)
+ dev_err(mpd->dev, "Poll timeout for reg 0x%x: 0x%llx\n",
+ polls[state.poll_idx].addr, state.val);
+
+ return err;
+}
+
+int mpnic_dev_init(struct mpnic_dev *mpd)
+{
+ int err;
+
+ mpnic_tdf_glbl_init(mpd);
+ mpnic_txb_init(mpd);
+ mpnic_rxb_init(mpd);
+ mpnic_desc_sram_init(mpd);
+ mpnic_axi_init(mpd);
+ mpnic_ro_init(mpd);
+ mpnic_rxglb_init(mpd);
+ mpnic_txglb_init(mpd);
+ mpnic_tqs_init(mpd);
+
+ err = mpnic_mem_init_poll(mpd);
+ if (err) {
+ dev_err(mpd->dev, "Device initialization failed: %d\n", err);
+ return err;
+ }
+
+ if (!mpnic_present(mpd))
+ return -EIO;
+
+ return 0;
+}
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
index 96393e781241..68a64377e6c5 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
@@ -58,6 +58,10 @@ static struct mpnic_dev *mpnic_alloc(struct pci_dev *pdev)
pci_set_drvdata(pdev, mpd);
mpd->dev = &pdev->dev;
+ mpd->mps = pcie_get_mps(pdev);
+ mpd->readrq = pcie_get_readrq(pdev);
+ mpd->relaxed_ord = pcie_relaxed_ordering_enabled(pdev);
+
return mpd;
}
@@ -108,6 +112,10 @@ static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
pci_set_master(pdev);
pci_save_state(pdev);
+ err = mpnic_dev_init(mpd);
+ if (err)
+ goto err_free_mpd;
+
return 0;
err_free_mpd:
--
2.52.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 2/8] eth: mpnic: add register init for the device
2026-09-23 1:43 ` [PATCH net-next 2/8] eth: mpnic: add register init for the device Daniel Zahka
@ 2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-24 16:16 ` Daniel Zahka
0 siblings, 1 reply; 22+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 2:05 UTC (permalink / raw)
To: daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] pci: missing pci_resource_len() check in mpnic_probe() allows
OOB MMIO access
- [Medium] The 1 MiB Tx buffer is split between MAC and BMC differently
in TXB and in TQS.
--
eth: mpnic: add register init for the device
This commit adds one-time CSR initialization to mpnic_probe() through a
new mpnic_dev_init(). It programs the TXB, RXB, TQS, descriptor SRAM,
AXI and relaxed ordering settings, starts the on-chip SRAM
initializations and polls until they complete.
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_init.c b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
> new file mode 100644
> index 0000000000000..f8ebb19766731
> --- /dev/null
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
> @@ -0,0 +1,553 @@
[ ... ]
> +/* A total of 1 MiB worth of Tx credits is available, in units of 128 B.
> + * The BMC gets a guaranteed share of them whether or not the host is
> + * routing anything its way, everything else goes to MAC TC0.
> + */
> +#define MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL 800
> +#define MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL \
> + (SZ_1M / 128 - 2 * MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL)
[ ... ]
> +/* The 1 MiB Tx buffer is partitioned between the MAC and the BMC in units
> + * of 1 KiB. The BMC portion is fixed at 100 KB.
> + */
> +#define MPNIC_TQS_GLBL_TXB_CRDT_BMC 100
> +#define MPNIC_TQS_GLBL_TXB_CRDT_MAC (SZ_1M / SZ_1K - \
> + MPNIC_TQS_GLBL_TXB_CRDT_BMC)
[ ... ]
> +static void mpnic_txb_init(struct mpnic_dev *mpd)
> +{
> + int i;
> +
> + mpnic_wr64(mpd, MPNIC_TXB_BMC, MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL);
> +
> + /* Zero the private credits of every traffic class, then hand the
> + * unreserved ones to MAC TC0.
> + */
> + for (i = 0; i < MPNIC_TXB_P0_CNT; i++)
> + mpnic_wr64(mpd, MPNIC_TXB_P0(i), 0);
> + mpnic_wr64(mpd, MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_MAC_0),
> + MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL);
> + mpnic_wr64(mpd, MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_BMC),
> + MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL);
[ ... ]
> +static void mpnic_tqs_init(struct mpnic_dev *mpd)
> +{
[ ... ]
> + /* Only port 0 is used. A single traffic class is in use as well, so
> + * give all of the Tx buffer credits to TC0.
> + */
> + mpnic_wr64(mpd, MPNIC_TQS_GLBL_P0_0,
> + FIELD_PREP(MPNIC_TQS_GLBL_P0_0_TXB_MAX_CRDTS_0,
> + MPNIC_TQS_GLBL_TXB_CRDT_MAC));
> + mpnic_wr64(mpd, MPNIC_TQS_GLBL_P0_1, 0);
> + mpnic_wr64(mpd, MPNIC_TQS_GLBL_BMC,
> + FIELD_PREP(MPNIC_TQS_GLBL_BMC_TXB_MAX_CRDTS,
> + MPNIC_TQS_GLBL_TXB_CRDT_BMC));
[Severity: Medium]
Do TXB and TQS split the 1 MiB Tx buffer the same way?
On the TXB side, mpnic_txb_init() writes MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL
(800 x 128 B = 100 KiB) to both MPNIC_TXB_BMC and
MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_BMC). MAC TC0 then gets:
SZ_1M / 128 - 2 * 800 = 6592 credits = 824 KiB
On the TQS side, mpnic_tqs_init() programs
MPNIC_TQS_GLBL_P0_0_TXB_MAX_CRDTS_0 with:
SZ_1M / SZ_1K - 100 = 924 (1 KiB units) = 924 KiB
That lets the scheduler use 100 KiB more for TC0 than TXB reserved as
TC0 private credit.
Suppose TXB_MAX_CRDTS limits how much data the scheduler can have
outstanding in the TC0 TXB pool. Could a Tx burst that fills TC0 then
overrun the pool once the Tx datapath is running?
If the BMC only needs a single 100 KiB share, as the TQS comment says,
does the "2 *" in MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL leave 100 KiB of Tx
buffer unused?
The TXB comment says "everything else goes to MAC TC0", but the code
subtracts the BMC share twice, so the two don't match either.
[ ... ]
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
> @@ -108,6 +112,10 @@ static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> pci_set_master(pdev);
> pci_save_state(pdev);
>
> + err = mpnic_dev_init(mpd);
> + if (err)
> + goto err_free_mpd;
> +
> return 0;
>
> err_free_mpd:
>
[Severity: High]
mpnic_dev_init() reaches much further into BAR 0 than anything the
driver touched before. Before this patch, the furthest access was
MPNIC_BDQ_SPARE in mpnic_rd64(). Now the TXB, RXB, RPC, ROF, TOF and
PEMO_WRAPPER blocks are all programmed.
The highest one is MPNIC_OB_ATTR_RDE_P in mpnic_ro_init(). It is
0x9a001a in u32 units, so the writeq() lands at byte offset 0x2680068.
BAR 0 therefore has to be at least ~38.5 MiB for this sequence to stay
inside the mapping.
mpnic_probe() maps BAR 0 with pcim_iomap_region(), which accepts
whatever length the device advertised. Nothing checks
pci_resource_len(pdev, 0) against the register file the driver is
about to use. mpnic_wr64() and mpnic_rd64() only test that uc_addr0
is non-NULL, then add the register index unconditionally.
Suppose a device presents a BAR 0 smaller than that. It could be a
misconfigured board, or something spoofing this ID behind
Thunderbolt/USB4. Won't the first out-of-range writeq() in
mpnic_dev_init() fault on an unmapped address during probe, or
scribble over whatever happens to be mapped after the BAR?
Could mpnic_probe() reject the device with -ENODEV before calling
mpnic_dev_init() when pci_resource_len(pdev, 0) is too small? Does the
hardware have a documented BAR 0 size that could serve as the bound?
That seems better than deriving one from the highest CSR currently
defined in mpnic_csr.h, which will keep moving as more blocks are
added.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-linux-mpnic-v1-0-236844f53072%40gmail.com
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 2/8] eth: mpnic: add register init for the device
2026-09-24 2:05 ` netdev-bot+sashiko
@ 2026-09-24 16:16 ` Daniel Zahka
2026-09-24 16:22 ` Jakub Kicinski
0 siblings, 1 reply; 22+ messages in thread
From: Daniel Zahka @ 2026-09-24 16:16 UTC (permalink / raw)
To: netdev-bot+sashiko, daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
On Wed Sep 23, 2026 at 10:05 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] pci: missing pci_resource_len() check in mpnic_probe() allows
> OOB MMIO access
> - [Medium] The 1 MiB Tx buffer is split between MAC and BMC differently
> in TXB and in TQS.
>
> --
>
> eth: mpnic: add register init for the device
>
> This commit adds one-time CSR initialization to mpnic_probe() through a
> new mpnic_dev_init(). It programs the TXB, RXB, TQS, descriptor SRAM,
> AXI and relaxed ordering settings, starts the on-chip SRAM
> initializations and polls until they complete.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_init.c b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
>> new file mode 100644
>> index 0000000000000..f8ebb19766731
>> --- /dev/null
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
>> @@ -0,0 +1,553 @@
>
> [ ... ]
>
>> +/* A total of 1 MiB worth of Tx credits is available, in units of 128 B.
>> + * The BMC gets a guaranteed share of them whether or not the host is
>> + * routing anything its way, everything else goes to MAC TC0.
>> + */
>> +#define MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL 800
>> +#define MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL \
>> + (SZ_1M / 128 - 2 * MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL)
>
> [ ... ]
>
>> +/* The 1 MiB Tx buffer is partitioned between the MAC and the BMC in units
>> + * of 1 KiB. The BMC portion is fixed at 100 KB.
>> + */
>> +#define MPNIC_TQS_GLBL_TXB_CRDT_BMC 100
>> +#define MPNIC_TQS_GLBL_TXB_CRDT_MAC (SZ_1M / SZ_1K - \
>> + MPNIC_TQS_GLBL_TXB_CRDT_BMC)
>
> [ ... ]
>
>> +static void mpnic_txb_init(struct mpnic_dev *mpd)
>> +{
>> + int i;
>> +
>> + mpnic_wr64(mpd, MPNIC_TXB_BMC, MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL);
>> +
>> + /* Zero the private credits of every traffic class, then hand the
>> + * unreserved ones to MAC TC0.
>> + */
>> + for (i = 0; i < MPNIC_TXB_P0_CNT; i++)
>> + mpnic_wr64(mpd, MPNIC_TXB_P0(i), 0);
>> + mpnic_wr64(mpd, MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_MAC_0),
>> + MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL);
>> + mpnic_wr64(mpd, MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_BMC),
>> + MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL);
>
> [ ... ]
>
>> +static void mpnic_tqs_init(struct mpnic_dev *mpd)
>> +{
>
> [ ... ]
>
>> + /* Only port 0 is used. A single traffic class is in use as well, so
>> + * give all of the Tx buffer credits to TC0.
>> + */
>> + mpnic_wr64(mpd, MPNIC_TQS_GLBL_P0_0,
>> + FIELD_PREP(MPNIC_TQS_GLBL_P0_0_TXB_MAX_CRDTS_0,
>> + MPNIC_TQS_GLBL_TXB_CRDT_MAC));
>> + mpnic_wr64(mpd, MPNIC_TQS_GLBL_P0_1, 0);
>> + mpnic_wr64(mpd, MPNIC_TQS_GLBL_BMC,
>> + FIELD_PREP(MPNIC_TQS_GLBL_BMC_TXB_MAX_CRDTS,
>> + MPNIC_TQS_GLBL_TXB_CRDT_BMC));
>
> [Severity: Medium]
> Do TXB and TQS split the 1 MiB Tx buffer the same way?
>
> On the TXB side, mpnic_txb_init() writes MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL
> (800 x 128 B = 100 KiB) to both MPNIC_TXB_BMC and
> MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_BMC). MAC TC0 then gets:
>
> SZ_1M / 128 - 2 * 800 = 6592 credits = 824 KiB
>
> On the TQS side, mpnic_tqs_init() programs
> MPNIC_TQS_GLBL_P0_0_TXB_MAX_CRDTS_0 with:
>
> SZ_1M / SZ_1K - 100 = 924 (1 KiB units) = 924 KiB
>
> That lets the scheduler use 100 KiB more for TC0 than TXB reserved as
> TC0 private credit.
>
> Suppose TXB_MAX_CRDTS limits how much data the scheduler can have
> outstanding in the TC0 TXB pool. Could a Tx burst that fills TC0 then
> overrun the pool once the Tx datapath is running?
>
> If the BMC only needs a single 100 KiB share, as the TQS comment says,
> does the "2 *" in MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL leave 100 KiB of Tx
> buffer unused?
>
> The TXB comment says "everything else goes to MAC TC0", but the code
> subtracts the BMC share twice, so the two don't match either.
>
> [ ... ]
Will fix in v2.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
>> @@ -108,6 +112,10 @@ static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> pci_set_master(pdev);
>> pci_save_state(pdev);
>>
>> + err = mpnic_dev_init(mpd);
>> + if (err)
>> + goto err_free_mpd;
>> +
>> return 0;
>>
>> err_free_mpd:
>>
>
> [Severity: High]
> mpnic_dev_init() reaches much further into BAR 0 than anything the
> driver touched before. Before this patch, the furthest access was
> MPNIC_BDQ_SPARE in mpnic_rd64(). Now the TXB, RXB, RPC, ROF, TOF and
> PEMO_WRAPPER blocks are all programmed.
>
> The highest one is MPNIC_OB_ATTR_RDE_P in mpnic_ro_init(). It is
> 0x9a001a in u32 units, so the writeq() lands at byte offset 0x2680068.
> BAR 0 therefore has to be at least ~38.5 MiB for this sequence to stay
> inside the mapping.
>
> mpnic_probe() maps BAR 0 with pcim_iomap_region(), which accepts
> whatever length the device advertised. Nothing checks
> pci_resource_len(pdev, 0) against the register file the driver is
> about to use. mpnic_wr64() and mpnic_rd64() only test that uc_addr0
> is non-NULL, then add the register index unconditionally.
>
> Suppose a device presents a BAR 0 smaller than that. It could be a
> misconfigured board, or something spoofing this ID behind
> Thunderbolt/USB4. Won't the first out-of-range writeq() in
> mpnic_dev_init() fault on an unmapped address during probe, or
> scribble over whatever happens to be mapped after the BAR?
>
> Could mpnic_probe() reject the device with -ENODEV before calling
> mpnic_dev_init() when pci_resource_len(pdev, 0) is too small? Does the
> hardware have a documented BAR 0 size that could serve as the bound?
> That seems better than deriving one from the highest CSR currently
> defined in mpnic_csr.h, which will keep moving as more blocks are
> added.
I don't believe this is necessary. It doesn't seem that common in
pci based network drivers. In our case, the bar size is static. It will
always be 64MiB for this pci device id.
pw-bot: cr
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 2/8] eth: mpnic: add register init for the device
2026-09-24 16:16 ` Daniel Zahka
@ 2026-09-24 16:22 ` Jakub Kicinski
0 siblings, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2026-09-24 16:22 UTC (permalink / raw)
To: Daniel Zahka
Cc: netdev-bot+sashiko, alexanderduyck, kernel-team, andrew+netdev,
davem, edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
On Thu, 24 Sep 2026 12:16:33 -0400 Daniel Zahka wrote:
> I don't believe this is necessary. It doesn't seem that common in
> pci based network drivers. In our case, the bar size is static. It will
> always be 64MiB for this pci device id.
Yes, we trust PCIe devices to not be malicious and/or mangled.
I need to update the prompts :S
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next 3/8] eth: mpnic: allocate MSI-X vectors
2026-09-23 1:43 [PATCH net-next 0/8] eth: mpnic: initial support for Meta Platforms NIC Daniel Zahka
2026-09-23 1:43 ` [PATCH net-next 1/8] eth: mpnic: add scaffolding " Daniel Zahka
2026-09-23 1:43 ` [PATCH net-next 2/8] eth: mpnic: add register init for the device Daniel Zahka
@ 2026-09-23 1:43 ` Daniel Zahka
2026-09-23 1:43 ` [PATCH net-next 4/8] eth: mpnic: implement Tx queue allocation and cleanup Daniel Zahka
` (4 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Daniel Zahka @ 2026-09-23 1:43 UTC (permalink / raw)
To: Alexander Duyck, Jakub Kicinski, kernel-team, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Dimitri Daskalakis, Mohsin Bashir
Cc: linux-kernel, netdev, bpf
Request one vector for FW generated interrupts and then one vector per
CPU to use for the completion queues. Nothing requests a handler yet,
that comes with the NAPI vectors.
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
drivers/net/ethernet/meta/mpnic/Makefile | 1 +
drivers/net/ethernet/meta/mpnic/mpnic.h | 15 +++++++
drivers/net/ethernet/meta/mpnic/mpnic_irq.c | 64 +++++++++++++++++++++++++++++
drivers/net/ethernet/meta/mpnic/mpnic_pci.c | 9 +++-
4 files changed, 88 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/meta/mpnic/Makefile b/drivers/net/ethernet/meta/mpnic/Makefile
index 9dcaa1c72605..3aa7c6a5ab5e 100644
--- a/drivers/net/ethernet/meta/mpnic/Makefile
+++ b/drivers/net/ethernet/meta/mpnic/Makefile
@@ -9,5 +9,6 @@ obj-$(CONFIG_MPNIC) += mpnic.o
mpnic-y := \
mpnic_init.o \
+ mpnic_irq.o \
mpnic_pci.o \
# End of mpnic-y
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic.h b/drivers/net/ethernet/meta/mpnic/mpnic.h
index 67d48b7ce315..628fd48c997d 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic.h
@@ -4,6 +4,7 @@
#ifndef _MPNIC_H_
#define _MPNIC_H_
+#include <linux/interrupt.h>
#include <linux/io-64-nonatomic-lo-hi.h>
#include <linux/types.h>
@@ -14,11 +15,19 @@
#define MPNIC_MAX_TXQS 1024u
#define MPNIC_MAX_RXQS 1024u
+/* misc IRQ entries are allocated before the completion queue IRQs */
+enum {
+ MPNIC_FW_MSIX_ENTRY,
+ MPNIC_NON_NAPI_VECTORS
+};
+
struct mpnic_dev {
struct device *dev;
u32 __iomem *uc_addr0;
+ u16 num_irqs;
+
u32 mps;
u32 readrq;
u8 relaxed_ord;
@@ -28,6 +37,12 @@ u64 mpnic_rd64(struct mpnic_dev *mpd, u32 reg);
int mpnic_dev_init(struct mpnic_dev *mpd);
+int mpnic_request_irq(struct mpnic_dev *mpd, int nr, irq_handler_t handler,
+ unsigned long flags, const char *name, void *data);
+void mpnic_free_irq(struct mpnic_dev *mpd, int nr, void *data);
+void mpnic_free_irqs(struct mpnic_dev *mpd);
+int mpnic_alloc_irqs(struct mpnic_dev *mpd);
+
static inline void mpnic_wr64(struct mpnic_dev *mpd, u32 reg, u64 val)
{
u32 __iomem *csr = READ_ONCE(mpd->uc_addr0);
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_irq.c b/drivers/net/ethernet/meta/mpnic/mpnic_irq.c
new file mode 100644
index 000000000000..bcc33655cbbe
--- /dev/null
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_irq.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <linux/cpumask.h>
+#include <linux/interrupt.h>
+#include <linux/minmax.h>
+#include <linux/pci.h>
+
+#include "mpnic.h"
+
+int mpnic_request_irq(struct mpnic_dev *mpd, int nr, irq_handler_t handler,
+ unsigned long flags, const char *name, void *data)
+{
+ struct pci_dev *pdev = to_pci_dev(mpd->dev);
+ int irq = pci_irq_vector(pdev, nr);
+
+ if (irq < 0)
+ return irq;
+
+ return request_irq(irq, handler, flags, name, data);
+}
+
+void mpnic_free_irq(struct mpnic_dev *mpd, int nr, void *data)
+{
+ struct pci_dev *pdev = to_pci_dev(mpd->dev);
+ int irq = pci_irq_vector(pdev, nr);
+
+ if (irq < 0)
+ return;
+
+ free_irq(irq, data);
+}
+
+void mpnic_free_irqs(struct mpnic_dev *mpd)
+{
+ struct pci_dev *pdev = to_pci_dev(mpd->dev);
+
+ mpd->num_irqs = 0;
+ pci_free_irq_vectors(pdev);
+}
+
+int mpnic_alloc_irqs(struct mpnic_dev *mpd)
+{
+ unsigned int wanted_irqs = MPNIC_NON_NAPI_VECTORS;
+ struct pci_dev *pdev = to_pci_dev(mpd->dev);
+ int num_irqs;
+
+ wanted_irqs += min_t(unsigned int, num_online_cpus(), MPNIC_MAX_RXQS);
+ num_irqs = pci_alloc_irq_vectors(pdev, MPNIC_NON_NAPI_VECTORS + 1,
+ wanted_irqs, PCI_IRQ_MSIX);
+ if (num_irqs < 0) {
+ dev_err(mpd->dev, "Failed to allocate MSI-X entries: %d\n",
+ num_irqs);
+ return num_irqs;
+ }
+
+ if (num_irqs < wanted_irqs)
+ dev_warn(mpd->dev, "Allocated %d IRQs, expected %u\n",
+ num_irqs, wanted_irqs);
+
+ mpd->num_irqs = num_irqs;
+
+ return 0;
+}
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
index 68a64377e6c5..127b71b44b0f 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
@@ -112,12 +112,18 @@ static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
pci_set_master(pdev);
pci_save_state(pdev);
- err = mpnic_dev_init(mpd);
+ err = mpnic_alloc_irqs(mpd);
if (err)
goto err_free_mpd;
+ err = mpnic_dev_init(mpd);
+ if (err)
+ goto err_free_irqs;
+
return 0;
+err_free_irqs:
+ mpnic_free_irqs(mpd);
err_free_mpd:
kfree(mpd);
@@ -132,6 +138,7 @@ static void mpnic_remove(struct pci_dev *pdev)
{
struct mpnic_dev *mpd = pci_get_drvdata(pdev);
+ mpnic_free_irqs(mpd);
kfree(mpd);
}
--
2.52.0
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH net-next 4/8] eth: mpnic: implement Tx queue allocation and cleanup
2026-09-23 1:43 [PATCH net-next 0/8] eth: mpnic: initial support for Meta Platforms NIC Daniel Zahka
` (2 preceding siblings ...)
2026-09-23 1:43 ` [PATCH net-next 3/8] eth: mpnic: allocate MSI-X vectors Daniel Zahka
@ 2026-09-23 1:43 ` Daniel Zahka
2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-23 1:43 ` [PATCH net-next 5/8] eth: mpnic: start and stop the Tx HW queues Daniel Zahka
` (3 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Daniel Zahka @ 2026-09-23 1:43 UTC (permalink / raw)
To: Alexander Duyck, Jakub Kicinski, kernel-team, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Dimitri Daskalakis, Mohsin Bashir
Cc: linux-kernel, netdev, bpf
Queues come in triads of two work queues and one completion queue. The
Tx triad only uses the first work queue for now, the second one will be
used for the XDP ring later.
Add the ring structures, the per NAPI vector allocation of triads,
descriptor memory and interrupts, and the completion processing. The
completion queue does not report one entry per packet, it reports a
work queue head, so a single completion can release many packets.
Nothing enables the queues in hardware or submits anything to them yet,
that comes with the next two changes.
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
drivers/net/ethernet/meta/mpnic/Makefile | 1 +
drivers/net/ethernet/meta/mpnic/mpnic_csr.h | 23 ++
drivers/net/ethernet/meta/mpnic/mpnic_netdev.h | 26 ++
drivers/net/ethernet/meta/mpnic/mpnic_txrx.c | 496 +++++++++++++++++++++++++
drivers/net/ethernet/meta/mpnic/mpnic_txrx.h | 80 ++++
5 files changed, 626 insertions(+)
diff --git a/drivers/net/ethernet/meta/mpnic/Makefile b/drivers/net/ethernet/meta/mpnic/Makefile
index 3aa7c6a5ab5e..67d617f49a8a 100644
--- a/drivers/net/ethernet/meta/mpnic/Makefile
+++ b/drivers/net/ethernet/meta/mpnic/Makefile
@@ -11,4 +11,5 @@ mpnic-y := \
mpnic_init.o \
mpnic_irq.o \
mpnic_pci.o \
+ mpnic_txrx.o \
# End of mpnic-y
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index 42eee18de474..49595c495141 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -16,6 +16,13 @@
#define MPNIC_TWD_L2_HLEN DESC_GENMASK(5, 0)
#define MPNIC_TWD_FLAG_REQ_COMPLETION DESC_BIT(37)
+#define MPNIC_TWD_ADDR DESC_GENMASK(45, 0)
+#define MPNIC_TWD_LEN DESC_GENMASK(63, 48)
+
+/* Tx Completion Descriptor Format */
+#define MPNIC_TCD_TYPE0_HEAD0 DESC_GENMASK(15, 0)
+#define MPNIC_TCD_DONE DESC_BIT(63)
+
/* Common fields for all DESC_CFG CSRs */
#define MPNIC_DESC_CFG_NUM_DESCS CSR_GENMASK(2, 0)
#define MPNIC_DESC_CFG_START_ADDR CSR_GENMASK(19, 8)
@@ -29,6 +36,22 @@
* Name Index Address
*****************************************************************************/
+/* NIC_CORE_TDF */
+#define MPNIC_TWQ_TAIL(i, j) (0x4 + 1024 * (i) + 2 * (j))
+ /* 0x10 */
+
+/* NIC_CORE_TCM */
+#define MPNIC_TCQ_HEAD(i) (0x8e + 1024 * (i)) /* 0x238 */
+
+/* NIC_CORE_TIM */
+#define MPNIC_TIM_CTL1(i) (0xc0 + 1024 * (i)) /* 0x300 */
+#define MPNIC_TIM_CTL1_UPD_IGN_LONG_EVENT_CNT CSR_BIT(48)
+#define MPNIC_TIM_CTL1_UPD_IGN_LONG_TIME_CNT CSR_BIT(49)
+#define MPNIC_TIM_CTL1_UPD_IGN_SHORT_TIME_CNT CSR_BIT(50)
+#define MPNIC_TIM_CTL1_MASK CSR_BIT(51)
+#define MPNIC_TIM_CTL1_MASK_EN CSR_BIT(52)
+#define MPNIC_TIM_CTL1_TRIGGER CSR_BIT(53)
+
/* NIC_CORE_RBP_HP_GLBL */
#define MPNIC_BDQ_GLBL_CTL0 0x420080 /* 0x1080200 */
#define MPNIC_BDQ_GLBL_CTL0_MAX_REQ_SIZE CSR_GENMASK(26, 18)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
new file mode 100644
index 000000000000..f98adf209b45
--- /dev/null
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#ifndef _MPNIC_NETDEV_H_
+#define _MPNIC_NETDEV_H_
+
+#include <linux/types.h>
+
+#include "mpnic.h"
+#include "mpnic_txrx.h"
+
+struct mpnic_net {
+ struct mpnic_ring *tx[MPNIC_MAX_TXQS];
+
+ struct mpnic_napi_vector *napi[MPNIC_MAX_NAPI_VECTORS];
+
+ struct net_device *netdev;
+ struct mpnic_dev *mpd;
+
+ u32 txq_size;
+
+ u16 num_napi;
+ u16 num_tx_queues;
+};
+
+#endif /* _MPNIC_NETDEV_H_ */
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
new file mode 100644
index 000000000000..fe360a26a27b
--- /dev/null
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -0,0 +1,496 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <linux/bitfield.h>
+#include <linux/dma-mapping.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+
+#include "mpnic.h"
+#include "mpnic_netdev.h"
+#include "mpnic_txrx.h"
+
+struct mpnic_xmit_cb {
+ u32 bytecount;
+ u8 desc_count;
+};
+
+#define MPNIC_XMIT_CB(__skb) ((struct mpnic_xmit_cb *)((__skb)->cb))
+
+/* Leave the interrupt moderation counters alone when arming or masking */
+#define MPNIC_TIM_PARAM_CFG_PRESERVE_MASK \
+ (MPNIC_TIM_CTL1_UPD_IGN_LONG_EVENT_CNT | \
+ MPNIC_TIM_CTL1_UPD_IGN_LONG_TIME_CNT | \
+ MPNIC_TIM_CTL1_UPD_IGN_SHORT_TIME_CNT)
+
+static void mpnic_nv_irq_disable(struct mpnic_napi_vector *nv)
+{
+ mpnic_wr64(nv->mpd, MPNIC_TIM_CTL1(nv->qt[0].cmpl.q_idx),
+ MPNIC_TIM_PARAM_CFG_PRESERVE_MASK |
+ MPNIC_TIM_CTL1_MASK_EN | MPNIC_TIM_CTL1_MASK);
+}
+
+static void mpnic_nv_irq_rearm(struct mpnic_napi_vector *nv)
+{
+ /* Rearming a single queue on a given IRQ rearms all the other
+ * queues mapped to the same IRQ.
+ */
+ mpnic_wr64(nv->mpd, MPNIC_TIM_CTL1(nv->qt[0].cmpl.q_idx),
+ MPNIC_TIM_PARAM_CFG_PRESERVE_MASK | MPNIC_TIM_CTL1_MASK_EN);
+}
+
+static unsigned int mpnic_desc_unused(struct mpnic_ring *ring)
+{
+ return (ring->head - ring->tail - 1) & ring->size_mask;
+}
+
+static struct netdev_queue *mpnic_txring_txq(const struct net_device *dev,
+ const struct mpnic_ring *ring)
+{
+ return netdev_get_tx_queue(dev, ring->q_idx);
+}
+
+static void mpnic_unmap_single_twd(struct device *dev, __le64 *twd)
+{
+ u64 raw_twd = le64_to_cpu(*twd);
+
+ dma_unmap_single(dev, FIELD_GET(MPNIC_TWD_ADDR, raw_twd),
+ FIELD_GET(MPNIC_TWD_LEN, raw_twd), DMA_TO_DEVICE);
+}
+
+static void mpnic_unmap_page_twd(struct device *dev, __le64 *twd)
+{
+ u64 raw_twd = le64_to_cpu(*twd);
+
+ dma_unmap_page(dev, FIELD_GET(MPNIC_TWD_ADDR, raw_twd),
+ FIELD_GET(MPNIC_TWD_LEN, raw_twd), DMA_TO_DEVICE);
+}
+
+static void mpnic_clean_twq0(struct mpnic_napi_vector *nv, int napi_budget,
+ struct mpnic_ring *ring, bool discard,
+ unsigned int hw_head)
+{
+ u64 total_bytes = 0, total_packets = 0;
+ unsigned int head = ring->head;
+ struct netdev_queue *txq;
+ unsigned int clean_desc;
+
+ clean_desc = (hw_head - head) & ring->size_mask;
+
+ while (clean_desc) {
+ struct sk_buff *skb = ring->tx_buf[head];
+ unsigned int desc_cnt;
+
+ desc_cnt = MPNIC_XMIT_CB(skb)->desc_count;
+ if (desc_cnt > clean_desc)
+ break;
+
+ ring->tx_buf[head] = NULL;
+
+ clean_desc -= desc_cnt;
+
+ /* Step over the metadata descriptor */
+ head++;
+ head &= ring->size_mask;
+ desc_cnt--;
+
+ mpnic_unmap_single_twd(nv->dev, &ring->desc[head]);
+ head++;
+ head &= ring->size_mask;
+ desc_cnt--;
+
+ while (desc_cnt--) {
+ mpnic_unmap_page_twd(nv->dev, &ring->desc[head]);
+ head++;
+ head &= ring->size_mask;
+ }
+
+ total_bytes += MPNIC_XMIT_CB(skb)->bytecount;
+ total_packets++;
+
+ napi_consume_skb(skb, napi_budget);
+ }
+
+ if (!total_bytes)
+ return;
+
+ ring->head = head;
+
+ if (discard)
+ return;
+
+ txq = mpnic_txring_txq(nv->napi.dev, ring);
+ netif_txq_completed_wake(txq, total_packets, total_bytes,
+ mpnic_desc_unused(ring),
+ MPNIC_TX_DESC_WAKEUP);
+}
+
+static void mpnic_commit_cq_head(struct mpnic_ring *cmpl)
+{
+ u32 head = cmpl->head;
+
+ /* The tail shadows the last value written to the doorbell, so a
+ * completion queue which has not moved costs no MMIO write.
+ */
+ if (cmpl->tail != head) {
+ cmpl->tail = head;
+ writeq(head & cmpl->size_mask, cmpl->doorbell);
+ }
+}
+
+static void mpnic_clean_tcq(struct mpnic_napi_vector *nv,
+ struct mpnic_q_triad *qt, int napi_budget)
+{
+ struct mpnic_ring *cmpl = &qt->cmpl;
+ __le64 *raw_tcd, done;
+ u32 head = cmpl->head;
+ s32 head0 = -1;
+
+ done = (head & (cmpl->size_mask + 1)) ? 0 : cpu_to_le64(MPNIC_TCD_DONE);
+ raw_tcd = &cmpl->desc[head & cmpl->size_mask];
+
+ /* Walk the completion queue collecting the heads reported by NIC.
+ * Only the first work queue is enabled and no packet asks for a
+ * timestamp, so every completion is a plain head update and the
+ * descriptor type does not have to be decoded.
+ */
+ while ((*raw_tcd & cpu_to_le64(MPNIC_TCD_DONE)) == done) {
+ u64 tcd;
+
+ dma_rmb();
+
+ tcd = le64_to_cpu(*raw_tcd);
+ head0 = FIELD_GET(MPNIC_TCD_TYPE0_HEAD0, tcd);
+
+ raw_tcd++;
+ head++;
+
+ if (unlikely(!(head & cmpl->size_mask))) {
+ done ^= cpu_to_le64(MPNIC_TCD_DONE);
+ raw_tcd = &cmpl->desc[0];
+ }
+ }
+
+ cmpl->head = head;
+
+ if (head0 >= 0)
+ mpnic_clean_twq0(nv, napi_budget, &qt->sub0, false, head0);
+}
+
+static int mpnic_poll(struct napi_struct *napi, int budget)
+{
+ struct mpnic_napi_vector *nv = container_of(napi,
+ struct mpnic_napi_vector,
+ napi);
+ int i;
+
+ for (i = 0; i < nv->txt_count; i++)
+ mpnic_clean_tcq(nv, &nv->qt[i], budget);
+
+ for (i = 0; i < nv->txt_count; i++)
+ mpnic_commit_cq_head(&nv->qt[i].cmpl);
+
+ if (likely(napi_complete_done(napi, 0)))
+ mpnic_nv_irq_rearm(nv);
+
+ return 0;
+}
+
+static irqreturn_t mpnic_msix_clean_rings(int __always_unused irq, void *data)
+{
+ struct mpnic_napi_vector *nv = data;
+
+ napi_schedule_irqoff(&nv->napi);
+
+ return IRQ_HANDLED;
+}
+
+static void mpnic_free_napi_vector(struct mpnic_net *mpn,
+ struct mpnic_napi_vector *nv)
+{
+ int i;
+
+ for (i = 0; i < nv->txt_count; i++)
+ mpn->tx[nv->qt[i].sub0.q_idx] = NULL;
+
+ mpnic_free_irq(nv->mpd, nv->v_idx, nv);
+ netif_napi_del_locked(&nv->napi);
+ mpn->napi[nv->v_idx - MPNIC_NON_NAPI_VECTORS] = NULL;
+ kfree(nv);
+}
+
+void mpnic_free_napi_vectors(struct mpnic_net *mpn)
+{
+ int i;
+
+ for (i = 0; i < mpn->num_napi; i++)
+ if (mpn->napi[i])
+ mpnic_free_napi_vector(mpn, mpn->napi[i]);
+}
+
+static void mpnic_ring_init(struct mpnic_ring *ring, u32 __iomem *doorbell,
+ int q_idx)
+{
+ ring->doorbell = doorbell;
+ ring->q_idx = q_idx;
+}
+
+static int mpnic_alloc_napi_vector(struct mpnic_dev *mpd,
+ struct mpnic_net *mpn, unsigned int idx)
+{
+ u32 __iomem *uc_addr = READ_ONCE(mpd->uc_addr0);
+ struct mpnic_napi_vector *nv;
+ int err;
+
+ /* Doorbells are plain pointers into the register window, they have
+ * no way of noticing that it went away.
+ */
+ if (!uc_addr)
+ return -EIO;
+
+ nv = kzalloc_flex(*nv, qt, 1);
+ if (!nv)
+ return -ENOMEM;
+
+ nv->txt_count = 1;
+ nv->mpd = mpd;
+ nv->dev = mpd->dev;
+ nv->v_idx = idx + MPNIC_NON_NAPI_VECTORS;
+
+ mpn->napi[idx] = nv;
+ netif_napi_add_config_locked(mpn->netdev, &nv->napi, mpnic_poll, idx);
+ netif_napi_set_irq_locked(&nv->napi,
+ pci_irq_vector(to_pci_dev(mpd->dev),
+ nv->v_idx));
+
+ snprintf(nv->name, sizeof(nv->name), "%s-TxRx-%u",
+ mpn->netdev->name, idx);
+
+ err = mpnic_request_irq(mpd, nv->v_idx, mpnic_msix_clean_rings, 0,
+ nv->name, nv);
+ if (err)
+ goto err_napi_del;
+
+ mpnic_ring_init(&nv->qt[0].sub0, &uc_addr[MPNIC_TWQ_TAIL(idx, 0)], idx);
+ mpnic_ring_init(&nv->qt[0].cmpl, &uc_addr[MPNIC_TCQ_HEAD(idx)], idx);
+ mpn->tx[idx] = &nv->qt[0].sub0;
+
+ return 0;
+
+err_napi_del:
+ netif_napi_del_locked(&nv->napi);
+ mpn->napi[idx] = NULL;
+ kfree(nv);
+ return err;
+}
+
+int mpnic_alloc_napi_vectors(struct mpnic_net *mpn)
+{
+ unsigned int i;
+ int err;
+
+ for (i = 0; i < mpn->num_napi; i++) {
+ err = mpnic_alloc_napi_vector(mpn->mpd, mpn, i);
+ if (err)
+ goto err_free_vectors;
+ }
+
+ return 0;
+
+err_free_vectors:
+ mpnic_free_napi_vectors(mpn);
+
+ return err;
+}
+
+static void mpnic_free_ring_resources(struct device *dev,
+ struct mpnic_ring *ring)
+{
+ kvfree(ring->tx_buf);
+ ring->tx_buf = NULL;
+
+ /* If size is not set there are no descriptors present */
+ if (!ring->size)
+ return;
+
+ dma_free_coherent(dev, ring->size, ring->desc, ring->dma);
+ ring->size_mask = 0;
+ ring->size = 0;
+}
+
+static int mpnic_alloc_ring_desc(struct mpnic_net *mpn,
+ struct mpnic_ring *ring, u32 count)
+{
+ struct device *dev = mpn->netdev->dev.parent;
+ size_t size;
+
+ size = ALIGN(array_size(sizeof(*ring->desc), count), 4096);
+
+ ring->desc = dma_alloc_coherent(dev, size, &ring->dma,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!ring->desc)
+ return -ENOMEM;
+
+ ring->size_mask = count - 1;
+ ring->size = size;
+
+ return 0;
+}
+
+static void mpnic_free_tx_qt_resources(struct mpnic_net *mpn,
+ struct mpnic_q_triad *qt)
+{
+ struct device *dev = mpn->netdev->dev.parent;
+
+ mpnic_free_ring_resources(dev, &qt->cmpl);
+ mpnic_free_ring_resources(dev, &qt->sub0);
+}
+
+static int mpnic_alloc_tx_qt_resources(struct mpnic_net *mpn,
+ struct mpnic_q_triad *qt)
+{
+ int err;
+
+ err = mpnic_alloc_ring_desc(mpn, &qt->sub0, mpn->txq_size);
+ if (err)
+ return err;
+
+ qt->sub0.tx_buf = kvzalloc_objs(*qt->sub0.tx_buf, mpn->txq_size,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!qt->sub0.tx_buf) {
+ err = -ENOMEM;
+ goto err_free_qt;
+ }
+
+ err = mpnic_alloc_ring_desc(mpn, &qt->cmpl, mpn->txq_size);
+ if (err)
+ goto err_free_qt;
+
+ return 0;
+
+err_free_qt:
+ mpnic_free_tx_qt_resources(mpn, qt);
+ return err;
+}
+
+static void mpnic_free_nv_resources(struct mpnic_net *mpn,
+ struct mpnic_napi_vector *nv)
+{
+ int i;
+
+ for (i = 0; i < nv->txt_count; i++)
+ mpnic_free_tx_qt_resources(mpn, &nv->qt[i]);
+}
+
+static int mpnic_alloc_nv_resources(struct mpnic_net *mpn,
+ struct mpnic_napi_vector *nv)
+{
+ int i, err;
+
+ for (i = 0; i < nv->txt_count; i++) {
+ err = mpnic_alloc_tx_qt_resources(mpn, &nv->qt[i]);
+ if (err)
+ goto err_free_qt_resources;
+ }
+
+ return 0;
+
+err_free_qt_resources:
+ while (i--)
+ mpnic_free_tx_qt_resources(mpn, &nv->qt[i]);
+ return err;
+}
+
+void mpnic_free_resources(struct mpnic_net *mpn)
+{
+ int i;
+
+ for (i = 0; i < mpn->num_napi; i++)
+ mpnic_free_nv_resources(mpn, mpn->napi[i]);
+}
+
+int mpnic_alloc_resources(struct mpnic_net *mpn)
+{
+ int i, err;
+
+ for (i = 0; i < mpn->num_napi; i++) {
+ err = mpnic_alloc_nv_resources(mpn, mpn->napi[i]);
+ if (err)
+ goto err_free_resources;
+ }
+
+ return 0;
+
+err_free_resources:
+ while (i--)
+ mpnic_free_nv_resources(mpn, mpn->napi[i]);
+
+ return err;
+}
+
+int mpnic_set_netif_queues(struct mpnic_net *mpn)
+{
+ int i, j, err;
+
+ err = netif_set_real_num_tx_queues(mpn->netdev, mpn->num_tx_queues);
+ if (err)
+ return err;
+
+ for (i = 0; i < mpn->num_napi; i++) {
+ struct mpnic_napi_vector *nv = mpn->napi[i];
+
+ for (j = 0; j < nv->txt_count; j++)
+ netif_queue_set_napi(mpn->netdev, nv->qt[j].sub0.q_idx,
+ NETDEV_QUEUE_TYPE_TX, &nv->napi);
+ }
+
+ return 0;
+}
+
+void mpnic_reset_netif_queues(struct mpnic_net *mpn)
+{
+ int i, j;
+
+ for (i = 0; i < mpn->num_napi; i++) {
+ struct mpnic_napi_vector *nv = mpn->napi[i];
+
+ for (j = 0; j < nv->txt_count; j++)
+ netif_queue_set_napi(mpn->netdev, nv->qt[j].sub0.q_idx,
+ NETDEV_QUEUE_TYPE_TX, NULL);
+ }
+}
+
+void mpnic_napi_disable(struct mpnic_net *mpn)
+{
+ int i;
+
+ for (i = 0; i < mpn->num_napi; i++) {
+ napi_disable_locked(&mpn->napi[i]->napi);
+
+ mpnic_nv_irq_disable(mpn->napi[i]);
+ }
+}
+
+void mpnic_napi_enable(struct mpnic_net *mpn)
+{
+ int i;
+
+ for (i = 0; i < mpn->num_napi; i++)
+ napi_enable_locked(&mpn->napi[i]->napi);
+
+ /* Force the first interrupt on each vector to guarantee that any
+ * completions posted during bringup are processed. Use the TRIGGER
+ * pulse rather than the level triggered global interrupt set, which
+ * can jam the mask/pending state machine if it collides with a
+ * concurrent unmask.
+ */
+ for (i = 0; i < mpn->num_napi; i++) {
+ struct mpnic_napi_vector *nv = mpn->napi[i];
+
+ mpnic_wr64(mpn->mpd, MPNIC_TIM_CTL1(nv->qt[0].cmpl.q_idx),
+ MPNIC_TIM_PARAM_CFG_PRESERVE_MASK |
+ MPNIC_TIM_CTL1_MASK_EN | MPNIC_TIM_CTL1_TRIGGER);
+ }
+
+ mpnic_wrfl(mpn->mpd);
+}
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
new file mode 100644
index 000000000000..0d4667666b5b
--- /dev/null
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
@@ -0,0 +1,80 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#ifndef _MPNIC_TXRX_H_
+#define _MPNIC_TXRX_H_
+
+#include <linux/if_ether.h>
+#include <linux/netdevice.h>
+#include <linux/skbuff.h>
+#include <linux/types.h>
+#include <net/netdev_queues.h>
+
+#include "mpnic.h"
+
+struct mpnic_net;
+
+/* Space we have to have available in a work queue to take a packet:
+ * 1 descriptor per page
+ * + 1 descriptor for the skb head
+ * + 1 descriptor for the metadata
+ * + 7 descriptors to keep the tail out of the head's cacheline
+ * If we cannot guarantee that we return NETDEV_TX_BUSY.
+ */
+#define MPNIC_MAX_SKB_DESC (MAX_SKB_FRAGS + 9)
+#define MPNIC_TX_DESC_WAKEUP (MPNIC_MAX_SKB_DESC * 2)
+
+#define MPNIC_MAX_NAPI_VECTORS 1024u
+
+struct mpnic_ring {
+ void **tx_buf; /* Packets outstanding in a TWQ */
+
+ u32 __iomem *doorbell; /* Pointer to CSR space for ring */
+ __le64 *desc; /* Descriptor ring memory */
+ u16 size_mask; /* Size of ring in descriptors - 1 */
+ u16 q_idx; /* Hardware queue index */
+
+ u32 head, tail; /* Head/Tail of ring */
+
+ /* TWQ only, index of the metadata descriptor of the last packet
+ * placed in the ring without ringing the doorbell, -1 if the
+ * doorbell is in sync with the tail.
+ */
+ s32 deferred_meta;
+
+ /* Slow path fields follow */
+ dma_addr_t dma; /* Phys addr of descriptor memory */
+ size_t size; /* Size of descriptor ring in memory */
+};
+
+/* The device pairs two work queues with one completion queue. On the Tx
+ * side only the first work queue is used for now, the second one becomes
+ * the XDP ring.
+ */
+struct mpnic_q_triad {
+ struct mpnic_ring sub0, sub1, cmpl;
+};
+
+struct mpnic_napi_vector {
+ struct napi_struct napi;
+ struct device *dev; /* Device for DMA unmapping */
+ struct mpnic_dev *mpd;
+
+ u16 v_idx;
+ u16 txt_count;
+
+ char name[IFNAMSIZ + 11];
+
+ struct mpnic_q_triad qt[];
+};
+
+int mpnic_alloc_napi_vectors(struct mpnic_net *mpn);
+void mpnic_free_napi_vectors(struct mpnic_net *mpn);
+int mpnic_alloc_resources(struct mpnic_net *mpn);
+void mpnic_free_resources(struct mpnic_net *mpn);
+int mpnic_set_netif_queues(struct mpnic_net *mpn);
+void mpnic_reset_netif_queues(struct mpnic_net *mpn);
+void mpnic_napi_enable(struct mpnic_net *mpn);
+void mpnic_napi_disable(struct mpnic_net *mpn);
+
+#endif /* _MPNIC_TXRX_H_ */
--
2.52.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 4/8] eth: mpnic: implement Tx queue allocation and cleanup
2026-09-23 1:43 ` [PATCH net-next 4/8] eth: mpnic: implement Tx queue allocation and cleanup Daniel Zahka
@ 2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-24 16:39 ` Daniel Zahka
0 siblings, 1 reply; 22+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 2:05 UTC (permalink / raw)
To: daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] Cached doorbell pointers let queue-head MMIO writes reach BAR0
after the driver has marked the device as gone.
- [Low] Concern (baseline: rejected): mpnic_alloc_ring_desc() stores
`count - 1` into the u16 ring->size_mask (line 334) without checking…
- [Low] Concern (baseline: rejected): mpnic_clean_tcq() takes HEAD0 from
the device-written completion descriptor (line 163) and passes it…
--
eth: mpnic: implement Tx queue allocation and cleanup
This adds the Tx ring structures and allocates queue triads per NAPI
vector, along with their descriptor memory and interrupts. It also adds
Tx completion processing, where one completion reports a work queue head
and can release many packets. Nothing enables the queues in hardware yet.
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> new file mode 100644
> index 0000000000000..fe360a26a27be
> --- /dev/null
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> @@ -0,0 +1,496 @@
[ ... ]
> +static void mpnic_commit_cq_head(struct mpnic_ring *cmpl)
> +{
> + u32 head = cmpl->head;
> +
> + /* The tail shadows the last value written to the doorbell, so a
> + * completion queue which has not moved costs no MMIO write.
> + */
> + if (cmpl->tail != head) {
> + cmpl->tail = head;
> + writeq(head & cmpl->size_mask, cmpl->doorbell);
> + }
> +}
[Severity: Low]
Can this doorbell write still reach BAR0 after the driver has marked the
device as gone?
mpnic_alloc_napi_vector() reads mpd->uc_addr0 once and stores raw BAR0
addresses in the rings:
mpnic_ring_init(&nv->qt[0].sub0, &uc_addr[MPNIC_TWQ_TAIL(idx, 0)], idx);
mpnic_ring_init(&nv->qt[0].cmpl, &uc_addr[MPNIC_TCQ_HEAD(idx)], idx);
When mpnic_rd64() sees all-ones reads, mpnic_mmio_err() in mpnic_pci.c
clears only the accessor pointer:
WRITE_ONCE(mpd->uc_addr0, NULL);
BAR0 stays mapped through pcim_iomap_region(). mpnic_wr64() checks for
the NULL pointer, but mpnic_commit_cq_head() always writes through
cmpl->doorbell.
So suppose one mpnic_poll() run still has unprocessed completions that
were posted before the failure. The TCQ head doorbell write goes out,
while the mpnic_nv_irq_rearm() write through mpnic_wr64() is dropped.
The scaffolding commit says that once the device is gone, no further
accesses reach the bus. Should that also cover the doorbells?
Later in the series, mpnic_tx_doorbell() and __mpnic_bdq_commit_tail()
use the same cached-pointer pattern. Would a mpnic_present() check help
in those places and here?
The comment in mpnic_alloc_napi_vector() already says the doorbells
cannot notice the window going away, and fbnic does the same thing. The
practical effect is probably limited to posted writes to a dead link.
> +static void mpnic_clean_tcq(struct mpnic_napi_vector *nv,
> + struct mpnic_q_triad *qt, int napi_budget)
> +{
[ ... ]
> + while ((*raw_tcd & cpu_to_le64(MPNIC_TCD_DONE)) == done) {
> + u64 tcd;
> +
> + dma_rmb();
> +
> + tcd = le64_to_cpu(*raw_tcd);
> + head0 = FIELD_GET(MPNIC_TCD_TYPE0_HEAD0, tcd);
[ ... ]
> + cmpl->head = head;
> +
> + if (head0 >= 0)
> + mpnic_clean_twq0(nv, napi_budget, &qt->sub0, false, head0);
> +}
[Severity: Low]
This isn't a bug with a conforming device, but head0 comes straight from
the completion descriptor the device writes. It goes to
mpnic_clean_twq0() without any check against sub0.tail.
If the device ever reported a head past the posted tail,
mpnic_clean_twq0() would reach an empty slot and dereference NULL:
struct sk_buff *skb = ring->tx_buf[head];
...
desc_cnt = MPNIC_XMIT_CB(skb)->desc_count;
The completion walk loop is also not bounded by the NAPI budget. In
practice the device cannot post past the CQ head, and that head is only
committed after the loop.
Stale entries from an earlier bringup do not look reachable. The
descriptor memory is freshly zeroed by dma_alloc_coherent(), the head
and tail are reset to 0 later in the series, and at head 0 the expected
DONE bit is 1.
Would a cheap sanity check on head0, or a NULL check on skb, be worth
adding as hardening?
[ ... ]
> +static int mpnic_alloc_ring_desc(struct mpnic_net *mpn,
> + struct mpnic_ring *ring, u32 count)
> +{
[ ... ]
> + ring->size_mask = count - 1;
> + ring->size = size;
[Severity: Low]
This isn't a bug with the current callers, but count is a u32 while
ring->size_mask is a u16 in struct mpnic_ring. Nothing here checks that
count is a non-zero power of two no larger than 65536.
This cannot be reached today. The only caller passes mpn->txq_size,
which is only ever set to MPNIC_TXQ_SIZE_DEFAULT (1024), and there is
no set_ringparam path.
If a runtime ring size setter is added later, a count that is zero, not
a power of two, or above 65536 would break the masked indexes in
mpnic_clean_twq0() and mpnic_clean_tcq().
Would a WARN_ON_ONCE() or an explicit check here be worthwhile?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-linux-mpnic-v1-0-236844f53072%40gmail.com
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 4/8] eth: mpnic: implement Tx queue allocation and cleanup
2026-09-24 2:05 ` netdev-bot+sashiko
@ 2026-09-24 16:39 ` Daniel Zahka
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Zahka @ 2026-09-24 16:39 UTC (permalink / raw)
To: netdev-bot+sashiko, daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
On Wed Sep 23, 2026 at 10:05 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] Cached doorbell pointers let queue-head MMIO writes reach BAR0
> after the driver has marked the device as gone.
> - [Low] Concern (baseline: rejected): mpnic_alloc_ring_desc() stores
> `count - 1` into the u16 ring->size_mask (line 334) without checking…
> - [Low] Concern (baseline: rejected): mpnic_clean_tcq() takes HEAD0 from
> the device-written completion descriptor (line 163) and passes it…
>
> --
>
> eth: mpnic: implement Tx queue allocation and cleanup
>
> This adds the Tx ring structures and allocates queue triads per NAPI
> vector, along with their descriptor memory and interrupts. It also adds
> Tx completion processing, where one completion reports a work queue head
> and can release many packets. Nothing enables the queues in hardware yet.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> new file mode 100644
>> index 0000000000000..fe360a26a27be
>> --- /dev/null
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> @@ -0,0 +1,496 @@
>
> [ ... ]
>
>> +static void mpnic_commit_cq_head(struct mpnic_ring *cmpl)
>> +{
>> + u32 head = cmpl->head;
>> +
>> + /* The tail shadows the last value written to the doorbell, so a
>> + * completion queue which has not moved costs no MMIO write.
>> + */
>> + if (cmpl->tail != head) {
>> + cmpl->tail = head;
>> + writeq(head & cmpl->size_mask, cmpl->doorbell);
>> + }
>> +}
>
> [Severity: Low]
> Can this doorbell write still reach BAR0 after the driver has marked the
> device as gone?
>
> mpnic_alloc_napi_vector() reads mpd->uc_addr0 once and stores raw BAR0
> addresses in the rings:
>
> mpnic_ring_init(&nv->qt[0].sub0, &uc_addr[MPNIC_TWQ_TAIL(idx, 0)], idx);
> mpnic_ring_init(&nv->qt[0].cmpl, &uc_addr[MPNIC_TCQ_HEAD(idx)], idx);
>
> When mpnic_rd64() sees all-ones reads, mpnic_mmio_err() in mpnic_pci.c
> clears only the accessor pointer:
>
> WRITE_ONCE(mpd->uc_addr0, NULL);
>
> BAR0 stays mapped through pcim_iomap_region(). mpnic_wr64() checks for
> the NULL pointer, but mpnic_commit_cq_head() always writes through
> cmpl->doorbell.
>
> So suppose one mpnic_poll() run still has unprocessed completions that
> were posted before the failure. The TCQ head doorbell write goes out,
> while the mpnic_nv_irq_rearm() write through mpnic_wr64() is dropped.
>
> The scaffolding commit says that once the device is gone, no further
> accesses reach the bus. Should that also cover the doorbells?
>
> Later in the series, mpnic_tx_doorbell() and __mpnic_bdq_commit_tail()
> use the same cached-pointer pattern. Would a mpnic_present() check help
> in those places and here?
>
> The comment in mpnic_alloc_napi_vector() already says the doorbells
> cannot notice the window going away, and fbnic does the same thing. The
> practical effect is probably limited to posted writes to a dead link.
>
The observation is correct, but this is by design. Won't fix. If we lose
the device, we can let these writes get dropped by the bus instead of
trying to stop them.
>> +static void mpnic_clean_tcq(struct mpnic_napi_vector *nv,
>> + struct mpnic_q_triad *qt, int napi_budget)
>> +{
>
> [ ... ]
>
>> + while ((*raw_tcd & cpu_to_le64(MPNIC_TCD_DONE)) == done) {
>> + u64 tcd;
>> +
>> + dma_rmb();
>> +
>> + tcd = le64_to_cpu(*raw_tcd);
>> + head0 = FIELD_GET(MPNIC_TCD_TYPE0_HEAD0, tcd);
>
> [ ... ]
>
>> + cmpl->head = head;
>> +
>> + if (head0 >= 0)
>> + mpnic_clean_twq0(nv, napi_budget, &qt->sub0, false, head0);
>> +}
>
> [Severity: Low]
> This isn't a bug with a conforming device, but head0 comes straight from
> the completion descriptor the device writes. It goes to
> mpnic_clean_twq0() without any check against sub0.tail.
>
In this case, and in general, we do not perform this type of hardening
against completion fields written by the device. We trust its outputs
for things like offsets into buffers, rings, etc.
> If the device ever reported a head past the posted tail,
> mpnic_clean_twq0() would reach an empty slot and dereference NULL:
>
> struct sk_buff *skb = ring->tx_buf[head];
> ...
> desc_cnt = MPNIC_XMIT_CB(skb)->desc_count;
>
> The completion walk loop is also not bounded by the NAPI budget. In
> practice the device cannot post past the CQ head, and that head is only
> committed after the loop.
>
> Stale entries from an earlier bringup do not look reachable. The
> descriptor memory is freshly zeroed by dma_alloc_coherent(), the head
> and tail are reset to 0 later in the series, and at head 0 the expected
> DONE bit is 1.
>
> Would a cheap sanity check on head0, or a NULL check on skb, be worth
> adding as hardening?
>
> [ ... ]
>
>> +static int mpnic_alloc_ring_desc(struct mpnic_net *mpn,
>> + struct mpnic_ring *ring, u32 count)
>> +{
>
> [ ... ]
>
>> + ring->size_mask = count - 1;
>> + ring->size = size;
>
> [Severity: Low]
> This isn't a bug with the current callers, but count is a u32 while
> ring->size_mask is a u16 in struct mpnic_ring. Nothing here checks that
> count is a non-zero power of two no larger than 65536.
>
Ack, but won't fix. When ethool is implemented, caller will have the
responsibility of ensuring these.
> This cannot be reached today. The only caller passes mpn->txq_size,
> which is only ever set to MPNIC_TXQ_SIZE_DEFAULT (1024), and there is
> no set_ringparam path.
>
> If a runtime ring size setter is added later, a count that is zero, not
> a power of two, or above 65536 would break the masked indexes in
> mpnic_clean_twq0() and mpnic_clean_tcq().
>
> Would a WARN_ON_ONCE() or an explicit check here be worthwhile?
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next 5/8] eth: mpnic: start and stop the Tx HW queues
2026-09-23 1:43 [PATCH net-next 0/8] eth: mpnic: initial support for Meta Platforms NIC Daniel Zahka
` (3 preceding siblings ...)
2026-09-23 1:43 ` [PATCH net-next 4/8] eth: mpnic: implement Tx queue allocation and cleanup Daniel Zahka
@ 2026-09-23 1:43 ` Daniel Zahka
2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-23 1:43 ` [PATCH net-next 6/8] eth: mpnic: add a netdevice and basic Tx handling Daniel Zahka
` (2 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Daniel Zahka @ 2026-09-23 1:43 UTC (permalink / raw)
To: Alexander Duyck, Jakub Kicinski, kernel-team, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Dimitri Daskalakis, Mohsin Bashir
Cc: linux-kernel, netdev, bpf
Point the hardware at the descriptor rings and provide functions for
turning the Tx queues on and off.
Teardown has to wait for the DMA engines to go idle before the ring
memory can be freed: disabling a queue only stops the device picking up
new work, it does not tell us that the work already fetched has been
retired. Each block exposes an idle bitmap per queue, and every one of
the four blocks a packet passes through on its way out has to report
idle before the descriptors are safe to free.
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
drivers/net/ethernet/meta/mpnic/mpnic_csr.h | 28 +++++
drivers/net/ethernet/meta/mpnic/mpnic_txrx.c | 175 +++++++++++++++++++++++++++
drivers/net/ethernet/meta/mpnic/mpnic_txrx.h | 4 +
3 files changed, 207 insertions(+)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index 49595c495141..a2133024b2cf 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -37,11 +37,26 @@
*****************************************************************************/
/* NIC_CORE_TDF */
+#define MPNIC_TWQ_CTL(i, j) (0x0 + 1024 * (i) + 2 * (j))
+ /* 0x0 */
+#define MPNIC_TWQ_CTL_RESET CSR_BIT(0)
+#define MPNIC_TWQ_CTL_ENABLE CSR_BIT(1)
#define MPNIC_TWQ_TAIL(i, j) (0x4 + 1024 * (i) + 2 * (j))
/* 0x10 */
+#define MPNIC_TWQ_SIZE(i, j) (0x10 + 1024 * (i) + 2 * (j))
+ /* 0x40 */
+#define MPNIC_TWQ_SIZE_SIZE CSR_GENMASK(3, 0)
+#define MPNIC_TWQ_BASE_ADDR(i, j) (0x1c + 1024 * (i) + 2 * (j))
+ /* 0x70 */
/* NIC_CORE_TCM */
+#define MPNIC_TCQ_CTL(i) (0x80 + 1024 * (i)) /* 0x200 */
+#define MPNIC_TCQ_CTL_RESET CSR_BIT(0)
+#define MPNIC_TCQ_CTL_ENABLE CSR_BIT(1)
+#define MPNIC_TCQ_BASE_ADDR(i) (0x86 + 1024 * (i)) /* 0x218 */
#define MPNIC_TCQ_HEAD(i) (0x8e + 1024 * (i)) /* 0x238 */
+#define MPNIC_TCQ_SIZE(i) (0x94 + 1024 * (i)) /* 0x250 */
+#define MPNIC_TCQ_SIZE_SIZE CSR_GENMASK(4, 0)
/* NIC_CORE_TIM */
#define MPNIC_TIM_CTL1(i) (0xc0 + 1024 * (i)) /* 0x300 */
@@ -51,6 +66,11 @@
#define MPNIC_TIM_CTL1_MASK CSR_BIT(51)
#define MPNIC_TIM_CTL1_MASK_EN CSR_BIT(52)
#define MPNIC_TIM_CTL1_TRIGGER CSR_BIT(53)
+#define MPNIC_TIM_INTR_MASK(i) (0xc8 + 1024 * (i)) /* 0x320 */
+#define MPNIC_TIM_INTR_MASK_MASK CSR_BIT(0)
+
+/* NIC_CORE_TIM_PRV */
+#define MPNIC_TIM_CTL(i) (0x100100 + 1024 * (i)) /* 0x400400 */
/* NIC_CORE_RBP_HP_GLBL */
#define MPNIC_BDQ_GLBL_CTL0 0x420080 /* 0x1080200 */
@@ -84,6 +104,8 @@
#define MPNIC_RNI_RCM_CTL 0x427004 /* 0x109c010 */
/* NIC_CORE_TDF_GLBL */
+#define MPNIC_TWQ_IDLE(i) (0x428042 + 2 * (i)) /* 0x10a0108 */
+#define MPNIC_TWQ_IDLE_CNT 32
#define MPNIC_TWQ_DEF_PRI_TWD 0x428082 /* 0x10a0208 */
#define MPNIC_TDF_MEM_INIT_REQ 0x42813a /* 0x10a04e8 */
#define MPNIC_TDF_MEM_INIT_DONE 0x42813c /* 0x10a04f0 */
@@ -101,6 +123,8 @@
#define MPNIC_TQS_SLOWDOWN_CTL_ENABLE CSR_BIT(6)
#define MPNIC_TQS_MTU_CTL0 0x42a030 /* 0x10a80c0 */
#define MPNIC_TQS_MTU_CTL1 0x42a032 /* 0x10a80c8 */
+#define MPNIC_TQS_IDLE(i) (0x42a040 + 2 * (i)) /* 0x10a8100 */
+#define MPNIC_TQS_IDLE_CNT 32
#define MPNIC_TQS_SET_P0_MAP0(i) (0x42a082 + 2 * (i)) /* 0x10a8208 */
#define MPNIC_TQS_SET_P0_MAP1(i) (0x42a092 + 2 * (i)) /* 0x10a8248 */
#define MPNIC_TQS_GLBL_SHAPING 0x42a108 /* 0x10a8420 */
@@ -136,10 +160,14 @@
#define MPNIC_TQS_PORT_CTL(i) (0x42a1e4 + 2 * (i)) /* 0x10a8790 */
/* NIC_CORE_TDE_GLBL */
+#define MPNIC_TDE_IDLE(i) (0x42b000 + 2 * (i)) /* 0x10ac000 */
+#define MPNIC_TDE_IDLE_CNT 32
#define MPNIC_TDE_MEM_INIT_REQ 0x42b1ee /* 0x10ac7b8 */
#define MPNIC_TDE_MEM_INIT_DONE 0x42b1f0 /* 0x10ac7c0 */
/* NIC_CORE_TCM_GLBL */
+#define MPNIC_TCQ_IDLE(i) (0x42c09e + 2 * (i)) /* 0x10b0278 */
+#define MPNIC_TCQ_IDLE_CNT 16
#define MPNIC_TCM_MEM_INIT_REQ 0x42c0be /* 0x10b02f8 */
#define MPNIC_TCM_MEM_INIT_DONE 0x42c0c0 /* 0x10b0300 */
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
index fe360a26a27b..cb4d1427434c 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -3,6 +3,7 @@
#include <linux/bitfield.h>
#include <linux/dma-mapping.h>
+#include <linux/iopoll.h>
#include <linux/pci.h>
#include <linux/slab.h>
@@ -460,6 +461,180 @@ void mpnic_reset_netif_queues(struct mpnic_net *mpn)
}
}
+static void mpnic_enable_twq(struct mpnic_dev *mpd, struct mpnic_ring *twq)
+{
+ u32 log_size = fls(twq->size_mask);
+ u32 i = twq->q_idx;
+
+ /* Reset head/tail */
+ mpnic_wr64(mpd, MPNIC_TWQ_CTL(i, 0), MPNIC_TWQ_CTL_RESET);
+ twq->tail = 0;
+ twq->head = 0;
+ twq->deferred_meta = -1;
+
+ /* Store descriptor ring address and size */
+ mpnic_wr64(mpd, MPNIC_TWQ_BASE_ADDR(i, 0), twq->dma);
+ mpnic_wr64(mpd, MPNIC_TWQ_SIZE(i, 0), log_size & MPNIC_TWQ_SIZE_SIZE);
+
+ mpnic_wr64(mpd, MPNIC_TWQ_CTL(i, 0), MPNIC_TWQ_CTL_ENABLE);
+}
+
+static void mpnic_enable_tcq(struct mpnic_dev *mpd,
+ struct mpnic_napi_vector *nv,
+ struct mpnic_ring *tcq)
+{
+ u32 log_size = fls(tcq->size_mask);
+ u32 i = tcq->q_idx;
+
+ /* Reset head/tail */
+ mpnic_wr64(mpd, MPNIC_TCQ_CTL(i), MPNIC_TCQ_CTL_RESET);
+ tcq->tail = 0;
+ tcq->head = 0;
+
+ /* Store descriptor ring address and size */
+ mpnic_wr64(mpd, MPNIC_TCQ_BASE_ADDR(i), tcq->dma);
+ mpnic_wr64(mpd, MPNIC_TCQ_SIZE(i), log_size & MPNIC_TCQ_SIZE_SIZE);
+
+ /* Store interrupt information for the completion queue */
+ mpnic_wr64(mpd, MPNIC_TIM_CTL(i), nv->v_idx);
+ mpnic_wr64(mpd, MPNIC_TIM_INTR_MASK(i), 0);
+
+ mpnic_wr64(mpd, MPNIC_TCQ_CTL(i), MPNIC_TCQ_CTL_ENABLE);
+}
+
+void mpnic_enable(struct mpnic_net *mpn)
+{
+ struct mpnic_dev *mpd = mpn->mpd;
+ int i, j;
+
+ for (i = 0; i < mpn->num_napi; i++) {
+ struct mpnic_napi_vector *nv = mpn->napi[i];
+
+ for (j = 0; j < nv->txt_count; j++) {
+ mpnic_enable_twq(mpd, &nv->qt[j].sub0);
+ mpnic_enable_tcq(mpd, nv, &nv->qt[j].cmpl);
+ }
+ }
+
+ mpnic_wrfl(mpd);
+}
+
+static void mpnic_disable_twq(struct mpnic_dev *mpd, struct mpnic_ring *txr)
+{
+ u64 twq_ctl = mpnic_rd64(mpd, MPNIC_TWQ_CTL(txr->q_idx, 0));
+
+ twq_ctl &= ~MPNIC_TWQ_CTL_ENABLE;
+ mpnic_wr64(mpd, MPNIC_TWQ_CTL(txr->q_idx, 0), twq_ctl);
+}
+
+static void mpnic_disable_tcq(struct mpnic_dev *mpd, struct mpnic_ring *txr)
+{
+ mpnic_wr64(mpd, MPNIC_TCQ_CTL(txr->q_idx), 0);
+ mpnic_wr64(mpd, MPNIC_TIM_INTR_MASK(txr->q_idx),
+ MPNIC_TIM_INTR_MASK_MASK);
+}
+
+void mpnic_disable(struct mpnic_net *mpn)
+{
+ struct mpnic_dev *mpd = mpn->mpd;
+ int i, j;
+
+ for (i = 0; i < mpn->num_napi; i++) {
+ struct mpnic_napi_vector *nv = mpn->napi[i];
+
+ for (j = 0; j < nv->txt_count; j++) {
+ mpnic_disable_twq(mpd, &nv->qt[j].sub0);
+ mpnic_disable_tcq(mpd, &nv->qt[j].cmpl);
+ }
+ }
+
+ mpnic_wrfl(mpd);
+}
+
+struct mpnic_idle_regs {
+ u32 reg_base;
+ u8 reg_cnt;
+ char name[4];
+};
+
+static u32 mpnic_non_idle_queues(struct mpnic_dev *mpd,
+ const struct mpnic_idle_regs *regs,
+ unsigned int nregs)
+{
+ u32 non_idle_bitmap = 0;
+ unsigned int i, j;
+
+ for (i = 0; i < nregs; i++) {
+ for (j = 0; j < regs[i].reg_cnt; j++) {
+ if (mpnic_rd64(mpd, regs[i].reg_base + 2 * j) !=
+ ~0ULL) {
+ non_idle_bitmap |= BIT(i);
+ break;
+ }
+ }
+ }
+
+ return non_idle_bitmap;
+}
+
+static void mpnic_idle_dump(struct mpnic_dev *mpd,
+ const struct mpnic_idle_regs *regs,
+ unsigned int nregs, u32 non_idle_bitmap, int err)
+{
+ unsigned int i, j;
+
+ dev_err(mpd->dev, "error waiting for queues idle %d\n", err);
+ for (i = 0; i < nregs; i++) {
+ if (!(non_idle_bitmap & BIT(i)))
+ continue;
+
+ dev_err(mpd->dev, "%s block not idle:\n", regs[i].name);
+ for (j = 0; j < regs[i].reg_cnt; j++)
+ dev_err(mpd->dev, " 0x%04x: %016llx\n",
+ regs[i].reg_base + 2 * j,
+ mpnic_rd64(mpd, regs[i].reg_base + 2 * j));
+ }
+}
+
+void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd)
+{
+ static const struct mpnic_idle_regs queues[] = {
+ { MPNIC_TWQ_IDLE(0), MPNIC_TWQ_IDLE_CNT, "TWQ" },
+ { MPNIC_TQS_IDLE(0), MPNIC_TQS_IDLE_CNT, "TQS" },
+ { MPNIC_TDE_IDLE(0), MPNIC_TDE_IDLE_CNT, "TDE" },
+ { MPNIC_TCQ_IDLE(0), MPNIC_TCQ_IDLE_CNT, "TCQ" },
+ };
+ u32 non_idle_bitmap;
+ int err;
+
+ err = read_poll_timeout(mpnic_non_idle_queues, non_idle_bitmap,
+ !non_idle_bitmap, 20, 500000, false, mpd,
+ queues, ARRAY_SIZE(queues));
+ if (err)
+ mpnic_idle_dump(mpd, queues, ARRAY_SIZE(queues),
+ non_idle_bitmap, err);
+}
+
+void mpnic_flush(struct mpnic_net *mpn)
+{
+ int i, j;
+
+ for (i = 0; i < mpn->num_napi; i++) {
+ struct mpnic_napi_vector *nv = mpn->napi[i];
+
+ for (j = 0; j < nv->txt_count; j++) {
+ struct mpnic_q_triad *qt = &nv->qt[j];
+ struct netdev_queue *txq;
+
+ /* Clean the work queue of unprocessed work */
+ mpnic_clean_twq0(nv, 0, &qt->sub0, true, qt->sub0.tail);
+
+ txq = netdev_get_tx_queue(mpn->netdev, qt->sub0.q_idx);
+ netdev_tx_reset_queue(txq);
+ }
+ }
+}
+
void mpnic_napi_disable(struct mpnic_net *mpn)
{
int i;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
index 0d4667666b5b..68a773133fa2 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
@@ -76,5 +76,9 @@ int mpnic_set_netif_queues(struct mpnic_net *mpn);
void mpnic_reset_netif_queues(struct mpnic_net *mpn);
void mpnic_napi_enable(struct mpnic_net *mpn);
void mpnic_napi_disable(struct mpnic_net *mpn);
+void mpnic_enable(struct mpnic_net *mpn);
+void mpnic_disable(struct mpnic_net *mpn);
+void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd);
+void mpnic_flush(struct mpnic_net *mpn);
#endif /* _MPNIC_TXRX_H_ */
--
2.52.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 5/8] eth: mpnic: start and stop the Tx HW queues
2026-09-23 1:43 ` [PATCH net-next 5/8] eth: mpnic: start and stop the Tx HW queues Daniel Zahka
@ 2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-24 17:49 ` Daniel Zahka
0 siblings, 1 reply; 22+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 2:05 UTC (permalink / raw)
To: daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] mpnic_wait_all_queues_idle()
(drivers/net/ethernet/meta/mpnic/mpnic_txrx.c:599) returns void.
- [Medium] mpnic_non_idle_queues() (mpnic_txrx.c:569-570) counts a block
as idle when every idle register reads ~0ULL.
--
eth: mpnic: start and stop the Tx HW queues
This commit programs the Tx work and completion queue base addresses and
sizes into the hardware, and adds helpers to enable and disable the Tx
queues. It also adds mpnic_wait_all_queues_idle(), which polls the idle
bitmaps of the TWQ, TQS, TDE and TCQ blocks so that teardown can wait for
the DMA engines before the rings are freed.
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index fe360a26a27be..cb4d1427434c9 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
[ ... ]
> +static u32 mpnic_non_idle_queues(struct mpnic_dev *mpd,
> + const struct mpnic_idle_regs *regs,
> + unsigned int nregs)
> +{
> + u32 non_idle_bitmap = 0;
> + unsigned int i, j;
> +
> + for (i = 0; i < nregs; i++) {
> + for (j = 0; j < regs[i].reg_cnt; j++) {
> + if (mpnic_rd64(mpd, regs[i].reg_base + 2 * j) !=
> + ~0ULL) {
> + non_idle_bitmap |= BIT(i);
> + break;
> + }
[Severity: Medium]
Could this report a device that can't be reached as idle? The idle test
here is ~0ULL, and mpnic_rd64() returns that same value once CSR access
has been disabled:
drivers/net/ethernet/meta/mpnic/mpnic_pci.c:mpnic_rd64() {
u32 __iomem *csr = READ_ONCE(mpd->uc_addr0);
...
if (!csr)
return ~0ULL;
...
mpnic_mmio_err(mpd, reg);
return ~0ULL;
}
mpnic_wr64() also drops writes silently in that state:
if (csr)
writeq(val, csr + reg);
After mpnic_mmio_err() clears uc_addr0, the writes in mpnic_disable()
that clear MPNIC_TWQ_CTL_ENABLE and MPNIC_TCQ_CTL never reach the device.
On the first poll, mpnic_wait_all_queues_idle() then sees all four blocks
as idle and returns without a timeout. The caller can't tell a confirmed
idle state apart from an unreadable device, so it goes on to free the
rings.
mpnic_mmio_err() logs "awaiting reset", which suggests the device may be
recoverable rather than gone. Suppose the device can still bus-master
after two all-ones reads in a row, for example after a transient MMIO
failure. Could its still-enabled queues then DMA into ring memory that
has already been freed?
[ ... ]
> +void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd)
> +{
> + static const struct mpnic_idle_regs queues[] = {
> + { MPNIC_TWQ_IDLE(0), MPNIC_TWQ_IDLE_CNT, "TWQ" },
> + { MPNIC_TQS_IDLE(0), MPNIC_TQS_IDLE_CNT, "TQS" },
> + { MPNIC_TDE_IDLE(0), MPNIC_TDE_IDLE_CNT, "TDE" },
> + { MPNIC_TCQ_IDLE(0), MPNIC_TCQ_IDLE_CNT, "TCQ" },
> + };
> + u32 non_idle_bitmap;
> + int err;
> +
> + err = read_poll_timeout(mpnic_non_idle_queues, non_idle_bitmap,
> + !non_idle_bitmap, 20, 500000, false, mpd,
> + queues, ARRAY_SIZE(queues));
> + if (err)
> + mpnic_idle_dump(mpd, queues, ARRAY_SIZE(queues),
> + non_idle_bitmap, err);
> +}
[Severity: Medium]
What happens to the rings when this poll times out? The -ETIMEDOUT in
err is only passed to mpnic_idle_dump() for logging and then dropped.
Because the function returns void, no caller can act on the timeout.
The commit message says:
every one of the four blocks a packet passes through on its way out
has to report idle before the descriptors are safe to free.
Later in the series, though, mpnic_stop() runs the teardown with no
condition between the steps:
mpnic_disable(mpn);
mpnic_wait_all_queues_idle(mpn->mpd);
mpnic_flush(mpn);
mpnic_reset_netif_queues(mpn);
mpnic_free_resources(mpn);
After a timeout, mpnic_flush() calls mpnic_clean_twq0() with
qt->sub0.tail. That unmaps every pending TWD and frees the in-flight
skbs. mpnic_free_ring_resources() then calls dma_free_coherent() on the
TWQ and TCQ descriptor rings, whose addresses mpnic_enable_twq() and
mpnic_enable_tcq() handed to the hardware.
If a block is still busy after 500 ms, can the device still read the
freed skb data and descriptors? Can it write completions into the freed
TCQ memory?
The final version of the series (the "eth: mpnic: add basic Rx handling"
commit) still returns void and still frees unconditionally. It also adds
the Rx HPQ/PPQ/RCQ blocks to this wait, which would expose the Rx rings
and page pool pages the same way.
Should mpnic_wait_all_queues_idle() return the error? Teardown could then
avoid freeing memory the device may still be using.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-linux-mpnic-v1-0-236844f53072%40gmail.com
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 5/8] eth: mpnic: start and stop the Tx HW queues
2026-09-24 2:05 ` netdev-bot+sashiko
@ 2026-09-24 17:49 ` Daniel Zahka
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Zahka @ 2026-09-24 17:49 UTC (permalink / raw)
To: netdev-bot+sashiko, daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
On Wed Sep 23, 2026 at 10:05 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 2 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] mpnic_wait_all_queues_idle()
> (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c:599) returns void.
> - [Medium] mpnic_non_idle_queues() (mpnic_txrx.c:569-570) counts a block
> as idle when every idle register reads ~0ULL.
>
> --
>
> eth: mpnic: start and stop the Tx HW queues
>
> This commit programs the Tx work and completion queue base addresses and
> sizes into the hardware, and adds helpers to enable and disable the Tx
> queues. It also adds mpnic_wait_all_queues_idle(), which polls the idle
> bitmaps of the TWQ, TQS, TDE and TCQ blocks so that teardown can wait for
> the DMA engines before the rings are freed.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index fe360a26a27be..cb4d1427434c9 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>
> [ ... ]
>
>> +static u32 mpnic_non_idle_queues(struct mpnic_dev *mpd,
>> + const struct mpnic_idle_regs *regs,
>> + unsigned int nregs)
>> +{
>> + u32 non_idle_bitmap = 0;
>> + unsigned int i, j;
>> +
>> + for (i = 0; i < nregs; i++) {
>> + for (j = 0; j < regs[i].reg_cnt; j++) {
>> + if (mpnic_rd64(mpd, regs[i].reg_base + 2 * j) !=
>> + ~0ULL) {
>> + non_idle_bitmap |= BIT(i);
>> + break;
>> + }
>
> [Severity: Medium]
> Could this report a device that can't be reached as idle? The idle test
> here is ~0ULL, and mpnic_rd64() returns that same value once CSR access
> has been disabled:
>
> drivers/net/ethernet/meta/mpnic/mpnic_pci.c:mpnic_rd64() {
> u32 __iomem *csr = READ_ONCE(mpd->uc_addr0);
> ...
> if (!csr)
> return ~0ULL;
> ...
> mpnic_mmio_err(mpd, reg);
>
> return ~0ULL;
> }
>
> mpnic_wr64() also drops writes silently in that state:
>
> if (csr)
> writeq(val, csr + reg);
>
> After mpnic_mmio_err() clears uc_addr0, the writes in mpnic_disable()
> that clear MPNIC_TWQ_CTL_ENABLE and MPNIC_TCQ_CTL never reach the device.
> On the first poll, mpnic_wait_all_queues_idle() then sees all four blocks
> as idle and returns without a timeout. The caller can't tell a confirmed
> idle state apart from an unreadable device, so it goes on to free the
> rings.
>
> mpnic_mmio_err() logs "awaiting reset", which suggests the device may be
> recoverable rather than gone. Suppose the device can still bus-master
> after two all-ones reads in a row, for example after a transient MMIO
> failure. Could its still-enabled queues then DMA into ring memory that
> has already been freed?
>
Won't fix. Our assumption is that losing the device is not a transient
failure mode, and will require a PCI reset (not implemented yet), or re
probing the device.
>
>> +void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd)
>> +{
>> + static const struct mpnic_idle_regs queues[] = {
>> + { MPNIC_TWQ_IDLE(0), MPNIC_TWQ_IDLE_CNT, "TWQ" },
>> + { MPNIC_TQS_IDLE(0), MPNIC_TQS_IDLE_CNT, "TQS" },
>> + { MPNIC_TDE_IDLE(0), MPNIC_TDE_IDLE_CNT, "TDE" },
>> + { MPNIC_TCQ_IDLE(0), MPNIC_TCQ_IDLE_CNT, "TCQ" },
>> + };
>> + u32 non_idle_bitmap;
>> + int err;
>> +
>> + err = read_poll_timeout(mpnic_non_idle_queues, non_idle_bitmap,
>> + !non_idle_bitmap, 20, 500000, false, mpd,
>> + queues, ARRAY_SIZE(queues));
>> + if (err)
>> + mpnic_idle_dump(mpd, queues, ARRAY_SIZE(queues),
>> + non_idle_bitmap, err);
>> +}
>
> [Severity: Medium]
> What happens to the rings when this poll times out? The -ETIMEDOUT in
> err is only passed to mpnic_idle_dump() for logging and then dropped.
> Because the function returns void, no caller can act on the timeout.
>
> The commit message says:
>
> every one of the four blocks a packet passes through on its way out
> has to report idle before the descriptors are safe to free.
>
> Later in the series, though, mpnic_stop() runs the teardown with no
> condition between the steps:
>
> mpnic_disable(mpn);
> mpnic_wait_all_queues_idle(mpn->mpd);
> mpnic_flush(mpn);
>
> mpnic_reset_netif_queues(mpn);
> mpnic_free_resources(mpn);
>
> After a timeout, mpnic_flush() calls mpnic_clean_twq0() with
> qt->sub0.tail. That unmaps every pending TWD and frees the in-flight
> skbs. mpnic_free_ring_resources() then calls dma_free_coherent() on the
> TWQ and TCQ descriptor rings, whose addresses mpnic_enable_twq() and
> mpnic_enable_tcq() handed to the hardware.
>
> If a block is still busy after 500 ms, can the device still read the
> freed skb data and descriptors? Can it write completions into the freed
> TCQ memory?
>
> The final version of the series (the "eth: mpnic: add basic Rx handling"
> commit) still returns void and still frees unconditionally. It also adds
> the Rx HPQ/PPQ/RCQ blocks to this wait, which would expose the Rx rings
> and page pool pages the same way.
>
> Should mpnic_wait_all_queues_idle() return the error? Teardown could then
> avoid freeing memory the device may still be using.
Won't fix. If the device is not reporting idle, I think something like
this could be possible with respect to stray DMAs, but I'm not sure what
we can do other than leaking the memory. Also, the device would be in
somewhat of an unknown state, so all bets are off anyway.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next 6/8] eth: mpnic: add a netdevice and basic Tx handling
2026-09-23 1:43 [PATCH net-next 0/8] eth: mpnic: initial support for Meta Platforms NIC Daniel Zahka
` (4 preceding siblings ...)
2026-09-23 1:43 ` [PATCH net-next 5/8] eth: mpnic: start and stop the Tx HW queues Daniel Zahka
@ 2026-09-23 1:43 ` Daniel Zahka
2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-23 1:43 ` [PATCH net-next 7/8] eth: mpnic: implement Rx queue allocation and cleanup Daniel Zahka
2026-09-23 1:43 ` [PATCH net-next 8/8] eth: mpnic: add basic Rx handling Daniel Zahka
7 siblings, 1 reply; 22+ messages in thread
From: Daniel Zahka @ 2026-09-23 1:43 UTC (permalink / raw)
To: Alexander Duyck, Jakub Kicinski, kernel-team, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Dimitri Daskalakis, Mohsin Bashir
Cc: linux-kernel, netdev, bpf
Register the network interface and let it transmit skbs.
The typical TWQ descriptor sequence for an skb is: 1 primary metadata,
followed by 0 or more non-last address + length (A/L) descriptors,
terminated by an always present last A/L descriptor.
The device does not write a Tx completion unless it is requested in the
primary metadata desc. The xmit path sets this bit when the stack does
not report netdev_xmit_more(). This can create a problem if the last
packet in a burst is dropped by the driver. To deal with that case, the
driver latches the last metadata for which a completion was not
requested, and uses this to request a completion and flush queued
packets when one is dropped.
There is no link state yet, so carrier follows the administrative
state.
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
drivers/net/ethernet/meta/mpnic/Makefile | 1 +
drivers/net/ethernet/meta/mpnic/mpnic.h | 2 +
drivers/net/ethernet/meta/mpnic/mpnic_csr.h | 7 +
drivers/net/ethernet/meta/mpnic/mpnic_netdev.c | 176 +++++++++++++++++++++++++
drivers/net/ethernet/meta/mpnic/mpnic_netdev.h | 4 +
drivers/net/ethernet/meta/mpnic/mpnic_pci.c | 25 ++++
drivers/net/ethernet/meta/mpnic/mpnic_txrx.c | 157 ++++++++++++++++++++++
drivers/net/ethernet/meta/mpnic/mpnic_txrx.h | 5 +
8 files changed, 377 insertions(+)
diff --git a/drivers/net/ethernet/meta/mpnic/Makefile b/drivers/net/ethernet/meta/mpnic/Makefile
index 67d617f49a8a..d5bdbd5bd1c7 100644
--- a/drivers/net/ethernet/meta/mpnic/Makefile
+++ b/drivers/net/ethernet/meta/mpnic/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_MPNIC) += mpnic.o
mpnic-y := \
mpnic_init.o \
mpnic_irq.o \
+ mpnic_netdev.o \
mpnic_pci.o \
mpnic_txrx.o \
# End of mpnic-y
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic.h b/drivers/net/ethernet/meta/mpnic/mpnic.h
index 628fd48c997d..78359ab6abb1 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic.h
@@ -23,11 +23,13 @@ enum {
struct mpnic_dev {
struct device *dev;
+ struct net_device *netdev;
u32 __iomem *uc_addr0;
u16 num_irqs;
+ u64 dsn;
u32 mps;
u32 readrq;
u8 relaxed_ord;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index a2133024b2cf..f7c50bc70d8a 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -15,6 +15,13 @@
/* Transmit Work Descriptor Format */
#define MPNIC_TWD_L2_HLEN DESC_GENMASK(5, 0)
#define MPNIC_TWD_FLAG_REQ_COMPLETION DESC_BIT(37)
+#define MPNIC_TWD_FLAG_DEST_MAC DESC_BIT(43)
+#define MPNIC_TWD_TYPE DESC_GENMASK(47, 46)
+enum {
+ MPNIC_TWD_TYPE_META = 0,
+ MPNIC_TWD_TYPE_AL = 2,
+ MPNIC_TWD_TYPE_LAST_AL = 3,
+};
#define MPNIC_TWD_ADDR DESC_GENMASK(45, 0)
#define MPNIC_TWD_LEN DESC_GENMASK(63, 48)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
new file mode 100644
index 000000000000..62dc9018f2f8
--- /dev/null
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <linux/etherdevice.h>
+#include <linux/ipv6.h>
+#include <linux/netdevice.h>
+#include <linux/pci.h>
+#include <linux/types.h>
+
+#include "mpnic.h"
+#include "mpnic_netdev.h"
+#include "mpnic_txrx.h"
+
+static int mpnic_open(struct net_device *netdev)
+{
+ struct mpnic_net *mpn = netdev_priv(netdev);
+ int err;
+
+ err = mpnic_alloc_napi_vectors(mpn);
+ if (err)
+ return err;
+
+ err = mpnic_alloc_resources(mpn);
+ if (err)
+ goto err_free_napi_vectors;
+
+ err = mpnic_set_netif_queues(mpn);
+ if (err)
+ goto err_free_resources;
+
+ mpnic_enable(mpn);
+ mpnic_napi_enable(mpn);
+
+ netif_tx_wake_all_queues(netdev);
+ netif_carrier_on(netdev);
+
+ return 0;
+
+err_free_resources:
+ mpnic_free_resources(mpn);
+err_free_napi_vectors:
+ mpnic_free_napi_vectors(mpn);
+ return err;
+}
+
+static int mpnic_stop(struct net_device *netdev)
+{
+ struct mpnic_net *mpn = netdev_priv(netdev);
+
+ netif_carrier_off(netdev);
+
+ mpnic_napi_disable(mpn);
+ netif_tx_disable(netdev);
+
+ mpnic_disable(mpn);
+ mpnic_wait_all_queues_idle(mpn->mpd);
+ mpnic_flush(mpn);
+
+ mpnic_reset_netif_queues(mpn);
+ mpnic_free_resources(mpn);
+ mpnic_free_napi_vectors(mpn);
+
+ return 0;
+}
+
+static const struct net_device_ops mpnic_netdev_ops = {
+ .ndo_open = mpnic_open,
+ .ndo_stop = mpnic_stop,
+ .ndo_validate_addr = eth_validate_addr,
+ .ndo_start_xmit = mpnic_xmit_frame,
+};
+
+/**
+ * mpnic_netdev_free - Free the netdev associated with mpnic
+ * @mpd: Driver specific structure to free netdev from
+ **/
+void mpnic_netdev_free(struct mpnic_dev *mpd)
+{
+ free_netdev(mpd->netdev);
+ mpd->netdev = NULL;
+}
+
+/**
+ * mpnic_netdev_alloc - Allocate a netdev and associate it with mpnic
+ * @mpd: Driver specific structure to associate the netdev with
+ *
+ * Return: NULL on failure.
+ **/
+struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
+{
+ struct net_device *netdev;
+ struct mpnic_net *mpn;
+ unsigned int queues;
+
+ netdev = alloc_etherdev_mq(sizeof(*mpn), MPNIC_MAX_RXQS);
+ if (!netdev)
+ return NULL;
+
+ SET_NETDEV_DEV(netdev, mpd->dev);
+ mpd->netdev = netdev;
+
+ netdev->netdev_ops = &mpnic_netdev_ops;
+ netdev->request_ops_lock = true;
+
+ mpn = netdev_priv(netdev);
+ mpn->netdev = netdev;
+ mpn->mpd = mpd;
+
+ mpn->txq_size = MPNIC_TXQ_SIZE_DEFAULT;
+
+ queues = min(netif_get_num_default_rss_queues(),
+ mpd->num_irqs - MPNIC_NON_NAPI_VECTORS);
+ mpn->num_tx_queues = queues;
+ mpn->num_napi = queues;
+
+ netdev->features |= NETIF_F_SG;
+ netdev->hw_features |= netdev->features;
+ netdev->vlan_features |= netdev->features;
+
+ netdev->min_mtu = IPV6_MIN_MTU;
+ netdev->max_mtu = MPNIC_MAX_JUMBO_FRAME_SIZE - ETH_HLEN;
+
+ netif_carrier_off(netdev);
+ netif_tx_stop_all_queues(netdev);
+
+ return netdev;
+}
+
+static int mpnic_dsn_to_mac_addr(u64 dsn, char *addr)
+{
+ addr[0] = (dsn >> 56) & 0xFF;
+ addr[1] = (dsn >> 48) & 0xFF;
+ addr[2] = (dsn >> 40) & 0xFF;
+ addr[3] = (dsn >> 16) & 0xFF;
+ addr[4] = (dsn >> 8) & 0xFF;
+ addr[5] = dsn & 0xFF;
+
+ return is_valid_ether_addr(addr) ? 0 : -EINVAL;
+}
+
+/**
+ * mpnic_netdev_register - Assign the MAC address and register the netdev
+ * @netdev: Netdev to register
+ *
+ * The permanent address is derived from the PCIe device serial number, the
+ * same way the firmware and the BMC derive it. A random address would break
+ * provisioning, so refuse to spawn the interface if the serial number does
+ * not yield a valid one.
+ *
+ * Return: non-zero on failure.
+ **/
+int mpnic_netdev_register(struct net_device *netdev)
+{
+ struct mpnic_net *mpn = netdev_priv(netdev);
+ struct mpnic_dev *mpd = mpn->mpd;
+ u8 addr[ETH_ALEN];
+ int err;
+
+ err = mpnic_dsn_to_mac_addr(mpd->dsn, addr);
+ if (err) {
+ dev_err(mpd->dev, "MAC addr %pM invalid\n", addr);
+ return err;
+ }
+
+ ether_addr_copy(netdev->perm_addr, addr);
+ eth_hw_addr_set(netdev, addr);
+
+ /* Abort if MMIO has failed. This has to be the last check before
+ * registration, the register accessors can only detach the device
+ * once it has been registered.
+ */
+ if (!mpnic_present(mpd))
+ return -EIO;
+
+ return register_netdev(netdev);
+}
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
index f98adf209b45..df682d194826 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
@@ -23,4 +23,8 @@ struct mpnic_net {
u16 num_tx_queues;
};
+struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd);
+void mpnic_netdev_free(struct mpnic_dev *mpd);
+int mpnic_netdev_register(struct net_device *netdev);
+
#endif /* _MPNIC_NETDEV_H_ */
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
index 127b71b44b0f..968cd611b8ea 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
@@ -4,11 +4,13 @@
#include <linux/dma-mapping.h>
#include <linux/err.h>
#include <linux/module.h>
+#include <linux/netdevice.h>
#include <linux/pci.h>
#include <linux/slab.h>
#include <linux/types.h>
#include "mpnic.h"
+#include "mpnic_netdev.h"
#define PCI_DEVICE_ID_META_MPNIC 0x0014
@@ -20,6 +22,10 @@ static void mpnic_mmio_err(struct mpnic_dev *mpd, u32 reg)
dev_err(mpd->dev,
"Failed read (idx 0x%x AKA addr 0x%x), disabled CSR access, awaiting reset\n",
reg, reg << 2);
+
+ /* Tell the stack the device has lost its PCIe link */
+ if (mpd->netdev)
+ netif_device_detach(mpd->netdev);
}
u64 mpnic_rd64(struct mpnic_dev *mpd, u32 reg)
@@ -58,6 +64,7 @@ static struct mpnic_dev *mpnic_alloc(struct pci_dev *pdev)
pci_set_drvdata(pdev, mpd);
mpd->dev = &pdev->dev;
+ mpd->dsn = pci_get_dsn(pdev);
mpd->mps = pcie_get_mps(pdev);
mpd->readrq = pcie_get_readrq(pdev);
mpd->relaxed_ord = pcie_relaxed_ordering_enabled(pdev);
@@ -74,6 +81,7 @@ static struct mpnic_dev *mpnic_alloc(struct pci_dev *pdev)
**/
static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
+ struct net_device *netdev;
void __iomem *uc_addr0;
struct mpnic_dev *mpd;
int err;
@@ -120,8 +128,23 @@ static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
if (err)
goto err_free_irqs;
+ netdev = mpnic_netdev_alloc(mpd);
+ if (!netdev) {
+ dev_err(&pdev->dev, "Netdev allocation failed\n");
+ err = -ENOMEM;
+ goto err_free_irqs;
+ }
+
+ err = mpnic_netdev_register(netdev);
+ if (err) {
+ dev_err(&pdev->dev, "Netdev registration failed: %d\n", err);
+ goto err_free_netdev;
+ }
+
return 0;
+err_free_netdev:
+ mpnic_netdev_free(mpd);
err_free_irqs:
mpnic_free_irqs(mpd);
err_free_mpd:
@@ -138,6 +161,8 @@ static void mpnic_remove(struct pci_dev *pdev)
{
struct mpnic_dev *mpd = pci_get_drvdata(pdev);
+ unregister_netdev(mpd->netdev);
+ mpnic_netdev_free(mpd);
mpnic_free_irqs(mpd);
kfree(mpd);
}
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
index cb4d1427434c..f4ad8caf6ce6 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -17,6 +17,8 @@ struct mpnic_xmit_cb {
};
#define MPNIC_XMIT_CB(__skb) ((struct mpnic_xmit_cb *)((__skb)->cb))
+#define MPNIC_TWD_TYPE_MASK(_type) \
+ cpu_to_le64(FIELD_PREP(MPNIC_TWD_TYPE, MPNIC_TWD_TYPE_##_type))
/* Leave the interrupt moderation counters alone when arming or masking */
#define MPNIC_TIM_PARAM_CFG_PRESERVE_MASK \
@@ -51,6 +53,29 @@ static struct netdev_queue *mpnic_txring_txq(const struct net_device *dev,
return netdev_get_tx_queue(dev, ring->q_idx);
}
+static void mpnic_tx_doorbell(struct mpnic_ring *ring, __le64 *meta)
+{
+ *meta |= cpu_to_le64(MPNIC_TWD_FLAG_REQ_COMPLETION);
+ ring->deferred_meta = -1;
+
+ /* Force DMA writes to flush before writing to tail */
+ dma_wmb();
+
+ writeq(ring->tail, ring->doorbell);
+}
+
+/* Packets handed to us with xmit_more set are left in the ring without a
+ * doorbell, and without a completion request, in the expectation that the
+ * packet ending the burst will ring for all of them. If that packet gets
+ * dropped instead we have to ring here, otherwise the descriptors sit in
+ * the ring until the next transmit, which may never come.
+ */
+static void mpnic_tx_flush_doorbell(struct mpnic_ring *ring)
+{
+ if (ring->deferred_meta >= 0)
+ mpnic_tx_doorbell(ring, &ring->desc[ring->deferred_meta]);
+}
+
static void mpnic_unmap_single_twd(struct device *dev, __le64 *twd)
{
u64 raw_twd = le64_to_cpu(*twd);
@@ -67,6 +92,138 @@ static void mpnic_unmap_page_twd(struct device *dev, __le64 *twd)
FIELD_GET(MPNIC_TWD_LEN, raw_twd), DMA_TO_DEVICE);
}
+static bool
+mpnic_tx_map(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
+{
+ struct device *dev = skb->dev->dev.parent;
+ unsigned int tail = ring->tail, first;
+ unsigned int size, data_len;
+ skb_frag_t *frag;
+ dma_addr_t dma;
+ __le64 *twd;
+
+ tail++;
+ tail &= ring->size_mask;
+ first = tail;
+
+ size = skb_headlen(skb);
+ data_len = skb->data_len;
+
+ if (size > FIELD_MAX(MPNIC_TWD_LEN))
+ goto err_dma;
+
+ dma = dma_map_single(dev, skb->data, size, DMA_TO_DEVICE);
+
+ for (frag = &skb_shinfo(skb)->frags[0];; frag++) {
+ twd = &ring->desc[tail];
+
+ if (dma_mapping_error(dev, dma))
+ goto err_dma;
+
+ *twd = cpu_to_le64(FIELD_PREP(MPNIC_TWD_ADDR, dma) |
+ FIELD_PREP(MPNIC_TWD_LEN, size) |
+ FIELD_PREP(MPNIC_TWD_TYPE,
+ MPNIC_TWD_TYPE_AL));
+
+ tail++;
+ tail &= ring->size_mask;
+
+ if (!data_len)
+ break;
+
+ size = skb_frag_size(frag);
+ data_len -= size;
+
+ if (size > FIELD_MAX(MPNIC_TWD_LEN))
+ goto err_dma;
+
+ dma = skb_frag_dma_map(dev, frag, 0, size, DMA_TO_DEVICE);
+ }
+
+ *twd |= MPNIC_TWD_TYPE_MASK(LAST_AL);
+
+ MPNIC_XMIT_CB(skb)->desc_count = ((twd - meta) + 1) & ring->size_mask;
+
+ skb_tx_timestamp(skb);
+
+ ring->tail = tail;
+
+ /* Verify there is room for another packet */
+ netif_txq_maybe_stop(mpnic_txring_txq(skb->dev, ring),
+ mpnic_desc_unused(ring), MPNIC_MAX_SKB_DESC,
+ MPNIC_TX_DESC_WAKEUP);
+
+ if (__netdev_tx_sent_queue(mpnic_txring_txq(skb->dev, ring),
+ MPNIC_XMIT_CB(skb)->bytecount,
+ netdev_xmit_more()))
+ mpnic_tx_doorbell(ring, meta);
+ else
+ ring->deferred_meta = meta - ring->desc;
+
+ return false;
+err_dma:
+ if (net_ratelimit())
+ netdev_err(skb->dev, "TX DMA map failed\n");
+
+ while (tail != first) {
+ tail--;
+ tail &= ring->size_mask;
+ twd = &ring->desc[tail];
+ if (tail == first)
+ mpnic_unmap_single_twd(dev, twd);
+ else
+ mpnic_unmap_page_twd(dev, twd);
+ }
+
+ return true;
+}
+
+#define MPNIC_MIN_FRAME_LEN 60
+
+static netdev_tx_t mpnic_xmit_frame_ring(struct sk_buff *skb,
+ struct mpnic_ring *ring)
+{
+ __le64 *meta = &ring->desc[ring->tail];
+ u32 tail = ring->tail;
+
+ if (skb_put_padto(skb, MPNIC_MIN_FRAME_LEN))
+ goto err_drop;
+
+ if (!netif_txq_maybe_stop(mpnic_txring_txq(skb->dev, ring),
+ mpnic_desc_unused(ring), MPNIC_MAX_SKB_DESC,
+ MPNIC_TX_DESC_WAKEUP)) {
+ mpnic_tx_flush_doorbell(ring);
+ return NETDEV_TX_BUSY;
+ }
+
+ ring->tx_buf[tail] = skb;
+ *meta = cpu_to_le64(MPNIC_TWD_FLAG_DEST_MAC);
+
+ MPNIC_XMIT_CB(skb)->bytecount = skb->len;
+ MPNIC_XMIT_CB(skb)->desc_count = 0;
+
+ if (mpnic_tx_map(ring, skb, meta))
+ goto err_free;
+
+ return NETDEV_TX_OK;
+
+err_free:
+ dev_kfree_skb_any(skb);
+ ring->tx_buf[tail] = NULL;
+ ring->tail = tail;
+err_drop:
+ mpnic_tx_flush_doorbell(ring);
+
+ return NETDEV_TX_OK;
+}
+
+netdev_tx_t mpnic_xmit_frame(struct sk_buff *skb, struct net_device *dev)
+{
+ struct mpnic_net *mpn = netdev_priv(dev);
+
+ return mpnic_xmit_frame_ring(skb, mpn->tx[skb_get_queue_mapping(skb)]);
+}
+
static void mpnic_clean_twq0(struct mpnic_napi_vector *nv, int napi_budget,
struct mpnic_ring *ring, bool discard,
unsigned int hw_head)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
index 68a773133fa2..2add4ad0d39a 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
@@ -26,6 +26,10 @@ struct mpnic_net;
#define MPNIC_MAX_NAPI_VECTORS 1024u
+#define MPNIC_TXQ_SIZE_DEFAULT 1024
+
+#define MPNIC_MAX_JUMBO_FRAME_SIZE 9742
+
struct mpnic_ring {
void **tx_buf; /* Packets outstanding in a TWQ */
@@ -68,6 +72,7 @@ struct mpnic_napi_vector {
struct mpnic_q_triad qt[];
};
+netdev_tx_t mpnic_xmit_frame(struct sk_buff *skb, struct net_device *dev);
int mpnic_alloc_napi_vectors(struct mpnic_net *mpn);
void mpnic_free_napi_vectors(struct mpnic_net *mpn);
int mpnic_alloc_resources(struct mpnic_net *mpn);
--
2.52.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 6/8] eth: mpnic: add a netdevice and basic Tx handling
2026-09-23 1:43 ` [PATCH net-next 6/8] eth: mpnic: add a netdevice and basic Tx handling Daniel Zahka
@ 2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-24 18:08 ` Daniel Zahka
0 siblings, 1 reply; 22+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 2:05 UTC (permalink / raw)
To: daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 3 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] mpnic_open() (drivers/net/ethernet/meta/mpnic/mpnic_netdev.c)
can succeed and show a dead device as up.
- [Medium] The new ndo_stop, mpnic_stop() in
drivers/net/ethernet/meta/mpnic/mpnic_netdev.c, reclaims DMA memory…
- [Medium] No TX traffic or drops are ever accounted.
--
eth: mpnic: add a netdevice and basic Tx handling
This patch registers a netdev for the mpnic driver and adds ndo_open and
ndo_stop. It also adds a basic transmit path that builds one metadata
descriptor plus address/length descriptors per skb, defers doorbells for
xmit_more bursts, and flushes the deferred doorbell when a packet is
dropped. mpnic_mmio_err() now detaches the netdev when MMIO fails.
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> new file mode 100644
> index 0000000000000..62dc9018f2f8c
> --- /dev/null
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> @@ -0,0 +1,176 @@
[ ... ]
> +static int mpnic_open(struct net_device *netdev)
> +{
> + struct mpnic_net *mpn = netdev_priv(netdev);
> + int err;
> +
> + err = mpnic_alloc_napi_vectors(mpn);
> + if (err)
> + return err;
> +
> + err = mpnic_alloc_resources(mpn);
> + if (err)
> + goto err_free_napi_vectors;
> +
> + err = mpnic_set_netif_queues(mpn);
> + if (err)
> + goto err_free_resources;
> +
> + mpnic_enable(mpn);
> + mpnic_napi_enable(mpn);
> +
> + netif_tx_wake_all_queues(netdev);
> + netif_carrier_on(netdev);
> +
> + return 0;
[Severity: Medium]
Can mpnic_open() return success for a device that has just been
detached?
mpnic_enable() and mpnic_napi_enable() both end with mpnic_wrfl(), which
is mpnic_rd64(mpd, MPNIC_BDQ_SPARE). If that read returns all ones, the
confirmation read is skipped because reg == MPNIC_BDQ_SPARE. As a result,
mpnic_mmio_err() runs. With this patch that handler now does:
/* Tell the stack the device has lost its PCIe link */
if (mpd->netdev)
netif_device_detach(mpd->netdev);
__dev_open() has already set __LINK_STATE_START before calling ndo_open.
netif_running() is therefore true, and the detach stops all tx queues.
mpnic_open() then continues without checking anything:
mpnic_open()
mpnic_enable()
mpnic_wrfl()->mpnic_rd64()->mpnic_mmio_err()
netif_device_detach()->netif_tx_stop_all_queues()
mpnic_napi_enable()
netif_tx_wake_all_queues()
netif_carrier_on()
return 0
netif_tx_wake_queue() does not check whether the device is present. So
the queues are awake again, carrier is on and IFF_UP gets set, while
__LINK_STATE_PRESENT is clear and uc_addr0 is NULL.
mpnic_xmit_frame() doesn't check presence either. It keeps filling
descriptors and calling writeq() on the cached ring->doorbell. No
completions come back, and dev_watchdog skips non-present devices, so no
tx timeout fires either.
Would it make sense to check mpnic_present() or netif_device_present()
after the enable steps, and unwind if the check fails?
> +
> +err_free_resources:
> + mpnic_free_resources(mpn);
> +err_free_napi_vectors:
> + mpnic_free_napi_vectors(mpn);
> + return err;
> +}
> +
> +static int mpnic_stop(struct net_device *netdev)
> +{
> + struct mpnic_net *mpn = netdev_priv(netdev);
> +
> + netif_carrier_off(netdev);
> +
> + mpnic_napi_disable(mpn);
> + netif_tx_disable(netdev);
> +
> + mpnic_disable(mpn);
> + mpnic_wait_all_queues_idle(mpn->mpd);
> + mpnic_flush(mpn);
> +
> + mpnic_reset_netif_queues(mpn);
> + mpnic_free_resources(mpn);
> + mpnic_free_napi_vectors(mpn);
[Severity: Medium]
What happens here if mpnic_wait_all_queues_idle() times out? It returns
void and only logs the failure:
mpnic_wait_all_queues_idle() {
...
err = read_poll_timeout(mpnic_non_idle_queues, non_idle_bitmap,
!non_idle_bitmap, 20, 500000, false, mpd,
queues, ARRAY_SIZE(queues));
if (err)
mpnic_idle_dump(mpd, queues, ARRAY_SIZE(queues),
non_idle_bitmap, err);
}
mpnic_stop() then always carries on with:
mpnic_flush()
mpnic_clean_twq0(nv, 0, &qt->sub0, true, qt->sub0.tail)
unmaps and frees every outstanding skb
mpnic_free_resources()->mpnic_free_ring_resources()
dma_free_coherent() on the TWQ and TCQ rings
If a Tx block (TWQ/TQS/TDE/TCQ) is still busy after 500 ms, can the
device read from unmapped skb pages or write completions into freed
coherent memory? The TCQ is only disabled by writing TCQ_CTL = 0.
This path has no fallback: no flush retry, no function reset, no clearing
of bus mastering. For comparison, fbnic calls fbnic_tx_flush() and polls
again before giving up.
A related case: if an earlier MMIO failure already cleared uc_addr0,
mpnic_disable() skips all its writes, and mpnic_rd64() returns ~0 for
every idle register. Does the wait then report idle straight away, so
the rings are freed even though the queues were never disabled?
> +
> + return 0;
> +}
[ ... ]
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index cb4d1427434c9..f4ad8caf6ce66 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
[ ... ]
> +static netdev_tx_t mpnic_xmit_frame_ring(struct sk_buff *skb,
> + struct mpnic_ring *ring)
> +{
> + __le64 *meta = &ring->desc[ring->tail];
> + u32 tail = ring->tail;
> +
> + if (skb_put_padto(skb, MPNIC_MIN_FRAME_LEN))
> + goto err_drop;
[ ... ]
> + if (mpnic_tx_map(ring, skb, meta))
> + goto err_free;
> +
> + return NETDEV_TX_OK;
> +
> +err_free:
> + dev_kfree_skb_any(skb);
> + ring->tx_buf[tail] = NULL;
> + ring->tail = tail;
> +err_drop:
> + mpnic_tx_flush_doorbell(ring);
> +
> + return NETDEV_TX_OK;
> +}
[Severity: Medium]
Are these drops counted anywhere?
Both drop paths return NETDEV_TX_OK without calling
dev_core_stats_tx_dropped_inc() or updating any other counter:
- err_drop, when skb_put_padto() fails
- err_free, when mpnic_tx_map() fails on a DMA mapping error or an
oversized head or fragment
The only trace is the rate-limited netdev_err() on the DMA path.
There is also no stats path in general. mpnic_netdev_ops has no
.ndo_get_stats64 or .ndo_get_stats, and netdev->stats is never updated.
mpnic_clean_twq0() passes its totals only to BQL, through
netif_txq_completed_wake().
Won't ip -s link then show tx_packets, tx_bytes and tx_dropped as 0 on an
interface that is transmitting and dropping packets?
The struct net_device_ops statistics comment in include/linux/netdevice.h
says drivers must implement one of the supported statistics mechanisms.
This still seems to be the case at the end of the series.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-linux-mpnic-v1-0-236844f53072%40gmail.com
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 6/8] eth: mpnic: add a netdevice and basic Tx handling
2026-09-24 2:05 ` netdev-bot+sashiko
@ 2026-09-24 18:08 ` Daniel Zahka
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Zahka @ 2026-09-24 18:08 UTC (permalink / raw)
To: netdev-bot+sashiko, daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
On Wed Sep 23, 2026 at 10:05 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 3 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] mpnic_open() (drivers/net/ethernet/meta/mpnic/mpnic_netdev.c)
> can succeed and show a dead device as up.
> - [Medium] The new ndo_stop, mpnic_stop() in
> drivers/net/ethernet/meta/mpnic/mpnic_netdev.c, reclaims DMA memory…
> - [Medium] No TX traffic or drops are ever accounted.
>
> --
>
> eth: mpnic: add a netdevice and basic Tx handling
>
> This patch registers a netdev for the mpnic driver and adds ndo_open and
> ndo_stop. It also adds a basic transmit path that builds one metadata
> descriptor plus address/length descriptors per skb, defers doorbells for
> xmit_more bursts, and flushes the deferred doorbell when a packet is
> dropped. mpnic_mmio_err() now detaches the netdev when MMIO fails.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
>> new file mode 100644
>> index 0000000000000..62dc9018f2f8c
>> --- /dev/null
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
>> @@ -0,0 +1,176 @@
>
> [ ... ]
>
>> +static int mpnic_open(struct net_device *netdev)
>> +{
>> + struct mpnic_net *mpn = netdev_priv(netdev);
>> + int err;
>> +
>> + err = mpnic_alloc_napi_vectors(mpn);
>> + if (err)
>> + return err;
>> +
>> + err = mpnic_alloc_resources(mpn);
>> + if (err)
>> + goto err_free_napi_vectors;
>> +
>> + err = mpnic_set_netif_queues(mpn);
>> + if (err)
>> + goto err_free_resources;
>> +
>> + mpnic_enable(mpn);
>> + mpnic_napi_enable(mpn);
>> +
>> + netif_tx_wake_all_queues(netdev);
>> + netif_carrier_on(netdev);
>> +
>> + return 0;
>
> [Severity: Medium]
> Can mpnic_open() return success for a device that has just been
> detached?
>
> mpnic_enable() and mpnic_napi_enable() both end with mpnic_wrfl(), which
> is mpnic_rd64(mpd, MPNIC_BDQ_SPARE). If that read returns all ones, the
> confirmation read is skipped because reg == MPNIC_BDQ_SPARE. As a result,
> mpnic_mmio_err() runs. With this patch that handler now does:
>
> /* Tell the stack the device has lost its PCIe link */
> if (mpd->netdev)
> netif_device_detach(mpd->netdev);
>
> __dev_open() has already set __LINK_STATE_START before calling ndo_open.
> netif_running() is therefore true, and the detach stops all tx queues.
> mpnic_open() then continues without checking anything:
>
> mpnic_open()
> mpnic_enable()
> mpnic_wrfl()->mpnic_rd64()->mpnic_mmio_err()
> netif_device_detach()->netif_tx_stop_all_queues()
> mpnic_napi_enable()
> netif_tx_wake_all_queues()
> netif_carrier_on()
> return 0
>
> netif_tx_wake_queue() does not check whether the device is present. So
> the queues are awake again, carrier is on and IFF_UP gets set, while
> __LINK_STATE_PRESENT is clear and uc_addr0 is NULL.
>
> mpnic_xmit_frame() doesn't check presence either. It keeps filling
> descriptors and calling writeq() on the cached ring->doorbell. No
> completions come back, and dev_watchdog skips non-present devices, so no
> tx timeout fires either.
>
> Would it make sense to check mpnic_present() or netif_device_present()
> after the enable steps, and unwind if the check fails?
>
I don't think so. Losing the device here should be benign, and will
require a reset, or the device to be re probed.
>> +
>> +err_free_resources:
>> + mpnic_free_resources(mpn);
>> +err_free_napi_vectors:
>> + mpnic_free_napi_vectors(mpn);
>> + return err;
>> +}
>> +
>> +static int mpnic_stop(struct net_device *netdev)
>> +{
>> + struct mpnic_net *mpn = netdev_priv(netdev);
>> +
>> + netif_carrier_off(netdev);
>> +
>> + mpnic_napi_disable(mpn);
>> + netif_tx_disable(netdev);
>> +
>> + mpnic_disable(mpn);
>> + mpnic_wait_all_queues_idle(mpn->mpd);
>> + mpnic_flush(mpn);
>> +
>> + mpnic_reset_netif_queues(mpn);
>> + mpnic_free_resources(mpn);
>> + mpnic_free_napi_vectors(mpn);
>
> [Severity: Medium]
> What happens here if mpnic_wait_all_queues_idle() times out? It returns
> void and only logs the failure:
>
> mpnic_wait_all_queues_idle() {
> ...
> err = read_poll_timeout(mpnic_non_idle_queues, non_idle_bitmap,
> !non_idle_bitmap, 20, 500000, false, mpd,
> queues, ARRAY_SIZE(queues));
> if (err)
> mpnic_idle_dump(mpd, queues, ARRAY_SIZE(queues),
> non_idle_bitmap, err);
> }
>
> mpnic_stop() then always carries on with:
>
> mpnic_flush()
> mpnic_clean_twq0(nv, 0, &qt->sub0, true, qt->sub0.tail)
> unmaps and frees every outstanding skb
> mpnic_free_resources()->mpnic_free_ring_resources()
> dma_free_coherent() on the TWQ and TCQ rings
>
> If a Tx block (TWQ/TQS/TDE/TCQ) is still busy after 500 ms, can the
> device read from unmapped skb pages or write completions into freed
> coherent memory? The TCQ is only disabled by writing TCQ_CTL = 0.
>
> This path has no fallback: no flush retry, no function reset, no clearing
> of bus mastering. For comparison, fbnic calls fbnic_tx_flush() and polls
> again before giving up.
>
> A related case: if an earlier MMIO failure already cleared uc_addr0,
> mpnic_disable() skips all its writes, and mpnic_rd64() returns ~0 for
> every idle register. Does the wait then report idle straight away, so
> the rings are freed even though the queues were never disabled?
>
This was addressed in prior patch.
>> +
>> + return 0;
>> +}
>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index cb4d1427434c9..f4ad8caf6ce66 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>
> [ ... ]
>
>> +static netdev_tx_t mpnic_xmit_frame_ring(struct sk_buff *skb,
>> + struct mpnic_ring *ring)
>> +{
>> + __le64 *meta = &ring->desc[ring->tail];
>> + u32 tail = ring->tail;
>> +
>> + if (skb_put_padto(skb, MPNIC_MIN_FRAME_LEN))
>> + goto err_drop;
>
> [ ... ]
>
>> + if (mpnic_tx_map(ring, skb, meta))
>> + goto err_free;
>> +
>> + return NETDEV_TX_OK;
>> +
>> +err_free:
>> + dev_kfree_skb_any(skb);
>> + ring->tx_buf[tail] = NULL;
>> + ring->tail = tail;
>> +err_drop:
>> + mpnic_tx_flush_doorbell(ring);
>> +
>> + return NETDEV_TX_OK;
>> +}
>
> [Severity: Medium]
> Are these drops counted anywhere?
>
> Both drop paths return NETDEV_TX_OK without calling
> dev_core_stats_tx_dropped_inc() or updating any other counter:
doesn't sound like dev_core_stats_tx_dropped_inc() is supposed to be
used in drivers.
>
> - err_drop, when skb_put_padto() fails
> - err_free, when mpnic_tx_map() fails on a DMA mapping error or an
> oversized head or fragment
>
> The only trace is the rate-limited netdev_err() on the DMA path.
>
> There is also no stats path in general. mpnic_netdev_ops has no
> .ndo_get_stats64 or .ndo_get_stats, and netdev->stats is never updated.
> mpnic_clean_twq0() passes its totals only to BQL, through
> netif_txq_completed_wake().
>
> Won't ip -s link then show tx_packets, tx_bytes and tx_dropped as 0 on an
> interface that is transmitting and dropping packets?
>
> The struct net_device_ops statistics comment in include/linux/netdevice.h
> says drivers must implement one of the supported statistics mechanisms.
> This still seems to be the case at the end of the series.
Stats are planned as the next series.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next 7/8] eth: mpnic: implement Rx queue allocation and cleanup
2026-09-23 1:43 [PATCH net-next 0/8] eth: mpnic: initial support for Meta Platforms NIC Daniel Zahka
` (5 preceding siblings ...)
2026-09-23 1:43 ` [PATCH net-next 6/8] eth: mpnic: add a netdevice and basic Tx handling Daniel Zahka
@ 2026-09-23 1:43 ` Daniel Zahka
2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-23 1:43 ` [PATCH net-next 8/8] eth: mpnic: add basic Rx handling Daniel Zahka
7 siblings, 1 reply; 22+ messages in thread
From: Daniel Zahka @ 2026-09-23 1:43 UTC (permalink / raw)
To: Alexander Duyck, Jakub Kicinski, kernel-team, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Dimitri Daskalakis, Mohsin Bashir
Cc: linux-kernel, netdev, bpf
The Rx side of a queue triad is a header queue, a payload queue and a
completion queue. With HDS not active yet, frames are split between the
two buffer queues at a fixed header boundary: everything up to 1536
bytes lands in the header page, the rest goes to the payload page. Small
frames therefore never touch the payload queue, and several of them
share one header page.
The device is set to leave headroom and tailroom for XDP and
skb_shared_info respectively.
Both queues draw from one page pool per triad and post buffer
descriptors to the device in batches.
Even though we do not support XDP yet, use the XDP APIs for tracking
pre-skb buffers. The XDP buffer handling helpers save us from
reimplementing the same logic in the driver.
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
drivers/net/ethernet/meta/Kconfig | 1 +
drivers/net/ethernet/meta/mpnic/mpnic_csr.h | 67 ++++
drivers/net/ethernet/meta/mpnic/mpnic_netdev.c | 5 +
drivers/net/ethernet/meta/mpnic/mpnic_netdev.h | 5 +
drivers/net/ethernet/meta/mpnic/mpnic_txrx.c | 451 +++++++++++++++++++++++--
drivers/net/ethernet/meta/mpnic/mpnic_txrx.h | 72 +++-
6 files changed, 557 insertions(+), 44 deletions(-)
diff --git a/drivers/net/ethernet/meta/Kconfig b/drivers/net/ethernet/meta/Kconfig
index f940048a5e37..f8b5fe7e31b7 100644
--- a/drivers/net/ethernet/meta/Kconfig
+++ b/drivers/net/ethernet/meta/Kconfig
@@ -40,6 +40,7 @@ config MPNIC
depends on 64BIT || COMPILE_TEST
depends on !S390
depends on PCI_MSI
+ select PAGE_POOL
help
This driver supports the Meta Platforms Network Interface
Controller.
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index f7c50bc70d8a..96ed2386bd6a 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -30,6 +30,38 @@ enum {
#define MPNIC_TCD_TYPE0_HEAD0 DESC_GENMASK(15, 0)
#define MPNIC_TCD_DONE DESC_BIT(63)
+/* Rx Buffer Descriptor Format */
+#define MPNIC_BD_DESC_ADDR DESC_GENMASK(39, 2)
+#define MPNIC_BD_DESC_ID DESC_GENMASK(57, 40)
+#define MPNIC_BD_DESC_BUF_SZ_LOG2 DESC_GENMASK(62, 58)
+
+/* Rx Completion Queue Descriptors */
+#define MPNIC_RCD_TYPE DESC_GENMASK(62, 61)
+enum {
+ MPNIC_RCD_TYPE_HDR_AL = 0,
+ MPNIC_RCD_TYPE_PAY_AL = 1,
+ MPNIC_RCD_TYPE_META = 3,
+};
+
+#define MPNIC_RCD_DONE DESC_BIT(63)
+
+#define MPNIC_RCD_HDR_SUBTYPE DESC_GENMASK(60, 59)
+enum {
+ MPNIC_RCD_HDR_SUBTYPE_HDR = 2,
+};
+
+/* Address/Length Completion Descriptors */
+#define MPNIC_RCD_AL_BUFF_OFF DESC_GENMASK(15, 0)
+#define MPNIC_RCD_AL_BUFF_ID DESC_GENMASK(33, 16)
+#define MPNIC_RCD_AL_BUFF_LEN DESC_GENMASK(47, 34)
+#define MPNIC_RCD_AL_PAGE_FIN DESC_BIT(53)
+
+/* Metadata Completion Descriptors */
+#define MPNIC_RCD_META_ERR_MAC_EOP DESC_BIT(53)
+#define MPNIC_RCD_META_ERR_TRUNCATED_FRAME DESC_BIT(54)
+#define MPNIC_RCD_META_UNCORRECTABLE_ERR_MASK \
+ (MPNIC_RCD_META_ERR_MAC_EOP | MPNIC_RCD_META_ERR_TRUNCATED_FRAME)
+
/* Common fields for all DESC_CFG CSRs */
#define MPNIC_DESC_CFG_NUM_DESCS CSR_GENMASK(2, 0)
#define MPNIC_DESC_CFG_START_ADDR CSR_GENMASK(19, 8)
@@ -76,10 +108,43 @@ enum {
#define MPNIC_TIM_INTR_MASK(i) (0xc8 + 1024 * (i)) /* 0x320 */
#define MPNIC_TIM_INTR_MASK_MASK CSR_BIT(0)
+/* NIC_CORE_RBP */
+#define MPNIC_BDQ_CTL(i) (0x200 + 1024 * (i)) /* 0x800 */
+#define MPNIC_BDQ_CTL_RESET CSR_BIT(0)
+#define MPNIC_BDQ_CTL_ENABLE CSR_BIT(1)
+#define MPNIC_BDQ_CTL_ENABLE_PPQ CSR_BIT(3)
+#define MPNIC_HPQ_TAIL(i) (0x202 + 1024 * (i)) /* 0x808 */
+#define MPNIC_PPQ_TAIL(i) (0x204 + 1024 * (i)) /* 0x810 */
+#define MPNIC_HPQ_SIZE(i) (0x20a + 1024 * (i)) /* 0x828 */
+#define MPNIC_HPQ_SIZE_SIZE CSR_GENMASK(4, 0)
+#define MPNIC_PPQ_SIZE(i) (0x20c + 1024 * (i)) /* 0x830 */
+#define MPNIC_PPQ_SIZE_SIZE CSR_GENMASK(4, 0)
+#define MPNIC_HPQ_BASE_ADDR(i) (0x216 + 1024 * (i)) /* 0x858 */
+#define MPNIC_PPQ_BASE_ADDR(i) (0x218 + 1024 * (i)) /* 0x860 */
+
+/* NIC_CORE_RCM */
+#define MPNIC_RCQ_CTL(i) (0x280 + 1024 * (i)) /* 0xa00 */
+#define MPNIC_RCQ_CTL_RESET CSR_BIT(0)
+#define MPNIC_RCQ_CTL_ENABLE CSR_BIT(1)
+#define MPNIC_RCQ_BASE_ADDR(i) (0x286 + 1024 * (i)) /* 0xa18 */
+#define MPNIC_RCQ_HEAD(i) (0x28e + 1024 * (i)) /* 0xa38 */
+#define MPNIC_RCQ_SIZE(i) (0x294 + 1024 * (i)) /* 0xa50 */
+#define MPNIC_RCQ_SIZE_SIZE CSR_GENMASK(4, 0)
+
/* NIC_CORE_TIM_PRV */
#define MPNIC_TIM_CTL(i) (0x100100 + 1024 * (i)) /* 0x400400 */
+/* NIC_CORE_RDE */
+#define MPNIC_RDE_CFG(i) (0x10021c + 1024 * (i)) /* 0x400870 */
+#define MPNIC_RDE_CFG_MIN_TAIL_ROOM CSR_GENMASK(9, 0)
+#define MPNIC_RDE_CFG_MIN_HEAD_ROOM CSR_GENMASK(18, 10)
+#define MPNIC_RDE_CFG_MAX_HEADER_BYTES CSR_GENMASK(45, 32)
+
/* NIC_CORE_RBP_HP_GLBL */
+#define MPNIC_HPQ_IDLE(i) (0x420000 + 2 * (i)) /* 0x1080000 */
+#define MPNIC_HPQ_IDLE_CNT 16
+#define MPNIC_PPQ_IDLE(i) (0x420060 + 2 * (i)) /* 0x1080180 */
+#define MPNIC_PPQ_IDLE_CNT 16
#define MPNIC_BDQ_GLBL_CTL0 0x420080 /* 0x1080200 */
#define MPNIC_BDQ_GLBL_CTL0_MAX_REQ_SIZE CSR_GENMASK(26, 18)
#define MPNIC_BDQ_GLBL_CTL0_PREFETCH_SPACE_THRESH \
@@ -100,6 +165,8 @@ enum {
#define MPNIC_RDE_MEM_INIT_DONE 0x4240e8 /* 0x10903a0 */
/* NIC_CORE_RCM_GLBL */
+#define MPNIC_RCQ_IDLE(i) (0x42505e + 2 * (i)) /* 0x1094178 */
+#define MPNIC_RCQ_IDLE_CNT 16
#define MPNIC_RCM_MEM_INIT_REQ 0x42507e /* 0x10941f8 */
#define MPNIC_RCM_MEM_INIT_DONE 0x425080 /* 0x1094200 */
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
index 62dc9018f2f8..fd34f48a654c 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
@@ -29,6 +29,7 @@ static int mpnic_open(struct net_device *netdev)
goto err_free_resources;
mpnic_enable(mpn);
+ mpnic_fill(mpn);
mpnic_napi_enable(mpn);
netif_tx_wake_all_queues(netdev);
@@ -107,10 +108,14 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
mpn->mpd = mpd;
mpn->txq_size = MPNIC_TXQ_SIZE_DEFAULT;
+ mpn->hpq_size = MPNIC_HPQ_SIZE_DEFAULT;
+ mpn->ppq_size = MPNIC_PPQ_SIZE_DEFAULT;
+ mpn->rcq_size = MPNIC_RCQ_SIZE_DEFAULT;
queues = min(netif_get_num_default_rss_queues(),
mpd->num_irqs - MPNIC_NON_NAPI_VECTORS);
mpn->num_tx_queues = queues;
+ mpn->num_rx_queues = queues;
mpn->num_napi = queues;
netdev->features |= NETIF_F_SG;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
index df682d194826..ccb0929f9180 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
@@ -11,6 +11,7 @@
struct mpnic_net {
struct mpnic_ring *tx[MPNIC_MAX_TXQS];
+ struct mpnic_ring *rx[MPNIC_MAX_RXQS];
struct mpnic_napi_vector *napi[MPNIC_MAX_NAPI_VECTORS];
@@ -18,9 +19,13 @@ struct mpnic_net {
struct mpnic_dev *mpd;
u32 txq_size;
+ u32 hpq_size;
+ u32 ppq_size;
+ u32 rcq_size;
u16 num_napi;
u16 num_tx_queues;
+ u16 num_rx_queues;
};
struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd);
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
index f4ad8caf6ce6..6d2123bee97b 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -6,6 +6,7 @@
#include <linux/iopoll.h>
#include <linux/pci.h>
#include <linux/slab.h>
+#include <net/page_pool/helpers.h>
#include "mpnic.h"
#include "mpnic_netdev.h"
@@ -335,6 +336,109 @@ static void mpnic_clean_tcq(struct mpnic_napi_vector *nv,
mpnic_clean_twq0(nv, napi_budget, &qt->sub0, false, head0);
}
+static void mpnic_bd_prep(struct mpnic_ring *bdq, u32 idx, struct page *page)
+{
+ dma_addr_t dma = page_pool_get_dma_addr(page);
+
+ bdq->desc[idx] = cpu_to_le64(FIELD_PREP(MPNIC_BD_DESC_ADDR, dma >> 10) |
+ FIELD_PREP(MPNIC_BD_DESC_ID, idx) |
+ FIELD_PREP(MPNIC_BD_DESC_BUF_SZ_LOG2,
+ page_shift(page) - 10));
+}
+
+/* Descriptors are only handed to the device in whole batches, so the slot
+ * the device is working on and everything up to the next batch boundary
+ * stay untouched while it does.
+ */
+static unsigned int mpnic_bdq_desc_unused(struct mpnic_ring *bdq)
+{
+ return (ALIGN_DOWN(bdq->head - 1, MPNIC_BDQ_BATCH_SIZE) - bdq->tail) &
+ bdq->size_mask;
+}
+
+static unsigned int __mpnic_fill_bdq(struct mpnic_ring *bdq)
+{
+ unsigned int i = bdq->tail;
+ unsigned int count;
+
+ for (count = mpnic_bdq_desc_unused(bdq); count; count--) {
+ struct page *page;
+
+ page = page_pool_dev_alloc_pages(bdq->page_pool);
+ if (!page)
+ break;
+
+ bdq->rx_buf[i] = page;
+ mpnic_bd_prep(bdq, i, page);
+
+ i++;
+ i &= bdq->size_mask;
+ }
+
+ return i;
+}
+
+static void __mpnic_bdq_commit_tail(struct mpnic_ring *bdq, unsigned int tail)
+{
+ if (bdq->tail != tail) {
+ bdq->tail = tail;
+
+ writeq(tail, bdq->doorbell);
+ }
+}
+
+static void mpnic_fill_qt_bdqs(struct mpnic_q_triad *qt)
+{
+ unsigned int ppq_i = __mpnic_fill_bdq(&qt->sub1);
+ unsigned int hpq_i = __mpnic_fill_bdq(&qt->sub0);
+
+ /* Force DMA writes to flush before writing to tail(s) */
+ dma_wmb();
+
+ /* Flush out the completions we are done with */
+ mpnic_commit_cq_head(&qt->cmpl);
+
+ __mpnic_bdq_commit_tail(&qt->sub0, hpq_i);
+ __mpnic_bdq_commit_tail(&qt->sub1, ppq_i);
+}
+
+static void mpnic_flush_pg_ctxt(struct mpnic_pg_ctxt *ctxt, bool napi)
+{
+ long pagecnt_bias = ctxt->pagecnt_bias;
+
+ if (pagecnt_bias) {
+ struct page *page = ctxt->page;
+
+ if (!page_pool_unref_page(page, pagecnt_bias))
+ page_pool_put_unrefed_page(page->pp, page, -1, napi);
+ }
+}
+
+static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
+{
+ struct xdp_buff *buff = &ctxt->buff;
+ struct page *page;
+
+ if (!buff->data_hard_start)
+ return;
+
+ if (unlikely(xdp_buff_has_frags(buff))) {
+ struct skb_shared_info *shinfo;
+ int nr_frags;
+
+ shinfo = xdp_get_shared_info_from_buff(buff);
+ nr_frags = shinfo->nr_frags;
+
+ while (nr_frags--) {
+ page = skb_frag_page(&shinfo->frags[nr_frags]);
+ page_pool_put_full_page(page->pp, page, napi);
+ }
+ }
+
+ page = virt_to_head_page(buff->data_hard_start);
+ page_pool_put_full_page(page->pp, page, napi);
+}
+
static int mpnic_poll(struct napi_struct *napi, int budget)
{
struct mpnic_napi_vector *nv = container_of(napi,
@@ -366,11 +470,14 @@ static irqreturn_t mpnic_msix_clean_rings(int __always_unused irq, void *data)
static void mpnic_free_napi_vector(struct mpnic_net *mpn,
struct mpnic_napi_vector *nv)
{
- int i;
+ int i, j;
for (i = 0; i < nv->txt_count; i++)
mpn->tx[nv->qt[i].sub0.q_idx] = NULL;
+ for (j = 0; j < nv->rxt_count; j++, i++)
+ mpn->rx[nv->qt[i].cmpl.q_idx] = NULL;
+
mpnic_free_irq(nv->mpd, nv->v_idx, nv);
netif_napi_del_locked(&nv->napi);
mpn->napi[nv->v_idx - MPNIC_NON_NAPI_VECTORS] = NULL;
@@ -406,11 +513,12 @@ static int mpnic_alloc_napi_vector(struct mpnic_dev *mpd,
if (!uc_addr)
return -EIO;
- nv = kzalloc_flex(*nv, qt, 1);
+ nv = kzalloc_flex(*nv, qt, 2);
if (!nv)
return -ENOMEM;
nv->txt_count = 1;
+ nv->rxt_count = 1;
nv->mpd = mpd;
nv->dev = mpd->dev;
nv->v_idx = idx + MPNIC_NON_NAPI_VECTORS;
@@ -433,6 +541,11 @@ static int mpnic_alloc_napi_vector(struct mpnic_dev *mpd,
mpnic_ring_init(&nv->qt[0].cmpl, &uc_addr[MPNIC_TCQ_HEAD(idx)], idx);
mpn->tx[idx] = &nv->qt[0].sub0;
+ mpnic_ring_init(&nv->qt[1].sub0, &uc_addr[MPNIC_HPQ_TAIL(idx)], idx);
+ mpnic_ring_init(&nv->qt[1].sub1, &uc_addr[MPNIC_PPQ_TAIL(idx)], idx);
+ mpnic_ring_init(&nv->qt[1].cmpl, &uc_addr[MPNIC_RCQ_HEAD(idx)], idx);
+ mpn->rx[idx] = &nv->qt[1].cmpl;
+
return 0;
err_napi_del:
@@ -464,8 +577,8 @@ int mpnic_alloc_napi_vectors(struct mpnic_net *mpn)
static void mpnic_free_ring_resources(struct device *dev,
struct mpnic_ring *ring)
{
- kvfree(ring->tx_buf);
- ring->tx_buf = NULL;
+ kvfree(ring->buffer);
+ ring->buffer = NULL;
/* If size is not set there are no descriptors present */
if (!ring->size)
@@ -531,19 +644,134 @@ static int mpnic_alloc_tx_qt_resources(struct mpnic_net *mpn,
return err;
}
+static int
+mpnic_alloc_qt_page_pool(struct mpnic_net *mpn, struct mpnic_napi_vector *nv,
+ struct mpnic_q_triad *qt)
+{
+ struct page_pool_params pp_params = {
+ .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
+ .pool_size = min(mpn->hpq_size + mpn->ppq_size, 32768u),
+ .nid = NUMA_NO_NODE,
+ .dev = nv->dev,
+ .dma_dir = DMA_FROM_DEVICE,
+ .max_len = PAGE_SIZE,
+ .napi = &nv->napi,
+ .netdev = mpn->netdev,
+ .queue_idx = qt->cmpl.q_idx,
+ };
+ struct page_pool *pp;
+
+ pp = page_pool_create(&pp_params);
+ if (IS_ERR(pp))
+ return PTR_ERR(pp);
+
+ qt->sub0.page_pool = pp;
+ page_pool_get(pp);
+ qt->sub1.page_pool = pp;
+
+ return 0;
+}
+
+static void mpnic_free_rx_qt_resources(struct mpnic_net *mpn,
+ struct mpnic_q_triad *qt)
+{
+ struct device *dev = mpn->netdev->dev.parent;
+
+ mpnic_free_ring_resources(dev, &qt->cmpl);
+ mpnic_free_ring_resources(dev, &qt->sub1);
+ mpnic_free_ring_resources(dev, &qt->sub0);
+
+ if (xdp_rxq_info_is_reg(&qt->xdp_rxq)) {
+ xdp_rxq_info_unreg(&qt->xdp_rxq);
+ page_pool_destroy(qt->sub1.page_pool);
+ page_pool_destroy(qt->sub0.page_pool);
+ }
+}
+
+static int mpnic_alloc_rx_qt_resources(struct mpnic_net *mpn,
+ struct mpnic_napi_vector *nv,
+ struct mpnic_q_triad *qt)
+{
+ int err;
+
+ err = mpnic_alloc_qt_page_pool(mpn, nv, qt);
+ if (err)
+ return err;
+
+ err = xdp_rxq_info_reg(&qt->xdp_rxq, mpn->netdev, qt->cmpl.q_idx,
+ nv->napi.napi_id);
+ if (err)
+ goto err_free_page_pool;
+
+ err = xdp_rxq_info_reg_mem_model(&qt->xdp_rxq, MEM_TYPE_PAGE_POOL,
+ qt->sub0.page_pool);
+ if (err)
+ goto err_unreg_rxq;
+
+ err = mpnic_alloc_ring_desc(mpn, &qt->sub0, mpn->hpq_size);
+ if (err)
+ goto err_unreg_mm;
+
+ qt->sub0.rx_buf = kvzalloc_objs(*qt->sub0.rx_buf, mpn->hpq_size,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!qt->sub0.rx_buf) {
+ err = -ENOMEM;
+ goto err_free_qt;
+ }
+
+ err = mpnic_alloc_ring_desc(mpn, &qt->sub1, mpn->ppq_size);
+ if (err)
+ goto err_free_qt;
+
+ qt->sub1.rx_buf = kvzalloc_objs(*qt->sub1.rx_buf, mpn->ppq_size,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!qt->sub1.rx_buf) {
+ err = -ENOMEM;
+ goto err_free_qt;
+ }
+
+ err = mpnic_alloc_ring_desc(mpn, &qt->cmpl, mpn->rcq_size);
+ if (err)
+ goto err_free_qt;
+
+ qt->cmpl.state = kvzalloc_obj(*qt->cmpl.state,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!qt->cmpl.state) {
+ err = -ENOMEM;
+ goto err_free_qt;
+ }
+
+ return 0;
+
+err_free_qt:
+ mpnic_free_rx_qt_resources(mpn, qt);
+ return err;
+err_unreg_mm:
+ xdp_rxq_info_unreg_mem_model(&qt->xdp_rxq);
+err_unreg_rxq:
+ xdp_rxq_info_unreg(&qt->xdp_rxq);
+err_free_page_pool:
+ page_pool_destroy(qt->sub1.page_pool);
+ page_pool_destroy(qt->sub0.page_pool);
+ return err;
+}
+
static void mpnic_free_nv_resources(struct mpnic_net *mpn,
struct mpnic_napi_vector *nv)
{
- int i;
+ int i, j;
for (i = 0; i < nv->txt_count; i++)
mpnic_free_tx_qt_resources(mpn, &nv->qt[i]);
+
+ for (j = 0; j < nv->rxt_count; j++, i++)
+ mpnic_free_rx_qt_resources(mpn, &nv->qt[i]);
}
static int mpnic_alloc_nv_resources(struct mpnic_net *mpn,
struct mpnic_napi_vector *nv)
{
- int i, err;
+ int i, j, err;
for (i = 0; i < nv->txt_count; i++) {
err = mpnic_alloc_tx_qt_resources(mpn, &nv->qt[i]);
@@ -551,11 +779,21 @@ static int mpnic_alloc_nv_resources(struct mpnic_net *mpn,
goto err_free_qt_resources;
}
+ for (j = 0; j < nv->rxt_count; j++, i++) {
+ err = mpnic_alloc_rx_qt_resources(mpn, nv, &nv->qt[i]);
+ if (err)
+ goto err_free_qt_resources;
+ }
+
return 0;
err_free_qt_resources:
- while (i--)
- mpnic_free_tx_qt_resources(mpn, &nv->qt[i]);
+ while (i--) {
+ if (i < nv->txt_count)
+ mpnic_free_tx_qt_resources(mpn, &nv->qt[i]);
+ else
+ mpnic_free_rx_qt_resources(mpn, &nv->qt[i]);
+ }
return err;
}
@@ -586,36 +824,41 @@ int mpnic_alloc_resources(struct mpnic_net *mpn)
return err;
}
+static void mpnic_set_netif_napi(struct mpnic_napi_vector *nv,
+ struct napi_struct *napi)
+{
+ int i, j;
+
+ for (i = 0; i < nv->txt_count; i++)
+ netif_queue_set_napi(nv->napi.dev, nv->qt[i].sub0.q_idx,
+ NETDEV_QUEUE_TYPE_TX, napi);
+
+ for (j = 0; j < nv->rxt_count; j++, i++)
+ netif_queue_set_napi(nv->napi.dev, nv->qt[i].cmpl.q_idx,
+ NETDEV_QUEUE_TYPE_RX, napi);
+}
+
int mpnic_set_netif_queues(struct mpnic_net *mpn)
{
- int i, j, err;
+ int i, err;
- err = netif_set_real_num_tx_queues(mpn->netdev, mpn->num_tx_queues);
+ err = netif_set_real_num_queues(mpn->netdev, mpn->num_tx_queues,
+ mpn->num_rx_queues);
if (err)
return err;
- for (i = 0; i < mpn->num_napi; i++) {
- struct mpnic_napi_vector *nv = mpn->napi[i];
-
- for (j = 0; j < nv->txt_count; j++)
- netif_queue_set_napi(mpn->netdev, nv->qt[j].sub0.q_idx,
- NETDEV_QUEUE_TYPE_TX, &nv->napi);
- }
+ for (i = 0; i < mpn->num_napi; i++)
+ mpnic_set_netif_napi(mpn->napi[i], &mpn->napi[i]->napi);
return 0;
}
void mpnic_reset_netif_queues(struct mpnic_net *mpn)
{
- int i, j;
-
- for (i = 0; i < mpn->num_napi; i++) {
- struct mpnic_napi_vector *nv = mpn->napi[i];
+ int i;
- for (j = 0; j < nv->txt_count; j++)
- netif_queue_set_napi(mpn->netdev, nv->qt[j].sub0.q_idx,
- NETDEV_QUEUE_TYPE_TX, NULL);
- }
+ for (i = 0; i < mpn->num_napi; i++)
+ mpnic_set_netif_napi(mpn->napi[i], NULL);
}
static void mpnic_enable_twq(struct mpnic_dev *mpd, struct mpnic_ring *twq)
@@ -659,17 +902,77 @@ static void mpnic_enable_tcq(struct mpnic_dev *mpd,
mpnic_wr64(mpd, MPNIC_TCQ_CTL(i), MPNIC_TCQ_CTL_ENABLE);
}
+static void mpnic_enable_bdq(struct mpnic_dev *mpd, struct mpnic_ring *hpq,
+ struct mpnic_ring *ppq)
+{
+ u32 hpq_log_size = fls(hpq->size_mask);
+ u32 ppq_log_size = fls(ppq->size_mask);
+ u32 i = hpq->q_idx;
+
+ /* Reset head/tail */
+ mpnic_wr64(mpd, MPNIC_BDQ_CTL(i), MPNIC_BDQ_CTL_RESET);
+ hpq->tail = 0;
+ hpq->head = 0;
+ ppq->tail = 0;
+ ppq->head = 0;
+
+ /* Store descriptor ring addresses and sizes */
+ mpnic_wr64(mpd, MPNIC_HPQ_BASE_ADDR(i), hpq->dma);
+ mpnic_wr64(mpd, MPNIC_HPQ_SIZE(i), hpq_log_size & MPNIC_HPQ_SIZE_SIZE);
+ mpnic_wr64(mpd, MPNIC_PPQ_BASE_ADDR(i), ppq->dma);
+ mpnic_wr64(mpd, MPNIC_PPQ_SIZE(i), ppq_log_size & MPNIC_PPQ_SIZE_SIZE);
+
+ mpnic_wr64(mpd, MPNIC_BDQ_CTL(i),
+ MPNIC_BDQ_CTL_ENABLE | MPNIC_BDQ_CTL_ENABLE_PPQ);
+}
+
+static void mpnic_set_rde_cfg(struct mpnic_dev *mpd, struct mpnic_ring *rcq)
+{
+ BUILD_BUG_ON(FIELD_MAX(MPNIC_RDE_CFG_MIN_HEAD_ROOM) < MPNIC_RX_HROOM);
+ BUILD_BUG_ON(FIELD_MAX(MPNIC_RDE_CFG_MIN_TAIL_ROOM) < MPNIC_RX_TROOM);
+
+ mpnic_wr64(mpd, MPNIC_RDE_CFG(rcq->q_idx),
+ FIELD_PREP(MPNIC_RDE_CFG_MIN_HEAD_ROOM, MPNIC_RX_HROOM) |
+ FIELD_PREP(MPNIC_RDE_CFG_MIN_TAIL_ROOM, MPNIC_RX_TROOM) |
+ FIELD_PREP(MPNIC_RDE_CFG_MAX_HEADER_BYTES,
+ MPNIC_RX_MAX_HDR));
+}
+
+static void mpnic_enable_rcq(struct mpnic_dev *mpd, struct mpnic_ring *rcq)
+{
+ u32 log_size = fls(rcq->size_mask);
+ u32 i = rcq->q_idx;
+
+ mpnic_set_rde_cfg(mpd, rcq);
+
+ /* Reset head/tail */
+ mpnic_wr64(mpd, MPNIC_RCQ_CTL(i), MPNIC_RCQ_CTL_RESET);
+ rcq->head = 0;
+ rcq->tail = 0;
+
+ /* Store descriptor ring address and size */
+ mpnic_wr64(mpd, MPNIC_RCQ_BASE_ADDR(i), rcq->dma);
+ mpnic_wr64(mpd, MPNIC_RCQ_SIZE(i), log_size & MPNIC_RCQ_SIZE_SIZE);
+
+ mpnic_wr64(mpd, MPNIC_RCQ_CTL(i), MPNIC_RCQ_CTL_ENABLE);
+}
+
void mpnic_enable(struct mpnic_net *mpn)
{
struct mpnic_dev *mpd = mpn->mpd;
- int i, j;
+ int i, j, t;
for (i = 0; i < mpn->num_napi; i++) {
struct mpnic_napi_vector *nv = mpn->napi[i];
- for (j = 0; j < nv->txt_count; j++) {
- mpnic_enable_twq(mpd, &nv->qt[j].sub0);
- mpnic_enable_tcq(mpd, nv, &nv->qt[j].cmpl);
+ for (t = 0; t < nv->txt_count; t++) {
+ mpnic_enable_twq(mpd, &nv->qt[t].sub0);
+ mpnic_enable_tcq(mpd, nv, &nv->qt[t].cmpl);
+ }
+
+ for (j = 0; j < nv->rxt_count; j++, t++) {
+ mpnic_enable_bdq(mpd, &nv->qt[t].sub0, &nv->qt[t].sub1);
+ mpnic_enable_rcq(mpd, &nv->qt[t].cmpl);
}
}
@@ -691,17 +994,35 @@ static void mpnic_disable_tcq(struct mpnic_dev *mpd, struct mpnic_ring *txr)
MPNIC_TIM_INTR_MASK_MASK);
}
+static void mpnic_disable_bdq(struct mpnic_dev *mpd, struct mpnic_ring *hpq)
+{
+ u64 bdq_ctl = mpnic_rd64(mpd, MPNIC_BDQ_CTL(hpq->q_idx));
+
+ bdq_ctl &= ~(MPNIC_BDQ_CTL_ENABLE | MPNIC_BDQ_CTL_ENABLE_PPQ);
+ mpnic_wr64(mpd, MPNIC_BDQ_CTL(hpq->q_idx), bdq_ctl);
+}
+
+static void mpnic_disable_rcq(struct mpnic_dev *mpd, struct mpnic_ring *rcq)
+{
+ mpnic_wr64(mpd, MPNIC_RCQ_CTL(rcq->q_idx), 0);
+}
+
void mpnic_disable(struct mpnic_net *mpn)
{
struct mpnic_dev *mpd = mpn->mpd;
- int i, j;
+ int i, j, t;
for (i = 0; i < mpn->num_napi; i++) {
struct mpnic_napi_vector *nv = mpn->napi[i];
- for (j = 0; j < nv->txt_count; j++) {
- mpnic_disable_twq(mpd, &nv->qt[j].sub0);
- mpnic_disable_tcq(mpd, &nv->qt[j].cmpl);
+ for (t = 0; t < nv->txt_count; t++) {
+ mpnic_disable_twq(mpd, &nv->qt[t].sub0);
+ mpnic_disable_tcq(mpd, &nv->qt[t].cmpl);
+ }
+
+ for (j = 0; j < nv->rxt_count; j++, t++) {
+ mpnic_disable_bdq(mpd, &nv->qt[t].sub0);
+ mpnic_disable_rcq(mpd, &nv->qt[t].cmpl);
}
}
@@ -760,6 +1081,9 @@ void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd)
{ MPNIC_TQS_IDLE(0), MPNIC_TQS_IDLE_CNT, "TQS" },
{ MPNIC_TDE_IDLE(0), MPNIC_TDE_IDLE_CNT, "TDE" },
{ MPNIC_TCQ_IDLE(0), MPNIC_TCQ_IDLE_CNT, "TCQ" },
+ { MPNIC_HPQ_IDLE(0), MPNIC_HPQ_IDLE_CNT, "HPQ" },
+ { MPNIC_PPQ_IDLE(0), MPNIC_PPQ_IDLE_CNT, "PPQ" },
+ { MPNIC_RCQ_IDLE(0), MPNIC_RCQ_IDLE_CNT, "RCQ" },
};
u32 non_idle_bitmap;
int err;
@@ -772,15 +1096,31 @@ void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd)
non_idle_bitmap, err);
}
+static void mpnic_clean_bdq(struct mpnic_ring *bdq)
+{
+ unsigned int head = bdq->head;
+
+ while (head != bdq->tail) {
+ struct page *page = bdq->rx_buf[head];
+
+ page_pool_put_full_page(page->pp, page, false);
+
+ head++;
+ head &= bdq->size_mask;
+ }
+
+ bdq->head = head;
+}
+
void mpnic_flush(struct mpnic_net *mpn)
{
- int i, j;
+ int i, j, t;
for (i = 0; i < mpn->num_napi; i++) {
struct mpnic_napi_vector *nv = mpn->napi[i];
- for (j = 0; j < nv->txt_count; j++) {
- struct mpnic_q_triad *qt = &nv->qt[j];
+ for (t = 0; t < nv->txt_count; t++) {
+ struct mpnic_q_triad *qt = &nv->qt[t];
struct netdev_queue *txq;
/* Clean the work queue of unprocessed work */
@@ -789,6 +1129,45 @@ void mpnic_flush(struct mpnic_net *mpn)
txq = netdev_get_tx_queue(mpn->netdev, qt->sub0.q_idx);
netdev_tx_reset_queue(txq);
}
+
+ for (j = 0; j < nv->rxt_count; j++, t++) {
+ struct mpnic_q_triad *qt = &nv->qt[t];
+ struct mpnic_rcq_state *state = qt->cmpl.state;
+
+ /* Release the partially assembled frame and the
+ * pages the queues are still handing out.
+ */
+ mpnic_put_pkt_buff(&state->pkt, false);
+ mpnic_flush_pg_ctxt(&state->hdr, false);
+ mpnic_flush_pg_ctxt(&state->payld, false);
+ memset(state, 0, sizeof(*state));
+
+ mpnic_clean_bdq(&qt->sub0);
+ mpnic_clean_bdq(&qt->sub1);
+ }
+ }
+}
+
+void mpnic_fill(struct mpnic_net *mpn)
+{
+ int i, j, t;
+
+ for (i = 0; i < mpn->num_napi; i++) {
+ struct mpnic_napi_vector *nv = mpn->napi[i];
+
+ for (j = 0, t = nv->txt_count; j < nv->rxt_count; j++, t++) {
+ struct mpnic_q_triad *qt = &nv->qt[t];
+ struct mpnic_rcq_state *state = qt->cmpl.state;
+
+ /* Point the page contexts at an index the device
+ * cannot report, so the first buffer coming out of
+ * either queue is not taken for a page we hold.
+ */
+ state->hdr.idx = UINT_MAX;
+ state->payld.idx = UINT_MAX;
+
+ mpnic_fill_qt_bdqs(qt);
+ }
}
}
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
index 2add4ad0d39a..ba118dd2f3de 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
@@ -9,6 +9,7 @@
#include <linux/skbuff.h>
#include <linux/types.h>
#include <net/netdev_queues.h>
+#include <net/xdp.h>
#include "mpnic.h"
@@ -26,12 +27,58 @@ struct mpnic_net;
#define MPNIC_MAX_NAPI_VECTORS 1024u
+/* Number of buffer descriptors the driver posts before ringing the
+ * doorbell. The device consumes whatever the doorbell points at, this is
+ * purely to keep the driver from writing the CSR for every descriptor.
+ */
+#define MPNIC_BDQ_BATCH_SIZE 64u
+
#define MPNIC_TXQ_SIZE_DEFAULT 1024
+#define MPNIC_HPQ_SIZE_DEFAULT 256
+#define MPNIC_PPQ_SIZE_DEFAULT 256
+#define MPNIC_RCQ_SIZE_DEFAULT 1024
+
+/* Room the device has to leave in front of and behind every header so the
+ * driver can build an skb around it in place. The headroom is padded out
+ * so that consecutive headers in one page start 128 B aligned.
+ */
+#define MPNIC_RX_TROOM \
+ SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
+#define MPNIC_RX_HROOM \
+ (ALIGN(MPNIC_RX_TROOM + XDP_PACKET_HEADROOM, 128) - MPNIC_RX_TROOM)
+
+/* Headers longer than this are split off into the payload queue */
+#define MPNIC_RX_MAX_HDR 1536
#define MPNIC_MAX_JUMBO_FRAME_SIZE 9742
+/* The page a buffer descriptor queue is currently handing out. Records
+ * how many of the references taken on it are still unused.
+ */
+struct mpnic_pg_ctxt {
+ struct page *page;
+ long pagecnt_bias;
+ u32 idx;
+};
+
+struct mpnic_pkt_ctxt {
+ struct xdp_buff buff;
+ u32 data_truesize;
+};
+
+struct mpnic_rcq_state {
+ struct mpnic_pkt_ctxt pkt;
+ struct mpnic_pg_ctxt hdr;
+ struct mpnic_pg_ctxt payld;
+};
+
struct mpnic_ring {
- void **tx_buf; /* Packets outstanding in a TWQ */
+ union {
+ struct mpnic_rcq_state *state; /* RCQ */
+ struct page **rx_buf; /* BDQ */
+ void **tx_buf; /* TWQ */
+ void *buffer; /* Generic pointer */
+ };
u32 __iomem *doorbell; /* Pointer to CSR space for ring */
__le64 *desc; /* Descriptor ring memory */
@@ -40,22 +87,29 @@ struct mpnic_ring {
u32 head, tail; /* Head/Tail of ring */
- /* TWQ only, index of the metadata descriptor of the last packet
- * placed in the ring without ringing the doorbell, -1 if the
- * doorbell is in sync with the tail.
- */
- s32 deferred_meta;
+ union {
+ /* BDQ only */
+ struct page_pool *page_pool;
+
+ /* TWQ only, index of the metadata descriptor of the last
+ * packet placed in the ring without ringing the doorbell,
+ * -1 if the doorbell is in sync with the tail.
+ */
+ s32 deferred_meta;
+ };
/* Slow path fields follow */
dma_addr_t dma; /* Phys addr of descriptor memory */
size_t size; /* Size of descriptor ring in memory */
};
-/* The device pairs two work queues with one completion queue. On the Tx
- * side only the first work queue is used for now, the second one becomes
+/* The device pairs two work queues with one completion queue. On the Rx
+ * side they are the header and the payload buffer descriptor queues; on
+ * the Tx side only the first one is used for now, the second one becomes
* the XDP ring.
*/
struct mpnic_q_triad {
+ struct xdp_rxq_info xdp_rxq;
struct mpnic_ring sub0, sub1, cmpl;
};
@@ -66,6 +120,7 @@ struct mpnic_napi_vector {
u16 v_idx;
u16 txt_count;
+ u16 rxt_count;
char name[IFNAMSIZ + 11];
@@ -85,5 +140,6 @@ void mpnic_enable(struct mpnic_net *mpn);
void mpnic_disable(struct mpnic_net *mpn);
void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd);
void mpnic_flush(struct mpnic_net *mpn);
+void mpnic_fill(struct mpnic_net *mpn);
#endif /* _MPNIC_TXRX_H_ */
--
2.52.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 7/8] eth: mpnic: implement Rx queue allocation and cleanup
2026-09-23 1:43 ` [PATCH net-next 7/8] eth: mpnic: implement Rx queue allocation and cleanup Daniel Zahka
@ 2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-24 18:23 ` Daniel Zahka
0 siblings, 1 reply; 22+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 2:05 UTC (permalink / raw)
To: daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The Rx engines are added to an idle wait whose timeout is only
logged, and teardown then frees memory the device writes into.
- [Low] The two comments about Rx buffer batching disagree with each
other and with the code.
--
eth: mpnic: implement Rx queue allocation and cleanup
This adds the Rx side of the mpnic queue triad: a header buffer queue
and a payload buffer queue, both fed from one page pool per triad, plus
an Rx completion queue. Buffers are posted to the device in batches, and
the device is set up to leave XDP headroom and skb_shared_info tailroom.
Teardown releases any partially assembled frame and the buffers still
posted.
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index f4ad8caf6ce66..6d2123bee97b4 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
[ ... ]
> @@ -335,6 +336,109 @@ static void mpnic_clean_tcq(struct mpnic_napi_vector *nv,
[ ... ]
> +/* Descriptors are only handed to the device in whole batches, so the slot
> + * the device is working on and everything up to the next batch boundary
> + * stay untouched while it does.
> + */
> +static unsigned int mpnic_bdq_desc_unused(struct mpnic_ring *bdq)
> +{
> + return (ALIGN_DOWN(bdq->head - 1, MPNIC_BDQ_BATCH_SIZE) - bdq->tail) &
> + bdq->size_mask;
> +}
> +
> +static unsigned int __mpnic_fill_bdq(struct mpnic_ring *bdq)
> +{
> + unsigned int i = bdq->tail;
> + unsigned int count;
> +
> + for (count = mpnic_bdq_desc_unused(bdq); count; count--) {
> + struct page *page;
> +
> + page = page_pool_dev_alloc_pages(bdq->page_pool);
> + if (!page)
> + break;
> +
> + bdq->rx_buf[i] = page;
> + mpnic_bd_prep(bdq, i, page);
> +
> + i++;
> + i &= bdq->size_mask;
> + }
> +
> + return i;
> +}
> +
> +static void __mpnic_bdq_commit_tail(struct mpnic_ring *bdq, unsigned int tail)
> +{
> + if (bdq->tail != tail) {
> + bdq->tail = tail;
> +
> + writeq(tail, bdq->doorbell);
> + }
> +}
[Severity: Low]
This isn't a bug, but is the comment on mpnic_bdq_desc_unused() accurate?
__mpnic_fill_bdq() stops as soon as page_pool_dev_alloc_pages() returns
NULL. __mpnic_bdq_commit_tail() then writes that tail to the doorbell,
so a tail that is not on a 64 descriptor boundary can reach the device.
In that case descriptors are not handed over in whole batches.
It also looks like the slot at head - 1 stays untouched because of the
"head - 1" term. Whole-batch posting is not what protects it. The
ALIGN_DOWN only holds back up to 63 more slots that have already been
consumed.
The comment on MPNIC_BDQ_BATCH_SIZE in mpnic_txrx.h gives a different
reason for batching:
* doorbell. The device consumes whatever the doorbell points at, this is
* purely to keep the driver from writing the CSR for every descriptor.
Could the two comments be made consistent with each other and with what
the refill code actually does?
[ ... ]
> @@ -760,6 +1081,9 @@ void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd)
> { MPNIC_TQS_IDLE(0), MPNIC_TQS_IDLE_CNT, "TQS" },
> { MPNIC_TDE_IDLE(0), MPNIC_TDE_IDLE_CNT, "TDE" },
> { MPNIC_TCQ_IDLE(0), MPNIC_TCQ_IDLE_CNT, "TCQ" },
> + { MPNIC_HPQ_IDLE(0), MPNIC_HPQ_IDLE_CNT, "HPQ" },
> + { MPNIC_PPQ_IDLE(0), MPNIC_PPQ_IDLE_CNT, "PPQ" },
> + { MPNIC_RCQ_IDLE(0), MPNIC_RCQ_IDLE_CNT, "RCQ" },
> };
> u32 non_idle_bitmap;
> int err;
[Severity: Medium]
What happens in mpnic_stop() if one of the new Rx engines is still busy
when this wait times out?
mpnic_wait_all_queues_idle() returns void. It only logs on timeout:
err = read_poll_timeout(mpnic_non_idle_queues, non_idle_bitmap,
!non_idle_bitmap, 20, 500000, false, mpd,
queues, ARRAY_SIZE(queues));
if (err)
mpnic_idle_dump(mpd, queues, ARRAY_SIZE(queues),
non_idle_bitmap, err);
mpnic_stop() then keeps tearing down:
mpnic_disable(mpn);
mpnic_wait_all_queues_idle(mpn->mpd);
mpnic_flush(mpn);
mpnic_reset_netif_queues(mpn);
mpnic_free_resources(mpn);
mpnic_flush()->mpnic_clean_bdq() hands every posted HPQ/PPQ page back to
the pool with page_pool_put_full_page(). After that,
mpnic_free_resources()->mpnic_free_nv_resources()->
mpnic_free_rx_qt_resources() frees the RCQ ring with dma_free_coherent()
and calls page_pool_destroy(), which unmaps and frees the pages.
The earlier commit "eth: mpnic: start and stop the Tx HW queues" says
disabling a queue "only stops the device picking up new work, it does
not tell us that the work already fetched has been retired". If so, a
DMA_FROM_DEVICE write could still be in flight after a timeout. Examples
are descriptors the prefetcher has already fetched, or a frame the RDE
is partway through.
Could such a write land in RCQ memory or Rx pages that have already been
unmapped and possibly reused? That would cause an IOMMU fault, or silent
memory corruption on systems without an IOMMU. Before this patch, no
device-written Rx buffers were covered by this best-effort wait.
Whether this can happen depends on whether a non-idle Rx engine can
still complete DMA after the timeout. fbnic_down() follows the same
ordering. Would it make sense to return the timeout to mpnic_stop() and
avoid freeing device-writable memory (or reset the device first) when
the queues never go idle?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-linux-mpnic-v1-0-236844f53072%40gmail.com
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 7/8] eth: mpnic: implement Rx queue allocation and cleanup
2026-09-24 2:05 ` netdev-bot+sashiko
@ 2026-09-24 18:23 ` Daniel Zahka
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Zahka @ 2026-09-24 18:23 UTC (permalink / raw)
To: netdev-bot+sashiko, daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
On Wed Sep 23, 2026 at 10:05 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The Rx engines are added to an idle wait whose timeout is only
> logged, and teardown then frees memory the device writes into.
> - [Low] The two comments about Rx buffer batching disagree with each
> other and with the code.
>
> --
>
> eth: mpnic: implement Rx queue allocation and cleanup
>
> This adds the Rx side of the mpnic queue triad: a header buffer queue
> and a payload buffer queue, both fed from one page pool per triad, plus
> an Rx completion queue. Buffers are posted to the device in batches, and
> the device is set up to leave XDP headroom and skb_shared_info tailroom.
> Teardown releases any partially assembled frame and the buffers still
> posted.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index f4ad8caf6ce66..6d2123bee97b4 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>
> [ ... ]
>
>> @@ -335,6 +336,109 @@ static void mpnic_clean_tcq(struct mpnic_napi_vector *nv,
>
> [ ... ]
>
>> +/* Descriptors are only handed to the device in whole batches, so the slot
>> + * the device is working on and everything up to the next batch boundary
>> + * stay untouched while it does.
>> + */
>> +static unsigned int mpnic_bdq_desc_unused(struct mpnic_ring *bdq)
>> +{
>> + return (ALIGN_DOWN(bdq->head - 1, MPNIC_BDQ_BATCH_SIZE) - bdq->tail) &
>> + bdq->size_mask;
>> +}
>> +
>> +static unsigned int __mpnic_fill_bdq(struct mpnic_ring *bdq)
>> +{
>> + unsigned int i = bdq->tail;
>> + unsigned int count;
>> +
>> + for (count = mpnic_bdq_desc_unused(bdq); count; count--) {
>> + struct page *page;
>> +
>> + page = page_pool_dev_alloc_pages(bdq->page_pool);
>> + if (!page)
>> + break;
>> +
>> + bdq->rx_buf[i] = page;
>> + mpnic_bd_prep(bdq, i, page);
>> +
>> + i++;
>> + i &= bdq->size_mask;
>> + }
>> +
>> + return i;
>> +}
>> +
>> +static void __mpnic_bdq_commit_tail(struct mpnic_ring *bdq, unsigned int tail)
>> +{
>> + if (bdq->tail != tail) {
>> + bdq->tail = tail;
>> +
>> + writeq(tail, bdq->doorbell);
>> + }
>> +}
>
> [Severity: Low]
> This isn't a bug, but is the comment on mpnic_bdq_desc_unused() accurate?
>
> __mpnic_fill_bdq() stops as soon as page_pool_dev_alloc_pages() returns
> NULL. __mpnic_bdq_commit_tail() then writes that tail to the doorbell,
> so a tail that is not on a 64 descriptor boundary can reach the device.
> In that case descriptors are not handed over in whole batches.
>
> It also looks like the slot at head - 1 stays untouched because of the
> "head - 1" term. Whole-batch posting is not what protects it. The
> ALIGN_DOWN only holds back up to 63 more slots that have already been
> consumed.
>
> The comment on MPNIC_BDQ_BATCH_SIZE in mpnic_txrx.h gives a different
> reason for batching:
>
> * doorbell. The device consumes whatever the doorbell points at, this is
> * purely to keep the driver from writing the CSR for every descriptor.
>
> Could the two comments be made consistent with each other and with what
> the refill code actually does?
>
> [ ... ]
>
>> @@ -760,6 +1081,9 @@ void mpnic_wait_all_queues_idle(struct mpnic_dev *mpd)
>> { MPNIC_TQS_IDLE(0), MPNIC_TQS_IDLE_CNT, "TQS" },
>> { MPNIC_TDE_IDLE(0), MPNIC_TDE_IDLE_CNT, "TDE" },
>> { MPNIC_TCQ_IDLE(0), MPNIC_TCQ_IDLE_CNT, "TCQ" },
>> + { MPNIC_HPQ_IDLE(0), MPNIC_HPQ_IDLE_CNT, "HPQ" },
>> + { MPNIC_PPQ_IDLE(0), MPNIC_PPQ_IDLE_CNT, "PPQ" },
>> + { MPNIC_RCQ_IDLE(0), MPNIC_RCQ_IDLE_CNT, "RCQ" },
>> };
>> u32 non_idle_bitmap;
>> int err;
>
> [Severity: Medium]
> What happens in mpnic_stop() if one of the new Rx engines is still busy
> when this wait times out?
>
> mpnic_wait_all_queues_idle() returns void. It only logs on timeout:
>
> err = read_poll_timeout(mpnic_non_idle_queues, non_idle_bitmap,
> !non_idle_bitmap, 20, 500000, false, mpd,
> queues, ARRAY_SIZE(queues));
> if (err)
> mpnic_idle_dump(mpd, queues, ARRAY_SIZE(queues),
> non_idle_bitmap, err);
>
> mpnic_stop() then keeps tearing down:
>
> mpnic_disable(mpn);
> mpnic_wait_all_queues_idle(mpn->mpd);
> mpnic_flush(mpn);
>
> mpnic_reset_netif_queues(mpn);
> mpnic_free_resources(mpn);
>
> mpnic_flush()->mpnic_clean_bdq() hands every posted HPQ/PPQ page back to
> the pool with page_pool_put_full_page(). After that,
> mpnic_free_resources()->mpnic_free_nv_resources()->
> mpnic_free_rx_qt_resources() frees the RCQ ring with dma_free_coherent()
> and calls page_pool_destroy(), which unmaps and frees the pages.
>
> The earlier commit "eth: mpnic: start and stop the Tx HW queues" says
> disabling a queue "only stops the device picking up new work, it does
> not tell us that the work already fetched has been retired". If so, a
> DMA_FROM_DEVICE write could still be in flight after a timeout. Examples
> are descriptors the prefetcher has already fetched, or a frame the RDE
> is partway through.
>
> Could such a write land in RCQ memory or Rx pages that have already been
> unmapped and possibly reused? That would cause an IOMMU fault, or silent
> memory corruption on systems without an IOMMU. Before this patch, no
> device-written Rx buffers were covered by this best-effort wait.
>
> Whether this can happen depends on whether a non-idle Rx engine can
> still complete DMA after the timeout. fbnic_down() follows the same
> ordering. Would it make sense to return the timeout to mpnic_stop() and
> avoid freeing device-writable memory (or reset the device first) when
> the queues never go idle?
>
Same complaint addressed already.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next 8/8] eth: mpnic: add basic Rx handling
2026-09-23 1:43 [PATCH net-next 0/8] eth: mpnic: initial support for Meta Platforms NIC Daniel Zahka
` (6 preceding siblings ...)
2026-09-23 1:43 ` [PATCH net-next 7/8] eth: mpnic: implement Rx queue allocation and cleanup Daniel Zahka
@ 2026-09-23 1:43 ` Daniel Zahka
2026-09-24 2:05 ` netdev-bot+sashiko
7 siblings, 1 reply; 22+ messages in thread
From: Daniel Zahka @ 2026-09-23 1:43 UTC (permalink / raw)
To: Alexander Duyck, Jakub Kicinski, kernel-team, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Stanislav Fomichev, Dimitri Daskalakis, Mohsin Bashir
Cc: linux-kernel, netdev, bpf
A frame arrives as a run of completion descriptors: one header
address/length descriptor, one address/length descriptor per payload
page, and a metadata descriptor that closes the frame. The frame is
assembled in an xdp_buff as the descriptors come in and handed to the
stack when the metadata descriptor arrives.
A page holds several frames, so rather than taking a reference per
frame the driver takes a batch of references when it starts handing the
page out and returns whatever is left over once the device moves on to
the next one. Payload fragments that turn out to be contiguous within
one page are merged so that a frame spread over a page does not eat one
skb fragment slot per descriptor.
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
drivers/net/ethernet/meta/mpnic/mpnic_csr.h | 7 +
drivers/net/ethernet/meta/mpnic/mpnic_txrx.c | 205 ++++++++++++++++++++++++++-
drivers/net/ethernet/meta/mpnic/mpnic_txrx.h | 14 +-
3 files changed, 213 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index 96ed2386bd6a..423378ca802c 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -131,6 +131,10 @@ enum {
#define MPNIC_RCQ_SIZE(i) (0x294 + 1024 * (i)) /* 0xa50 */
#define MPNIC_RCQ_SIZE_SIZE CSR_GENMASK(4, 0)
+/* NIC_CORE_RIM */
+#define MPNIC_RIM_INTR_MASK(i) (0x2c8 + 1024 * (i)) /* 0xb20 */
+#define MPNIC_RIM_INTR_MASK_MASK CSR_BIT(0)
+
/* NIC_CORE_TIM_PRV */
#define MPNIC_TIM_CTL(i) (0x100100 + 1024 * (i)) /* 0x400400 */
@@ -140,6 +144,9 @@ enum {
#define MPNIC_RDE_CFG_MIN_HEAD_ROOM CSR_GENMASK(18, 10)
#define MPNIC_RDE_CFG_MAX_HEADER_BYTES CSR_GENMASK(45, 32)
+/* NIC_CORE_RIM_PRV */
+#define MPNIC_RIM_CTL(i) (0x100280 + 1024 * (i)) /* 0x400a00 */
+
/* NIC_CORE_RBP_HP_GLBL */
#define MPNIC_HPQ_IDLE(i) (0x420000 + 2 * (i)) /* 0x1080000 */
#define MPNIC_HPQ_IDLE_CNT 16
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
index 6d2123bee97b..9878ea5a2f8e 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -402,6 +402,34 @@ static void mpnic_fill_qt_bdqs(struct mpnic_q_triad *qt)
__mpnic_bdq_commit_tail(&qt->sub1, ppq_i);
}
+/* Take one of the references batched on the page at @idx. If the device
+ * has moved on to a new page, first drop the unused references left on
+ * the previous one.
+ */
+static struct page *
+mpnic_page_pool_get(struct mpnic_pg_ctxt *pg_ctxt, struct mpnic_ring *ring,
+ u32 idx)
+{
+ struct page *page = pg_ctxt->page;
+
+ if (unlikely(pg_ctxt->idx != idx)) {
+ if (pg_ctxt->pagecnt_bias &&
+ !page_pool_unref_page(page, pg_ctxt->pagecnt_bias))
+ page_pool_put_unrefed_page(page->pp, page, -1, true);
+
+ page = ring->rx_buf[idx];
+ page_pool_fragment_page(page, MPNIC_PAGECNT_BIAS_MAX);
+
+ pg_ctxt->page = page;
+ pg_ctxt->pagecnt_bias = MPNIC_PAGECNT_BIAS_MAX;
+ pg_ctxt->idx = idx;
+ }
+
+ pg_ctxt->pagecnt_bias--;
+
+ return page;
+}
+
static void mpnic_flush_pg_ctxt(struct mpnic_pg_ctxt *ctxt, bool napi)
{
long pagecnt_bias = ctxt->pagecnt_bias;
@@ -414,9 +442,90 @@ static void mpnic_flush_pg_ctxt(struct mpnic_pg_ctxt *ctxt, bool napi)
}
}
-static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
+static unsigned int mpnic_hdr_pg_start(unsigned int pg_off)
+{
+ /* The headroom of the first header may be larger than
+ * MPNIC_RX_HROOM due to alignment. So account for that by just
+ * making the page offset 0 if we are starting at the first header.
+ */
+ if (ALIGN(MPNIC_RX_HROOM, 128) > MPNIC_RX_HROOM &&
+ pg_off == ALIGN(MPNIC_RX_HROOM, 128))
+ return 0;
+
+ return pg_off - MPNIC_RX_HROOM;
+}
+
+static unsigned int mpnic_hdr_pg_end(unsigned int pg_off, unsigned int len)
+{
+ /* Determine the end of the buffer by finding the start of the next
+ * and then subtracting the headroom from that frame.
+ */
+ pg_off += len + MPNIC_RX_TROOM + MPNIC_RX_HROOM;
+
+ return ALIGN(pg_off, 128) - MPNIC_RX_HROOM;
+}
+
+static void
+mpnic_pkt_prepare(struct mpnic_napi_vector *nv, u64 rcd,
+ struct mpnic_rcq_state *state, struct mpnic_q_triad *qt)
+{
+ unsigned int pg_off = FIELD_GET(MPNIC_RCD_AL_BUFF_OFF, rcd);
+ unsigned int pg_idx = FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd);
+ unsigned int len = FIELD_GET(MPNIC_RCD_AL_BUFF_LEN, rcd);
+ bool fin = FIELD_GET(MPNIC_RCD_AL_PAGE_FIN, rcd);
+ unsigned int frame_sz, pg_start, pg_end;
+ struct xdp_buff *buff = &state->pkt;
+ struct page *page;
+
+ pg_start = mpnic_hdr_pg_start(pg_off);
+
+ page = mpnic_page_pool_get(&state->hdr, &qt->sub0, pg_idx);
+ qt->sub0.head = (pg_idx + 1) & qt->sub0.size_mask;
+
+ /* Short-cut the end calculation if the page is fully consumed */
+ pg_end = fin ? page_size(page) : mpnic_hdr_pg_end(pg_off, len);
+ frame_sz = pg_end - pg_start;
+
+ dma_sync_single_range_for_cpu(nv->dev, page_pool_get_dma_addr(page),
+ pg_start, frame_sz, DMA_FROM_DEVICE);
+
+ xdp_init_buff(buff, frame_sz, &qt->xdp_rxq);
+ xdp_prepare_buff(buff, page_address(page) + pg_start,
+ pg_off - pg_start, len, true);
+ net_prefetch(buff->data);
+
+ state->add_frag_failed = false;
+}
+
+static void
+mpnic_add_rx_frag(struct mpnic_napi_vector *nv, u64 rcd,
+ struct mpnic_rcq_state *state, struct mpnic_q_triad *qt)
+{
+ unsigned int pg_off = FIELD_GET(MPNIC_RCD_AL_BUFF_OFF, rcd);
+ unsigned int pg_idx = FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd);
+ unsigned int len = FIELD_GET(MPNIC_RCD_AL_BUFF_LEN, rcd);
+ bool fin = FIELD_GET(MPNIC_RCD_AL_PAGE_FIN, rcd);
+ struct xdp_buff *buff = &state->pkt;
+ unsigned int truesz;
+ struct page *page;
+
+ page = mpnic_page_pool_get(&state->payld, &qt->sub1, pg_idx);
+ qt->sub1.head = (pg_idx + 1) & qt->sub1.size_mask;
+
+ truesz = (fin ? page_size(page) : ALIGN(pg_off + len, 128)) - pg_off;
+
+ dma_sync_single_range_for_cpu(nv->dev, page_pool_get_dma_addr(page),
+ pg_off, truesz, DMA_FROM_DEVICE);
+
+ if (!xdp_buff_add_frag(buff, page_to_netmem(page), pg_off, len,
+ truesz)) {
+ state->payld.pagecnt_bias++;
+ state->add_frag_failed = true;
+ }
+}
+
+static void mpnic_put_pkt_buff(struct xdp_buff *buff, bool napi)
{
- struct xdp_buff *buff = &ctxt->buff;
struct page *page;
if (!buff->data_hard_start)
@@ -439,23 +548,97 @@ static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
page_pool_put_full_page(page->pp, page, napi);
}
+static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
+ struct mpnic_q_triad *qt, int budget)
+{
+ struct mpnic_ring *rcq = &qt->cmpl;
+ struct mpnic_rcq_state *state;
+ unsigned int packets = 0;
+ __le64 *raw_rcd, done;
+ u32 head = rcq->head;
+
+ done = (head & (rcq->size_mask + 1)) ? 0 : cpu_to_le64(MPNIC_RCD_DONE);
+ raw_rcd = &rcq->desc[head & rcq->size_mask];
+ state = rcq->state;
+
+ while (packets < budget) {
+ u64 rcd;
+
+ if ((*raw_rcd & cpu_to_le64(MPNIC_RCD_DONE)) != done)
+ break;
+
+ dma_rmb();
+
+ rcd = le64_to_cpu(*raw_rcd);
+
+ switch (FIELD_GET(MPNIC_RCD_TYPE, rcd)) {
+ case MPNIC_RCD_TYPE_HDR_AL:
+ if (FIELD_GET(MPNIC_RCD_HDR_SUBTYPE, rcd) ==
+ MPNIC_RCD_HDR_SUBTYPE_HDR)
+ mpnic_pkt_prepare(nv, rcd, state, qt);
+ break;
+ case MPNIC_RCD_TYPE_PAY_AL:
+ mpnic_add_rx_frag(nv, rcd, state, qt);
+ break;
+ case MPNIC_RCD_TYPE_META: {
+ struct sk_buff *skb = NULL;
+
+ if (likely(!(rcd &
+ MPNIC_RCD_META_UNCORRECTABLE_ERR_MASK) &&
+ !state->add_frag_failed))
+ skb = xdp_build_skb_from_buff(&state->pkt);
+
+ if (likely(skb))
+ napi_gro_receive(&nv->napi, skb);
+ else
+ mpnic_put_pkt_buff(&state->pkt, true);
+
+ state->pkt.data_hard_start = NULL;
+ packets++;
+ break;
+ }
+ }
+
+ raw_rcd++;
+ head++;
+
+ if (unlikely(!(head & rcq->size_mask))) {
+ done ^= cpu_to_le64(MPNIC_RCD_DONE);
+ raw_rcd = &rcq->desc[0];
+ }
+ }
+
+ rcq->head = head;
+
+ /* Allocate buffers, force dma_wmb(), and then start writing tails */
+ mpnic_fill_qt_bdqs(qt);
+
+ return packets;
+}
+
static int mpnic_poll(struct napi_struct *napi, int budget)
{
struct mpnic_napi_vector *nv = container_of(napi,
struct mpnic_napi_vector,
napi);
- int i;
+ int i, j, work_done = 0;
for (i = 0; i < nv->txt_count; i++)
mpnic_clean_tcq(nv, &nv->qt[i], budget);
+ for (j = 0; j < nv->rxt_count; j++, i++)
+ work_done += mpnic_clean_rcq(nv, &nv->qt[i], budget);
+
for (i = 0; i < nv->txt_count; i++)
mpnic_commit_cq_head(&nv->qt[i].cmpl);
- if (likely(napi_complete_done(napi, 0)))
+ if (work_done >= budget)
+ return budget;
+
+ if (likely(napi_complete_done(napi, work_done)))
mpnic_nv_irq_rearm(nv);
- return 0;
+ return work_done;
}
static irqreturn_t mpnic_msix_clean_rings(int __always_unused irq, void *data)
@@ -938,7 +1121,9 @@ static void mpnic_set_rde_cfg(struct mpnic_dev *mpd, struct mpnic_ring *rcq)
MPNIC_RX_MAX_HDR));
}
-static void mpnic_enable_rcq(struct mpnic_dev *mpd, struct mpnic_ring *rcq)
+static void mpnic_enable_rcq(struct mpnic_dev *mpd,
+ struct mpnic_napi_vector *nv,
+ struct mpnic_ring *rcq)
{
u32 log_size = fls(rcq->size_mask);
u32 i = rcq->q_idx;
@@ -954,6 +1139,10 @@ static void mpnic_enable_rcq(struct mpnic_dev *mpd, struct mpnic_ring *rcq)
mpnic_wr64(mpd, MPNIC_RCQ_BASE_ADDR(i), rcq->dma);
mpnic_wr64(mpd, MPNIC_RCQ_SIZE(i), log_size & MPNIC_RCQ_SIZE_SIZE);
+ /* Store interrupt information for the completion queue */
+ mpnic_wr64(mpd, MPNIC_RIM_CTL(i), nv->v_idx);
+ mpnic_wr64(mpd, MPNIC_RIM_INTR_MASK(i), 0);
+
mpnic_wr64(mpd, MPNIC_RCQ_CTL(i), MPNIC_RCQ_CTL_ENABLE);
}
@@ -972,7 +1161,7 @@ void mpnic_enable(struct mpnic_net *mpn)
for (j = 0; j < nv->rxt_count; j++, t++) {
mpnic_enable_bdq(mpd, &nv->qt[t].sub0, &nv->qt[t].sub1);
- mpnic_enable_rcq(mpd, &nv->qt[t].cmpl);
+ mpnic_enable_rcq(mpd, nv, &nv->qt[t].cmpl);
}
}
@@ -1005,6 +1194,8 @@ static void mpnic_disable_bdq(struct mpnic_dev *mpd, struct mpnic_ring *hpq)
static void mpnic_disable_rcq(struct mpnic_dev *mpd, struct mpnic_ring *rcq)
{
mpnic_wr64(mpd, MPNIC_RCQ_CTL(rcq->q_idx), 0);
+ mpnic_wr64(mpd, MPNIC_RIM_INTR_MASK(rcq->q_idx),
+ MPNIC_RIM_INTR_MASK_MASK);
}
void mpnic_disable(struct mpnic_net *mpn)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
index ba118dd2f3de..936ad791a346 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
@@ -50,6 +50,12 @@ struct mpnic_net;
/* Headers longer than this are split off into the payload queue */
#define MPNIC_RX_MAX_HDR 1536
+/* A page is handed out to many packets, each of which takes one reference.
+ * Rather than a locked increment per packet the driver takes a batch of
+ * references up front and returns whatever is left when the page is done.
+ */
+#define MPNIC_PAGECNT_BIAS_MAX (PAGE_SIZE + 1)
+
#define MPNIC_MAX_JUMBO_FRAME_SIZE 9742
/* The page a buffer descriptor queue is currently handing out. Records
@@ -61,15 +67,11 @@ struct mpnic_pg_ctxt {
u32 idx;
};
-struct mpnic_pkt_ctxt {
- struct xdp_buff buff;
- u32 data_truesize;
-};
-
struct mpnic_rcq_state {
- struct mpnic_pkt_ctxt pkt;
+ struct xdp_buff pkt;
struct mpnic_pg_ctxt hdr;
struct mpnic_pg_ctxt payld;
+ bool add_frag_failed;
};
struct mpnic_ring {
--
2.52.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 8/8] eth: mpnic: add basic Rx handling
2026-09-23 1:43 ` [PATCH net-next 8/8] eth: mpnic: add basic Rx handling Daniel Zahka
@ 2026-09-24 2:05 ` netdev-bot+sashiko
2026-09-24 18:38 ` Daniel Zahka
0 siblings, 1 reply; 22+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 2:05 UTC (permalink / raw)
To: daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 5 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] net: mpnic: missing descriptor sequence validation causes NULL
pointer dereference
- [Medium] mpnic_poll() calls mpnic_clean_rcq() even when budget is 0.
- [Medium] When page_pool_dev_alloc_pages() returns NULL,
__mpnic_fill_bdq() stops refilling and returns the partial tail.
- [Medium] mpnic_pkt_prepare() re-initialises state->pkt with
xdp_init_buff()/xdp_prepare_buff() without checking whether a frame is…
- [Medium] mpnic_page_pool_get() indexes ring->rx_buf[idx] with idx =
FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd), an 18-bit device-written field…
- [Medium] The device-supplied completion fields BUFF_OFF (16 bits) and
BUFF_LEN (14 bits) are never checked against page_size(page),…
--
eth: mpnic: add basic Rx handling
This adds Rx completion processing to mpnic. A frame is assembled into an
xdp_buff from a header descriptor, one descriptor per payload page and a
closing metadata descriptor, and then handed to the stack. Page references
are taken in batches per page instead of once per frame.
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 6d2123bee97b4..9878ea5a2f8e1 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> @@ -402,6 +402,34 @@ static void mpnic_fill_qt_bdqs(struct mpnic_q_triad *qt)
> __mpnic_bdq_commit_tail(&qt->sub1, ppq_i);
> }
>
> +/* Take one of the references batched on the page at @idx. If the device
> + * has moved on to a new page, first drop the unused references left on
> + * the previous one.
> + */
> +static struct page *
> +mpnic_page_pool_get(struct mpnic_pg_ctxt *pg_ctxt, struct mpnic_ring *ring,
> + u32 idx)
> +{
> + struct page *page = pg_ctxt->page;
> +
> + if (unlikely(pg_ctxt->idx != idx)) {
> + if (pg_ctxt->pagecnt_bias &&
> + !page_pool_unref_page(page, pg_ctxt->pagecnt_bias))
> + page_pool_put_unrefed_page(page->pp, page, -1, true);
> +
> + page = ring->rx_buf[idx];
> + page_pool_fragment_page(page, MPNIC_PAGECNT_BIAS_MAX);
[Severity: Medium]
Can idx index past the end of ring->rx_buf[] here?
idx comes straight from the completion descriptor:
unsigned int pg_idx = FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd);
MPNIC_RCD_AL_BUFF_ID is an 18-bit field (DESC_GENMASK(33, 16)). rx_buf[]
is sized by hpq_size/ppq_size, which is 256 entries by default.
Both callers mask the value only when they update head:
qt->sub0.head = (pg_idx + 1) & qt->sub0.size_mask;
If the device reports a BUFF_ID at or above the ring size, the pointer
read from past the end of rx_buf[] is written through by
page_pool_fragment_page(). It is then passed to page_address(), used for
the DMA sync and attached to the skb.
A conforming device only echoes IDs the driver posted, so hitting this
needs buggy firmware or an untrusted device. Would it make sense to mask
idx with ring->size_mask before the lookup?
[ ... ]
> @@ -414,9 +442,90 @@ static void mpnic_flush_pg_ctxt(struct mpnic_pg_ctxt *ctxt, bool napi)
> }
> }
>
> -static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
> +static unsigned int mpnic_hdr_pg_start(unsigned int pg_off)
> +{
> + /* The headroom of the first header may be larger than
> + * MPNIC_RX_HROOM due to alignment. So account for that by just
> + * making the page offset 0 if we are starting at the first header.
> + */
> + if (ALIGN(MPNIC_RX_HROOM, 128) > MPNIC_RX_HROOM &&
> + pg_off == ALIGN(MPNIC_RX_HROOM, 128))
> + return 0;
> +
> + return pg_off - MPNIC_RX_HROOM;
> +}
[ ... ]
> +static void
> +mpnic_pkt_prepare(struct mpnic_napi_vector *nv, u64 rcd,
> + struct mpnic_rcq_state *state, struct mpnic_q_triad *qt)
> +{
> + unsigned int pg_off = FIELD_GET(MPNIC_RCD_AL_BUFF_OFF, rcd);
> + unsigned int pg_idx = FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd);
> + unsigned int len = FIELD_GET(MPNIC_RCD_AL_BUFF_LEN, rcd);
> + bool fin = FIELD_GET(MPNIC_RCD_AL_PAGE_FIN, rcd);
> + unsigned int frame_sz, pg_start, pg_end;
> + struct xdp_buff *buff = &state->pkt;
> + struct page *page;
> +
> + pg_start = mpnic_hdr_pg_start(pg_off);
> +
> + page = mpnic_page_pool_get(&state->hdr, &qt->sub0, pg_idx);
> + qt->sub0.head = (pg_idx + 1) & qt->sub0.size_mask;
> +
> + /* Short-cut the end calculation if the page is fully consumed */
> + pg_end = fin ? page_size(page) : mpnic_hdr_pg_end(pg_off, len);
> + frame_sz = pg_end - pg_start;
[Severity: High]
What happens if a PAY_AL or META descriptor reaches this switch without
a header descriptor opening the frame first?
In that case state->pkt.data_hard_start is NULL. Either the previous
META cleared it, or no header has been seen since the queue was
enabled. Nothing on the PAY_AL or META paths checks for this.
For PAY_AL, mpnic_add_rx_frag() first takes a reference through
mpnic_page_pool_get(). It then calls xdp_buff_add_frag(), which finds
skb_shared_info through xdp_get_shared_info_from_buff(), computed from
data_hard_start + frame_sz. With data_hard_start NULL, and frame_sz and
flags left over from the previous frame, the frag is written through a
wild pointer near address zero.
For META, the build path runs unless the error bit or add_frag_failed
is set. add_frag_failed may also be stale from the previous frame.
xdp_build_skb_from_buff() then calls napi_build_skb() with a NULL
data_hard_start. On a queue that has never seen a header, xdp->rxq may
also be unset when rxq->mem.type is read. mpnic_put_pkt_buff() already
copes with a NULL data_hard_start, but it only runs after the build
path has failed.
As with the other descriptor trust issues above, a conforming device
should always open a frame with a header descriptor, so reaching this
needs buggy or untrusted firmware. The result is a host crash rather
than a dropped frame, though.
Would it make sense to check state->pkt.data_hard_start in the PAY_AL
and META cases and drop the descriptor when no frame is being
assembled? On the PAY_AL path that would also mean returning the
reference taken by mpnic_page_pool_get(), the same way the
xdp_buff_add_frag() failure path does.
[Severity: Medium]
Should BUFF_OFF and BUFF_LEN from the completion be range checked before
they are used here?
Neither value is checked against page_size(page), MPNIC_RX_HROOM or
MPNIC_RX_TROOM:
- mpnic_hdr_pg_start() returns pg_off - MPNIC_RX_HROOM as an unsigned
value, so it wraps if pg_off is below the headroom.
- mpnic_hdr_pg_end() can return an end past the page, and
frame_sz = pg_end - pg_start can wrap. After that,
xdp_build_skb_from_buff()->napi_build_skb(data_hard_start, frame_sz)
would place and zero skb_shared_info outside the page.
- In mpnic_add_rx_frag(), truesz underflows when fin is set and pg_off is
larger than the page:
truesz = (fin ? page_size(page) : ALIGN(pg_off + len, 128)) - pg_off;
The frag [pg_off, pg_off + len) is also attached with no check that it
stays inside the page, which would expose adjacent memory through the
skb.
The dma_sync_single_range_for_cpu() calls use the same unchecked ranges.
A conforming device honours the headroom and tailroom programmed in
mpnic_set_rde_cfg() and stays within the page size, so hitting this needs
buggy or untrusted hardware.
> +
> + dma_sync_single_range_for_cpu(nv->dev, page_pool_get_dma_addr(page),
> + pg_start, frame_sz, DMA_FROM_DEVICE);
> +
> + xdp_init_buff(buff, frame_sz, &qt->xdp_rxq);
> + xdp_prepare_buff(buff, page_address(page) + pg_start,
> + pg_off - pg_start, len, true);
[Severity: Medium]
What happens here to a frame that is still being assembled when a new
header descriptor arrives?
mpnic_clean_rcq() calls mpnic_pkt_prepare() for every header descriptor.
It does not check whether state->pkt.data_hard_start is still set by an
earlier frame that never got its META descriptor:
case MPNIC_RCD_TYPE_HDR_AL:
if (FIELD_GET(MPNIC_RCD_HDR_SUBTYPE, rcd) ==
MPNIC_RCD_HDR_SUBTYPE_HDR)
mpnic_pkt_prepare(nv, rcd, state, qt);
In that case xdp_init_buff() clears buff->flags and xdp_prepare_buff()
replaces data_hard_start. The old frame's header page reference from
mpnic_page_pool_get() and its frag references from xdp_buff_add_frag()
can no longer be reached.
The META path and mpnic_flush() cannot release them either, because
mpnic_flush() only calls mpnic_put_pkt_buff() on the current state->pkt.
Could the page pool pages leak here, leaving page_pool_destroy() waiting
forever on inflight pages?
The code does not show whether the hardware can produce HDR, [PAY_AL ...],
HDR without a META in between. fbnic has a
WARN_ON_ONCE(pkt->buff.data_hard_start) for this case. Would a similar
check here, plus releasing the old frame, make sense?
[ ... ]
> @@ -439,23 +548,97 @@ static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
> page_pool_put_full_page(page->pp, page, napi);
> }
>
> +static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
> + struct mpnic_q_triad *qt, int budget)
> +{
[ ... ]
> + while (packets < budget) {
[ ... ]
> + rcq->head = head;
> +
> + /* Allocate buffers, force dma_wmb(), and then start writing tails */
> + mpnic_fill_qt_bdqs(qt);
> +
> + return packets;
> +}
> +
> static int mpnic_poll(struct napi_struct *napi, int budget)
> {
> struct mpnic_napi_vector *nv = container_of(napi,
> struct mpnic_napi_vector,
> napi);
> - int i;
> + int i, j, work_done = 0;
>
> for (i = 0; i < nv->txt_count; i++)
> mpnic_clean_tcq(nv, &nv->qt[i], budget);
>
> + for (j = 0; j < nv->rxt_count; j++, i++)
> + work_done += mpnic_clean_rcq(nv, &nv->qt[i], budget);
[Severity: Medium]
Is it safe to do the Rx side of the poll when budget is 0?
netpoll calls napi->poll(napi, 0) via
netpoll_poll_dev()->poll_napi()->poll_one_napi(), and netconsole can reach
that path from any context. With budget 0 the descriptor loop in
mpnic_clean_rcq() is skipped, but the refill still runs:
mpnic_clean_rcq()
mpnic_fill_qt_bdqs()
__mpnic_fill_bdq()
page_pool_dev_alloc_pages()
The allocation happens whenever mpnic_bdq_desc_unused() is non-zero, for
example after an earlier allocation failure.
Documentation/networking/napi.rst says "Rx specific APIs like page pool or
XDP cannot be used at all when budget is 0".
The page_pool allocation fast path, __page_pool_get_cached(), and
page_pool_refill_alloc_cache() access pool->alloc.cache and the ptr_ring
consumer side without taking a lock.
The pool was created with .napi = &nv->napi. That means
page_pool_napi_local() allows direct recycling into alloc.cache from
softirq on the NAPI's list_owner CPU, and netpoll's poll_owner does not
exclude that.
Could this corrupt alloc.cache or the ring and hand out the same page
twice? Skipping Rx cleaning and refill when budget is 0 would avoid it.
> +
> for (i = 0; i < nv->txt_count; i++)
> mpnic_commit_cq_head(&nv->qt[i].cmpl);
>
> - if (likely(napi_complete_done(napi, 0)))
> + if (work_done >= budget)
> + return budget;
> +
> + if (likely(napi_complete_done(napi, work_done)))
> mpnic_nv_irq_rearm(nv);
[Severity: Medium]
Can Rx stop for good after a run of allocation failures?
When page_pool_dev_alloc_pages() returns NULL, __mpnic_fill_bdq() just
stops:
page = page_pool_dev_alloc_pages(bdq->page_pool);
if (!page)
break;
mpnic_clean_rcq() returns only the packet count. So whenever
work_done < budget, mpnic_poll() completes NAPI and rearms the IRQ, even
if the refill did not succeed.
If allocations keep failing, successive polls can drain HPQ/PPQ until no
buffers are posted. The device then has nothing to DMA into and raises no
Rx interrupts. The only napi_schedule() in the driver is in
mpnic_msix_clean_rings().
Rx would then stay stopped after memory is available again, until a Tx
completion on the same vector triggers a poll or the interface is taken
down and up. Should the poll stay scheduled, or set up a retry, when the
BDQs could not be refilled?
>
> - return 0;
> + return work_done;
> }
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-linux-mpnic-v1-0-236844f53072%40gmail.com
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH net-next 8/8] eth: mpnic: add basic Rx handling
2026-09-24 2:05 ` netdev-bot+sashiko
@ 2026-09-24 18:38 ` Daniel Zahka
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Zahka @ 2026-09-24 18:38 UTC (permalink / raw)
To: netdev-bot+sashiko, daniel.zahka
Cc: alexanderduyck, kuba, kernel-team, andrew+netdev, davem,
edumazet, pabeni, ast, daniel, hawk, john.fastabend, sdf,
dimitri.daskalakis1, mohsin.bashr, linux-kernel, netdev, bpf
On Wed Sep 23, 2026 at 10:05 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 5 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] net: mpnic: missing descriptor sequence validation causes NULL
> pointer dereference
> - [Medium] mpnic_poll() calls mpnic_clean_rcq() even when budget is 0.
> - [Medium] When page_pool_dev_alloc_pages() returns NULL,
> __mpnic_fill_bdq() stops refilling and returns the partial tail.
> - [Medium] mpnic_pkt_prepare() re-initialises state->pkt with
> xdp_init_buff()/xdp_prepare_buff() without checking whether a frame is…
> - [Medium] mpnic_page_pool_get() indexes ring->rx_buf[idx] with idx =
> FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd), an 18-bit device-written field…
> - [Medium] The device-supplied completion fields BUFF_OFF (16 bits) and
> BUFF_LEN (14 bits) are never checked against page_size(page),…
>
> --
>
> eth: mpnic: add basic Rx handling
>
> This adds Rx completion processing to mpnic. A frame is assembled into an
> xdp_buff from a header descriptor, one descriptor per payload page and a
> closing metadata descriptor, and then handed to the stack. Page references
> are taken in batches per page instead of once per frame.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index 6d2123bee97b4..9878ea5a2f8e1 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> @@ -402,6 +402,34 @@ static void mpnic_fill_qt_bdqs(struct mpnic_q_triad *qt)
>> __mpnic_bdq_commit_tail(&qt->sub1, ppq_i);
>> }
>>
>> +/* Take one of the references batched on the page at @idx. If the device
>> + * has moved on to a new page, first drop the unused references left on
>> + * the previous one.
>> + */
>> +static struct page *
>> +mpnic_page_pool_get(struct mpnic_pg_ctxt *pg_ctxt, struct mpnic_ring *ring,
>> + u32 idx)
>> +{
>> + struct page *page = pg_ctxt->page;
>> +
>> + if (unlikely(pg_ctxt->idx != idx)) {
>> + if (pg_ctxt->pagecnt_bias &&
>> + !page_pool_unref_page(page, pg_ctxt->pagecnt_bias))
>> + page_pool_put_unrefed_page(page->pp, page, -1, true);
>> +
>> + page = ring->rx_buf[idx];
>> + page_pool_fragment_page(page, MPNIC_PAGECNT_BIAS_MAX);
>
> [Severity: Medium]
> Can idx index past the end of ring->rx_buf[] here?
>
Like I mentioned about the head field in the tx completion descriptor,
we don't harden against device produced completion fields like this.
> idx comes straight from the completion descriptor:
>
> unsigned int pg_idx = FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd);
>
> MPNIC_RCD_AL_BUFF_ID is an 18-bit field (DESC_GENMASK(33, 16)). rx_buf[]
> is sized by hpq_size/ppq_size, which is 256 entries by default.
>
> Both callers mask the value only when they update head:
>
> qt->sub0.head = (pg_idx + 1) & qt->sub0.size_mask;
>
> If the device reports a BUFF_ID at or above the ring size, the pointer
> read from past the end of rx_buf[] is written through by
> page_pool_fragment_page(). It is then passed to page_address(), used for
> the DMA sync and attached to the skb.
>
> A conforming device only echoes IDs the driver posted, so hitting this
> needs buggy firmware or an untrusted device. Would it make sense to mask
> idx with ring->size_mask before the lookup?
>
> [ ... ]
>
>> @@ -414,9 +442,90 @@ static void mpnic_flush_pg_ctxt(struct mpnic_pg_ctxt *ctxt, bool napi)
>> }
>> }
>>
>> -static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
>> +static unsigned int mpnic_hdr_pg_start(unsigned int pg_off)
>> +{
>> + /* The headroom of the first header may be larger than
>> + * MPNIC_RX_HROOM due to alignment. So account for that by just
>> + * making the page offset 0 if we are starting at the first header.
>> + */
>> + if (ALIGN(MPNIC_RX_HROOM, 128) > MPNIC_RX_HROOM &&
>> + pg_off == ALIGN(MPNIC_RX_HROOM, 128))
>> + return 0;
>> +
>> + return pg_off - MPNIC_RX_HROOM;
>> +}
>
> [ ... ]
>
>> +static void
>> +mpnic_pkt_prepare(struct mpnic_napi_vector *nv, u64 rcd,
>> + struct mpnic_rcq_state *state, struct mpnic_q_triad *qt)
>> +{
>> + unsigned int pg_off = FIELD_GET(MPNIC_RCD_AL_BUFF_OFF, rcd);
>> + unsigned int pg_idx = FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd);
>> + unsigned int len = FIELD_GET(MPNIC_RCD_AL_BUFF_LEN, rcd);
>> + bool fin = FIELD_GET(MPNIC_RCD_AL_PAGE_FIN, rcd);
>> + unsigned int frame_sz, pg_start, pg_end;
>> + struct xdp_buff *buff = &state->pkt;
>> + struct page *page;
>> +
>> + pg_start = mpnic_hdr_pg_start(pg_off);
>> +
>> + page = mpnic_page_pool_get(&state->hdr, &qt->sub0, pg_idx);
>> + qt->sub0.head = (pg_idx + 1) & qt->sub0.size_mask;
>> +
>> + /* Short-cut the end calculation if the page is fully consumed */
>> + pg_end = fin ? page_size(page) : mpnic_hdr_pg_end(pg_off, len);
>> + frame_sz = pg_end - pg_start;
>
> [Severity: High]
> What happens if a PAY_AL or META descriptor reaches this switch without
> a header descriptor opening the frame first?
>
> In that case state->pkt.data_hard_start is NULL. Either the previous
> META cleared it, or no header has been seen since the queue was
> enabled. Nothing on the PAY_AL or META paths checks for this.
>
> For PAY_AL, mpnic_add_rx_frag() first takes a reference through
> mpnic_page_pool_get(). It then calls xdp_buff_add_frag(), which finds
> skb_shared_info through xdp_get_shared_info_from_buff(), computed from
> data_hard_start + frame_sz. With data_hard_start NULL, and frame_sz and
> flags left over from the previous frame, the frag is written through a
> wild pointer near address zero.
>
> For META, the build path runs unless the error bit or add_frag_failed
> is set. add_frag_failed may also be stale from the previous frame.
> xdp_build_skb_from_buff() then calls napi_build_skb() with a NULL
> data_hard_start. On a queue that has never seen a header, xdp->rxq may
> also be unset when rxq->mem.type is read. mpnic_put_pkt_buff() already
> copes with a NULL data_hard_start, but it only runs after the build
> path has failed.
>
> As with the other descriptor trust issues above, a conforming device
> should always open a frame with a header descriptor, so reaching this
> needs buggy or untrusted firmware. The result is a host crash rather
> than a dropped frame, though.
>
> Would it make sense to check state->pkt.data_hard_start in the PAY_AL
> and META cases and drop the descriptor when no frame is being
> assembled? On the PAY_AL path that would also mean returning the
> reference taken by mpnic_page_pool_get(), the same way the
> xdp_buff_add_frag() failure path does.
>
We trust the descriptor stream to be sequenced correctly.
> [Severity: Medium]
> Should BUFF_OFF and BUFF_LEN from the completion be range checked before
> they are used here?
>
Won't fix.
> Neither value is checked against page_size(page), MPNIC_RX_HROOM or
> MPNIC_RX_TROOM:
>
> - mpnic_hdr_pg_start() returns pg_off - MPNIC_RX_HROOM as an unsigned
> value, so it wraps if pg_off is below the headroom.
>
> - mpnic_hdr_pg_end() can return an end past the page, and
> frame_sz = pg_end - pg_start can wrap. After that,
> xdp_build_skb_from_buff()->napi_build_skb(data_hard_start, frame_sz)
> would place and zero skb_shared_info outside the page.
>
> - In mpnic_add_rx_frag(), truesz underflows when fin is set and pg_off is
> larger than the page:
>
> truesz = (fin ? page_size(page) : ALIGN(pg_off + len, 128)) - pg_off;
>
> The frag [pg_off, pg_off + len) is also attached with no check that it
> stays inside the page, which would expose adjacent memory through the
> skb.
>
> The dma_sync_single_range_for_cpu() calls use the same unchecked ranges.
>
> A conforming device honours the headroom and tailroom programmed in
> mpnic_set_rde_cfg() and stays within the page size, so hitting this needs
> buggy or untrusted hardware.
>
>> +
>> + dma_sync_single_range_for_cpu(nv->dev, page_pool_get_dma_addr(page),
>> + pg_start, frame_sz, DMA_FROM_DEVICE);
>> +
>> + xdp_init_buff(buff, frame_sz, &qt->xdp_rxq);
>> + xdp_prepare_buff(buff, page_address(page) + pg_start,
>> + pg_off - pg_start, len, true);
>
> [Severity: Medium]
> What happens here to a frame that is still being assembled when a new
> header descriptor arrives?
>
> mpnic_clean_rcq() calls mpnic_pkt_prepare() for every header descriptor.
> It does not check whether state->pkt.data_hard_start is still set by an
> earlier frame that never got its META descriptor:
>
> case MPNIC_RCD_TYPE_HDR_AL:
> if (FIELD_GET(MPNIC_RCD_HDR_SUBTYPE, rcd) ==
> MPNIC_RCD_HDR_SUBTYPE_HDR)
> mpnic_pkt_prepare(nv, rcd, state, qt);
>
> In that case xdp_init_buff() clears buff->flags and xdp_prepare_buff()
> replaces data_hard_start. The old frame's header page reference from
> mpnic_page_pool_get() and its frag references from xdp_buff_add_frag()
> can no longer be reached.
>
> The META path and mpnic_flush() cannot release them either, because
> mpnic_flush() only calls mpnic_put_pkt_buff() on the current state->pkt.
> Could the page pool pages leak here, leaving page_pool_destroy() waiting
> forever on inflight pages?
>
> The code does not show whether the hardware can produce HDR, [PAY_AL ...],
> HDR without a META in between. fbnic has a
We trust that the hw cannot produce this sequence.
> WARN_ON_ONCE(pkt->buff.data_hard_start) for this case. Would a similar
> check here, plus releasing the old frame, make sense?
>
> [ ... ]
>
>> @@ -439,23 +548,97 @@ static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
>> page_pool_put_full_page(page->pp, page, napi);
>> }
>>
>> +static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
>> + struct mpnic_q_triad *qt, int budget)
>> +{
>
> [ ... ]
>
>> + while (packets < budget) {
>
> [ ... ]
>
>> + rcq->head = head;
>> +
>> + /* Allocate buffers, force dma_wmb(), and then start writing tails */
>> + mpnic_fill_qt_bdqs(qt);
>> +
>> + return packets;
>> +}
>> +
>> static int mpnic_poll(struct napi_struct *napi, int budget)
>> {
>> struct mpnic_napi_vector *nv = container_of(napi,
>> struct mpnic_napi_vector,
>> napi);
>> - int i;
>> + int i, j, work_done = 0;
>>
>> for (i = 0; i < nv->txt_count; i++)
>> mpnic_clean_tcq(nv, &nv->qt[i], budget);
>>
>> + for (j = 0; j < nv->rxt_count; j++, i++)
>> + work_done += mpnic_clean_rcq(nv, &nv->qt[i], budget);
>
> [Severity: Medium]
> Is it safe to do the Rx side of the poll when budget is 0?
>
Will address in v2.
> netpoll calls napi->poll(napi, 0) via
> netpoll_poll_dev()->poll_napi()->poll_one_napi(), and netconsole can reach
> that path from any context. With budget 0 the descriptor loop in
> mpnic_clean_rcq() is skipped, but the refill still runs:
>
> mpnic_clean_rcq()
> mpnic_fill_qt_bdqs()
> __mpnic_fill_bdq()
> page_pool_dev_alloc_pages()
>
> The allocation happens whenever mpnic_bdq_desc_unused() is non-zero, for
> example after an earlier allocation failure.
>
> Documentation/networking/napi.rst says "Rx specific APIs like page pool or
> XDP cannot be used at all when budget is 0".
>
> The page_pool allocation fast path, __page_pool_get_cached(), and
> page_pool_refill_alloc_cache() access pool->alloc.cache and the ptr_ring
> consumer side without taking a lock.
>
> The pool was created with .napi = &nv->napi. That means
> page_pool_napi_local() allows direct recycling into alloc.cache from
> softirq on the NAPI's list_owner CPU, and netpoll's poll_owner does not
> exclude that.
>
> Could this corrupt alloc.cache or the ring and hand out the same page
> twice? Skipping Rx cleaning and refill when budget is 0 would avoid it.
>
>> +
>> for (i = 0; i < nv->txt_count; i++)
>> mpnic_commit_cq_head(&nv->qt[i].cmpl);
>>
>> - if (likely(napi_complete_done(napi, 0)))
>> + if (work_done >= budget)
>> + return budget;
>> +
>> + if (likely(napi_complete_done(napi, work_done)))
>> mpnic_nv_irq_rearm(nv);
>
> [Severity: Medium]
> Can Rx stop for good after a run of allocation failures?
>
> When page_pool_dev_alloc_pages() returns NULL, __mpnic_fill_bdq() just
> stops:
>
> page = page_pool_dev_alloc_pages(bdq->page_pool);
> if (!page)
> break;
>
> mpnic_clean_rcq() returns only the packet count. So whenever
> work_done < budget, mpnic_poll() completes NAPI and rearms the IRQ, even
> if the refill did not succeed.
>
> If allocations keep failing, successive polls can drain HPQ/PPQ until no
> buffers are posted. The device then has nothing to DMA into and raises no
> Rx interrupts. The only napi_schedule() in the driver is in
> mpnic_msix_clean_rings().
>
> Rx would then stay stopped after memory is available again, until a Tx
> completion on the same vector triggers a poll or the interface is taken
> down and up. Should the poll stay scheduled, or set up a retry, when the
> BDQs could not be refilled?
>
This will be addressed in v2. napi depletion check was improperly left
out of this posting.
>>
>> - return 0;
>> + return work_done;
>> }
>
> [ ... ]
^ permalink raw reply [flat|nested] 22+ messages in thread