* [PATCH net-next v10 1/5] net: rnpgbe: Add interrupt handling
[not found] <20260831073608.401988-1-dong100@mucse.com>
@ 2026-08-31 7:36 ` Dong Yibo
2026-09-04 21:54 ` Jakub Kicinski
2026-08-31 7:36 ` [PATCH net-next v10 2/5] net: rnpgbe: Add basic TX packet transmission support Dong Yibo
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Dong Yibo @ 2026-08-31 7:36 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, vadim.fedorenko,
u.kleine-koenig
Cc: netdev, linux-kernel, dong100, yaojun
Add interrupt support for the RNPGBE driver.
Set up and tear down MSI-X/MSI vectors and NAPI. Add mailbox IRQ and
workqueue plumbing to defer firmware mailbox notifications from hard-IRQ
context. Queue mailbox work on system_percpu_wq.
Signed-off-by: Dong Yibo <dong100@mucse.com>
---
drivers/net/ethernet/mucse/rnpgbe/Makefile | 3 +-
drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 55 ++
.../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 4 +
drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 2 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_lib.c | 663 ++++++++++++++++++
.../net/ethernet/mucse/rnpgbe/rnpgbe_lib.h | 34 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_main.c | 52 +-
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 12 +-
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c | 11 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h | 1 +
10 files changed, 828 insertions(+), 9 deletions(-)
create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
index de8bcb7772ab..17574cad392a 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
+++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
@@ -8,4 +8,5 @@ obj-$(CONFIG_MGBE) += rnpgbe.o
rnpgbe-objs := rnpgbe_main.o\
rnpgbe_chip.o\
rnpgbe_mbx.o\
- rnpgbe_mbx_fw.o
+ rnpgbe_mbx_fw.o\
+ rnpgbe_lib.o
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 5b024f9f7e17..77304196c2b6 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -6,6 +6,11 @@
#include <linux/types.h>
#include <linux/mutex.h>
+#include <linux/netdevice.h>
+#include <linux/if.h>
+#include <linux/workqueue.h>
+
+#include "rnpgbe_hw.h"
enum rnpgbe_boards {
board_n500,
@@ -35,21 +40,69 @@ enum {
struct mucse_hw {
void __iomem *hw_addr;
+ void __iomem *ring_msix_base;
struct pci_dev *pdev;
struct mucse_mbx_info mbx;
int port;
u8 pfvfnum;
};
+struct mucse_ring {
+ struct mucse_ring *next;
+ struct mucse_q_vector *q_vector;
+ void __iomem *ring_addr;
+ void __iomem *irq_mask;
+ void __iomem *trig;
+ u8 queue_index;
+ /* hw ring idx */
+ u8 rnpgbe_queue_idx;
+} ____cacheline_internodealigned_in_smp;
+
+struct mucse_ring_container {
+ struct mucse_ring *ring;
+ u16 count;
+};
+
+struct mucse_q_vector {
+ struct mucse *mucse;
+ /* hardware interrupt vector number */
+ int hw_vector;
+ struct mucse_ring_container rx, tx;
+ struct napi_struct napi;
+ char name[IFNAMSIZ + 18];
+ /* for dynamic allocation of rings associated with this q_vector */
+ struct mucse_ring ring[] ____cacheline_internodealigned_in_smp;
+};
+
struct mucse_stats {
u64 tx_dropped;
};
+#define MAX_Q_VECTORS 8
+
+enum mucse_state_t {
+ __MUCSE_DOWN,
+};
+
struct mucse {
struct net_device *netdev;
struct pci_dev *pdev;
struct mucse_hw hw;
struct mucse_stats stats;
+#define M_FLAG_MSIX_SINGLE_EN BIT(0)
+#define M_FLAG_MSIX_EN BIT(1)
+ u32 flags;
+ struct mucse_ring *tx_ring[RNPGBE_MAX_QUEUES]
+ ____cacheline_aligned_in_smp;
+ struct mucse_ring *rx_ring[RNPGBE_MAX_QUEUES]
+ ____cacheline_aligned_in_smp;
+ struct mucse_q_vector *q_vector[MAX_Q_VECTORS];
+ int num_tx_queues;
+ int num_q_vectors;
+ int num_rx_queues;
+ char mbx_name[32];
+ unsigned long state;
+ struct work_struct mbx_work;
};
int rnpgbe_get_permanent_mac(struct mucse_hw *hw, u8 *perm_addr);
@@ -68,4 +121,6 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type);
#define mucse_hw_wr32(hw, reg, val) \
writel((val), (hw)->hw_addr + (reg))
+#define mucse_hw_rd32(hw, reg) \
+ readl((hw)->hw_addr + (reg))
#endif /* _RNPGBE_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
index ebc7b3750157..921cc325a991 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
@@ -89,6 +89,8 @@ static void rnpgbe_init_n500(struct mucse_hw *hw)
{
struct mucse_mbx_info *mbx = &hw->mbx;
+ hw->ring_msix_base = hw->hw_addr + MUCSE_N500_RING_MSIX_BASE;
+
mbx->fwpf_ctrl_base = MUCSE_N500_FWPF_CTRL_BASE;
mbx->fwpf_shm_base = MUCSE_N500_FWPF_SHM_BASE;
}
@@ -104,6 +106,8 @@ static void rnpgbe_init_n210(struct mucse_hw *hw)
{
struct mucse_mbx_info *mbx = &hw->mbx;
+ hw->ring_msix_base = hw->hw_addr + MUCSE_N210_RING_MSIX_BASE;
+
mbx->fwpf_ctrl_base = MUCSE_N210_FWPF_CTRL_BASE;
mbx->fwpf_shm_base = MUCSE_N210_FWPF_SHM_BASE;
}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
index e77e6bc3d3e3..0dce78e4a91b 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
@@ -6,10 +6,12 @@
#define MUCSE_N500_FWPF_CTRL_BASE 0x28b00
#define MUCSE_N500_FWPF_SHM_BASE 0x2d000
+#define MUCSE_N500_RING_MSIX_BASE 0x28700
#define MUCSE_GBE_PFFW_MBX_CTRL_OFFSET 0x5500
#define MUCSE_GBE_FWPF_MBX_MASK_OFFSET 0x5700
#define MUCSE_N210_FWPF_CTRL_BASE 0x29400
#define MUCSE_N210_FWPF_SHM_BASE 0x2d900
+#define MUCSE_N210_RING_MSIX_BASE 0x29000
#define RNPGBE_DMA_AXI_EN 0x0010
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
new file mode 100644
index 000000000000..f511a3a5f916
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
@@ -0,0 +1,663 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#include <linux/pci.h>
+#include <linux/netdevice.h>
+
+#include "rnpgbe_lib.h"
+#include "rnpgbe.h"
+#include "rnpgbe_mbx_fw.h"
+
+static void rnpgbe_mbx_work(struct work_struct *work)
+{
+ struct mucse *mucse = container_of(work, struct mucse, mbx_work);
+
+ mucse_fw_irq_handler(&mucse->hw);
+}
+
+/**
+ * rnpgbe_msix_other - Other irq handler
+ * @irq: interrupt number
+ * @data: private data
+ *
+ * Return: IRQ_HANDLED
+ **/
+static irqreturn_t rnpgbe_msix_other(int irq, void *data)
+{
+ struct mucse *mucse = (struct mucse *)data;
+
+ queue_work(system_percpu_wq, &mucse->mbx_work);
+
+ return IRQ_HANDLED;
+}
+
+static void rnpgbe_irq_disable_queues(struct mucse_q_vector *q_vector)
+{
+ struct mucse_ring *ring;
+
+ /*
+ * TX/RX pairs share interrupt control registers; update them through
+ * the TX ring list.
+ */
+ mucse_for_each_ring(ring, q_vector->tx) {
+ writel(INT_VALID, ring->trig);
+ writel((RX_INT_MASK | TX_INT_MASK), ring->irq_mask);
+ }
+
+ /* flush posted writes to ensure hardware sees the mask */
+ if (q_vector->tx.ring)
+ readl(q_vector->tx.ring->irq_mask);
+}
+
+/**
+ * rnpgbe_int_single - MSI-X single-vector/MSI interrupt handler
+ * @irq: interrupt number
+ * @data: private data
+ *
+ * Return: IRQ_HANDLED
+ **/
+static irqreturn_t rnpgbe_int_single(int irq, void *data)
+{
+ struct mucse *mucse = (struct mucse *)data;
+ struct mucse_q_vector *q_vector;
+
+ queue_work(system_percpu_wq, &mucse->mbx_work);
+
+ if (test_bit(__MUCSE_DOWN, &mucse->state))
+ return IRQ_HANDLED;
+
+ q_vector = mucse->q_vector[0];
+ rnpgbe_irq_disable_queues(q_vector);
+ napi_schedule_irqoff(&q_vector->napi);
+
+ return IRQ_HANDLED;
+}
+
+static void rnpgbe_irq_enable_queues(struct mucse_q_vector *q_vector)
+{
+ struct mucse_ring *ring;
+
+ /*
+ * TX/RX pairs share interrupt control registers; update them through
+ * the TX ring list.
+ */
+ mucse_for_each_ring(ring, q_vector->tx) {
+ writel(0, ring->irq_mask);
+
+ /* Re-trigger hw to re-check events lost while masked. */
+ writel(INT_VALID | TX_INT_MASK | RX_INT_MASK, ring->trig);
+ }
+}
+
+/**
+ * rnpgbe_poll - NAPI polling callback
+ * @napi: structure for representing this polling device
+ * @budget: polling budget
+ *
+ * Complete NAPI polling and re-enable queue interrupts. Ring cleaning is
+ * added when TX and RX support is enabled.
+ *
+ * Return: 0
+ **/
+static int rnpgbe_poll(struct napi_struct *napi, int budget)
+{
+ struct mucse_q_vector *q_vector =
+ container_of(napi, struct mucse_q_vector, napi);
+ int work_done = 0;
+
+ /* Exit if we are called by netpoll */
+ if (unlikely(!budget))
+ return 0;
+
+ if (likely(napi_complete_done(napi, work_done)))
+ rnpgbe_irq_enable_queues(q_vector);
+
+ return work_done;
+}
+
+/**
+ * rnpgbe_request_mbx_irq - Register mbx routine
+ * @mucse: pointer to private structure
+ *
+ * In MSIX mode, register a dedicated handler for vector 0 (mailbox)
+ * In MSI/MSI-X_SINGLE mode, mailbox is multiplexed through
+ * data tx/rx handler.
+ *
+ * Return: 0 on success, negative on failure
+ **/
+int rnpgbe_request_mbx_irq(struct mucse *mucse)
+{
+ struct pci_dev *pdev = mucse->pdev;
+ int err = 0;
+
+ snprintf(mucse->mbx_name, sizeof(mucse->mbx_name),
+ "rnpgbe-mbx:%s", pci_name(pdev));
+ INIT_WORK(&mucse->mbx_work, rnpgbe_mbx_work);
+
+ if (mucse->flags & M_FLAG_MSIX_EN) {
+ err = request_irq(pci_irq_vector(pdev, 0),
+ rnpgbe_msix_other, 0, mucse->mbx_name,
+ mucse);
+ } else {
+ err = request_irq(pci_irq_vector(pdev, 0),
+ rnpgbe_int_single, 0, mucse->mbx_name,
+ mucse);
+ }
+
+ return err;
+}
+
+/**
+ * rnpgbe_free_mbx_irq - Remove mbx routine
+ * @mucse: pointer to private structure
+ **/
+void rnpgbe_free_mbx_irq(struct mucse *mucse)
+{
+ struct pci_dev *pdev = mucse->pdev;
+
+ free_irq(pci_irq_vector(pdev, 0), mucse);
+ cancel_work_sync(&mucse->mbx_work);
+}
+
+/**
+ * rnpgbe_set_num_queues - Allocate queues for device, feature dependent
+ * @mucse: pointer to private structure
+ *
+ * Determine tx/rx queue counts
+ **/
+static void rnpgbe_set_num_queues(struct mucse *mucse)
+{
+ /* start from 1 queue */
+ mucse->num_tx_queues = 1;
+ mucse->num_rx_queues = 1;
+}
+
+/**
+ * rnpgbe_set_interrupt_capability - Set MSI-X or MSI if supported
+ * @mucse: pointer to private structure
+ *
+ * Attempt to configure the interrupts using the best available
+ * capabilities of the hardware.
+ *
+ * Return: 0 on success, negative on failure
+ **/
+static int rnpgbe_set_interrupt_capability(struct mucse *mucse)
+{
+ int v_budget;
+
+ v_budget = min3(mucse->num_tx_queues, mucse->num_rx_queues,
+ MAX_Q_VECTORS);
+ v_budget = min_t(int, v_budget, num_online_cpus());
+ /* add one vector for mbx */
+ v_budget += 1;
+
+ /* Hardware limitation: only 1 MSI vector is supported even
+ * if multiple messages are requested. MSI mode falls back
+ * to single vector automatically.
+ */
+ v_budget = pci_alloc_irq_vectors(mucse->pdev, 1, v_budget,
+ PCI_IRQ_MSI | PCI_IRQ_MSIX);
+ if (v_budget < 0)
+ return v_budget;
+
+ if (mucse->pdev->msix_enabled) {
+ /* q_vector not include mbx */
+ if (v_budget > 1) {
+ mucse->flags |= M_FLAG_MSIX_EN;
+ mucse->num_q_vectors = v_budget - 1;
+ } else {
+ mucse->flags |= M_FLAG_MSIX_SINGLE_EN;
+ mucse->num_q_vectors = 1;
+ }
+ } else {
+ /* Hardware supports only one MSI interrupt. */
+ mucse->num_q_vectors = 1;
+ }
+
+ return 0;
+}
+
+/**
+ * mucse_add_ring - Add ring to ring container
+ * @ring: ring to be added
+ * @head: ring container
+ **/
+static void mucse_add_ring(struct mucse_ring *ring,
+ struct mucse_ring_container *head)
+{
+ ring->next = head->ring;
+ head->ring = ring;
+ head->count++;
+}
+
+/**
+ * rnpgbe_alloc_q_vector - Allocate memory for a single interrupt vector
+ * @mucse: pointer to private structure
+ * @eth_queue_idx: queue_index idx for this q_vector
+ * @vector_idx: q_vector array index
+ * @r_idx: starting hardware ring index
+ * @r_count: number of TX/RX ring pairs
+ * @step: ring step
+ *
+ * Return: 0 on success. If allocation fails we return -ENOMEM.
+ **/
+static int rnpgbe_alloc_q_vector(struct mucse *mucse,
+ int eth_queue_idx, int vector_idx, int r_idx,
+ int r_count, int step)
+{
+ int rxr_idx = r_idx, txr_idx = r_idx;
+ struct mucse_hw *hw = &mucse->hw;
+ struct mucse_q_vector *q_vector;
+ int txr_count, rxr_count, idx;
+ struct mucse_ring *ring;
+ int ring_count;
+
+ /*
+ * TX and RX rings are always allocated as pairs with the same hardware
+ * ring index. Interrupt control is shared by the pair, so the TX ring
+ * list is used to update the common registers.
+ */
+ txr_count = r_count;
+ rxr_count = r_count;
+ ring_count = txr_count + rxr_count;
+
+ q_vector = kzalloc_flex(*q_vector, ring, ring_count);
+ if (!q_vector)
+ return -ENOMEM;
+
+ netif_napi_add(mucse->netdev, &q_vector->napi, rnpgbe_poll);
+ /* tie q_vector and mucse together */
+ mucse->q_vector[vector_idx] = q_vector;
+ q_vector->mucse = mucse;
+ q_vector->hw_vector = vector_idx;
+ /* if mbx use separate irq, we should add 1 */
+ if (mucse->flags & M_FLAG_MSIX_EN)
+ q_vector->hw_vector++;
+
+ ring = q_vector->ring;
+
+ for (idx = 0; idx < txr_count; idx++) {
+ mucse_add_ring(ring, &q_vector->tx);
+ ring->queue_index = eth_queue_idx + idx;
+ ring->rnpgbe_queue_idx = txr_idx;
+ ring->ring_addr = hw->hw_addr + RING_OFFSET(txr_idx);
+ ring->irq_mask = ring->ring_addr + RNPGBE_DMA_INT_MASK;
+ ring->trig = ring->ring_addr + RNPGBE_DMA_INT_TRIG;
+ ring->q_vector = q_vector;
+ mucse->tx_ring[ring->queue_index] = ring;
+ txr_idx += step;
+ ring++;
+ }
+
+ for (idx = 0; idx < rxr_count; idx++) {
+ mucse_add_ring(ring, &q_vector->rx);
+ ring->queue_index = eth_queue_idx + idx;
+ ring->rnpgbe_queue_idx = rxr_idx;
+ ring->ring_addr = hw->hw_addr + RING_OFFSET(rxr_idx);
+ ring->irq_mask = ring->ring_addr + RNPGBE_DMA_INT_MASK;
+ ring->trig = ring->ring_addr + RNPGBE_DMA_INT_TRIG;
+ ring->q_vector = q_vector;
+ mucse->rx_ring[ring->queue_index] = ring;
+ rxr_idx += step;
+ ring++;
+ }
+
+ return 0;
+}
+
+/**
+ * rnpgbe_free_q_vector - Free memory allocated for specific interrupt vector
+ * @mucse: pointer to private structure
+ * @vector_idx: q_vector array index
+ *
+ * This function frees the memory allocated to the q_vector. In addition if
+ * NAPI is enabled it will delete any references to the NAPI struct prior
+ * to freeing the q_vector.
+ **/
+static void rnpgbe_free_q_vector(struct mucse *mucse, int vector_idx)
+{
+ struct mucse_q_vector *q_vector = mucse->q_vector[vector_idx];
+ struct mucse_ring *ring;
+
+ mucse_for_each_ring(ring, q_vector->tx)
+ mucse->tx_ring[ring->queue_index] = NULL;
+ mucse_for_each_ring(ring, q_vector->rx)
+ mucse->rx_ring[ring->queue_index] = NULL;
+ mucse->q_vector[vector_idx] = NULL;
+ netif_napi_del(&q_vector->napi);
+ kfree(q_vector);
+}
+
+/**
+ * rnpgbe_alloc_q_vectors - Allocate memory for interrupt vectors
+ * @mucse: pointer to private structure
+ *
+ * Return: 0 on success, or -ENOMEM on allocation failure.
+ **/
+static int rnpgbe_alloc_q_vectors(struct mucse *mucse)
+{
+ int err, ring_cnt, v_remaining = mucse->num_q_vectors;
+ int r_remaining = min_t(int, mucse->num_tx_queues,
+ mucse->num_rx_queues);
+ int num_q_vectors = 0;
+ int eth_queue_idx = 0;
+ int vector_idx = 0;
+ int ring_step = 1;
+ int ring_idx = 0;
+
+ for (; r_remaining > 0 && v_remaining > 0; v_remaining--) {
+ ring_cnt = DIV_ROUND_UP(r_remaining, v_remaining);
+ err = rnpgbe_alloc_q_vector(mucse, eth_queue_idx,
+ vector_idx, ring_idx, ring_cnt,
+ ring_step);
+ if (err)
+ goto err_free_q_vector;
+ ring_idx += ring_step * ring_cnt;
+ eth_queue_idx += ring_cnt;
+ r_remaining -= ring_cnt;
+ num_q_vectors++;
+ vector_idx++;
+ }
+ /* Record the number of queue vectors actually allocated */
+ mucse->num_q_vectors = num_q_vectors;
+ mucse->num_tx_queues = eth_queue_idx;
+ mucse->num_rx_queues = eth_queue_idx;
+
+ return 0;
+
+err_free_q_vector:
+ mucse->num_tx_queues = 0;
+ mucse->num_rx_queues = 0;
+ mucse->num_q_vectors = 0;
+
+ while (vector_idx--)
+ rnpgbe_free_q_vector(mucse, vector_idx);
+
+ return err;
+}
+
+/**
+ * rnpgbe_reset_interrupt_capability - Reset irq capability setup
+ * @mucse: pointer to private structure
+ **/
+static void rnpgbe_reset_interrupt_capability(struct mucse *mucse)
+{
+ pci_free_irq_vectors(mucse->pdev);
+ mucse->flags &= ~(M_FLAG_MSIX_EN |
+ M_FLAG_MSIX_SINGLE_EN);
+}
+
+/**
+ * rnpgbe_init_interrupt_scheme - Determine proper interrupt scheme
+ * @mucse: pointer to private structure
+ *
+ * We determine which interrupt scheme to use based on...
+ * - Hardware queue count
+ * - cpu count
+ * - interrupt mode (MSI and single-vector MSI-X use one vector)
+ *
+ * Return: 0 on success, negative on failure
+ **/
+int rnpgbe_init_interrupt_scheme(struct mucse *mucse)
+{
+ int err;
+
+ rnpgbe_set_num_queues(mucse);
+
+ err = rnpgbe_set_interrupt_capability(mucse);
+ if (err)
+ return err;
+
+ err = rnpgbe_alloc_q_vectors(mucse);
+ if (err) {
+ rnpgbe_reset_interrupt_capability(mucse);
+ return err;
+ }
+
+ return 0;
+}
+
+/**
+ * rnpgbe_free_q_vectors - Free memory allocated for interrupt vectors
+ * @mucse: pointer to private structure
+ *
+ * This function frees the memory allocated to the q_vectors. In addition if
+ * NAPI is enabled it will delete any references to the NAPI struct prior
+ * to freeing the q_vector.
+ **/
+static void rnpgbe_free_q_vectors(struct mucse *mucse)
+{
+ int vector_idx = mucse->num_q_vectors;
+
+ mucse->num_rx_queues = 0;
+ mucse->num_tx_queues = 0;
+ mucse->num_q_vectors = 0;
+
+ while (vector_idx--)
+ rnpgbe_free_q_vector(mucse, vector_idx);
+}
+
+/**
+ * rnpgbe_clear_interrupt_scheme - Clear the current interrupt scheme settings
+ * @mucse: pointer to private structure
+ *
+ * Clear interrupt specific resources and reset the structure
+ **/
+void rnpgbe_clear_interrupt_scheme(struct mucse *mucse)
+{
+ mucse->num_tx_queues = 0;
+ mucse->num_rx_queues = 0;
+ rnpgbe_free_q_vectors(mucse);
+ rnpgbe_reset_interrupt_capability(mucse);
+}
+
+/**
+ * rnpgbe_msix_clean_rings - MSI-x interrupt handler for ring irq
+ * @irq: interrupt number
+ * @data: private data
+ *
+ * rnpgbe_msix_clean_rings handle irq from ring, start napi
+ * Return: IRQ_HANDLED
+ **/
+static irqreturn_t rnpgbe_msix_clean_rings(int irq, void *data)
+{
+ struct mucse_q_vector *q_vector = (struct mucse_q_vector *)data;
+
+ rnpgbe_irq_disable_queues(q_vector);
+ napi_schedule_irqoff(&q_vector->napi);
+
+ return IRQ_HANDLED;
+}
+
+/**
+ * rnpgbe_request_irq - Initialize interrupts
+ * @mucse: pointer to private structure
+ *
+ * Attempts to configure interrupts using the best available
+ * capabilities of the hardware and kernel.
+ *
+ * Return: 0 on success, negative value on failure
+ **/
+int rnpgbe_request_irq(struct mucse *mucse)
+{
+ struct net_device *netdev = mucse->netdev;
+ struct pci_dev *pdev = mucse->pdev;
+ struct mucse_q_vector *q_vector;
+ int err, i;
+
+ if (mucse->flags & M_FLAG_MSIX_EN) {
+ for (i = 0; i < mucse->num_q_vectors; i++) {
+ q_vector = mucse->q_vector[i];
+
+ snprintf(q_vector->name, sizeof(q_vector->name),
+ "%s-%s-%d", netdev->name, "TxRx", i);
+
+ err = request_irq(pci_irq_vector(pdev, i + 1),
+ rnpgbe_msix_clean_rings, 0,
+ q_vector->name,
+ q_vector);
+ if (err) {
+ dev_err(&pdev->dev, "MSI-X req err %d: %d\n",
+ i + 1, err);
+ goto err_free_irqs;
+ }
+ }
+ }
+
+ return 0;
+err_free_irqs:
+ while (i--) {
+ q_vector = mucse->q_vector[i];
+ synchronize_irq(pci_irq_vector(pdev, i + 1));
+ free_irq(pci_irq_vector(pdev, i + 1), q_vector);
+ }
+
+ return err;
+}
+
+/**
+ * rnpgbe_free_irq - Free interrupts
+ * @mucse: pointer to private structure
+ *
+ * Attempts to free interrupts according initialized type.
+ **/
+void rnpgbe_free_irq(struct mucse *mucse)
+{
+ struct pci_dev *pdev = mucse->pdev;
+ struct mucse_q_vector *q_vector;
+
+ if (mucse->flags & M_FLAG_MSIX_EN) {
+ for (int i = 0; i < mucse->num_q_vectors; i++) {
+ q_vector = mucse->q_vector[i];
+ if (!q_vector)
+ continue;
+
+ free_irq(pci_irq_vector(pdev, i + 1), q_vector);
+ }
+ }
+}
+
+/**
+ * rnpgbe_set_ring_vector - Set the ring_vector registers,
+ * mapping interrupt causes to vectors
+ * @mucse: pointer to private structure
+ * @queue: queue to map the corresponding interrupt to
+ * @vector: the vector num to map to the corresponding queue
+ *
+ */
+static void rnpgbe_set_ring_vector(struct mucse *mucse,
+ u8 queue, u8 vector)
+{
+ struct mucse_hw *hw = &mucse->hw;
+ u32 data;
+
+ data = hw->pfvfnum << 24;
+ data |= (vector << 8);
+ data |= vector;
+ writel(data, hw->ring_msix_base + RING_VECTOR(queue));
+}
+
+/**
+ * rnpgbe_configure_msi - Configure MSI hardware
+ * @mucse: pointer to private structure
+ *
+ * rnpgbe_configure_msi sets up the hardware to properly generate MSI
+ * interrupts.
+ **/
+static void rnpgbe_configure_msi(struct mucse *mucse)
+{
+ struct mucse_q_vector *q_vector = mucse->q_vector[0];
+ struct mucse_ring *ring;
+
+ /* TX/RX pairs share vector registers. Update them through TX. */
+ mucse_for_each_ring(ring, q_vector->tx)
+ rnpgbe_set_ring_vector(mucse, ring->rnpgbe_queue_idx, 0);
+}
+
+/**
+ * rnpgbe_configure_msix - Configure MSI-X hardware
+ * @mucse: pointer to private structure
+ *
+ * rnpgbe_configure_msix sets up the hardware to properly generate MSI-X
+ * interrupts.
+ **/
+static void rnpgbe_configure_msix(struct mucse *mucse)
+{
+ struct mucse_q_vector *q_vector;
+
+ for (int i = 0; i < mucse->num_q_vectors; i++) {
+ struct mucse_ring *ring;
+
+ q_vector = mucse->q_vector[i];
+ /* TX/RX pairs share vector registers.
+ * Update them through TX.
+ */
+ mucse_for_each_ring(ring, q_vector->tx) {
+ rnpgbe_set_ring_vector(mucse, ring->rnpgbe_queue_idx,
+ q_vector->hw_vector);
+ }
+ }
+}
+
+static void rnpgbe_irq_enable(struct mucse *mucse)
+{
+ for (int i = 0; i < mucse->num_q_vectors; i++)
+ rnpgbe_irq_enable_queues(mucse->q_vector[i]);
+}
+
+/**
+ * rnpgbe_irq_disable - Mask off interrupt generation on the NIC
+ * @mucse: board private structure
+ **/
+void rnpgbe_irq_disable(struct mucse *mucse)
+{
+ struct pci_dev *pdev = mucse->pdev;
+
+ if (mucse->flags & M_FLAG_MSIX_EN) {
+ for (int i = 0; i < mucse->num_q_vectors; i++) {
+ rnpgbe_irq_disable_queues(mucse->q_vector[i]);
+ synchronize_irq(pci_irq_vector(pdev, i + 1));
+ }
+ } else {
+ rnpgbe_irq_disable_queues(mucse->q_vector[0]);
+ synchronize_irq(pci_irq_vector(pdev, 0));
+ }
+}
+
+static void rnpgbe_napi_enable_all(struct mucse *mucse)
+{
+ for (int i = 0; i < mucse->num_q_vectors; i++)
+ napi_enable(&mucse->q_vector[i]->napi);
+}
+
+static void rnpgbe_napi_disable_all(struct mucse *mucse)
+{
+ for (int i = 0; i < mucse->num_q_vectors; i++)
+ napi_disable(&mucse->q_vector[i]->napi);
+}
+
+bool rnpgbe_down(struct mucse *mucse)
+{
+ if (test_and_set_bit(__MUCSE_DOWN, &mucse->state))
+ return false;
+
+ rnpgbe_napi_disable_all(mucse);
+ rnpgbe_irq_disable(mucse);
+
+ return true;
+}
+
+/**
+ * rnpgbe_up_complete - Final step for port up
+ * @mucse: pointer to private structure
+ **/
+void rnpgbe_up_complete(struct mucse *mucse)
+{
+ if (mucse->flags & (M_FLAG_MSIX_EN | M_FLAG_MSIX_SINGLE_EN))
+ rnpgbe_configure_msix(mucse);
+ else
+ rnpgbe_configure_msi(mucse);
+ rnpgbe_napi_enable_all(mucse);
+ clear_bit(__MUCSE_DOWN, &mucse->state);
+ rnpgbe_irq_enable(mucse);
+}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
new file mode 100644
index 000000000000..c03f7aad2c08
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#ifndef _RNPGBE_LIB_H
+#define _RNPGBE_LIB_H
+
+struct mucse;
+struct mucse_hw;
+
+#define RING_OFFSET(n) (0x1000 + 0x100 * (n))
+#define RNPGBE_DMA_INT_MASK 0x24
+#define TX_INT_MASK BIT(1)
+#define RX_INT_MASK BIT(0)
+#define INT_VALID (BIT(16) | BIT(17))
+#define RNPGBE_DMA_INT_TRIG 0x2c /* lost-interrupt recovery trigger */
+/* | 31:24 | .... | 15:8 | 7:0 | */
+/* | pfvfnum | | tx vector | rx vector | */
+#define RING_VECTOR(n) (0x04 * (n))
+
+#define mucse_for_each_ring(pos, head)\
+ for (typeof((head).ring) __pos = (head).ring;\
+ __pos ? ({ pos = __pos; 1; }) : 0;\
+ __pos = __pos->next)
+
+int rnpgbe_init_interrupt_scheme(struct mucse *mucse);
+void rnpgbe_clear_interrupt_scheme(struct mucse *mucse);
+int rnpgbe_request_mbx_irq(struct mucse *mucse);
+void rnpgbe_free_mbx_irq(struct mucse *mucse);
+int rnpgbe_request_irq(struct mucse *mucse);
+void rnpgbe_free_irq(struct mucse *mucse);
+void rnpgbe_irq_disable(struct mucse *mucse);
+bool rnpgbe_down(struct mucse *mucse);
+void rnpgbe_up_complete(struct mucse *mucse);
+#endif
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
index 70a2b0082ba8..b7472dba2a19 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
@@ -7,6 +7,7 @@
#include "rnpgbe.h"
#include "rnpgbe_hw.h"
+#include "rnpgbe_lib.h"
#include "rnpgbe_mbx_fw.h"
static const char rnpgbe_driver_name[] = "rnpgbe";
@@ -32,11 +33,28 @@ static struct pci_device_id rnpgbe_pci_tbl[] = {
* The open entry point is called when a network interface is made
* active by the system (IFF_UP).
*
- * Return: 0
+ * Return: 0 on success, negative value on failure
**/
static int rnpgbe_open(struct net_device *netdev)
{
+ struct mucse *mucse = netdev_priv(netdev);
+ int err;
+
+ err = rnpgbe_request_irq(mucse);
+ if (err)
+ return err;
+
+ err = netif_set_real_num_queues(netdev, mucse->num_tx_queues,
+ mucse->num_rx_queues);
+ if (err)
+ goto err_free_irqs;
+
+ rnpgbe_up_complete(mucse);
+
return 0;
+err_free_irqs:
+ rnpgbe_free_irq(mucse);
+ return err;
}
/**
@@ -50,6 +68,13 @@ static int rnpgbe_open(struct net_device *netdev)
**/
static int rnpgbe_close(struct net_device *netdev)
{
+ struct mucse *mucse = netdev_priv(netdev);
+
+ if (!rnpgbe_down(mucse))
+ return 0;
+
+ rnpgbe_free_irq(mucse);
+
return 0;
}
@@ -106,6 +131,7 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
mucse = netdev_priv(netdev);
mucse->netdev = netdev;
mucse->pdev = pdev;
+ set_bit(__MUCSE_DOWN, &mucse->state);
pci_set_drvdata(pdev, mucse);
hw = &mucse->hw;
@@ -166,11 +192,28 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
goto err_powerdown;
}
+ err = rnpgbe_init_interrupt_scheme(mucse);
+ if (err) {
+ dev_err(&pdev->dev, "init interrupt failed %d\n", err);
+ goto err_powerdown;
+ }
+
+ err = rnpgbe_request_mbx_irq(mucse);
+ if (err) {
+ dev_err(&pdev->dev, "register mbx irq failed %d\n", err);
+ goto err_clear_interrupt;
+ }
+
err = register_netdev(netdev);
if (err)
- goto err_powerdown;
+ goto err_remove_mbx;
return 0;
+
+err_remove_mbx:
+ rnpgbe_free_mbx_irq(mucse);
+err_clear_interrupt:
+ rnpgbe_clear_interrupt_scheme(mucse);
err_powerdown:
/* notify powerdown only powerup ok */
if (!err_notify) {
@@ -253,9 +296,11 @@ static void rnpgbe_rm_adapter(struct pci_dev *pdev)
return;
netdev = mucse->netdev;
unregister_netdev(netdev);
+ rnpgbe_free_mbx_irq(mucse);
err = rnpgbe_send_notify(hw, false, mucse_fw_powerup);
if (err)
dev_warn(&pdev->dev, "Send powerdown to hw failed %d\n", err);
+ rnpgbe_clear_interrupt_scheme(mucse);
free_netdev(netdev);
}
@@ -289,6 +334,9 @@ static void rnpgbe_dev_shutdown(struct pci_dev *pdev)
if (netif_running(netdev))
rnpgbe_close(netdev);
rtnl_unlock();
+
+ rnpgbe_free_mbx_irq(mucse);
+ rnpgbe_clear_interrupt_scheme(mucse);
pci_disable_device(pdev);
}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
index c46408698263..2310f01df1f5 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
@@ -84,7 +84,7 @@ static u32 mucse_mbx_get_lock_pf(struct mucse_hw *hw)
* @hw: pointer to the HW structure
*
* Pair with mucse_release_mbx_lock_pf()
- * This function maybe used in an irq handler.
+ * All mailbox access runs in process context.
*
* Return: 0 on success, negative errno on failure
**/
@@ -93,11 +93,11 @@ static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw)
struct mucse_mbx_info *mbx = &hw->mbx;
u32 val;
- return read_poll_timeout_atomic(mucse_mbx_get_lock_pf,
- val, val & MUCSE_MBX_PFU,
- mbx->delay_us,
- mbx->timeout_us,
- false, hw);
+ return read_poll_timeout(mucse_mbx_get_lock_pf,
+ val, val & MUCSE_MBX_PFU,
+ mbx->delay_us,
+ mbx->timeout_us,
+ false, hw);
}
/**
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
index 5ba74997beac..e28f3193aa85 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
@@ -197,3 +197,14 @@ int mucse_mbx_get_macaddr(struct mucse_hw *hw, int pfvfnum,
return 0;
}
+
+/**
+ * mucse_fw_irq_handler - Deferred firmware mailbox notification hook
+ * @hw: pointer to the HW structure
+ *
+ * No asynchronous firmware events are enabled until link status support is
+ * added.
+ **/
+void mucse_fw_irq_handler(struct mucse_hw *hw)
+{
+}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
index fe996aeffc4d..a6bf5de55aa3 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
@@ -99,4 +99,5 @@ int mucse_mbx_powerup(struct mucse_hw *hw, bool is_powerup);
int mucse_mbx_reset_hw(struct mucse_hw *hw);
int mucse_mbx_get_macaddr(struct mucse_hw *hw, int pfvfnum,
u8 *mac_addr, int port);
+void mucse_fw_irq_handler(struct mucse_hw *hw);
#endif /* _RNPGBE_MBX_FW_H */
--
2.50.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH net-next v10 1/5] net: rnpgbe: Add interrupt handling
2026-08-31 7:36 ` [PATCH net-next v10 1/5] net: rnpgbe: Add interrupt handling Dong Yibo
@ 2026-09-04 21:54 ` Jakub Kicinski
2026-09-07 3:50 ` Yibo Dong
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2026-09-04 21:54 UTC (permalink / raw)
To: Dong Yibo
Cc: andrew+netdev, davem, edumazet, pabeni, vadim.fedorenko,
u.kleine-koenig, netdev, linux-kernel, yaojun
On Mon, 31 Aug 2026 15:36:04 +0800 Dong Yibo wrote:
> +/**
> + * rnpgbe_int_single - MSI-X single-vector/MSI interrupt handler
> + * @irq: interrupt number
> + * @data: private data
> + *
> + * Return: IRQ_HANDLED
> + **/
> +static irqreturn_t rnpgbe_int_single(int irq, void *data)
> +{
> + struct mucse *mucse = (struct mucse *)data;
> + struct mucse_q_vector *q_vector;
> +
> + queue_work(system_percpu_wq, &mucse->mbx_work);
> +
> + if (test_bit(__MUCSE_DOWN, &mucse->state))
> + return IRQ_HANDLED;
> +
> + q_vector = mucse->q_vector[0];
> + rnpgbe_irq_disable_queues(q_vector);
> + napi_schedule_irqoff(&q_vector->napi);
> +
> + return IRQ_HANDLED;
> +}
It's not reasonable to have a single IRQ handle datapath and also
unconditionally queue a mbox work item. Just require at least 2 IRQs.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH net-next v10 1/5] net: rnpgbe: Add interrupt handling
2026-09-04 21:54 ` Jakub Kicinski
@ 2026-09-07 3:50 ` Yibo Dong
0 siblings, 0 replies; 15+ messages in thread
From: Yibo Dong @ 2026-09-07 3:50 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, davem, edumazet, pabeni, vadim.fedorenko,
u.kleine-koenig, netdev, linux-kernel, yaojun
Hi, Jakub:
On Fri, Sep 04, 2026 at 02:54:10PM -0700, Jakub Kicinski wrote:
> On Mon, 31 Aug 2026 15:36:04 +0800 Dong Yibo wrote:
> > +/**
> > + * rnpgbe_int_single - MSI-X single-vector/MSI interrupt handler
> > + * @irq: interrupt number
> > + * @data: private data
> > + *
> > + * Return: IRQ_HANDLED
> > + **/
> > +static irqreturn_t rnpgbe_int_single(int irq, void *data)
> > +{
> > + struct mucse *mucse = (struct mucse *)data;
> > + struct mucse_q_vector *q_vector;
> > +
> > + queue_work(system_percpu_wq, &mucse->mbx_work);
> > +
> > + if (test_bit(__MUCSE_DOWN, &mucse->state))
> > + return IRQ_HANDLED;
> > +
> > + q_vector = mucse->q_vector[0];
> > + rnpgbe_irq_disable_queues(q_vector);
> > + napi_schedule_irqoff(&q_vector->napi);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> It's not reasonable to have a single IRQ handle datapath and also
> unconditionally queue a mbox work item. Just require at least 2 IRQs.
>
The hardware supports only one MSI vector, and it does not provide a
reliable interrupt-cause register to distinguish mailbox events from
Tx/Rx events on a shared vector.
I will remove the shared MSI/single-vector MSI-X path and require at
least two MSI-X vectors instead. Vector 0 will be dedicated to mailbox
events, while the remaining vectors will be used for the data path.
Thanks for the feedback.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v10 2/5] net: rnpgbe: Add basic TX packet transmission support
[not found] <20260831073608.401988-1-dong100@mucse.com>
2026-08-31 7:36 ` [PATCH net-next v10 1/5] net: rnpgbe: Add interrupt handling Dong Yibo
@ 2026-08-31 7:36 ` Dong Yibo
2026-09-04 21:57 ` Jakub Kicinski
2026-08-31 7:36 ` [PATCH net-next v10 3/5] net: rnpgbe: Add RX packet reception support Dong Yibo
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Dong Yibo @ 2026-08-31 7:36 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, vadim.fedorenko,
u.kleine-koenig
Cc: netdev, linux-kernel, dong100, yaojun
Add the basic TX path for the RNPGBE driver.
Introduce the TX descriptor structure and buffer management, the
rnpgbe_xmit_frame_ring() transmit function, TX ring allocation and
teardown, and TX completion handling in rnpgbe_clean_tx_irq(). Track TX
packet and byte statistics, and report dropped and tx_dropped counts
through ndo_get_stats64().
Require the hardware's 56-bit streaming and coherent DMA mask. Add
cycles_per_us to struct mucse_hw to scale the TX interrupt timer, and
enable NETIF_F_HIGHDMA when the DMA mask allows it.
Signed-off-by: Dong Yibo <dong100@mucse.com>
---
drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 80 +-
.../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 4 +
drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 7 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_lib.c | 697 +++++++++++++++++-
.../net/ethernet/mucse/rnpgbe/rnpgbe_lib.h | 30 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_main.c | 62 +-
6 files changed, 864 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 77304196c2b6..6d0b9f424c64 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -44,20 +44,85 @@ struct mucse_hw {
struct pci_dev *pdev;
struct mucse_mbx_info mbx;
int port;
+ u16 cycles_per_us;
u8 pfvfnum;
};
+struct rnpgbe_tx_desc {
+ __le64 pkt_addr; /* Packet buffer address */
+ union {
+ __le64 vlan_cmd_bsz;
+ struct {
+ __le32 blen_mac_ip_len;
+ __le32 vlan_cmd; /* vlan & cmd status */
+ };
+ };
+#define M_TXD_CMD_RS 0x040000 /* Report Status */
+#define M_TXD_STAT_DD 0x020000 /* Descriptor Done */
+#define M_TXD_CMD_EOP 0x010000 /* End of Packet */
+};
+
+#define M_TX_DESC(R, i) (&(((struct rnpgbe_tx_desc *)((R)->desc))[i]))
+
+struct mucse_tx_buffer {
+ struct rnpgbe_tx_desc *next_to_watch;
+ struct sk_buff *skb;
+ unsigned int bytecount;
+ unsigned short gso_segs;
+ DEFINE_DMA_UNMAP_ADDR(dma);
+ DEFINE_DMA_UNMAP_LEN(len);
+ bool mapped_as_page; /* true if dma was mapped with dma_map_page */
+};
+
+struct mucse_queue_stats {
+ u64 packets;
+ u64 bytes;
+ atomic64_t dropped;
+};
+
struct mucse_ring {
struct mucse_ring *next;
struct mucse_q_vector *q_vector;
+ struct net_device *netdev;
+ struct device *dev;
+ void *desc;
+ struct mucse_tx_buffer *tx_buffer_info;
void __iomem *ring_addr;
+ void __iomem *tail;
void __iomem *irq_mask;
void __iomem *trig;
u8 queue_index;
/* hw ring idx */
u8 rnpgbe_queue_idx;
+ u8 pfvfnum;
+ u16 count;
+ u16 next_to_use;
+ u16 next_to_clean;
+ dma_addr_t dma;
+ unsigned int size;
+ struct mucse_queue_stats stats;
+ struct u64_stats_sync syncp;
} ____cacheline_internodealigned_in_smp;
+static inline u16 mucse_desc_unused(struct mucse_ring *ring)
+{
+ u16 ntc = ring->next_to_clean;
+ u16 ntu = ring->next_to_use;
+
+ return ((ntc > ntu) ? 0 : ring->count) + ntc - ntu - 1;
+}
+
+static inline __le64 build_ctob(u32 vlan_cmd, u32 mac_ip_len, u32 size)
+{
+ return cpu_to_le64(((u64)vlan_cmd << 32) | ((u64)mac_ip_len << 16) |
+ ((u64)size));
+}
+
+static inline struct netdev_queue *txring_txq(const struct mucse_ring *ring)
+{
+ return netdev_get_tx_queue(ring->netdev, ring->queue_index);
+}
+
struct mucse_ring_container {
struct mucse_ring *ring;
u16 count;
@@ -74,21 +139,20 @@ struct mucse_q_vector {
struct mucse_ring ring[] ____cacheline_internodealigned_in_smp;
};
-struct mucse_stats {
- u64 tx_dropped;
-};
-
#define MAX_Q_VECTORS 8
+#define M_DEFAULT_TXD 512
+#define M_DEFAULT_TX_WORK 256
+
enum mucse_state_t {
__MUCSE_DOWN,
+ __MUCSE_AXI_FAULT,
};
struct mucse {
struct net_device *netdev;
struct pci_dev *pdev;
struct mucse_hw hw;
- struct mucse_stats stats;
#define M_FLAG_MSIX_SINGLE_EN BIT(0)
#define M_FLAG_MSIX_EN BIT(1)
u32 flags;
@@ -97,6 +161,8 @@ struct mucse {
struct mucse_ring *rx_ring[RNPGBE_MAX_QUEUES]
____cacheline_aligned_in_smp;
struct mucse_q_vector *q_vector[MAX_Q_VECTORS];
+ int tx_ring_item_count;
+ int tx_work_limit;
int num_tx_queues;
int num_q_vectors;
int num_rx_queues;
@@ -123,4 +189,8 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type);
writel((val), (hw)->hw_addr + (reg))
#define mucse_hw_rd32(hw, reg) \
readl((hw)->hw_addr + (reg))
+#define mucse_ring_wr32(ring, reg, val) \
+ writel((val), (ring)->ring_addr + (reg))
+#define mucse_ring_rd32(ring, reg) \
+ readl((ring)->ring_addr + (reg))
#endif /* _RNPGBE_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
index 921cc325a991..291e77d573fe 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
@@ -93,6 +93,8 @@ static void rnpgbe_init_n500(struct mucse_hw *hw)
mbx->fwpf_ctrl_base = MUCSE_N500_FWPF_CTRL_BASE;
mbx->fwpf_shm_base = MUCSE_N500_FWPF_SHM_BASE;
+
+ hw->cycles_per_us = M_DEFAULT_N500_MHZ;
}
/**
@@ -110,6 +112,8 @@ static void rnpgbe_init_n210(struct mucse_hw *hw)
mbx->fwpf_ctrl_base = MUCSE_N210_FWPF_CTRL_BASE;
mbx->fwpf_shm_base = MUCSE_N210_FWPF_SHM_BASE;
+
+ hw->cycles_per_us = M_DEFAULT_N210_MHZ;
}
/**
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
index 0dce78e4a91b..6dc29ebe6fa7 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
@@ -7,13 +7,20 @@
#define MUCSE_N500_FWPF_CTRL_BASE 0x28b00
#define MUCSE_N500_FWPF_SHM_BASE 0x2d000
#define MUCSE_N500_RING_MSIX_BASE 0x28700
+#define M_DEFAULT_N500_MHZ 125
#define MUCSE_GBE_PFFW_MBX_CTRL_OFFSET 0x5500
#define MUCSE_GBE_FWPF_MBX_MASK_OFFSET 0x5700
#define MUCSE_N210_FWPF_CTRL_BASE 0x29400
#define MUCSE_N210_FWPF_SHM_BASE 0x2d900
#define MUCSE_N210_RING_MSIX_BASE 0x29000
+#define M_DEFAULT_N210_MHZ 62
+#define RNPGBE_DMA_STATUS 0x0008
+#define TX_AXI_RW_EN 0xc
+/* DMA_STATUS_REG[23:20]: tx_wr, tx_rd, rx_wr, rx_rd done status. */
+#define RNPGBE_DMA_TX_STATUS GENMASK_U32(23, 22)
#define RNPGBE_DMA_AXI_EN 0x0010
+#define RNPGBE_TX_MIN_PKT_LEN 33
#define RNPGBE_MAX_QUEUES 8
#endif /* _RNPGBE_HW_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
index f511a3a5f916..ff81d222edd3 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
@@ -3,6 +3,9 @@
#include <linux/pci.h>
#include <linux/netdevice.h>
+#include <linux/iopoll.h>
+#include <linux/vmalloc.h>
+#include <net/netdev_queues.h>
#include "rnpgbe_lib.h"
#include "rnpgbe.h"
@@ -43,7 +46,6 @@ static void rnpgbe_irq_disable_queues(struct mucse_q_vector *q_vector)
writel(INT_VALID, ring->trig);
writel((RX_INT_MASK | TX_INT_MASK), ring->irq_mask);
}
-
/* flush posted writes to ensure hardware sees the mask */
if (q_vector->tx.ring)
readl(q_vector->tx.ring->irq_mask);
@@ -89,26 +91,143 @@ static void rnpgbe_irq_enable_queues(struct mucse_q_vector *q_vector)
}
}
+/**
+ * rnpgbe_clean_tx_irq - Reclaim resources after transmit completes
+ * @q_vector: structure containing interrupt and ring information
+ * @tx_ring: tx ring to clean
+ * @napi_budget: Used to determine if we are in netpoll
+ *
+ * Return: true if cleaning completed within the budget, otherwise false
+ **/
+static bool rnpgbe_clean_tx_irq(struct mucse_q_vector *q_vector,
+ struct mucse_ring *tx_ring,
+ int napi_budget)
+{
+ int budget = q_vector->mucse->tx_work_limit;
+ u64 total_bytes = 0, total_packets = 0;
+ struct mucse *mucse = q_vector->mucse;
+ struct mucse_tx_buffer *tx_buffer;
+ struct rnpgbe_tx_desc *tx_desc;
+ int i = tx_ring->next_to_clean;
+
+ tx_buffer = &tx_ring->tx_buffer_info[i];
+ tx_desc = M_TX_DESC(tx_ring, i);
+ i -= tx_ring->count;
+
+ do {
+ struct rnpgbe_tx_desc *eop_desc = tx_buffer->next_to_watch;
+
+ /* if next_to_watch is not set then there is no work pending */
+ if (!eop_desc)
+ break;
+
+ /* prevent any other reads prior to eop_desc */
+ dma_rmb();
+
+ /* if eop DD is not set pending work has not been completed */
+ if (!(eop_desc->vlan_cmd & cpu_to_le32(M_TXD_STAT_DD)))
+ break;
+ /* clear next_to_watch to prevent false hangs */
+ tx_buffer->next_to_watch = NULL;
+ total_bytes += tx_buffer->bytecount;
+ total_packets += tx_buffer->gso_segs;
+ napi_consume_skb(tx_buffer->skb, napi_budget);
+ if (tx_buffer->mapped_as_page) {
+ dma_unmap_page(tx_ring->dev,
+ dma_unmap_addr(tx_buffer, dma),
+ dma_unmap_len(tx_buffer, len),
+ DMA_TO_DEVICE);
+ } else {
+ dma_unmap_single(tx_ring->dev,
+ dma_unmap_addr(tx_buffer, dma),
+ dma_unmap_len(tx_buffer, len),
+ DMA_TO_DEVICE);
+ }
+ tx_buffer->skb = NULL;
+ dma_unmap_len_set(tx_buffer, len, 0);
+
+ /* unmap remaining buffers */
+ while (tx_desc != eop_desc) {
+ tx_buffer++;
+ tx_desc++;
+ i++;
+ if (unlikely(!i)) {
+ i -= tx_ring->count;
+ tx_buffer = tx_ring->tx_buffer_info;
+ tx_desc = M_TX_DESC(tx_ring, 0);
+ }
+
+ /* unmap any remaining paged data */
+ if (dma_unmap_len(tx_buffer, len)) {
+ dma_unmap_page(tx_ring->dev,
+ dma_unmap_addr(tx_buffer, dma),
+ dma_unmap_len(tx_buffer, len),
+ DMA_TO_DEVICE);
+ dma_unmap_len_set(tx_buffer, len, 0);
+ }
+ }
+
+ /* move us one more past the eop_desc for start of next pkt */
+ tx_buffer++;
+ tx_desc++;
+ i++;
+ if (unlikely(!i)) {
+ i -= tx_ring->count;
+ tx_buffer = tx_ring->tx_buffer_info;
+ tx_desc = M_TX_DESC(tx_ring, 0);
+ }
+
+ prefetch(tx_desc);
+ budget--;
+ } while (likely(budget > 0));
+
+ i += tx_ring->count;
+ tx_ring->next_to_clean = i;
+ u64_stats_update_begin(&tx_ring->syncp);
+ tx_ring->stats.bytes += total_bytes;
+ tx_ring->stats.packets += total_packets;
+ u64_stats_update_end(&tx_ring->syncp);
+
+#define TX_WAKE_THRESHOLD (DESC_NEEDED * 2)
+ __netif_txq_completed_wake(txring_txq(tx_ring),
+ total_packets, total_bytes,
+ mucse_desc_unused(tx_ring),
+ TX_WAKE_THRESHOLD,
+ test_bit(__MUCSE_DOWN, &mucse->state));
+
+ return !!budget;
+}
+
/**
* rnpgbe_poll - NAPI polling callback
* @napi: structure for representing this polling device
* @budget: polling budget
*
- * Complete NAPI polling and re-enable queue interrupts. Ring cleaning is
- * added when TX and RX support is enabled.
+ * Clean completed TX packets and complete NAPI polling when all queues are
+ * clean.
*
- * Return: 0
+ * Return: 0 if all TX queues are clean, otherwise @budget
**/
static int rnpgbe_poll(struct napi_struct *napi, int budget)
{
struct mucse_q_vector *q_vector =
container_of(napi, struct mucse_q_vector, napi);
+ bool clean_complete = true;
+ struct mucse_ring *ring;
int work_done = 0;
+ mucse_for_each_ring(ring, q_vector->tx) {
+ if (!rnpgbe_clean_tx_irq(q_vector, ring, budget))
+ clean_complete = false;
+ }
+
/* Exit if we are called by netpoll */
if (unlikely(!budget))
return 0;
+ if (!clean_complete)
+ return budget;
+
if (likely(napi_complete_done(napi, work_done)))
rnpgbe_irq_enable_queues(q_vector);
@@ -277,13 +396,18 @@ static int rnpgbe_alloc_q_vector(struct mucse *mucse,
ring = q_vector->ring;
for (idx = 0; idx < txr_count; idx++) {
+ ring->dev = &mucse->pdev->dev;
mucse_add_ring(ring, &q_vector->tx);
+ ring->count = mucse->tx_ring_item_count;
+ ring->netdev = mucse->netdev;
ring->queue_index = eth_queue_idx + idx;
ring->rnpgbe_queue_idx = txr_idx;
ring->ring_addr = hw->hw_addr + RING_OFFSET(txr_idx);
ring->irq_mask = ring->ring_addr + RNPGBE_DMA_INT_MASK;
ring->trig = ring->ring_addr + RNPGBE_DMA_INT_TRIG;
ring->q_vector = q_vector;
+ ring->pfvfnum = hw->pfvfnum;
+ u64_stats_init(&ring->syncp);
mucse->tx_ring[ring->queue_index] = ring;
txr_idx += step;
ring++;
@@ -636,13 +760,169 @@ static void rnpgbe_napi_disable_all(struct mucse *mucse)
napi_disable(&mucse->q_vector[i]->napi);
}
+static void rnpgbe_stop_tx_ring(struct mucse_ring *tx_ring)
+{
+ if (!tx_ring->tx_buffer_info)
+ return;
+
+ /* Stop hw. No new descriptors are fetched after TX_START=0.
+ * DMA for descriptors fetched before the stop may still be in flight.
+ */
+ mucse_ring_wr32(tx_ring, RNPGBE_TX_START, 0);
+ /* Flush posted write to ensure hardware sees TX_START=0 */
+ (void)mucse_ring_rd32(tx_ring, RNPGBE_TX_START);
+}
+
+static int rnpgbe_wait_tx_dma_idle(struct mucse *mucse)
+{
+ struct mucse_hw *hw = &mucse->hw;
+ u32 dma_status;
+ int err;
+
+ /* A timeout indicates a terminal AXI fault. Hardware stops DMA
+ * accesses, so teardown may release mappings, but configuration
+ * must not re-enable TX DMA.
+ */
+ err = readl_poll_timeout(hw->hw_addr + RNPGBE_DMA_STATUS,
+ dma_status,
+ (dma_status & RNPGBE_DMA_TX_STATUS) ==
+ RNPGBE_DMA_TX_STATUS,
+ 10, 100000);
+ if (err) {
+ set_bit(__MUCSE_AXI_FAULT, &mucse->state);
+ dev_err(&mucse->pdev->dev,
+ "TX DMA failed to quiesce, status %#x\n", dma_status);
+ }
+
+ return err;
+}
+
+/**
+ * rnpgbe_stop_all_tx_rings - Stop TX DMA on all queues
+ * @mucse: board private structure
+ *
+ * Return: 0 when TX DMA was quiesced, negative errno otherwise
+ **/
+static int rnpgbe_stop_all_tx_rings(struct mucse *mucse)
+{
+ struct mucse_hw *hw = &mucse->hw;
+ u32 dma_axi_ctl;
+ int err = 0;
+
+ for (int i = 0; i < mucse->num_tx_queues; i++)
+ rnpgbe_stop_tx_ring(mucse->tx_ring[i]);
+
+ if (mucse->num_tx_queues &&
+ !test_bit(__MUCSE_AXI_FAULT, &mucse->state))
+ err = rnpgbe_wait_tx_dma_idle(mucse);
+
+ dma_axi_ctl = mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
+ dma_axi_ctl &= ~TX_AXI_RW_EN;
+ mucse_hw_wr32(hw, RNPGBE_DMA_AXI_EN, dma_axi_ctl);
+ /* Flush the posted write before continuing. */
+ (void)mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
+
+ if (!test_bit(__MUCSE_AXI_FAULT, &mucse->state))
+ return 0;
+
+ return err ? err : -EIO;
+}
+
+/**
+ * rnpgbe_clean_tx_ring - Free Tx Buffers
+ * @tx_ring: ring to be cleaned
+ **/
+static void rnpgbe_clean_tx_ring(struct mucse_ring *tx_ring)
+{
+ struct mucse_tx_buffer *tx_buffer;
+ u16 i = tx_ring->next_to_clean;
+ unsigned long size;
+
+ /* ring already cleared, nothing to do */
+ if (!tx_ring->tx_buffer_info)
+ return;
+
+ tx_buffer = &tx_ring->tx_buffer_info[i];
+
+ while (i != tx_ring->next_to_use) {
+ struct rnpgbe_tx_desc *eop_desc, *tx_desc;
+
+ dev_kfree_skb_any(tx_buffer->skb);
+ /* unmap skb header data */
+ if (dma_unmap_len(tx_buffer, len)) {
+ if (tx_buffer->mapped_as_page) {
+ dma_unmap_page(tx_ring->dev,
+ dma_unmap_addr(tx_buffer, dma),
+ dma_unmap_len(tx_buffer, len),
+ DMA_TO_DEVICE);
+ } else {
+ dma_unmap_single(tx_ring->dev,
+ dma_unmap_addr(tx_buffer, dma),
+ dma_unmap_len(tx_buffer, len),
+ DMA_TO_DEVICE);
+ }
+ }
+ eop_desc = tx_buffer->next_to_watch;
+ tx_desc = M_TX_DESC(tx_ring, i);
+ /* unmap remaining buffers */
+ while (tx_desc != eop_desc) {
+ tx_buffer++;
+ tx_desc++;
+ i++;
+ if (unlikely(i == tx_ring->count)) {
+ i = 0;
+ tx_buffer = tx_ring->tx_buffer_info;
+ tx_desc = M_TX_DESC(tx_ring, 0);
+ }
+
+ /* unmap any remaining paged data */
+ if (dma_unmap_len(tx_buffer, len))
+ dma_unmap_page(tx_ring->dev,
+ dma_unmap_addr(tx_buffer, dma),
+ dma_unmap_len(tx_buffer, len),
+ DMA_TO_DEVICE);
+ }
+ /* move us one more past the eop_desc for start of next pkt */
+ tx_buffer++;
+ i++;
+ if (unlikely(i == tx_ring->count)) {
+ i = 0;
+ tx_buffer = tx_ring->tx_buffer_info;
+ }
+ }
+
+ netdev_tx_reset_queue(txring_txq(tx_ring));
+ size = sizeof(struct mucse_tx_buffer) * tx_ring->count;
+ memset(tx_ring->tx_buffer_info, 0, size);
+ /* Zero out the descriptor ring */
+ memset(tx_ring->desc, 0, tx_ring->size);
+ tx_ring->next_to_use = 0;
+ tx_ring->next_to_clean = 0;
+}
+
+/**
+ * rnpgbe_clean_all_tx_rings - Stop TX DMA and free Tx buffers for all queues
+ * @mucse: board private structure
+ **/
+void rnpgbe_clean_all_tx_rings(struct mucse *mucse)
+{
+ rnpgbe_stop_all_tx_rings(mucse);
+
+ for (int i = 0; i < mucse->num_tx_queues; i++)
+ rnpgbe_clean_tx_ring(mucse->tx_ring[i]);
+}
+
bool rnpgbe_down(struct mucse *mucse)
{
+ struct net_device *netdev = mucse->netdev;
+
if (test_and_set_bit(__MUCSE_DOWN, &mucse->state))
return false;
+ netif_tx_disable(netdev);
rnpgbe_napi_disable_all(mucse);
rnpgbe_irq_disable(mucse);
+ rnpgbe_clean_all_tx_rings(mucse);
return true;
}
@@ -653,11 +933,420 @@ bool rnpgbe_down(struct mucse *mucse)
**/
void rnpgbe_up_complete(struct mucse *mucse)
{
+ struct net_device *netdev = mucse->netdev;
+
if (mucse->flags & (M_FLAG_MSIX_EN | M_FLAG_MSIX_SINGLE_EN))
rnpgbe_configure_msix(mucse);
else
rnpgbe_configure_msi(mucse);
+
rnpgbe_napi_enable_all(mucse);
clear_bit(__MUCSE_DOWN, &mucse->state);
rnpgbe_irq_enable(mucse);
+ netif_tx_start_all_queues(netdev);
+}
+
+/**
+ * rnpgbe_free_tx_resources - Free Tx Resources per Queue
+ * @tx_ring: tx descriptor ring for a specific queue
+ *
+ * Free all transmit software resources
+ **/
+static void rnpgbe_free_tx_resources(struct mucse_ring *tx_ring)
+{
+ vfree(tx_ring->tx_buffer_info);
+ tx_ring->tx_buffer_info = NULL;
+ /* if not set, then don't free */
+ if (!tx_ring->desc)
+ return;
+
+ dma_free_coherent(tx_ring->dev, tx_ring->size, tx_ring->desc,
+ tx_ring->dma);
+ tx_ring->desc = NULL;
+}
+
+/**
+ * rnpgbe_setup_tx_resources - allocate Tx resources (Descriptors)
+ * @tx_ring: tx descriptor ring (for a specific queue) to setup
+ * @mucse: pointer to private structure
+ *
+ * Return: 0 on success, negative on failure
+ **/
+static int rnpgbe_setup_tx_resources(struct mucse_ring *tx_ring,
+ struct mucse *mucse)
+{
+ struct device *dev = tx_ring->dev;
+ int size;
+
+ size = sizeof(struct mucse_tx_buffer) * tx_ring->count;
+
+ tx_ring->tx_buffer_info = vzalloc(size);
+ if (!tx_ring->tx_buffer_info)
+ goto err_return;
+ /* round up to nearest 4K */
+ tx_ring->size = tx_ring->count * sizeof(struct rnpgbe_tx_desc);
+ tx_ring->size = ALIGN(tx_ring->size, 4096);
+ tx_ring->desc = dma_alloc_coherent(dev, tx_ring->size, &tx_ring->dma,
+ GFP_KERNEL);
+ if (!tx_ring->desc)
+ goto err_free_buffer;
+
+ tx_ring->next_to_use = 0;
+ tx_ring->next_to_clean = 0;
+
+ return 0;
+
+err_free_buffer:
+ vfree(tx_ring->tx_buffer_info);
+err_return:
+ tx_ring->tx_buffer_info = NULL;
+ return -ENOMEM;
+}
+
+/**
+ * rnpgbe_configure_tx_ring - Configure Tx ring after Reset
+ * @mucse: pointer to private structure
+ * @ring: structure containing ring specific data
+ *
+ * Configure the Tx descriptor ring after a reset.
+ **/
+static void rnpgbe_configure_tx_ring(struct mucse *mucse,
+ struct mucse_ring *ring)
+{
+ struct mucse_hw *hw = &mucse->hw;
+
+ mucse_ring_wr32(ring, RNPGBE_TX_BASE_ADDR_LO, (u32)ring->dma);
+ mucse_ring_wr32(ring, RNPGBE_TX_BASE_ADDR_HI,
+ (u32)(((u64)ring->dma) >> 32) |
+ ((u32)hw->pfvfnum << 24));
+ mucse_ring_wr32(ring, RNPGBE_TX_LEN, ring->count);
+ ring->next_to_clean = mucse_ring_rd32(ring, RNPGBE_TX_HEAD) %
+ ring->count;
+ ring->next_to_use = ring->next_to_clean;
+ ring->tail = ring->ring_addr + RNPGBE_TX_TAIL;
+ writel(ring->next_to_use, ring->tail);
+ mucse_ring_wr32(ring, RNPGBE_TX_FETCH_CTRL, M_DEFAULT_TX_FETCH);
+ mucse_ring_wr32(ring, RNPGBE_TX_INT_TIMER,
+ M_DEFAULT_INT_TIMER * hw->cycles_per_us);
+ mucse_ring_wr32(ring, RNPGBE_TX_INT_PKTCNT, M_DEFAULT_INT_PKTCNT);
+}
+
+/**
+ * rnpgbe_configure_tx - Configure Transmit Unit after Reset
+ * @mucse: pointer to private structure
+ *
+ * Configure the Tx DMA after a reset.
+ *
+ * Return: 0 on success, negative errno if TX DMA did not quiesce
+ **/
+int rnpgbe_configure_tx(struct mucse *mucse)
+{
+ struct mucse_hw *hw = &mucse->hw;
+ u32 i, dma_axi_ctl;
+ int err;
+
+ err = rnpgbe_stop_all_tx_rings(mucse);
+ if (err)
+ return err;
+
+ for (i = 0; i < mucse->num_tx_queues; i++)
+ rnpgbe_configure_tx_ring(mucse, mucse->tx_ring[i]);
+
+ /* Ensure all ring configuration is visible before enabling TX DMA. */
+ wmb();
+ dma_axi_ctl = mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
+ dma_axi_ctl |= TX_AXI_RW_EN;
+ mucse_hw_wr32(hw, RNPGBE_DMA_AXI_EN, dma_axi_ctl);
+ /* Ensure TX_AXI_RW_EN is visible before starting the rings. */
+ (void)mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
+
+ for (i = 0; i < mucse->num_tx_queues; i++)
+ mucse_ring_wr32(mucse->tx_ring[i], RNPGBE_TX_START, 1);
+
+ return 0;
+}
+
+/**
+ * rnpgbe_setup_all_tx_resources - allocate all queues Tx resources
+ * @mucse: pointer to private structure
+ *
+ * Allocate memory for tx_ring.
+ *
+ * Return: 0 on success, negative on failure
+ **/
+int rnpgbe_setup_all_tx_resources(struct mucse *mucse)
+{
+ int i, err = 0;
+
+ for (i = 0; i < mucse->num_tx_queues; i++) {
+ err = rnpgbe_setup_tx_resources(mucse->tx_ring[i], mucse);
+ if (!err)
+ continue;
+
+ goto err_free_res;
+ }
+
+ return 0;
+err_free_res:
+ while (i--)
+ rnpgbe_free_tx_resources(mucse->tx_ring[i]);
+ return err;
+}
+
+/**
+ * rnpgbe_free_all_tx_resources - Free Tx Resources for All Queues
+ * @mucse: pointer to private structure
+ *
+ * Free all transmit software resources
+ **/
+void rnpgbe_free_all_tx_resources(struct mucse *mucse)
+{
+ for (int i = 0; i < (mucse->num_tx_queues); i++)
+ rnpgbe_free_tx_resources(mucse->tx_ring[i]);
+}
+
+static int rnpgbe_tx_map(struct mucse_ring *tx_ring,
+ struct mucse_tx_buffer *first, u32 mac_ip_len,
+ u32 tx_flags)
+{
+ struct mucse_tx_buffer *tx_buffer;
+ struct rnpgbe_tx_desc *tx_desc;
+ struct skb_shared_info *shinfo;
+ unsigned int data_len, size;
+ struct sk_buff *skb;
+ skb_frag_t *frag;
+ dma_addr_t dma;
+ int nr_frags;
+ int frag_idx;
+ /* Hardware requires this in the upper 8 bits of
+ * the descriptor address
+ */
+ u64 fun_id;
+ u16 i;
+
+ skb = first->skb;
+ shinfo = skb_shinfo(skb);
+ fun_id = (u64)tx_ring->pfvfnum << 56;
+ nr_frags = shinfo->nr_frags;
+ i = tx_ring->next_to_use;
+ frag_idx = 0;
+ tx_desc = M_TX_DESC(tx_ring, i);
+ size = skb_headlen(skb);
+ data_len = skb->data_len;
+
+ if (size) {
+ dma = dma_map_single(tx_ring->dev, skb->data, size,
+ DMA_TO_DEVICE);
+ first->mapped_as_page = false;
+ } else if (data_len) {
+ while (frag_idx < nr_frags &&
+ !skb_frag_size(&shinfo->frags[frag_idx]))
+ frag_idx++;
+
+ if (frag_idx == nr_frags)
+ goto err_unmap;
+
+ frag = &shinfo->frags[frag_idx];
+ size = skb_frag_size(frag);
+
+ dma = skb_frag_dma_map(tx_ring->dev, frag, 0,
+ size, DMA_TO_DEVICE);
+ first->mapped_as_page = true;
+ data_len -= size;
+ frag_idx++;
+ } else {
+ goto err_unmap;
+ }
+
+ tx_buffer = first;
+
+ dma_unmap_len_set(tx_buffer, len, 0);
+ dma_unmap_addr_set(tx_buffer, dma, 0);
+
+ for (;; frag_idx++) {
+ if (dma_mapping_error(tx_ring->dev, dma))
+ goto err_unmap;
+
+ /* record length, and DMA address */
+ dma_unmap_len_set(tx_buffer, len, size);
+ dma_unmap_addr_set(tx_buffer, dma, dma);
+
+ tx_desc->pkt_addr = cpu_to_le64(dma | fun_id);
+
+ while (unlikely(size > M_MAX_DATA_PER_TXD)) {
+ tx_desc->vlan_cmd_bsz = build_ctob(tx_flags,
+ mac_ip_len,
+ M_MAX_DATA_PER_TXD);
+ i++;
+ tx_desc++;
+ if (i == tx_ring->count) {
+ tx_desc = M_TX_DESC(tx_ring, 0);
+ i = 0;
+ }
+
+ tx_buffer = &tx_ring->tx_buffer_info[i];
+ dma_unmap_len_set(tx_buffer, len, 0);
+ dma += M_MAX_DATA_PER_TXD;
+ size -= M_MAX_DATA_PER_TXD;
+ tx_desc->pkt_addr = cpu_to_le64(dma | fun_id);
+ }
+
+ if (likely(!data_len))
+ break;
+ tx_desc->vlan_cmd_bsz = build_ctob(tx_flags, mac_ip_len, size);
+ i++;
+ tx_desc++;
+ if (i == tx_ring->count) {
+ tx_desc = M_TX_DESC(tx_ring, 0);
+ i = 0;
+ }
+
+ while (frag_idx < nr_frags &&
+ !skb_frag_size(&shinfo->frags[frag_idx]))
+ frag_idx++;
+
+ if (frag_idx == nr_frags)
+ goto err_unmap;
+
+ frag = &shinfo->frags[frag_idx];
+ size = skb_frag_size(frag);
+ data_len -= size;
+ dma = skb_frag_dma_map(tx_ring->dev, frag, 0, size,
+ DMA_TO_DEVICE);
+ tx_buffer = &tx_ring->tx_buffer_info[i];
+ tx_buffer->mapped_as_page = true;
+ }
+
+ /* write last descriptor with RS and EOP bits */
+ tx_desc->vlan_cmd_bsz = build_ctob(tx_flags | M_TXD_CMD_EOP |
+ M_TXD_CMD_RS,
+ mac_ip_len, size);
+
+ /* Force memory writes to complete before letting h/w know there
+ * are new descriptors to fetch. (Only applicable for weak-ordered
+ * memory model archs, such as IA-64).
+ *
+ * We also need this memory barrier to make certain all of the
+ * status bits have been updated before next_to_watch is written.
+ */
+ dma_wmb();
+ /* set next_to_watch value indicating a packet is present */
+ first->next_to_watch = tx_desc;
+ i++;
+ if (i == tx_ring->count)
+ i = 0;
+ tx_ring->next_to_use = i;
+ skb_tx_timestamp(skb);
+ netdev_tx_sent_queue(txring_txq(tx_ring), first->bytecount);
+ /* notify HW of packet */
+ writel(i, tx_ring->tail);
+
+ return 0;
+err_unmap:
+ for (;;) {
+ tx_buffer = &tx_ring->tx_buffer_info[i];
+ if (dma_unmap_len(tx_buffer, len)) {
+ if (tx_buffer->mapped_as_page) {
+ dma_unmap_page(tx_ring->dev,
+ dma_unmap_addr(tx_buffer, dma),
+ dma_unmap_len(tx_buffer, len),
+ DMA_TO_DEVICE);
+ } else {
+ dma_unmap_single(tx_ring->dev,
+ dma_unmap_addr(tx_buffer, dma),
+ dma_unmap_len(tx_buffer, len),
+ DMA_TO_DEVICE);
+ }
+ }
+ dma_unmap_len_set(tx_buffer, len, 0);
+ dma_unmap_addr_set(tx_buffer, dma, 0);
+ if (tx_buffer == first)
+ break;
+ if (i == 0)
+ i += tx_ring->count;
+ i--;
+ }
+ dev_kfree_skb_any(first->skb);
+ first->skb = NULL;
+ tx_ring->next_to_use = i;
+
+ return -ENOMEM;
+}
+
+netdev_tx_t rnpgbe_xmit_frame_ring(struct sk_buff *skb,
+ struct mucse_ring *tx_ring)
+{
+ /* hw requires non-zero mac_ip_len even without offload;
+ * Will be updated per-packet when offload is added
+ */
+ u32 mac_ip_len = M_DEFAULT_MAC_IP_LEN;
+ struct mucse_tx_buffer *first;
+ u32 tx_flags = 0;
+ unsigned short f;
+ u16 count;
+
+ count = TXD_USE_COUNT(skb_headlen(skb));
+ for (f = 0; f < skb_shinfo(skb)->nr_frags; f++) {
+ skb_frag_t *frag_temp = &skb_shinfo(skb)->frags[f];
+
+ count += TXD_USE_COUNT(skb_frag_size(frag_temp));
+ }
+
+ if (!netif_txq_maybe_stop(txring_txq(tx_ring),
+ mucse_desc_unused(tx_ring),
+ count + RESV_DESC_NEEDED,
+ count + RESV_DESC_NEEDED))
+ return NETDEV_TX_BUSY;
+
+ /* record the location of the first descriptor for this packet */
+ first = &tx_ring->tx_buffer_info[tx_ring->next_to_use];
+ first->skb = skb;
+ first->bytecount = skb->len;
+ first->gso_segs = 1;
+
+ if (rnpgbe_tx_map(tx_ring, first, mac_ip_len, tx_flags)) {
+ atomic64_inc(&tx_ring->stats.dropped);
+
+ goto out;
+ }
+
+ /* NETDEV_TX_BUSY is expensive. So stop advancing the TX queue. */
+ netif_txq_maybe_stop(txring_txq(tx_ring),
+ mucse_desc_unused(tx_ring),
+ DESC_NEEDED, DESC_NEEDED);
+out:
+ return NETDEV_TX_OK;
+}
+
+/**
+ * rnpgbe_get_stats64 - Get stats for this netdev
+ * @netdev: network interface device structure
+ * @stats: stats data
+ **/
+void rnpgbe_get_stats64(struct net_device *netdev,
+ struct rtnl_link_stats64 *stats)
+{
+ struct mucse *mucse = netdev_priv(netdev);
+ int i;
+
+ rcu_read_lock();
+ for (i = 0; i < mucse->num_tx_queues; i++) {
+ struct mucse_ring *ring = READ_ONCE(mucse->tx_ring[i]);
+ u64 bytes, packets, dropped;
+ unsigned int start;
+
+ if (ring) {
+ do {
+ start = u64_stats_fetch_begin(&ring->syncp);
+ packets = ring->stats.packets;
+ bytes = ring->stats.bytes;
+ } while (u64_stats_fetch_retry(&ring->syncp, start));
+ dropped = atomic64_read(&ring->stats.dropped);
+
+ stats->tx_packets += packets;
+ stats->tx_dropped += dropped;
+ stats->tx_bytes += bytes;
+ }
+ }
+ rcu_read_unlock();
}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
index c03f7aad2c08..01e800f5f6a4 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
@@ -6,17 +6,39 @@
struct mucse;
struct mucse_hw;
+struct mucse_ring;
#define RING_OFFSET(n) (0x1000 + 0x100 * (n))
+#define RNPGBE_TX_START 0x18
#define RNPGBE_DMA_INT_MASK 0x24
#define TX_INT_MASK BIT(1)
#define RX_INT_MASK BIT(0)
#define INT_VALID (BIT(16) | BIT(17))
#define RNPGBE_DMA_INT_TRIG 0x2c /* lost-interrupt recovery trigger */
+#define RNPGBE_TX_BASE_ADDR_HI 0x60
+#define RNPGBE_TX_BASE_ADDR_LO 0x64
+#define RNPGBE_TX_LEN 0x68
+#define RNPGBE_TX_HEAD 0x6c
+#define RNPGBE_TX_TAIL 0x70
+#define M_DEFAULT_TX_FETCH 0x80008
+#define RNPGBE_TX_FETCH_CTRL 0x74
+#define M_DEFAULT_INT_TIMER 100
+#define RNPGBE_TX_INT_TIMER 0x78
+#define M_DEFAULT_INT_PKTCNT 48
+#define RNPGBE_TX_INT_PKTCNT 0x7c
/* | 31:24 | .... | 15:8 | 7:0 | */
/* | pfvfnum | | tx vector | rx vector | */
#define RING_VECTOR(n) (0x04 * (n))
+#define M_MAX_TXD_PWR 12
+#define M_MAX_DATA_PER_TXD (0x1 << M_MAX_TXD_PWR)
+#define TXD_USE_COUNT(S) DIV_ROUND_UP((S), M_MAX_DATA_PER_TXD)
+#define DESC_NEEDED (MAX_SKB_FRAGS + 4)
+/* 2 desc gap to keep tail from touching head */
+/* 1 desc for context descriptor */
+#define RESV_DESC_NEEDED 3
+/* Hardware requires this to be nonzero */
+#define M_DEFAULT_MAC_IP_LEN 20
#define mucse_for_each_ring(pos, head)\
for (typeof((head).ring) __pos = (head).ring;\
__pos ? ({ pos = __pos; 1; }) : 0;\
@@ -31,4 +53,12 @@ void rnpgbe_free_irq(struct mucse *mucse);
void rnpgbe_irq_disable(struct mucse *mucse);
bool rnpgbe_down(struct mucse *mucse);
void rnpgbe_up_complete(struct mucse *mucse);
+int rnpgbe_configure_tx(struct mucse *mucse);
+void rnpgbe_clean_all_tx_rings(struct mucse *mucse);
+int rnpgbe_setup_all_tx_resources(struct mucse *mucse);
+void rnpgbe_free_all_tx_resources(struct mucse *mucse);
+netdev_tx_t rnpgbe_xmit_frame_ring(struct sk_buff *skb,
+ struct mucse_ring *tx_ring);
+void rnpgbe_get_stats64(struct net_device *netdev,
+ struct rtnl_link_stats64 *stats);
#endif
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
index b7472dba2a19..5be78afd0b25 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
@@ -2,6 +2,7 @@
/* Copyright(c) 2020 - 2025 Mucse Corporation. */
#include <linux/pci.h>
+#include <linux/skbuff.h>
#include <net/rtnetlink.h>
#include <linux/etherdevice.h>
@@ -26,6 +27,19 @@ static struct pci_device_id rnpgbe_pci_tbl[] = {
{ },
};
+/**
+ * rnpgbe_configure - Configure the hardware
+ * @mucse: pointer to private structure
+ *
+ * rnpgbe_configure configure mac, tx, rx regs to hw
+ *
+ * Return: 0 on success, negative errno if hardware configuration fails
+ **/
+static int rnpgbe_configure(struct mucse *mucse)
+{
+ return rnpgbe_configure_tx(mucse);
+}
+
/**
* rnpgbe_open - Called when a network interface is made active
* @netdev: network interface device structure
@@ -40,6 +54,9 @@ static int rnpgbe_open(struct net_device *netdev)
struct mucse *mucse = netdev_priv(netdev);
int err;
+ if (test_bit(__MUCSE_AXI_FAULT, &mucse->state))
+ return -EIO;
+
err = rnpgbe_request_irq(mucse);
if (err)
return err;
@@ -49,9 +66,19 @@ static int rnpgbe_open(struct net_device *netdev)
if (err)
goto err_free_irqs;
+ err = rnpgbe_setup_all_tx_resources(mucse);
+ if (err)
+ goto err_free_irqs;
+
+ err = rnpgbe_configure(mucse);
+ if (err)
+ goto err_free_tx;
rnpgbe_up_complete(mucse);
return 0;
+err_free_tx:
+ rnpgbe_clean_all_tx_rings(mucse);
+ rnpgbe_free_all_tx_resources(mucse);
err_free_irqs:
rnpgbe_free_irq(mucse);
return err;
@@ -74,6 +101,7 @@ static int rnpgbe_close(struct net_device *netdev)
return 0;
rnpgbe_free_irq(mucse);
+ rnpgbe_free_all_tx_resources(mucse);
return 0;
}
@@ -83,25 +111,37 @@ static int rnpgbe_close(struct net_device *netdev)
* @skb: skb structure to be sent
* @netdev: network interface device structure
*
- * Return: NETDEV_TX_OK
+ * Return: NETDEV_TX_OK or NETDEV_TX_BUSY when insufficient descriptors
**/
static netdev_tx_t rnpgbe_xmit_frame(struct sk_buff *skb,
struct net_device *netdev)
{
struct mucse *mucse = netdev_priv(netdev);
+ struct mucse_ring *tx_ring;
- dev_kfree_skb_any(skb);
- mucse->stats.tx_dropped++;
+ tx_ring = mucse->tx_ring[skb_get_queue_mapping(skb)];
- return NETDEV_TX_OK;
+ if (unlikely(skb_put_padto(skb, RNPGBE_TX_MIN_PKT_LEN))) {
+ atomic64_inc(&tx_ring->stats.dropped);
+ return NETDEV_TX_OK;
+ }
+
+ return rnpgbe_xmit_frame_ring(skb, tx_ring);
}
static const struct net_device_ops rnpgbe_netdev_ops = {
.ndo_open = rnpgbe_open,
.ndo_stop = rnpgbe_close,
.ndo_start_xmit = rnpgbe_xmit_frame,
+ .ndo_get_stats64 = rnpgbe_get_stats64,
};
+static void rnpgbe_sw_init(struct mucse *mucse)
+{
+ mucse->tx_ring_item_count = M_DEFAULT_TXD;
+ mucse->tx_work_limit = M_DEFAULT_TX_WORK;
+}
+
/**
* rnpgbe_add_adapter - Add netdev for this pci_dev
* @pdev: PCI device information structure
@@ -175,6 +215,7 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
}
netdev->netdev_ops = &rnpgbe_netdev_ops;
+ rnpgbe_sw_init(mucse);
err = rnpgbe_reset_hw(hw);
if (err) {
dev_err(&pdev->dev, "Hw reset failed %d\n", err);
@@ -204,6 +245,13 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
goto err_clear_interrupt;
}
+ netdev->features |= NETIF_F_SG;
+ netdev->hw_features |= NETIF_F_SG;
+ if (dma_get_mask(&pdev->dev) > DMA_BIT_MASK(32)) {
+ netdev->features |= NETIF_F_HIGHDMA;
+ netdev->hw_features |= NETIF_F_HIGHDMA;
+ }
+
err = register_netdev(netdev);
if (err)
goto err_remove_mbx;
@@ -246,10 +294,10 @@ static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
if (err)
return err;
- err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(56));
+ err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(56));
if (err) {
- dev_err(&pdev->dev,
- "No usable DMA configuration, aborting %d\n", err);
+ dev_err(&pdev->dev, "No usable DMA configuration, aborting %d\n",
+ err);
goto err_disable_dev;
}
--
2.50.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH net-next v10 2/5] net: rnpgbe: Add basic TX packet transmission support
2026-08-31 7:36 ` [PATCH net-next v10 2/5] net: rnpgbe: Add basic TX packet transmission support Dong Yibo
@ 2026-09-04 21:57 ` Jakub Kicinski
2026-09-08 2:16 ` Yibo Dong
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2026-09-04 21:57 UTC (permalink / raw)
To: dong100
Cc: Jakub Kicinski, andrew+netdev, davem, edumazet, pabeni,
vadim.fedorenko, u.kleine-koenig, netdev, linux-kernel, yaojun
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: rnpgbe: Add basic TX packet transmission support
This adds the TX path for rnpgbe: TX descriptor and buffer structures,
rnpgbe_xmit_frame_ring(), TX ring setup/teardown, completion handling in
rnpgbe_clean_tx_irq(), and TX counters exported via ndo_get_stats64().
It also switches probe to dma_set_mask_and_coherent(DMA_BIT_MASK(56))
and adds hw->cycles_per_us plus NETIF_F_HIGHDMA.
A few questions below about the new fault state, the teardown error
handling, the stats reader and the missing TX watchdog.
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> index 77304196c2b6..6d0b9f424c64 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
[ ... ]
> @@ -74,21 +139,20 @@ struct mucse_q_vector {
> struct mucse_ring ring[] ____cacheline_internodealigned_in_smp;
> };
>
> -struct mucse_stats {
> - u64 tx_dropped;
> -};
> -
> #define MAX_Q_VECTORS 8
>
> +#define M_DEFAULT_TXD 512
> +#define M_DEFAULT_TX_WORK 256
> +
> enum mucse_state_t {
> __MUCSE_DOWN,
> + __MUCSE_AXI_FAULT,
> };
[ ... ]
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> index f511a3a5f916..ff81d222edd3 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
[ ... ]
> @@ -636,13 +760,169 @@ static void rnpgbe_napi_disable_all(struct mucse *mucse)
> napi_disable(&mucse->q_vector[i]->napi);
> }
>
> +static void rnpgbe_stop_tx_ring(struct mucse_ring *tx_ring)
> +{
> + if (!tx_ring->tx_buffer_info)
> + return;
> +
> + /* Stop hw. No new descriptors are fetched after TX_START=0.
> + * DMA for descriptors fetched before the stop may still be in flight.
> + */
> + mucse_ring_wr32(tx_ring, RNPGBE_TX_START, 0);
> + /* Flush posted write to ensure hardware sees TX_START=0 */
> + (void)mucse_ring_rd32(tx_ring, RNPGBE_TX_START);
> +}
> +
> +static int rnpgbe_wait_tx_dma_idle(struct mucse *mucse)
> +{
> + struct mucse_hw *hw = &mucse->hw;
> + u32 dma_status;
> + int err;
> +
> + /* A timeout indicates a terminal AXI fault. Hardware stops DMA
> + * accesses, so teardown may release mappings, but configuration
> + * must not re-enable TX DMA.
> + */
> + err = readl_poll_timeout(hw->hw_addr + RNPGBE_DMA_STATUS,
> + dma_status,
> + (dma_status & RNPGBE_DMA_TX_STATUS) ==
> + RNPGBE_DMA_TX_STATUS,
> + 10, 100000);
> + if (err) {
> + set_bit(__MUCSE_AXI_FAULT, &mucse->state);
> + dev_err(&mucse->pdev->dev,
> + "TX DMA failed to quiesce, status %#x\n", dma_status);
> + }
> +
> + return err;
> +}
[Severity: Medium]
Once __MUCSE_AXI_FAULT is set here, is there a way back?
Grepping drivers/net/ethernet/mucse/ at the end of the series shows no
clear_bit(__MUCSE_AXI_FAULT, ...) anywhere, and the existing
firmware-mediated reset rnpgbe_reset_hw() (which clears
RNPGBE_DMA_AXI_EN) is only called from rnpgbe_add_adapter() during
probe, never from this path.
After the bit is latched, rnpgbe_stop_all_tx_rings() always ends with
"return err ? err : -EIO;", so rnpgbe_configure_tx() can never succeed
again, and rnpgbe_open() bails out with -EIO on every subsequent
"ip link set <dev> up". Is unbind/rmmod the only intended recovery?
There is also an ordering question about when the poll first runs. The
quiesce poll is on the entry path:
rnpgbe_open()
rnpgbe_configure()
rnpgbe_configure_tx()
rnpgbe_stop_all_tx_rings()
rnpgbe_wait_tx_dma_idle()
mucse->num_tx_queues is already non-zero after probe, so this runs
before any ring has ever been started. If the tx_wr/tx_rd done bits in
RNPGBE_DMA_STATUS[23:22] do not read as set in the post-reset state,
would the very first open permanently latch the fault and leave the
netdev un-openable?
Could the commit message also mention the new terminal state? It
enumerates the descriptors, rnpgbe_xmit_frame_ring(), ring
alloc/teardown, rnpgbe_clean_tx_irq(), stats, the 56-bit DMA mask,
cycles_per_us and NETIF_F_HIGHDMA, but not that ndo_open can now fail
permanently on a hardware timeout.
> +
> +/**
> + * rnpgbe_stop_all_tx_rings - Stop TX DMA on all queues
> + * @mucse: board private structure
> + *
> + * Return: 0 when TX DMA was quiesced, negative errno otherwise
> + **/
> +static int rnpgbe_stop_all_tx_rings(struct mucse *mucse)
> +{
> + struct mucse_hw *hw = &mucse->hw;
> + u32 dma_axi_ctl;
> + int err = 0;
> +
> + for (int i = 0; i < mucse->num_tx_queues; i++)
> + rnpgbe_stop_tx_ring(mucse->tx_ring[i]);
> +
> + if (mucse->num_tx_queues &&
> + !test_bit(__MUCSE_AXI_FAULT, &mucse->state))
> + err = rnpgbe_wait_tx_dma_idle(mucse);
> +
> + dma_axi_ctl = mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
> + dma_axi_ctl &= ~TX_AXI_RW_EN;
> + mucse_hw_wr32(hw, RNPGBE_DMA_AXI_EN, dma_axi_ctl);
> + /* Flush the posted write before continuing. */
> + (void)mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
> +
> + if (!test_bit(__MUCSE_AXI_FAULT, &mucse->state))
> + return 0;
> +
> + return err ? err : -EIO;
> +}
> +
> +/**
> + * rnpgbe_clean_tx_ring - Free Tx Buffers
> + * @tx_ring: ring to be cleaned
> + **/
> +static void rnpgbe_clean_tx_ring(struct mucse_ring *tx_ring)
> +{
[ ... body of rnpgbe_clean_tx_ring() snipped ... ]
> + netdev_tx_reset_queue(txring_txq(tx_ring));
> + size = sizeof(struct mucse_tx_buffer) * tx_ring->count;
> + memset(tx_ring->tx_buffer_info, 0, size);
> + /* Zero out the descriptor ring */
> + memset(tx_ring->desc, 0, tx_ring->size);
> + tx_ring->next_to_use = 0;
> + tx_ring->next_to_clean = 0;
> +}
> +
> +/**
> + * rnpgbe_clean_all_tx_rings - Stop TX DMA and free Tx buffers for all queues
> + * @mucse: board private structure
> + **/
> +void rnpgbe_clean_all_tx_rings(struct mucse *mucse)
> +{
> + rnpgbe_stop_all_tx_rings(mucse);
> +
> + for (int i = 0; i < mucse->num_tx_queues; i++)
> + rnpgbe_clean_tx_ring(mucse->tx_ring[i]);
> +}
[Severity: Medium]
Is it safe to discard the return value of rnpgbe_stop_all_tx_rings()
here?
rnpgbe_stop_tx_ring() only writes RNPGBE_TX_START=0, and its own comment
says "DMA for descriptors fetched before the stop may still be in
flight". The only thing that establishes retirement is the
RNPGBE_DMA_TX_STATUS poll in rnpgbe_wait_tx_dma_idle(). When that poll
times out, rnpgbe_configure_tx() refuses to re-arm TX, but this caller
proceeds anyway:
rnpgbe_down()
rnpgbe_clean_all_tx_rings()
rnpgbe_stop_all_tx_rings() /* -EIO ignored */
rnpgbe_clean_tx_ring() /* dma_unmap_*() + memset(desc) */
and then:
rnpgbe_close()
rnpgbe_free_all_tx_resources()
rnpgbe_free_tx_resources()
dma_free_coherent(tx_ring->dev, tx_ring->size, tx_ring->desc, ...)
So the streaming mappings are released and the coherent descriptor ring
is handed back to the allocator while RNPGBE_TX_BASE_ADDR_LO/HI still
point at it, based on the comment's claim that "Hardware stops DMA
accesses". Can that claim be verified? It is asserted in exactly the
state where the register used to observe TX DMA retirement is the thing
that failed to report done.
Clearing TX_AXI_RW_EN gates new AXI transactions rather than draining
ones already accepted by the fabric, so does it substitute for the
quiesce handshake? If the engine is still active, a late write into
recycled pages or an IOMMU fault against the freed ring both look
possible. Would it be preferable to quarantine (leak) the mappings and
the coherent ring when the quiesce fails, instead of releasing them?
> +
> bool rnpgbe_down(struct mucse *mucse)
> {
> + struct net_device *netdev = mucse->netdev;
> +
> if (test_and_set_bit(__MUCSE_DOWN, &mucse->state))
> return false;
>
> + netif_tx_disable(netdev);
> rnpgbe_napi_disable_all(mucse);
> rnpgbe_irq_disable(mucse);
> + rnpgbe_clean_all_tx_rings(mucse);
>
> return true;
> }
> @@ -653,11 +933,420 @@ bool rnpgbe_down(struct mucse *mucse)
[ ... rnpgbe_up_complete(), TX resource setup/teardown, rnpgbe_tx_map()
and rnpgbe_xmit_frame_ring() snipped ... ]
> +/**
> + * rnpgbe_get_stats64 - Get stats for this netdev
> + * @netdev: network interface device structure
> + * @stats: stats data
> + **/
> +void rnpgbe_get_stats64(struct net_device *netdev,
> + struct rtnl_link_stats64 *stats)
> +{
> + struct mucse *mucse = netdev_priv(netdev);
> + int i;
> +
> + rcu_read_lock();
> + for (i = 0; i < mucse->num_tx_queues; i++) {
> + struct mucse_ring *ring = READ_ONCE(mucse->tx_ring[i]);
> + u64 bytes, packets, dropped;
> + unsigned int start;
> +
> + if (ring) {
> + do {
> + start = u64_stats_fetch_begin(&ring->syncp);
> + packets = ring->stats.packets;
> + bytes = ring->stats.bytes;
> + } while (u64_stats_fetch_retry(&ring->syncp, start));
> + dropped = atomic64_read(&ring->stats.dropped);
> +
> + stats->tx_packets += packets;
> + stats->tx_dropped += dropped;
> + stats->tx_bytes += bytes;
> + }
> + }
> + rcu_read_unlock();
> }
[Severity: Medium]
Does the rcu_read_lock() plus READ_ONCE() here actually protect the ring
memory?
mucse->tx_ring[i] points inside the struct mucse_q_vector allocation,
and the free side does not use an RCU grace period:
rnpgbe_free_q_vector()
mucse_for_each_ring(ring, q_vector->tx)
mucse->tx_ring[ring->queue_index] = NULL;
...
netif_napi_del(&q_vector->napi);
kfree(q_vector);
On the shutdown path the free also happens outside rtnl while the netdev
is still registered:
rnpgbe_dev_shutdown()
rtnl_lock();
netif_device_detach(netdev);
if (netif_running(netdev))
rnpgbe_close(netdev);
rtnl_unlock();
rnpgbe_free_mbx_irq(mucse);
rnpgbe_clear_interrupt_scheme(mucse); /* kfree(q_vector) */
So can this sequence read freed memory?
CPU0: reboot -> rnpgbe_shutdown() -> rnpgbe_dev_shutdown()
-> rnpgbe_clear_interrupt_scheme() -> kfree(q_vector)
CPU1: cat /proc/net/dev -> dev_seq_show() -> dev_get_stats()
-> rnpgbe_get_stats64()
ring = READ_ONCE(mucse->tx_ring[i]); /* loaded pre-NULL */
packets = ring->stats.packets; /* freed slab */
Would kfree_rcu() for the q_vector (or unregistering the netdev before
freeing the vectors) be the right pairing for this reader?
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index b7472dba2a19..5be78afd0b25 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
[ ... ]
> @@ -26,6 +27,19 @@ static struct pci_device_id rnpgbe_pci_tbl[] = {
> { },
> };
>
> +/**
> + * rnpgbe_configure - Configure the hardware
> + * @mucse: pointer to private structure
> + *
> + * rnpgbe_configure configure mac, tx, rx regs to hw
> + *
> + * Return: 0 on success, negative errno if hardware configuration fails
> + **/
> +static int rnpgbe_configure(struct mucse *mucse)
> +{
> + return rnpgbe_configure_tx(mucse);
> +}
[Severity: Low]
This isn't a bug, but the kernel-doc says "configure mac, tx, rx regs to
hw" while the body only calls rnpgbe_configure_tx(). This resolves later
in the same series: the final state adds rnpgbe_configure_rx() and
rnpgbe_set_rx_mode() to this function, at which point the description
matches. Mentioning it only for completeness.
> @@ -40,6 +54,9 @@ static int rnpgbe_open(struct net_device *netdev)
> struct mucse *mucse = netdev_priv(netdev);
> int err;
>
> + if (test_bit(__MUCSE_AXI_FAULT, &mucse->state))
> + return -EIO;
> +
> err = rnpgbe_request_irq(mucse);
> if (err)
> return err;
[ ... ]
> @@ -83,25 +111,37 @@ static int rnpgbe_close(struct net_device *netdev)
> * @skb: skb structure to be sent
> * @netdev: network interface device structure
> *
> - * Return: NETDEV_TX_OK
> + * Return: NETDEV_TX_OK or NETDEV_TX_BUSY when insufficient descriptors
> **/
> static netdev_tx_t rnpgbe_xmit_frame(struct sk_buff *skb,
> struct net_device *netdev)
> {
> struct mucse *mucse = netdev_priv(netdev);
> + struct mucse_ring *tx_ring;
>
> - dev_kfree_skb_any(skb);
> - mucse->stats.tx_dropped++;
> + tx_ring = mucse->tx_ring[skb_get_queue_mapping(skb)];
>
> - return NETDEV_TX_OK;
> + if (unlikely(skb_put_padto(skb, RNPGBE_TX_MIN_PKT_LEN))) {
> + atomic64_inc(&tx_ring->stats.dropped);
> + return NETDEV_TX_OK;
> + }
> +
> + return rnpgbe_xmit_frame_ring(skb, tx_ring);
> }
>
> static const struct net_device_ops rnpgbe_netdev_ops = {
> .ndo_open = rnpgbe_open,
> .ndo_stop = rnpgbe_close,
> .ndo_start_xmit = rnpgbe_xmit_frame,
> + .ndo_get_stats64 = rnpgbe_get_stats64,
> };
[Severity: Medium]
This patch makes the TX queues stoppable, but the ops table gains only
.ndo_get_stats64 and there is still no .ndo_tx_timeout (and
netdev->watchdog_timeo is never set). Was that intentional?
rnpgbe_xmit_frame_ring() stops the queue:
if (!netif_txq_maybe_stop(txring_txq(tx_ring),
mucse_desc_unused(tx_ring),
count + RESV_DESC_NEEDED,
count + RESV_DESC_NEEDED))
return NETDEV_TX_BUSY;
and the only wake is __netif_txq_completed_wake() inside
rnpgbe_clean_tx_irq(), which needs a TX completion interrupt to schedule
NAPI. Meanwhile the core watchdog never arms:
net/sched/sch_generic.c:netdev_watchdog_up() {
if (!dev->netdev_ops->ndo_tx_timeout)
return;
...
}
So if a TX completion interrupt is lost or the TX engine stalls, does
anything detect the permanently stopped queue? The driver itself defines
RNPGBE_DMA_INT_TRIG as a "lost-interrupt recovery trigger", which
suggests interrupt loss is a real case for this hardware. The ops table
is still open/stop/start_xmit/set_rx_mode/get_stats64 at the end of the
series.
[ ... ]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH net-next v10 2/5] net: rnpgbe: Add basic TX packet transmission support
2026-09-04 21:57 ` Jakub Kicinski
@ 2026-09-08 2:16 ` Yibo Dong
0 siblings, 0 replies; 15+ messages in thread
From: Yibo Dong @ 2026-09-08 2:16 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, davem, edumazet, pabeni, vadim.fedorenko,
u.kleine-koenig, netdev, linux-kernel, yaojun
On Fri, Sep 04, 2026 at 02:57:13PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: rnpgbe: Add basic TX packet transmission support
>
> This adds the TX path for rnpgbe: TX descriptor and buffer structures,
> rnpgbe_xmit_frame_ring(), TX ring setup/teardown, completion handling in
> rnpgbe_clean_tx_irq(), and TX counters exported via ndo_get_stats64().
> It also switches probe to dma_set_mask_and_coherent(DMA_BIT_MASK(56))
> and adds hw->cycles_per_us plus NETIF_F_HIGHDMA.
>
> A few questions below about the new fault state, the teardown error
> handling, the stats reader and the missing TX watchdog.
>
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > index 77304196c2b6..6d0b9f424c64 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
>
> [ ... ]
>
> > @@ -74,21 +139,20 @@ struct mucse_q_vector {
> > struct mucse_ring ring[] ____cacheline_internodealigned_in_smp;
> > };
> >
> > -struct mucse_stats {
> > - u64 tx_dropped;
> > -};
> > -
> > #define MAX_Q_VECTORS 8
> >
> > +#define M_DEFAULT_TXD 512
> > +#define M_DEFAULT_TX_WORK 256
> > +
> > enum mucse_state_t {
> > __MUCSE_DOWN,
> > + __MUCSE_AXI_FAULT,
> > };
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> > index f511a3a5f916..ff81d222edd3 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
>
> [ ... ]
>
> > @@ -636,13 +760,169 @@ static void rnpgbe_napi_disable_all(struct mucse *mucse)
> > napi_disable(&mucse->q_vector[i]->napi);
> > }
> >
> > +static void rnpgbe_stop_tx_ring(struct mucse_ring *tx_ring)
> > +{
> > + if (!tx_ring->tx_buffer_info)
> > + return;
> > +
> > + /* Stop hw. No new descriptors are fetched after TX_START=0.
> > + * DMA for descriptors fetched before the stop may still be in flight.
> > + */
> > + mucse_ring_wr32(tx_ring, RNPGBE_TX_START, 0);
> > + /* Flush posted write to ensure hardware sees TX_START=0 */
> > + (void)mucse_ring_rd32(tx_ring, RNPGBE_TX_START);
> > +}
> > +
> > +static int rnpgbe_wait_tx_dma_idle(struct mucse *mucse)
> > +{
> > + struct mucse_hw *hw = &mucse->hw;
> > + u32 dma_status;
> > + int err;
> > +
> > + /* A timeout indicates a terminal AXI fault. Hardware stops DMA
> > + * accesses, so teardown may release mappings, but configuration
> > + * must not re-enable TX DMA.
> > + */
> > + err = readl_poll_timeout(hw->hw_addr + RNPGBE_DMA_STATUS,
> > + dma_status,
> > + (dma_status & RNPGBE_DMA_TX_STATUS) ==
> > + RNPGBE_DMA_TX_STATUS,
> > + 10, 100000);
> > + if (err) {
> > + set_bit(__MUCSE_AXI_FAULT, &mucse->state);
> > + dev_err(&mucse->pdev->dev,
> > + "TX DMA failed to quiesce, status %#x\n", dma_status);
> > + }
> > +
> > + return err;
> > +}
>
> [Severity: Medium]
> Once __MUCSE_AXI_FAULT is set here, is there a way back?
>
> Grepping drivers/net/ethernet/mucse/ at the end of the series shows no
> clear_bit(__MUCSE_AXI_FAULT, ...) anywhere, and the existing
> firmware-mediated reset rnpgbe_reset_hw() (which clears
> RNPGBE_DMA_AXI_EN) is only called from rnpgbe_add_adapter() during
> probe, never from this path.
>
> After the bit is latched, rnpgbe_stop_all_tx_rings() always ends with
> "return err ? err : -EIO;", so rnpgbe_configure_tx() can never succeed
> again, and rnpgbe_open() bails out with -EIO on every subsequent
> "ip link set <dev> up". Is unbind/rmmod the only intended recovery?
The AXI timeout is terminal for the current driver instance. The device
requires a chip-level PCIe reset, which rnpgbe_reset_hw() cannot provide.
For now recovery requires a platform reset followed by driver unbind/bind
(or module reload) to create a fresh driver instance.
>
> There is also an ordering question about when the poll first runs. The
> quiesce poll is on the entry path:
>
> rnpgbe_open()
> rnpgbe_configure()
> rnpgbe_configure_tx()
> rnpgbe_stop_all_tx_rings()
> rnpgbe_wait_tx_dma_idle()
>
> mucse->num_tx_queues is already non-zero after probe, so this runs
> before any ring has ever been started. If the tx_wr/tx_rd done bits in
> RNPGBE_DMA_STATUS[23:22] do not read as set in the post-reset state,
> would the very first open permanently latch the fault and leave the
> netdev un-openable?
No. The DMA status bits report whether the engine is idle and are set
after a chip-level reset even before TX has been started. Therefore the
initial configuration path does not falsely latch the fault.
The check is intentionally kept on this path because __MUCSE_AXI_FAULT is
per-driver-instance state. A driver reload without a chip-level reset
clears that software bit but does not recover the hardware fault; polling
DMA idle again prevents the new instance from using fault status hardware.
>
> Could the commit message also mention the new terminal state? It
> enumerates the descriptors, rnpgbe_xmit_frame_ring(), ring
> alloc/teardown, rnpgbe_clean_tx_irq(), stats, the 56-bit DMA mask,
> cycles_per_us and NETIF_F_HIGHDMA, but not that ndo_open can now fail
> permanently on a hardware timeout.
>
> > +
> > +/**
> > + * rnpgbe_stop_all_tx_rings - Stop TX DMA on all queues
> > + * @mucse: board private structure
> > + *
> > + * Return: 0 when TX DMA was quiesced, negative errno otherwise
> > + **/
> > +static int rnpgbe_stop_all_tx_rings(struct mucse *mucse)
> > +{
> > + struct mucse_hw *hw = &mucse->hw;
> > + u32 dma_axi_ctl;
> > + int err = 0;
> > +
> > + for (int i = 0; i < mucse->num_tx_queues; i++)
> > + rnpgbe_stop_tx_ring(mucse->tx_ring[i]);
> > +
> > + if (mucse->num_tx_queues &&
> > + !test_bit(__MUCSE_AXI_FAULT, &mucse->state))
> > + err = rnpgbe_wait_tx_dma_idle(mucse);
> > +
> > + dma_axi_ctl = mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
> > + dma_axi_ctl &= ~TX_AXI_RW_EN;
> > + mucse_hw_wr32(hw, RNPGBE_DMA_AXI_EN, dma_axi_ctl);
> > + /* Flush the posted write before continuing. */
> > + (void)mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
> > +
> > + if (!test_bit(__MUCSE_AXI_FAULT, &mucse->state))
> > + return 0;
> > +
> > + return err ? err : -EIO;
> > +}
> > +
> > +/**
> > + * rnpgbe_clean_tx_ring - Free Tx Buffers
> > + * @tx_ring: ring to be cleaned
> > + **/
> > +static void rnpgbe_clean_tx_ring(struct mucse_ring *tx_ring)
> > +{
>
> [ ... body of rnpgbe_clean_tx_ring() snipped ... ]
>
> > + netdev_tx_reset_queue(txring_txq(tx_ring));
> > + size = sizeof(struct mucse_tx_buffer) * tx_ring->count;
> > + memset(tx_ring->tx_buffer_info, 0, size);
> > + /* Zero out the descriptor ring */
> > + memset(tx_ring->desc, 0, tx_ring->size);
> > + tx_ring->next_to_use = 0;
> > + tx_ring->next_to_clean = 0;
> > +}
> > +
> > +/**
> > + * rnpgbe_clean_all_tx_rings - Stop TX DMA and free Tx buffers for all queues
> > + * @mucse: board private structure
> > + **/
> > +void rnpgbe_clean_all_tx_rings(struct mucse *mucse)
> > +{
> > + rnpgbe_stop_all_tx_rings(mucse);
> > +
> > + for (int i = 0; i < mucse->num_tx_queues; i++)
> > + rnpgbe_clean_tx_ring(mucse->tx_ring[i]);
> > +}
>
> [Severity: Medium]
> Is it safe to discard the return value of rnpgbe_stop_all_tx_rings()
> here?
>
> rnpgbe_stop_tx_ring() only writes RNPGBE_TX_START=0, and its own comment
> says "DMA for descriptors fetched before the stop may still be in
> flight". The only thing that establishes retirement is the
> RNPGBE_DMA_TX_STATUS poll in rnpgbe_wait_tx_dma_idle(). When that poll
> times out, rnpgbe_configure_tx() refuses to re-arm TX, but this caller
> proceeds anyway:
>
> rnpgbe_down()
> rnpgbe_clean_all_tx_rings()
> rnpgbe_stop_all_tx_rings() /* -EIO ignored */
> rnpgbe_clean_tx_ring() /* dma_unmap_*() + memset(desc) */
>
> and then:
>
> rnpgbe_close()
> rnpgbe_free_all_tx_resources()
> rnpgbe_free_tx_resources()
> dma_free_coherent(tx_ring->dev, tx_ring->size, tx_ring->desc, ...)
>
> So the streaming mappings are released and the coherent descriptor ring
> is handed back to the allocator while RNPGBE_TX_BASE_ADDR_LO/HI still
> point at it, based on the comment's claim that "Hardware stops DMA
> accesses". Can that claim be verified? It is asserted in exactly the
> state where the register used to observe TX DMA retirement is the thing
> that failed to report done.
>
> Clearing TX_AXI_RW_EN gates new AXI transactions rather than draining
> ones already accepted by the fabric, so does it substitute for the
> quiesce handshake? If the engine is still active, a late write into
> recycled pages or an IOMMU fault against the freed ring both look
> possible. Would it be preferable to quarantine (leak) the mappings and
> the coherent ring when the quiesce fails, instead of releasing them?
>
The return value is intentionally ignored on the cleanup path. On the
first timeout the driver logs the failure and latches __MUCSE_AXI_FAULT.
In that hardware fault state the device guarantees that it issues no
DMA requests. Therefore the DMA mappings and descriptor memory can be
released safely.
> > +
> > bool rnpgbe_down(struct mucse *mucse)
> > {
> > + struct net_device *netdev = mucse->netdev;
> > +
> > if (test_and_set_bit(__MUCSE_DOWN, &mucse->state))
> > return false;
> >
> > + netif_tx_disable(netdev);
> > rnpgbe_napi_disable_all(mucse);
> > rnpgbe_irq_disable(mucse);
> > + rnpgbe_clean_all_tx_rings(mucse);
> >
> > return true;
> > }
>
> > @@ -653,11 +933,420 @@ bool rnpgbe_down(struct mucse *mucse)
>
> [ ... rnpgbe_up_complete(), TX resource setup/teardown, rnpgbe_tx_map()
> and rnpgbe_xmit_frame_ring() snipped ... ]
>
> > +/**
> > + * rnpgbe_get_stats64 - Get stats for this netdev
> > + * @netdev: network interface device structure
> > + * @stats: stats data
> > + **/
> > +void rnpgbe_get_stats64(struct net_device *netdev,
> > + struct rtnl_link_stats64 *stats)
> > +{
> > + struct mucse *mucse = netdev_priv(netdev);
> > + int i;
> > +
> > + rcu_read_lock();
> > + for (i = 0; i < mucse->num_tx_queues; i++) {
> > + struct mucse_ring *ring = READ_ONCE(mucse->tx_ring[i]);
> > + u64 bytes, packets, dropped;
> > + unsigned int start;
> > +
> > + if (ring) {
> > + do {
> > + start = u64_stats_fetch_begin(&ring->syncp);
> > + packets = ring->stats.packets;
> > + bytes = ring->stats.bytes;
> > + } while (u64_stats_fetch_retry(&ring->syncp, start));
> > + dropped = atomic64_read(&ring->stats.dropped);
> > +
> > + stats->tx_packets += packets;
> > + stats->tx_dropped += dropped;
> > + stats->tx_bytes += bytes;
> > + }
> > + }
> > + rcu_read_unlock();
> > }
>
> [Severity: Medium]
> Does the rcu_read_lock() plus READ_ONCE() here actually protect the ring
> memory?
>
> mucse->tx_ring[i] points inside the struct mucse_q_vector allocation,
> and the free side does not use an RCU grace period:
>
> rnpgbe_free_q_vector()
> mucse_for_each_ring(ring, q_vector->tx)
> mucse->tx_ring[ring->queue_index] = NULL;
> ...
> netif_napi_del(&q_vector->napi);
> kfree(q_vector);
>
> On the shutdown path the free also happens outside rtnl while the netdev
> is still registered:
>
> rnpgbe_dev_shutdown()
> rtnl_lock();
> netif_device_detach(netdev);
> if (netif_running(netdev))
> rnpgbe_close(netdev);
> rtnl_unlock();
>
> rnpgbe_free_mbx_irq(mucse);
> rnpgbe_clear_interrupt_scheme(mucse); /* kfree(q_vector) */
>
> So can this sequence read freed memory?
>
> CPU0: reboot -> rnpgbe_shutdown() -> rnpgbe_dev_shutdown()
> -> rnpgbe_clear_interrupt_scheme() -> kfree(q_vector)
>
> CPU1: cat /proc/net/dev -> dev_seq_show() -> dev_get_stats()
> -> rnpgbe_get_stats64()
> ring = READ_ONCE(mucse->tx_ring[i]); /* loaded pre-NULL */
> packets = ring->stats.packets; /* freed slab */
>
> Would kfree_rcu() for the q_vector (or unregistering the netdev before
> freeing the vectors) be the right pairing for this reader?
>
Got it, I should use kfree_rcu here. I will fix this in next version.
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> > index b7472dba2a19..5be78afd0b25 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
>
> [ ... ]
>
> > @@ -26,6 +27,19 @@ static struct pci_device_id rnpgbe_pci_tbl[] = {
> > { },
> > };
> >
> > +/**
> > + * rnpgbe_configure - Configure the hardware
> > + * @mucse: pointer to private structure
> > + *
> > + * rnpgbe_configure configure mac, tx, rx regs to hw
> > + *
> > + * Return: 0 on success, negative errno if hardware configuration fails
> > + **/
> > +static int rnpgbe_configure(struct mucse *mucse)
> > +{
> > + return rnpgbe_configure_tx(mucse);
> > +}
>
> [Severity: Low]
> This isn't a bug, but the kernel-doc says "configure mac, tx, rx regs to
> hw" while the body only calls rnpgbe_configure_tx(). This resolves later
> in the same series: the final state adds rnpgbe_configure_rx() and
> rnpgbe_set_rx_mode() to this function, at which point the description
> matches. Mentioning it only for completeness.
>
Will update this in next version.
> > @@ -40,6 +54,9 @@ static int rnpgbe_open(struct net_device *netdev)
> > struct mucse *mucse = netdev_priv(netdev);
> > int err;
> >
> > + if (test_bit(__MUCSE_AXI_FAULT, &mucse->state))
> > + return -EIO;
> > +
> > err = rnpgbe_request_irq(mucse);
> > if (err)
> > return err;
>
> [ ... ]
>
> > @@ -83,25 +111,37 @@ static int rnpgbe_close(struct net_device *netdev)
> > * @skb: skb structure to be sent
> > * @netdev: network interface device structure
> > *
> > - * Return: NETDEV_TX_OK
> > + * Return: NETDEV_TX_OK or NETDEV_TX_BUSY when insufficient descriptors
> > **/
> > static netdev_tx_t rnpgbe_xmit_frame(struct sk_buff *skb,
> > struct net_device *netdev)
> > {
> > struct mucse *mucse = netdev_priv(netdev);
> > + struct mucse_ring *tx_ring;
> >
> > - dev_kfree_skb_any(skb);
> > - mucse->stats.tx_dropped++;
> > + tx_ring = mucse->tx_ring[skb_get_queue_mapping(skb)];
> >
> > - return NETDEV_TX_OK;
> > + if (unlikely(skb_put_padto(skb, RNPGBE_TX_MIN_PKT_LEN))) {
> > + atomic64_inc(&tx_ring->stats.dropped);
> > + return NETDEV_TX_OK;
> > + }
> > +
> > + return rnpgbe_xmit_frame_ring(skb, tx_ring);
> > }
> >
> > static const struct net_device_ops rnpgbe_netdev_ops = {
> > .ndo_open = rnpgbe_open,
> > .ndo_stop = rnpgbe_close,
> > .ndo_start_xmit = rnpgbe_xmit_frame,
> > + .ndo_get_stats64 = rnpgbe_get_stats64,
> > };
>
> [Severity: Medium]
> This patch makes the TX queues stoppable, but the ops table gains only
> .ndo_get_stats64 and there is still no .ndo_tx_timeout (and
> netdev->watchdog_timeo is never set). Was that intentional?
>
> rnpgbe_xmit_frame_ring() stops the queue:
>
> if (!netif_txq_maybe_stop(txring_txq(tx_ring),
> mucse_desc_unused(tx_ring),
> count + RESV_DESC_NEEDED,
> count + RESV_DESC_NEEDED))
> return NETDEV_TX_BUSY;
>
> and the only wake is __netif_txq_completed_wake() inside
> rnpgbe_clean_tx_irq(), which needs a TX completion interrupt to schedule
> NAPI. Meanwhile the core watchdog never arms:
>
> net/sched/sch_generic.c:netdev_watchdog_up() {
> if (!dev->netdev_ops->ndo_tx_timeout)
> return;
> ...
> }
>
> So if a TX completion interrupt is lost or the TX engine stalls, does
> anything detect the permanently stopped queue? The driver itself defines
> RNPGBE_DMA_INT_TRIG as a "lost-interrupt recovery trigger", which
> suggests interrupt loss is a real case for this hardware. The ops table
> is still open/stop/start_xmit/set_rx_mode/get_stats64 at the end of the
> series.
>
> [ ... ]
>
This series is intentionally limited to establishing the basic Tx and Rx
data paths. Normal descriptor exhaustion is handled by the completion path,
which wakes the stopped Tx queue.
Would it be acceptable to add ndo_tx_timeout and the associated hardware
recovery sequence in a follow-up series?
The timeout recovery needs a complete reset/reinitialization design, so
I would prefer to keep it separate from this initial datapath enablement series.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v10 3/5] net: rnpgbe: Add RX packet reception support
[not found] <20260831073608.401988-1-dong100@mucse.com>
2026-08-31 7:36 ` [PATCH net-next v10 1/5] net: rnpgbe: Add interrupt handling Dong Yibo
2026-08-31 7:36 ` [PATCH net-next v10 2/5] net: rnpgbe: Add basic TX packet transmission support Dong Yibo
@ 2026-08-31 7:36 ` Dong Yibo
2026-09-04 21:57 ` Jakub Kicinski
2026-08-31 7:36 ` [PATCH net-next v10 4/5] net: rnpgbe: Add receive mode support Dong Yibo
2026-08-31 7:36 ` [PATCH net-next v10 5/5] net: rnpgbe: Add link status handling support Dong Yibo
4 siblings, 1 reply; 15+ messages in thread
From: Dong Yibo @ 2026-08-31 7:36 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, vadim.fedorenko,
u.kleine-koenig
Cc: netdev, linux-kernel, dong100, yaojun
Add basic RX packet reception for the RNPGBE driver.
Introduce the RX descriptor structure (union rnpgbe_rx_desc) with the
hardware write-back format, and manage RX buffers with page_pool using a
single page per descriptor without page splitting. Implement the NAPI
poll callback, RX ring setup and teardown, and packet assembly from page
buffers, along with RX statistics.
Keep a 16-descriptor gap in the fixed 512-entry RX ring so at most 496
descriptors are posted to hardware. Retry failed page allocations with
one timer per q_vector that schedules NAPI and is cancelled before RX
cleanup and q_vector teardown.
Disable NAPI before disabling interrupts and quiesce RX DMA once for all
rings before cancelling the refill timers and releasing RX resources. A
quiesce timeout is a terminal AXI fault: keep DMA disabled, release the
existing mappings safely, and reject subsequent opens until the device is
reset and reprobed. Do not re-enable queue interrupts while the device is
down.
Signed-off-by: Dong Yibo <dong100@mucse.com>
---
drivers/net/ethernet/mucse/Kconfig | 1 +
drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 58 +-
drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 2 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_lib.c | 780 +++++++++++++++++-
.../net/ethernet/mucse/rnpgbe/rnpgbe_lib.h | 21 +-
.../net/ethernet/mucse/rnpgbe/rnpgbe_main.c | 17 +-
6 files changed, 870 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/mucse/Kconfig b/drivers/net/ethernet/mucse/Kconfig
index 0b3e853d625f..be0fdf268484 100644
--- a/drivers/net/ethernet/mucse/Kconfig
+++ b/drivers/net/ethernet/mucse/Kconfig
@@ -19,6 +19,7 @@ if NET_VENDOR_MUCSE
config MGBE
tristate "Mucse(R) 1GbE PCI Express adapters support"
depends on PCI
+ select PAGE_POOL
help
This driver supports Mucse(R) 1GbE PCI Express family of
adapters.
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 6d0b9f424c64..8c3d8bbfcc1a 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -7,6 +7,7 @@
#include <linux/types.h>
#include <linux/mutex.h>
#include <linux/netdevice.h>
+#include <linux/timer.h>
#include <linux/if.h>
#include <linux/workqueue.h>
@@ -62,7 +63,32 @@ struct rnpgbe_tx_desc {
#define M_TXD_CMD_EOP 0x010000 /* End of Packet */
};
+union rnpgbe_rx_desc {
+ struct {
+ __le64 pkt_addr; /* Packet buffer address */
+ __le64 resv_cmd; /* cmd status */
+ };
+ struct {
+ __le32 rss_hash; /* RSS HASH */
+ __le16 mark; /* mark info */
+ __le16 rev1;
+ __le16 len; /* Packet length */
+ __le16 padding_len;
+ __le16 vlan; /* VLAN tag */
+ __le16 cmd; /* cmd status */
+#define M_RXD_STAT_DD BIT(1) /* Descriptor Done */
+#define M_RXD_STAT_EOP BIT(0) /* End of Packet */
+ } wb;
+};
+
#define M_TX_DESC(R, i) (&(((struct rnpgbe_tx_desc *)((R)->desc))[i]))
+#define M_RX_DESC(R, i) (&(((union rnpgbe_rx_desc *)((R)->desc))[i]))
+
+static inline __le16 rnpgbe_test_staterr(union rnpgbe_rx_desc *rx_desc,
+ const u16 stat_err_bits)
+{
+ return rx_desc->wb.cmd & cpu_to_le16(stat_err_bits);
+}
struct mucse_tx_buffer {
struct rnpgbe_tx_desc *next_to_watch;
@@ -80,13 +106,24 @@ struct mucse_queue_stats {
atomic64_t dropped;
};
+struct mucse_rx_buffer {
+ struct sk_buff *skb;
+ dma_addr_t dma;
+ struct page *page;
+ u32 page_offset;
+};
+
struct mucse_ring {
struct mucse_ring *next;
struct mucse_q_vector *q_vector;
struct net_device *netdev;
struct device *dev;
+ struct page_pool *page_pool;
void *desc;
- struct mucse_tx_buffer *tx_buffer_info;
+ union {
+ struct mucse_tx_buffer *tx_buffer_info;
+ struct mucse_rx_buffer *rx_buffer_info;
+ };
void __iomem *ring_addr;
void __iomem *tail;
void __iomem *irq_mask;
@@ -102,8 +139,12 @@ struct mucse_ring {
unsigned int size;
struct mucse_queue_stats stats;
struct u64_stats_sync syncp;
+ bool drop_status;
} ____cacheline_internodealigned_in_smp;
+/* Refill RX descriptors in batches of this size. */
+#define M_RX_BUFFER_WRITE 16
+
static inline u16 mucse_desc_unused(struct mucse_ring *ring)
{
u16 ntc = ring->next_to_clean;
@@ -112,6 +153,18 @@ static inline u16 mucse_desc_unused(struct mucse_ring *ring)
return ((ntc > ntu) ? 0 : ring->count) + ntc - ntu - 1;
}
+static inline u16 mucse_desc_unused_rx(struct mucse_ring *ring)
+{
+ u16 ntc = ring->next_to_clean;
+ u16 ntu = ring->next_to_use;
+
+ /* Keep M_RX_BUFFER_WRITE descriptors unused so the ring is not filled
+ * completely. Refill is attempted once at least this many descriptors
+ * are available.
+ */
+ return ((ntc > ntu) ? 0 : ring->count) + ntc - ntu - M_RX_BUFFER_WRITE;
+}
+
static inline __le64 build_ctob(u32 vlan_cmd, u32 mac_ip_len, u32 size)
{
return cpu_to_le64(((u64)vlan_cmd << 32) | ((u64)mac_ip_len << 16) |
@@ -134,6 +187,7 @@ struct mucse_q_vector {
int hw_vector;
struct mucse_ring_container rx, tx;
struct napi_struct napi;
+ struct timer_list rx_alloc_timer;
char name[IFNAMSIZ + 18];
/* for dynamic allocation of rings associated with this q_vector */
struct mucse_ring ring[] ____cacheline_internodealigned_in_smp;
@@ -142,6 +196,7 @@ struct mucse_q_vector {
#define MAX_Q_VECTORS 8
#define M_DEFAULT_TXD 512
+#define M_DEFAULT_RXD 512
#define M_DEFAULT_TX_WORK 256
enum mucse_state_t {
@@ -165,6 +220,7 @@ struct mucse {
int tx_work_limit;
int num_tx_queues;
int num_q_vectors;
+ int rx_ring_item_count;
int num_rx_queues;
char mbx_name[32];
unsigned long state;
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
index 6dc29ebe6fa7..1d87edfba3d7 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
@@ -19,6 +19,8 @@
#define TX_AXI_RW_EN 0xc
/* DMA_STATUS_REG[23:20]: tx_wr, tx_rd, rx_wr, rx_rd done status. */
#define RNPGBE_DMA_TX_STATUS GENMASK_U32(23, 22)
+#define RNPGBE_DMA_RX_STATUS GENMASK_U32(21, 20)
+#define RX_AXI_RW_EN 0x03
#define RNPGBE_DMA_AXI_EN 0x0010
#define RNPGBE_TX_MIN_PKT_LEN 33
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
index ff81d222edd3..346e9a1cca8a 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
@@ -4,8 +4,10 @@
#include <linux/pci.h>
#include <linux/netdevice.h>
#include <linux/iopoll.h>
+#include <linux/etherdevice.h>
#include <linux/vmalloc.h>
#include <net/netdev_queues.h>
+#include <net/page_pool/helpers.h>
#include "rnpgbe_lib.h"
#include "rnpgbe.h"
@@ -198,15 +200,419 @@ static bool rnpgbe_clean_tx_irq(struct mucse_q_vector *q_vector,
return !!budget;
}
+static bool mucse_alloc_mapped_page(struct mucse_ring *rx_ring,
+ struct mucse_rx_buffer *bi)
+{
+ struct page *page = bi->page;
+ dma_addr_t dma;
+
+ if (page) {
+ /* Buffer is being reused without going back through the
+ * page_pool. Do dma_sync for hw use.
+ */
+ dma_sync_single_range_for_device(rx_ring->dev, bi->dma,
+ bi->page_offset,
+ PAGE_SIZE - bi->page_offset,
+ DMA_FROM_DEVICE);
+ return true;
+ }
+
+ page = page_pool_dev_alloc_pages(rx_ring->page_pool);
+ if (unlikely(!page))
+ return false;
+ dma = page_pool_get_dma_addr(page);
+
+ bi->dma = dma;
+ bi->page = page;
+ bi->page_offset = RNPGBE_SKB_PAD;
+
+ return true;
+}
+
+static void mucse_update_rx_tail(struct mucse_ring *rx_ring,
+ u32 val)
+{
+ rx_ring->next_to_use = val;
+ writel(val, rx_ring->tail);
+}
+
+/**
+ * rnpgbe_alloc_rx_buffers - Replace used receive buffers
+ * @rx_ring: ring to place buffers on
+ * @cleaned_count: number of buffers to replace
+ *
+ * Return: true if alloc failed
+ **/
+static bool rnpgbe_alloc_rx_buffers(struct mucse_ring *rx_ring,
+ u16 cleaned_count)
+{
+ u64 fun_id = ((u64)(rx_ring->pfvfnum) << 56);
+ union rnpgbe_rx_desc *rx_desc;
+ u16 i = rx_ring->next_to_use;
+ struct mucse_rx_buffer *bi;
+ bool err = false;
+ u64 addr;
+ /* nothing to do */
+ if (!cleaned_count)
+ return err;
+
+ rx_desc = M_RX_DESC(rx_ring, i);
+ bi = &rx_ring->rx_buffer_info[i];
+ i -= rx_ring->count;
+
+ do {
+ if (!mucse_alloc_mapped_page(rx_ring, bi)) {
+ err = true;
+ break;
+ }
+
+ addr = (u64)(bi->dma + bi->page_offset);
+ rx_desc->pkt_addr = cpu_to_le64(addr | fun_id);
+ /* clean dd */
+ rx_desc->resv_cmd = 0;
+ rx_desc++;
+ bi++;
+ i++;
+ if (unlikely(!i)) {
+ rx_desc = M_RX_DESC(rx_ring, 0);
+ bi = rx_ring->rx_buffer_info;
+ i -= rx_ring->count;
+ }
+ cleaned_count--;
+ } while (cleaned_count);
+
+ i += rx_ring->count;
+
+ /* Publish the tail even when allocation stopped early, so a stale
+ * tail from a previous session cannot point hardware at a
+ * reallocated ring.
+ */
+ dma_wmb();
+ mucse_update_rx_tail(rx_ring, i);
+
+ return err;
+}
+
+/**
+ * rnpgbe_get_buffer - Get the rx_buffer to be used
+ * @rx_ring: pointer to rx ring
+ * @skb: pointer skb for this packet
+ * @size: data size in this desc
+ *
+ * Return: rx_buffer.
+ **/
+static struct mucse_rx_buffer *rnpgbe_get_buffer(struct mucse_ring *rx_ring,
+ struct sk_buff **skb,
+ const unsigned int size)
+{
+ struct mucse_rx_buffer *rx_buffer;
+
+ rx_buffer = &rx_ring->rx_buffer_info[rx_ring->next_to_clean];
+ *skb = rx_buffer->skb;
+ prefetchw(page_address(rx_buffer->page) + rx_buffer->page_offset);
+ /* we are reusing so sync this buffer for CPU use */
+ dma_sync_single_range_for_cpu(rx_ring->dev, rx_buffer->dma,
+ rx_buffer->page_offset, size,
+ DMA_FROM_DEVICE);
+
+ return rx_buffer;
+}
+
+/**
+ * rnpgbe_add_rx_frag - Add non-linear data to the skb
+ * @rx_buffer: pointer to rx_buffer
+ * @skb: pointer skb for this packet
+ * @size: data size in this desc
+ **/
+static void rnpgbe_add_rx_frag(struct mucse_rx_buffer *rx_buffer,
+ struct sk_buff *skb,
+ unsigned int size)
+{
+ unsigned int truesize = PAGE_SIZE;
+
+ skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buffer->page,
+ rx_buffer->page_offset, size, truesize);
+}
+
+/**
+ * rnpgbe_build_skb - Try to build a skb based on rx_buffer
+ * @rx_buffer: pointer to rx_buffer
+ * @size: data size in this desc
+ *
+ * Return: skb for this rx_buffer
+ **/
+static struct sk_buff *rnpgbe_build_skb(struct mucse_rx_buffer *rx_buffer,
+ unsigned int size)
+{
+ void *va = page_address(rx_buffer->page) + rx_buffer->page_offset;
+ unsigned int truesize = PAGE_SIZE;
+ struct sk_buff *skb;
+
+ net_prefetch(va);
+ /* build an skb around the page buffer */
+ skb = napi_build_skb(va - RNPGBE_SKB_PAD, truesize);
+ if (unlikely(!skb))
+ return NULL;
+ /* update pointers within the skb to store the data */
+ skb_reserve(skb, RNPGBE_SKB_PAD);
+ __skb_put(skb, size);
+ skb_mark_for_recycle(skb);
+
+ return skb;
+}
+
+/**
+ * rnpgbe_is_non_eop - Process handling of non-EOP buffers
+ * @rx_ring: rx ring being processed
+ * @rx_desc: rx descriptor for current buffer
+ * @skb: current socket buffer containing buffer in progress
+ *
+ * This function updates next to clean. If the buffer is an EOP buffer
+ * this function exits returning false, otherwise it will place the
+ * sk_buff in the next buffer to be chained and return true indicating
+ * that this is in fact a non-EOP buffer.
+ *
+ * Return: true for not end of packet
+ **/
+static bool rnpgbe_is_non_eop(struct mucse_ring *rx_ring,
+ union rnpgbe_rx_desc *rx_desc,
+ struct sk_buff *skb)
+{
+ u32 ntc = rx_ring->next_to_clean + 1;
+
+ /* fetch, update, and store next to clean */
+ ntc = (ntc < rx_ring->count) ? ntc : 0;
+ rx_ring->next_to_clean = ntc;
+ prefetch(M_RX_DESC(rx_ring, ntc));
+ /* if we are the last buffer then there is nothing else to do */
+ if (likely(rnpgbe_test_staterr(rx_desc, M_RXD_STAT_EOP)))
+ return false;
+ if (skb_shinfo(skb)->nr_frags < MAX_SKB_FRAGS) {
+ /* place skb in next buffer to be received */
+ rx_ring->rx_buffer_info[ntc].skb = skb;
+ } else {
+ atomic64_inc(&rx_ring->stats.dropped);
+ /* too much frags, force free */
+ dev_kfree_skb_any(skb);
+ rx_ring->drop_status = true;
+ }
+ /* we should clean it since we used all info in it */
+ rx_desc->wb.cmd = 0;
+
+ return true;
+}
+
+/**
+ * rnpgbe_cleanup_headers - Correct corrupted or empty headers
+ * @skb: current socket buffer containing buffer in progress
+ *
+ * Return: true if an error was encountered and skb was freed.
+ **/
+static bool rnpgbe_cleanup_headers(struct sk_buff *skb)
+{
+ /* if eth_skb_pad returns an error the skb was freed */
+ if (eth_skb_pad(skb))
+ return true;
+
+ return false;
+}
+
+/**
+ * rnpgbe_process_skb_fields - Set the RX queue and protocol fields
+ * @rx_ring: RX descriptor ring containing the queue information
+ * @skb: skb currently being received
+ *
+ * Records the RX queue that received the skb and sets its protocol from
+ * the Ethernet header.
+ **/
+static void rnpgbe_process_skb_fields(struct mucse_ring *rx_ring,
+ struct sk_buff *skb)
+{
+ struct net_device *dev = rx_ring->netdev;
+
+ skb_record_rx_queue(skb, rx_ring->queue_index);
+ skb->protocol = eth_type_trans(skb, dev);
+}
+
+/**
+ * rnpgbe_rx_alloc_retry - Retry RX buffer allocation
+ * @timer: RX allocation retry timer
+ *
+ * Schedules NAPI after RX buffer allocation fails during polling or
+ * initial ring configuration.
+ **/
+static void rnpgbe_rx_alloc_retry(struct timer_list *timer)
+{
+ struct mucse_q_vector *q_vector =
+ timer_container_of(q_vector, timer, rx_alloc_timer);
+
+ napi_schedule(&q_vector->napi);
+}
+
+/**
+ * rnpgbe_clean_rx_irq - Clean completed descriptors from Rx ring
+ * @q_vector: structure containing interrupt and ring information
+ * @rx_ring: rx descriptor ring to transact packets on
+ * @budget: total limit on number of packets to process
+ *
+ * rnpgbe_clean_rx_irq tries to check dd in desc, handle this desc
+ * if dd is set which means data is write-back by hw
+ *
+ * Return: amount of work completed.
+ **/
+static int rnpgbe_clean_rx_irq(struct mucse_q_vector *q_vector,
+ struct mucse_ring *rx_ring,
+ int budget)
+{
+ unsigned int max_size = SKB_WITH_OVERHEAD(PAGE_SIZE) - RNPGBE_SKB_PAD;
+ unsigned int total_rx_bytes = 0, total_rx_packets = 0;
+ u16 cleaned_count = mucse_desc_unused_rx(rx_ring);
+ unsigned int work_done = 0;
+
+ while (likely(work_done < budget)) {
+ struct mucse_rx_buffer *rx_buffer;
+ union rnpgbe_rx_desc *rx_desc;
+ struct sk_buff *skb;
+ unsigned int size;
+
+ if (cleaned_count >= M_RX_BUFFER_WRITE) {
+ if (rnpgbe_alloc_rx_buffers(rx_ring, cleaned_count)) {
+ mod_timer(&q_vector->rx_alloc_timer,
+ jiffies + msecs_to_jiffies(500));
+ cleaned_count = mucse_desc_unused_rx(rx_ring);
+ } else {
+ cleaned_count = 0;
+ }
+ }
+ rx_desc = M_RX_DESC(rx_ring, rx_ring->next_to_clean);
+
+ if (!rnpgbe_test_staterr(rx_desc, M_RXD_STAT_DD))
+ break;
+
+ /* This memory barrier is needed to keep us from reading
+ * any other fields out of the rx_desc until we know the
+ * descriptor has been written back
+ */
+ dma_rmb();
+ /* Hardware enforces: minimum 33-bytes descriptor(no 1-13 byte
+ * size), multi-descriptors only for jumbo frames > 1536 bytes
+ * (controlled by M_DEFAULT_SG=96, each descriptor no more than
+ * 1536 bytes). Small packets use single descriptor.
+ */
+ size = le16_to_cpu(rx_desc->wb.len);
+
+ if (unlikely(rx_ring->drop_status)) {
+ cleaned_count++;
+ /* drop data until eop */
+ if (rnpgbe_test_staterr(rx_desc, M_RXD_STAT_EOP)) {
+ rx_ring->drop_status = false;
+ work_done++;
+ }
+
+ rx_desc->wb.cmd = 0;
+ rx_ring->next_to_clean++;
+ if (rx_ring->next_to_clean >= rx_ring->count)
+ rx_ring->next_to_clean = 0;
+ continue;
+ }
+
+ if (unlikely(!size || size > max_size)) {
+ struct mucse_rx_buffer *err_rx_buffer;
+ u16 idx = rx_ring->next_to_clean;
+
+ cleaned_count++;
+ atomic64_inc(&rx_ring->stats.dropped);
+
+ /* Free the partial skb from a previous non-EOP
+ * descriptor before advancing next_to_clean.
+ */
+ err_rx_buffer = &rx_ring->rx_buffer_info[idx];
+ if (unlikely(err_rx_buffer->skb)) {
+ dev_kfree_skb_any(err_rx_buffer->skb);
+ err_rx_buffer->skb = NULL;
+ }
+
+ /* drop data until eop */
+ if (rnpgbe_test_staterr(rx_desc, M_RXD_STAT_EOP))
+ work_done++;
+ else
+ rx_ring->drop_status = true;
+
+ rx_desc->wb.cmd = 0;
+ rx_ring->next_to_clean++;
+ if (rx_ring->next_to_clean >= rx_ring->count)
+ rx_ring->next_to_clean = 0;
+ continue;
+ }
+
+ rx_buffer = rnpgbe_get_buffer(rx_ring, &skb, size);
+
+ if (skb)
+ rnpgbe_add_rx_frag(rx_buffer, skb, size);
+ else
+ skb = rnpgbe_build_skb(rx_buffer, size);
+
+ if (!skb) {
+ cleaned_count++;
+
+ /* drop until eop if multiple descriptors */
+ if (rnpgbe_test_staterr(rx_desc, M_RXD_STAT_EOP))
+ work_done++;
+ else
+ rx_ring->drop_status = true;
+
+ rx_desc->wb.cmd = 0;
+ rx_ring->next_to_clean++;
+ atomic64_inc(&rx_ring->stats.dropped);
+ if (rx_ring->next_to_clean >= rx_ring->count)
+ rx_ring->next_to_clean = 0;
+
+ continue;
+ }
+
+ rx_buffer->page = NULL;
+ rx_buffer->skb = NULL;
+ cleaned_count++;
+
+ if (rnpgbe_is_non_eop(rx_ring, rx_desc, skb))
+ continue;
+
+ /* verify the packet layout is correct */
+ if (rnpgbe_cleanup_headers(skb)) {
+ /* we should clean it since we used all info in it */
+ atomic64_inc(&rx_ring->stats.dropped);
+ rx_desc->wb.cmd = 0;
+ work_done++;
+ continue;
+ }
+
+ /* probably a little skewed due to removing CRC */
+ total_rx_bytes += skb->len;
+ rnpgbe_process_skb_fields(rx_ring, skb);
+ rx_desc->wb.cmd = 0;
+ napi_gro_receive(&q_vector->napi, skb);
+ /* Update packet statistics and NAPI work accounting. */
+ total_rx_packets++;
+ work_done++;
+ }
+
+ u64_stats_update_begin(&rx_ring->syncp);
+ rx_ring->stats.packets += total_rx_packets;
+ rx_ring->stats.bytes += total_rx_bytes;
+ u64_stats_update_end(&rx_ring->syncp);
+
+ return work_done;
+}
+
/**
* rnpgbe_poll - NAPI polling callback
* @napi: structure for representing this polling device
* @budget: polling budget
*
- * Clean completed TX packets and complete NAPI polling when all queues are
- * clean.
+ * Clean completed TX and RX packets and complete NAPI polling when all
+ * queues are clean.
*
- * Return: 0 if all TX queues are clean, otherwise @budget
+ * Return: number of RX packets processed, or @budget if polling continues
**/
static int rnpgbe_poll(struct napi_struct *napi, int budget)
{
@@ -214,6 +620,7 @@ static int rnpgbe_poll(struct napi_struct *napi, int budget)
container_of(napi, struct mucse_q_vector, napi);
bool clean_complete = true;
struct mucse_ring *ring;
+ int per_ring_budget;
int work_done = 0;
mucse_for_each_ring(ring, q_vector->tx) {
@@ -225,11 +632,27 @@ static int rnpgbe_poll(struct napi_struct *napi, int budget)
if (unlikely(!budget))
return 0;
+ if (q_vector->rx.count > 1)
+ per_ring_budget = max(budget / q_vector->rx.count, 1);
+ else
+ per_ring_budget = budget;
+
+ mucse_for_each_ring(ring, q_vector->rx) {
+ int cleaned = 0;
+
+ cleaned = rnpgbe_clean_rx_irq(q_vector, ring, per_ring_budget);
+ work_done += cleaned;
+ if (cleaned >= per_ring_budget)
+ clean_complete = false;
+ }
+
if (!clean_complete)
return budget;
- if (likely(napi_complete_done(napi, work_done)))
- rnpgbe_irq_enable_queues(q_vector);
+ if (likely(napi_complete_done(napi, work_done))) {
+ if (!test_bit(__MUCSE_DOWN, &q_vector->mucse->state))
+ rnpgbe_irq_enable_queues(q_vector);
+ }
return work_done;
}
@@ -388,6 +811,7 @@ static int rnpgbe_alloc_q_vector(struct mucse *mucse,
/* tie q_vector and mucse together */
mucse->q_vector[vector_idx] = q_vector;
q_vector->mucse = mucse;
+ timer_setup(&q_vector->rx_alloc_timer, rnpgbe_rx_alloc_retry, 0);
q_vector->hw_vector = vector_idx;
/* if mbx use separate irq, we should add 1 */
if (mucse->flags & M_FLAG_MSIX_EN)
@@ -414,13 +838,18 @@ static int rnpgbe_alloc_q_vector(struct mucse *mucse,
}
for (idx = 0; idx < rxr_count; idx++) {
+ ring->dev = &mucse->pdev->dev;
mucse_add_ring(ring, &q_vector->rx);
+ ring->count = mucse->rx_ring_item_count;
+ ring->netdev = mucse->netdev;
ring->queue_index = eth_queue_idx + idx;
ring->rnpgbe_queue_idx = rxr_idx;
ring->ring_addr = hw->hw_addr + RING_OFFSET(rxr_idx);
ring->irq_mask = ring->ring_addr + RNPGBE_DMA_INT_MASK;
ring->trig = ring->ring_addr + RNPGBE_DMA_INT_TRIG;
ring->q_vector = q_vector;
+ ring->pfvfnum = hw->pfvfnum;
+ u64_stats_init(&ring->syncp);
mucse->rx_ring[ring->queue_index] = ring;
rxr_idx += step;
ring++;
@@ -443,6 +872,8 @@ static void rnpgbe_free_q_vector(struct mucse *mucse, int vector_idx)
struct mucse_q_vector *q_vector = mucse->q_vector[vector_idx];
struct mucse_ring *ring;
+ timer_delete_sync(&q_vector->rx_alloc_timer);
+
mucse_for_each_ring(ring, q_vector->tx)
mucse->tx_ring[ring->queue_index] = NULL;
mucse_for_each_ring(ring, q_vector->rx)
@@ -797,6 +1228,30 @@ static int rnpgbe_wait_tx_dma_idle(struct mucse *mucse)
return err;
}
+static int rnpgbe_wait_rx_dma_idle(struct mucse *mucse)
+{
+ struct mucse_hw *hw = &mucse->hw;
+ u32 dma_status;
+ int err;
+
+ /* A timeout indicates a terminal AXI fault. Hardware stops DMA
+ * accesses, so teardown may release mappings, but configuration
+ * must not re-enable RX DMA.
+ */
+ err = readl_poll_timeout(hw->hw_addr + RNPGBE_DMA_STATUS,
+ dma_status,
+ (dma_status & RNPGBE_DMA_RX_STATUS) ==
+ RNPGBE_DMA_RX_STATUS,
+ 10, 100000);
+ if (err) {
+ set_bit(__MUCSE_AXI_FAULT, &mucse->state);
+ dev_err(&mucse->pdev->dev,
+ "RX DMA failed to quiesce, status %#x\n", dma_status);
+ }
+
+ return err;
+}
+
/**
* rnpgbe_stop_all_tx_rings - Stop TX DMA on all queues
* @mucse: board private structure
@@ -912,6 +1367,66 @@ void rnpgbe_clean_all_tx_rings(struct mucse *mucse)
rnpgbe_clean_tx_ring(mucse->tx_ring[i]);
}
+static void rnpgbe_stop_rx_ring(struct mucse_ring *rx_ring)
+{
+ /* Stop hw. hardware design guarantees:
+ * - No new descriptors will be fetched after RX_START=0
+ * - No DMA will be initiated for already-fetched descriptors
+ */
+ mucse_ring_wr32(rx_ring, RNPGBE_RX_START, 0);
+ /* Flush posted write to ensure hardware sees RX_START=0 */
+ (void)mucse_ring_rd32(rx_ring, RNPGBE_RX_START);
+}
+
+/**
+ * rnpgbe_stop_all_rx_rings - Stop RX DMA for all queues
+ * @mucse: board private structure
+ *
+ * Return: 0 when RX DMA was quiesced, negative errno otherwise
+ **/
+static int rnpgbe_stop_all_rx_rings(struct mucse *mucse)
+{
+ struct mucse_hw *hw = &mucse->hw;
+ u32 dma_axi_ctl;
+ int err = 0;
+
+ for (int i = 0; i < mucse->num_rx_queues; i++)
+ rnpgbe_stop_rx_ring(mucse->rx_ring[i]);
+
+ if (mucse->num_rx_queues &&
+ !test_bit(__MUCSE_AXI_FAULT, &mucse->state))
+ err = rnpgbe_wait_rx_dma_idle(mucse);
+
+ dma_axi_ctl = mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
+ dma_axi_ctl &= ~RX_AXI_RW_EN;
+ mucse_hw_wr32(hw, RNPGBE_DMA_AXI_EN, dma_axi_ctl);
+ /* Flush the posted write before continuing. */
+ (void)mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
+
+ if (!test_bit(__MUCSE_AXI_FAULT, &mucse->state))
+ return 0;
+
+ return err ? err : -EIO;
+}
+
+static void rnpgbe_clean_rx_ring(struct mucse_ring *rx_ring);
+
+/**
+ * rnpgbe_clean_all_rx_rings - Free Rx buffers for all queues
+ * @mucse: board private structure
+ **/
+static void rnpgbe_clean_all_rx_rings(struct mucse *mucse)
+{
+ for (int i = 0; i < mucse->num_rx_queues; i++)
+ rnpgbe_clean_rx_ring(mucse->rx_ring[i]);
+}
+
+static void rnpgbe_cancel_rx_retry_timers(struct mucse *mucse)
+{
+ for (int i = 0; i < mucse->num_q_vectors; i++)
+ timer_delete_sync(&mucse->q_vector[i]->rx_alloc_timer);
+}
+
bool rnpgbe_down(struct mucse *mucse)
{
struct net_device *netdev = mucse->netdev;
@@ -922,7 +1437,10 @@ bool rnpgbe_down(struct mucse *mucse)
netif_tx_disable(netdev);
rnpgbe_napi_disable_all(mucse);
rnpgbe_irq_disable(mucse);
+ rnpgbe_stop_all_rx_rings(mucse);
+ rnpgbe_cancel_rx_retry_timers(mucse);
rnpgbe_clean_all_tx_rings(mucse);
+ rnpgbe_clean_all_rx_rings(mucse);
return true;
}
@@ -1348,5 +1866,257 @@ void rnpgbe_get_stats64(struct net_device *netdev,
stats->tx_bytes += bytes;
}
}
+
+ for (i = 0; i < mucse->num_rx_queues; i++) {
+ struct mucse_ring *ring = READ_ONCE(mucse->rx_ring[i]);
+ u64 bytes, packets, dropped;
+ unsigned int start;
+
+ if (ring) {
+ do {
+ start = u64_stats_fetch_begin(&ring->syncp);
+ packets = ring->stats.packets;
+ bytes = ring->stats.bytes;
+ } while (u64_stats_fetch_retry(&ring->syncp, start));
+ dropped = atomic64_read(&ring->stats.dropped);
+
+ stats->rx_packets += packets;
+ stats->rx_dropped += dropped;
+ stats->rx_bytes += bytes;
+ }
+ }
rcu_read_unlock();
}
+
+static int mucse_alloc_page_pool(struct mucse_ring *rx_ring)
+{
+ struct page_pool_params pp_params = {
+ .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
+ .order = 0,
+ .pool_size = rx_ring->count,
+ .nid = dev_to_node(rx_ring->dev),
+ .dev = rx_ring->dev,
+ .napi = &rx_ring->q_vector->napi,
+ .dma_dir = DMA_FROM_DEVICE,
+ .offset = 0,
+ .max_len = PAGE_SIZE,
+ .netdev = rx_ring->netdev,
+ .queue_idx = rx_ring->queue_index,
+ };
+ int ret = 0;
+
+ rx_ring->page_pool = page_pool_create(&pp_params);
+ if (IS_ERR(rx_ring->page_pool)) {
+ ret = PTR_ERR(rx_ring->page_pool);
+ rx_ring->page_pool = NULL;
+ }
+
+ return ret;
+}
+
+/**
+ * rnpgbe_setup_rx_resources - allocate Rx resources (Descriptors)
+ * @rx_ring: rx descriptor ring (for a specific queue) to setup
+ *
+ * Return: 0 on success, negative on failure
+ **/
+static int rnpgbe_setup_rx_resources(struct mucse_ring *rx_ring)
+{
+ struct device *dev = rx_ring->dev;
+ int size;
+
+ size = sizeof(struct mucse_rx_buffer) * rx_ring->count;
+
+ rx_ring->rx_buffer_info = vzalloc(size);
+
+ if (!rx_ring->rx_buffer_info)
+ goto err_return;
+ /* Round up to nearest 4K */
+ rx_ring->size = rx_ring->count * sizeof(union rnpgbe_rx_desc);
+ rx_ring->size = ALIGN(rx_ring->size, 4096);
+ rx_ring->desc = dma_alloc_coherent(dev, rx_ring->size, &rx_ring->dma,
+ GFP_KERNEL);
+ if (!rx_ring->desc)
+ goto err_free_buffer;
+
+ rx_ring->next_to_clean = 0;
+ rx_ring->next_to_use = 0;
+
+ if (mucse_alloc_page_pool(rx_ring))
+ goto err_free_desc;
+
+ return 0;
+err_free_desc:
+ dma_free_coherent(dev, rx_ring->size, rx_ring->desc,
+ rx_ring->dma);
+ rx_ring->desc = NULL;
+err_free_buffer:
+ vfree(rx_ring->rx_buffer_info);
+err_return:
+ rx_ring->rx_buffer_info = NULL;
+ return -ENOMEM;
+}
+
+/**
+ * rnpgbe_clean_rx_ring - Free Rx Buffers per Queue
+ * @rx_ring: ring to free buffers from
+ **/
+static void rnpgbe_clean_rx_ring(struct mucse_ring *rx_ring)
+{
+ struct mucse_rx_buffer *rx_buffer;
+ u16 i;
+
+ /* ring already cleared, nothing to do */
+ if (!rx_ring->rx_buffer_info)
+ return;
+ /* Free all the Rx ring sk_buffs */
+ for (i = 0; i < rx_ring->count; i++) {
+ rx_buffer = &rx_ring->rx_buffer_info[i];
+
+ if (rx_buffer->skb) {
+ struct sk_buff *skb = rx_buffer->skb;
+
+ dev_kfree_skb(skb);
+ rx_buffer->skb = NULL;
+ }
+
+ if (rx_buffer->page) {
+ page_pool_put_full_page(rx_ring->page_pool,
+ rx_buffer->page, false);
+ rx_buffer->page = NULL;
+ }
+ }
+
+ rx_ring->next_to_clean = 0;
+ rx_ring->next_to_use = 0;
+}
+
+/**
+ * rnpgbe_free_rx_resources - Free Rx Resources
+ * @rx_ring: ring to clean the resources from
+ *
+ * Free all receive software resources
+ **/
+static void rnpgbe_free_rx_resources(struct mucse_ring *rx_ring)
+{
+ vfree(rx_ring->rx_buffer_info);
+ rx_ring->rx_buffer_info = NULL;
+ /* if not set, then don't free */
+ if (!rx_ring->desc)
+ return;
+
+ dma_free_coherent(rx_ring->dev, rx_ring->size, rx_ring->desc,
+ rx_ring->dma);
+ rx_ring->desc = NULL;
+ if (rx_ring->page_pool) {
+ page_pool_destroy(rx_ring->page_pool);
+ rx_ring->page_pool = NULL;
+ }
+}
+
+/**
+ * rnpgbe_setup_all_rx_resources - allocate all queues Rx resources
+ * @mucse: pointer to private structure
+ *
+ * Return: 0 on success, negative on failure
+ **/
+int rnpgbe_setup_all_rx_resources(struct mucse *mucse)
+{
+ int i, err = 0;
+
+ for (i = 0; i < mucse->num_rx_queues; i++) {
+ err = rnpgbe_setup_rx_resources(mucse->rx_ring[i]);
+ if (!err)
+ continue;
+
+ goto err_setup_rx;
+ }
+
+ return 0;
+err_setup_rx:
+ while (i--)
+ rnpgbe_free_rx_resources(mucse->rx_ring[i]);
+ return err;
+}
+
+/**
+ * rnpgbe_free_all_rx_resources - Free Rx Resources for All Queues
+ * @mucse: pointer to private structure
+ *
+ * Free all receive software resources
+ **/
+void rnpgbe_free_all_rx_resources(struct mucse *mucse)
+{
+ for (int i = 0; i < mucse->num_rx_queues; i++)
+ rnpgbe_free_rx_resources(mucse->rx_ring[i]);
+}
+
+/**
+ * rnpgbe_configure_rx_ring - Configure Rx ring info to hw
+ * @mucse: pointer to private structure
+ * @ring: structure containing ring specific data
+ *
+ * Configure the Rx descriptor ring after a reset.
+ **/
+static void rnpgbe_configure_rx_ring(struct mucse *mucse,
+ struct mucse_ring *ring)
+{
+ struct mucse_hw *hw = &mucse->hw;
+
+ /* Set the descriptor registers after RX DMA has quiesced. */
+ mucse_ring_wr32(ring, RNPGBE_RX_BASE_ADDR_LO, (u32)ring->dma);
+ mucse_ring_wr32(ring, RNPGBE_RX_BASE_ADDR_HI,
+ (u32)((u64)ring->dma >> 32) |
+ ((u32)hw->pfvfnum << 24));
+ mucse_ring_wr32(ring, RNPGBE_RX_LEN, ring->count);
+ ring->tail = ring->ring_addr + RNPGBE_RX_TAIL;
+ ring->next_to_clean = mucse_ring_rd32(ring, RNPGBE_RX_HEAD) %
+ ring->count;
+ ring->next_to_use = ring->next_to_clean;
+ ring->drop_status = false;
+ mucse_ring_wr32(ring, RNPGBE_RX_SG_LEN, M_DEFAULT_SG);
+ mucse_ring_wr32(ring, RNPGBE_RX_FETCH, M_DEFAULT_RX_FETCH);
+ mucse_ring_wr32(ring, RNPGBE_RX_TIMEOUT_TH, 0);
+ mucse_ring_wr32(ring, RNPGBE_RX_INT_TIMER,
+ M_DEFAULT_INT_TIMER_R * hw->cycles_per_us);
+ mucse_ring_wr32(ring, RNPGBE_RX_INT_PKTCNT, M_DEFAULT_RX_INT_PKTCNT);
+ if (rnpgbe_alloc_rx_buffers(ring, mucse_desc_unused_rx(ring))) {
+ mod_timer(&ring->q_vector->rx_alloc_timer,
+ jiffies + msecs_to_jiffies(500));
+ }
+}
+
+/**
+ * rnpgbe_configure_rx - Configure Receive Unit after Reset
+ * @mucse: pointer to private structure
+ *
+ * Configure the Rx unit after a reset.
+ *
+ * Return: 0 on success, negative errno if RX DMA did not quiesce
+ **/
+int rnpgbe_configure_rx(struct mucse *mucse)
+{
+ struct mucse_hw *hw = &mucse->hw;
+ u32 dma_axi_ctl;
+ int err;
+
+ err = rnpgbe_stop_all_rx_rings(mucse);
+ if (err)
+ return err;
+
+ for (int i = 0; i < mucse->num_rx_queues; i++)
+ rnpgbe_configure_rx_ring(mucse, mucse->rx_ring[i]);
+
+ /* Ensure all ring configuration is visible before enabling RX DMA. */
+ wmb();
+ dma_axi_ctl = mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
+ dma_axi_ctl |= RX_AXI_RW_EN;
+ mucse_hw_wr32(hw, RNPGBE_DMA_AXI_EN, dma_axi_ctl);
+ /* Ensure RX_AXI_RW_EN is visible before starting the rings. */
+ (void)mucse_hw_rd32(hw, RNPGBE_DMA_AXI_EN);
+
+ for (int i = 0; i < mucse->num_rx_queues; i++)
+ mucse_ring_wr32(mucse->rx_ring[i], RNPGBE_RX_START, 1);
+
+ return 0;
+}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
index 01e800f5f6a4..234b3bff94c4 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
@@ -9,12 +9,27 @@ struct mucse_hw;
struct mucse_ring;
#define RING_OFFSET(n) (0x1000 + 0x100 * (n))
+#define RNPGBE_RX_START 0x10
#define RNPGBE_TX_START 0x18
#define RNPGBE_DMA_INT_MASK 0x24
#define TX_INT_MASK BIT(1)
#define RX_INT_MASK BIT(0)
#define INT_VALID (BIT(16) | BIT(17))
#define RNPGBE_DMA_INT_TRIG 0x2c /* lost-interrupt recovery trigger */
+#define RNPGBE_RX_BASE_ADDR_HI 0x30
+#define RNPGBE_RX_BASE_ADDR_LO 0x34
+#define RNPGBE_RX_LEN 0x38
+#define RNPGBE_RX_HEAD 0x3c
+#define RNPGBE_RX_TAIL 0x40
+#define M_DEFAULT_RX_FETCH 0x100020
+#define RNPGBE_RX_FETCH 0x44
+#define M_DEFAULT_INT_TIMER_R 30
+#define RNPGBE_RX_INT_TIMER 0x48
+#define M_DEFAULT_RX_INT_PKTCNT 64
+#define RNPGBE_RX_INT_PKTCNT 0x4c
+#define RNPGBE_RX_TIMEOUT_TH 0x54
+#define M_DEFAULT_SG 96 /* unit 16b, 1536 bytes */
+#define RNPGBE_RX_SG_LEN 0x58
#define RNPGBE_TX_BASE_ADDR_HI 0x60
#define RNPGBE_TX_BASE_ADDR_LO 0x64
#define RNPGBE_TX_LEN 0x68
@@ -37,13 +52,14 @@ struct mucse_ring;
/* 2 desc gap to keep tail from touching head */
/* 1 desc for context descriptor */
#define RESV_DESC_NEEDED 3
+#define RNPGBE_SKB_PAD (NET_SKB_PAD + NET_IP_ALIGN)
+
/* Hardware requires this to be nonzero */
#define M_DEFAULT_MAC_IP_LEN 20
#define mucse_for_each_ring(pos, head)\
for (typeof((head).ring) __pos = (head).ring;\
__pos ? ({ pos = __pos; 1; }) : 0;\
__pos = __pos->next)
-
int rnpgbe_init_interrupt_scheme(struct mucse *mucse);
void rnpgbe_clear_interrupt_scheme(struct mucse *mucse);
int rnpgbe_request_mbx_irq(struct mucse *mucse);
@@ -54,6 +70,7 @@ void rnpgbe_irq_disable(struct mucse *mucse);
bool rnpgbe_down(struct mucse *mucse);
void rnpgbe_up_complete(struct mucse *mucse);
int rnpgbe_configure_tx(struct mucse *mucse);
+int rnpgbe_configure_rx(struct mucse *mucse);
void rnpgbe_clean_all_tx_rings(struct mucse *mucse);
int rnpgbe_setup_all_tx_resources(struct mucse *mucse);
void rnpgbe_free_all_tx_resources(struct mucse *mucse);
@@ -61,4 +78,6 @@ netdev_tx_t rnpgbe_xmit_frame_ring(struct sk_buff *skb,
struct mucse_ring *tx_ring);
void rnpgbe_get_stats64(struct net_device *netdev,
struct rtnl_link_stats64 *stats);
+int rnpgbe_setup_all_rx_resources(struct mucse *mucse);
+void rnpgbe_free_all_rx_resources(struct mucse *mucse);
#endif
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
index 5be78afd0b25..e62366291b2a 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
@@ -37,7 +37,13 @@ static struct pci_device_id rnpgbe_pci_tbl[] = {
**/
static int rnpgbe_configure(struct mucse *mucse)
{
- return rnpgbe_configure_tx(mucse);
+ int err;
+
+ err = rnpgbe_configure_tx(mucse);
+ if (err)
+ return err;
+
+ return rnpgbe_configure_rx(mucse);
}
/**
@@ -69,13 +75,18 @@ static int rnpgbe_open(struct net_device *netdev)
err = rnpgbe_setup_all_tx_resources(mucse);
if (err)
goto err_free_irqs;
+ err = rnpgbe_setup_all_rx_resources(mucse);
+ if (err)
+ goto err_free_tx;
err = rnpgbe_configure(mucse);
if (err)
- goto err_free_tx;
+ goto err_free_rx;
rnpgbe_up_complete(mucse);
return 0;
+err_free_rx:
+ rnpgbe_free_all_rx_resources(mucse);
err_free_tx:
rnpgbe_clean_all_tx_rings(mucse);
rnpgbe_free_all_tx_resources(mucse);
@@ -102,6 +113,7 @@ static int rnpgbe_close(struct net_device *netdev)
rnpgbe_free_irq(mucse);
rnpgbe_free_all_tx_resources(mucse);
+ rnpgbe_free_all_rx_resources(mucse);
return 0;
}
@@ -139,6 +151,7 @@ static const struct net_device_ops rnpgbe_netdev_ops = {
static void rnpgbe_sw_init(struct mucse *mucse)
{
mucse->tx_ring_item_count = M_DEFAULT_TXD;
+ mucse->rx_ring_item_count = M_DEFAULT_RXD;
mucse->tx_work_limit = M_DEFAULT_TX_WORK;
}
--
2.50.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH net-next v10 3/5] net: rnpgbe: Add RX packet reception support
2026-08-31 7:36 ` [PATCH net-next v10 3/5] net: rnpgbe: Add RX packet reception support Dong Yibo
@ 2026-09-04 21:57 ` Jakub Kicinski
2026-09-08 3:41 ` Yibo Dong
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2026-09-04 21:57 UTC (permalink / raw)
To: dong100
Cc: Jakub Kicinski, andrew+netdev, davem, edumazet, pabeni,
vadim.fedorenko, u.kleine-koenig, netdev, linux-kernel, yaojun
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: rnpgbe: Add RX packet reception support
This adds the RX path for rnpgbe: the RX descriptor union, page_pool
backed RX buffers, the NAPI poll callback, RX ring setup/teardown and RX
statistics. It also adds a per-q_vector timer that retries failed page
allocations by scheduling NAPI, and RX DMA quiesce handling on teardown.
A few questions about the new RX code below.
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> index ff81d222edd3..346e9a1cca8a 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
[ ... ]
> @@ -198,15 +200,419 @@ static bool rnpgbe_clean_tx_irq(struct mucse_q_vector *q_vector,
> return !!budget;
> }
>
[ ... ]
> +/**
> + * rnpgbe_rx_alloc_retry - Retry RX buffer allocation
> + * @timer: RX allocation retry timer
> + *
> + * Schedules NAPI after RX buffer allocation fails during polling or
> + * initial ring configuration.
> + **/
> +static void rnpgbe_rx_alloc_retry(struct timer_list *timer)
> +{
> + struct mucse_q_vector *q_vector =
> + timer_container_of(q_vector, timer, rx_alloc_timer);
> +
> + napi_schedule(&q_vector->napi);
> +}
[Severity: Medium]
Can this retry be silently lost when the timer is armed from
rnpgbe_configure_rx_ring()?
At that point NAPI has not been enabled yet. The open path is:
rnpgbe_open()
rnpgbe_configure()
rnpgbe_configure_rx()
rnpgbe_configure_rx_ring()
if (rnpgbe_alloc_rx_buffers(ring, mucse_desc_unused_rx(ring))) {
mod_timer(&ring->q_vector->rx_alloc_timer,
jiffies + msecs_to_jiffies(500));
}
rnpgbe_up_complete()
rnpgbe_napi_enable_all()
clear_bit(__MUCSE_DOWN, &mucse->state);
netif_napi_add_weight() leaves NAPI_STATE_SCHED and NAPI_STATE_NPSVC set
until napi_enable_locked() runs, so napi_schedule_prep() returns false and
only sets NAPIF_STATE_MISSED, which napi_enable_locked() does not act on.
rnpgbe_rx_alloc_retry() ignores the return value of napi_schedule() and does
not re-arm, so if the timer expires in the window before
rnpgbe_napi_enable_all() the retry is dropped. If the initial refill failed
for every descriptor, no descriptors are posted, no RX interrupt can arrive
and the ring stays empty until an administrative down/up.
Would arming the timer only after NAPI is enabled, or re-arming from the
callback when napi_schedule() returns false, be more robust?
[ ... ]
> +static int rnpgbe_clean_rx_irq(struct mucse_q_vector *q_vector,
> + struct mucse_ring *rx_ring,
> + int budget)
> +{
> + unsigned int max_size = SKB_WITH_OVERHEAD(PAGE_SIZE) - RNPGBE_SKB_PAD;
[ ... ]
> + dma_rmb();
> + /* Hardware enforces: minimum 33-bytes descriptor(no 1-13 byte
> + * size), multi-descriptors only for jumbo frames > 1536 bytes
> + * (controlled by M_DEFAULT_SG=96, each descriptor no more than
> + * 1536 bytes). Small packets use single descriptor.
> + */
> + size = le16_to_cpu(rx_desc->wb.len);
[ ... ]
> + if (unlikely(!size || size > max_size)) {
[Severity: Medium]
Is there anything that prevents a first descriptor with size in the 1..13
range from starting a fragmented packet here?
The only length check is the one above, which accepts any size from 1 to
max_size. If such a short descriptor is non-EOP, rnpgbe_is_non_eop() parks
the skb and the following descriptors are attached with skb_add_rx_frag(),
leaving an skb whose linear head is shorter than ETH_HLEN while data_len is
non-zero.
rnpgbe_cleanup_headers() only calls eth_skb_pad(), which does nothing once
skb->len is already at least ETH_ZLEN, so the short head survives.
rnpgbe_process_skb_fields() then does:
skb->protocol = eth_type_trans(skb, dev);
and eth_type_trans() -> eth_skb_pull_mac() -> skb_pull_inline(ETH_HLEN)
reaches the __skb_pull() check:
include/linux/skbuff.h:__skb_pull() {
...
BUG_ON(skb->len < skb->data_len);
...
}
The comment above states the hardware enforces a 33-byte minimum, but the
code does not enforce it. Would rejecting descriptors that cannot supply an
ETH_HLEN linear head, or a pskb_may_pull(skb, ETH_HLEN) before
eth_type_trans(), be worth adding?
[ ... ]
> @@ -225,11 +632,27 @@ static int rnpgbe_poll(struct napi_struct *napi, int budget)
> if (unlikely(!budget))
> return 0;
>
> + if (q_vector->rx.count > 1)
> + per_ring_budget = max(budget / q_vector->rx.count, 1);
> + else
> + per_ring_budget = budget;
> +
> + mucse_for_each_ring(ring, q_vector->rx) {
> + int cleaned = 0;
> +
> + cleaned = rnpgbe_clean_rx_irq(q_vector, ring, per_ring_budget);
> + work_done += cleaned;
> + if (cleaned >= per_ring_budget)
> + clean_complete = false;
> + }
> +
> if (!clean_complete)
> return budget;
>
> - if (likely(napi_complete_done(napi, work_done)))
> - rnpgbe_irq_enable_queues(q_vector);
> + if (likely(napi_complete_done(napi, work_done))) {
> + if (!test_bit(__MUCSE_DOWN, &q_vector->mucse->state))
> + rnpgbe_irq_enable_queues(q_vector);
> + }
[Severity: Medium]
Does this __MUCSE_DOWN test actually close the window the commit message
describes with "Do not re-enable queue interrupts while the device is
down"?
napi_complete_done() clears NAPI_STATE_SCHED before the test_bit() and the
MMIO unmask run, and napi_disable_locked() only waits for SCHED/NPSVC to
clear:
net/core/dev.c:napi_disable_locked() {
...
while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
usleep_range(20, 200);
val = READ_ONCE(n->state);
}
...
}
So this interleaving looks possible:
CPU0 rnpgbe_poll()
napi_complete_done() /* SCHED released */
test_bit(__MUCSE_DOWN) == 0
CPU1 rnpgbe_down()
test_and_set_bit(__MUCSE_DOWN, &mucse->state);
rnpgbe_napi_disable_all();
rnpgbe_irq_disable(); /* includes synchronize_irq() */
rnpgbe_stop_all_rx_rings();
rnpgbe_clean_all_rx_rings();
CPU0 (continues)
rnpgbe_irq_enable_queues(q_vector); /* unmask + trig write */
After rnpgbe_down() returns the queue interrupt would be left unmasked and
explicitly re-triggered, after RX DMA was quiesced and the RX buffers were
released, and just before rnpgbe_free_irq().
Would unmasking before napi_complete_done() releases SCHED, or re-masking
after rnpgbe_napi_disable_all() in rnpgbe_down(), give the guarantee the
changelog claims?
[ ... ]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH net-next v10 3/5] net: rnpgbe: Add RX packet reception support
2026-09-04 21:57 ` Jakub Kicinski
@ 2026-09-08 3:41 ` Yibo Dong
0 siblings, 0 replies; 15+ messages in thread
From: Yibo Dong @ 2026-09-08 3:41 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, davem, edumazet, pabeni, vadim.fedorenko,
u.kleine-koenig, netdev, linux-kernel, yaojun
On Fri, Sep 04, 2026 at 02:57:15PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: rnpgbe: Add RX packet reception support
>
> This adds the RX path for rnpgbe: the RX descriptor union, page_pool
> backed RX buffers, the NAPI poll callback, RX ring setup/teardown and RX
> statistics. It also adds a per-q_vector timer that retries failed page
> allocations by scheduling NAPI, and RX DMA quiesce handling on teardown.
>
> A few questions about the new RX code below.
>
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> > index ff81d222edd3..346e9a1cca8a 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
>
> [ ... ]
>
> > @@ -198,15 +200,419 @@ static bool rnpgbe_clean_tx_irq(struct mucse_q_vector *q_vector,
> > return !!budget;
> > }
> >
>
> [ ... ]
>
> > +/**
> > + * rnpgbe_rx_alloc_retry - Retry RX buffer allocation
> > + * @timer: RX allocation retry timer
> > + *
> > + * Schedules NAPI after RX buffer allocation fails during polling or
> > + * initial ring configuration.
> > + **/
> > +static void rnpgbe_rx_alloc_retry(struct timer_list *timer)
> > +{
> > + struct mucse_q_vector *q_vector =
> > + timer_container_of(q_vector, timer, rx_alloc_timer);
> > +
> > + napi_schedule(&q_vector->napi);
> > +}
>
> [Severity: Medium]
> Can this retry be silently lost when the timer is armed from
> rnpgbe_configure_rx_ring()?
>
> At that point NAPI has not been enabled yet. The open path is:
>
> rnpgbe_open()
> rnpgbe_configure()
> rnpgbe_configure_rx()
> rnpgbe_configure_rx_ring()
> if (rnpgbe_alloc_rx_buffers(ring, mucse_desc_unused_rx(ring))) {
> mod_timer(&ring->q_vector->rx_alloc_timer,
> jiffies + msecs_to_jiffies(500));
> }
> rnpgbe_up_complete()
> rnpgbe_napi_enable_all()
> clear_bit(__MUCSE_DOWN, &mucse->state);
>
> netif_napi_add_weight() leaves NAPI_STATE_SCHED and NAPI_STATE_NPSVC set
> until napi_enable_locked() runs, so napi_schedule_prep() returns false and
> only sets NAPIF_STATE_MISSED, which napi_enable_locked() does not act on.
>
> rnpgbe_rx_alloc_retry() ignores the return value of napi_schedule() and does
> not re-arm, so if the timer expires in the window before
> rnpgbe_napi_enable_all() the retry is dropped. If the initial refill failed
> for every descriptor, no descriptors are posted, no RX interrupt can arrive
> and the ring stays empty until an administrative down/up.
>
> Would arming the timer only after NAPI is enabled, or re-arming from the
> callback when napi_schedule() returns false, be more robust?
>
> [ ... ]
>
Got it, I will move mod_timer after NAPI is enabled.
> > +static int rnpgbe_clean_rx_irq(struct mucse_q_vector *q_vector,
> > + struct mucse_ring *rx_ring,
> > + int budget)
> > +{
> > + unsigned int max_size = SKB_WITH_OVERHEAD(PAGE_SIZE) - RNPGBE_SKB_PAD;
>
> [ ... ]
>
> > + dma_rmb();
> > + /* Hardware enforces: minimum 33-bytes descriptor(no 1-13 byte
> > + * size), multi-descriptors only for jumbo frames > 1536 bytes
> > + * (controlled by M_DEFAULT_SG=96, each descriptor no more than
> > + * 1536 bytes). Small packets use single descriptor.
> > + */
> > + size = le16_to_cpu(rx_desc->wb.len);
>
> [ ... ]
>
> > + if (unlikely(!size || size > max_size)) {
>
> [Severity: Medium]
> Is there anything that prevents a first descriptor with size in the 1..13
> range from starting a fragmented packet here?
>
> The only length check is the one above, which accepts any size from 1 to
> max_size. If such a short descriptor is non-EOP, rnpgbe_is_non_eop() parks
> the skb and the following descriptors are attached with skb_add_rx_frag(),
> leaving an skb whose linear head is shorter than ETH_HLEN while data_len is
> non-zero.
>
> rnpgbe_cleanup_headers() only calls eth_skb_pad(), which does nothing once
> skb->len is already at least ETH_ZLEN, so the short head survives.
> rnpgbe_process_skb_fields() then does:
>
> skb->protocol = eth_type_trans(skb, dev);
>
> and eth_type_trans() -> eth_skb_pull_mac() -> skb_pull_inline(ETH_HLEN)
> reaches the __skb_pull() check:
>
> include/linux/skbuff.h:__skb_pull() {
> ...
> BUG_ON(skb->len < skb->data_len);
> ...
> }
>
> The comment above states the hardware enforces a 33-byte minimum, but the
> code does not enforce it. Would rejecting descriptors that cannot supply an
> ETH_HLEN linear head, or a pskb_may_pull(skb, ETH_HLEN) before
> eth_type_trans(), be worth adding?
The RX descriptor write-back format guarantees that each descriptor is at
least 33 bytes, so the first descriptor always contains the Ethernet
header, including for multi-descriptor packets.
Would it be acceptable to rely on this hardware invariant rather than add
an additional defensive length check before eth_type_trans()?
>
> [ ... ]
>
> > @@ -225,11 +632,27 @@ static int rnpgbe_poll(struct napi_struct *napi, int budget)
> > if (unlikely(!budget))
> > return 0;
> >
> > + if (q_vector->rx.count > 1)
> > + per_ring_budget = max(budget / q_vector->rx.count, 1);
> > + else
> > + per_ring_budget = budget;
> > +
> > + mucse_for_each_ring(ring, q_vector->rx) {
> > + int cleaned = 0;
> > +
> > + cleaned = rnpgbe_clean_rx_irq(q_vector, ring, per_ring_budget);
> > + work_done += cleaned;
> > + if (cleaned >= per_ring_budget)
> > + clean_complete = false;
> > + }
> > +
> > if (!clean_complete)
> > return budget;
> >
> > - if (likely(napi_complete_done(napi, work_done)))
> > - rnpgbe_irq_enable_queues(q_vector);
> > + if (likely(napi_complete_done(napi, work_done))) {
> > + if (!test_bit(__MUCSE_DOWN, &q_vector->mucse->state))
> > + rnpgbe_irq_enable_queues(q_vector);
> > + }
>
> [Severity: Medium]
> Does this __MUCSE_DOWN test actually close the window the commit message
> describes with "Do not re-enable queue interrupts while the device is
> down"?
>
> napi_complete_done() clears NAPI_STATE_SCHED before the test_bit() and the
> MMIO unmask run, and napi_disable_locked() only waits for SCHED/NPSVC to
> clear:
>
> net/core/dev.c:napi_disable_locked() {
> ...
> while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
> usleep_range(20, 200);
> val = READ_ONCE(n->state);
> }
> ...
> }
>
> So this interleaving looks possible:
>
> CPU0 rnpgbe_poll()
> napi_complete_done() /* SCHED released */
> test_bit(__MUCSE_DOWN) == 0
>
> CPU1 rnpgbe_down()
> test_and_set_bit(__MUCSE_DOWN, &mucse->state);
> rnpgbe_napi_disable_all();
> rnpgbe_irq_disable(); /* includes synchronize_irq() */
> rnpgbe_stop_all_rx_rings();
> rnpgbe_clean_all_rx_rings();
>
> CPU0 (continues)
> rnpgbe_irq_enable_queues(q_vector); /* unmask + trig write */
>
> After rnpgbe_down() returns the queue interrupt would be left unmasked and
> explicitly re-triggered, after RX DMA was quiesced and the RX buffers were
> released, and just before rnpgbe_free_irq().
>
> Would unmasking before napi_complete_done() releases SCHED, or re-masking
> after rnpgbe_napi_disable_all() in rnpgbe_down(), give the guarantee the
> changelog claims?
>
> [ ... ]
>
I plan to keep the normal poll path unchanged and fix this in the down path:
rnpgbe_irq_disable(mucse);
rnpgbe_napi_disable_all(mucse);
synchronize_net();
rnpgbe_irq_disable(mucse);
The first mask prevents new hard IRQs from scheduling NAPI and
synchronizes any running IRQ handlers. After NAPI is disabled,
synchronize_net() waits for an in-flight NAPI poll tail which may already
have cleared NAPI_STATE_SCHED but has not yet returned. The final mask is
therefore not subject to a later rnpgbe_irq_enable_queues() write.
Would this teardown ordering be an acceptable way to close the race while
keeping the normal RX/TX poll path unchanged?
Thanks for your feedback.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v10 4/5] net: rnpgbe: Add receive mode support
[not found] <20260831073608.401988-1-dong100@mucse.com>
` (2 preceding siblings ...)
2026-08-31 7:36 ` [PATCH net-next v10 3/5] net: rnpgbe: Add RX packet reception support Dong Yibo
@ 2026-08-31 7:36 ` Dong Yibo
2026-09-04 21:57 ` Jakub Kicinski
2026-08-31 7:36 ` [PATCH net-next v10 5/5] net: rnpgbe: Add link status handling support Dong Yibo
4 siblings, 1 reply; 15+ messages in thread
From: Dong Yibo @ 2026-08-31 7:36 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, vadim.fedorenko,
u.kleine-koenig
Cc: netdev, linux-kernel, dong100, yaojun
Add chip-level receive filtering and the ndo_set_rx_mode callback.
Program the primary and additional unicast addresses into the available
RAR entries, and use the 4096-bit multicast hash table for multicast
addresses. Fall back to accepting all unicast addresses when the RAR
table is exhausted, and handle IFF_PROMISC and IFF_ALLMULTI through the
chip-level filter control register.
Signed-off-by: Dong Yibo <dong100@mucse.com>
---
drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 1 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 73 +++++++++++++++++++
drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 15 ++++
.../net/ethernet/mucse/rnpgbe/rnpgbe_main.c | 14 +++-
4 files changed, 99 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 8c3d8bbfcc1a..8d7c6f8301cd 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -233,6 +233,7 @@ int rnpgbe_send_notify(struct mucse_hw *hw,
bool enable,
int mode);
int rnpgbe_init_hw(struct mucse_hw *hw, int board_type);
+void rnpgbe_set_rx_mode(struct net_device *netdev);
/* Device IDs */
#define PCI_VENDOR_ID_MUCSE 0x8848
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
index 291e77d573fe..c7d5b53f6209 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
@@ -149,3 +149,76 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type)
return 0;
}
+
+static void rnpgbe_set_rar(struct mucse_hw *hw, u32 index, const u8 *addr)
+{
+ u32 rar_low, rar_high;
+
+ /* The RAR stores the Ethernet address in reverse byte order. */
+ rar_low = (u32)addr[5] | ((u32)addr[4] << 8) |
+ ((u32)addr[3] << 16) | ((u32)addr[2] << 24);
+ rar_high = (u32)addr[1] | ((u32)addr[0] << 8) |
+ RNPGBE_RX_RAR_VALID;
+ mucse_hw_wr32(hw, RNPGBE_RX_RAR_LOW(index), rar_low);
+ mucse_hw_wr32(hw, RNPGBE_RX_RAR_HIGH(index), rar_high);
+}
+
+static void rnpgbe_clear_rar(struct mucse_hw *hw, u32 index)
+{
+ mucse_hw_wr32(hw, RNPGBE_RX_RAR_LOW(index), 0);
+ mucse_hw_wr32(hw, RNPGBE_RX_RAR_HIGH(index), 0);
+}
+
+void rnpgbe_set_rx_mode(struct net_device *netdev)
+{
+ u32 mcast_hash[RNPGBE_RX_MCAST_HASH_ENTRIES] = {};
+ u32 mcast_ctrl = RNPGBE_RX_UCAST_TABLE_EN;
+ u32 filter_ctrl = RNPGBE_RX_FILTER_BCAST;
+ struct netdev_hw_addr *ha;
+ struct mucse_hw *hw;
+ struct mucse *mucse;
+ int rar = 1;
+ u16 hash;
+ int i;
+
+ mucse = netdev_priv(netdev);
+ hw = &mucse->hw;
+
+ /* RAR 0 always holds the interface's primary unicast address. */
+ rnpgbe_set_rar(hw, 0, netdev->dev_addr);
+
+ netdev_for_each_uc_addr(ha, netdev) {
+ if (rar == RNPGBE_RX_RAR_ENTRIES) {
+ filter_ctrl |= RNPGBE_RX_FILTER_UCAST_ALL;
+ break;
+ }
+
+ rnpgbe_set_rar(hw, rar, ha->addr);
+ rar++;
+ }
+
+ for (; rar < RNPGBE_RX_RAR_ENTRIES; rar++)
+ rnpgbe_clear_rar(hw, rar);
+
+ if (netdev->flags & IFF_PROMISC) {
+ filter_ctrl |= RNPGBE_RX_FILTER_UCAST_ALL |
+ RNPGBE_RX_FILTER_MCAST_ALL;
+ } else if (netdev->flags & IFF_ALLMULTI) {
+ filter_ctrl |= RNPGBE_RX_FILTER_MCAST_ALL;
+ } else {
+ /* The MTA index is the 12 address bits from 36 through 47. */
+ netdev_for_each_mc_addr(ha, netdev) {
+ hash = ((ha->addr[4] & 0xf) << 8) | ha->addr[5];
+ mcast_hash[hash >> 5] |= BIT(hash & 0x1f);
+ }
+
+ if (!netdev_mc_empty(netdev))
+ mcast_ctrl |= RNPGBE_RX_MCAST_HASH_EN;
+ }
+
+ for (i = 0; i < RNPGBE_RX_MCAST_HASH_ENTRIES; i++)
+ mucse_hw_wr32(hw, RNPGBE_RX_MCAST_HASH(i), mcast_hash[i]);
+
+ mucse_hw_wr32(hw, RNPGBE_RX_MCAST_CTRL, mcast_ctrl);
+ mucse_hw_wr32(hw, RNPGBE_RX_FILTER_CTRL, filter_ctrl);
+}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
index 1d87edfba3d7..feb84704d602 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
@@ -24,5 +24,20 @@
#define RNPGBE_DMA_AXI_EN 0x0010
#define RNPGBE_TX_MIN_PKT_LEN 33
+#define MUCSE_ETH_OFF(_n) (0x10000 + (_n))
+#define RNPGBE_RX_RAR_LOW(_n) MUCSE_ETH_OFF(0xa000 + 4 * (_n))
+#define RNPGBE_RX_RAR_HIGH(_n) MUCSE_ETH_OFF(0xa400 + 4 * (_n))
+#define RNPGBE_RX_MCAST_HASH(_n) MUCSE_ETH_OFF(0xac00 + 4 * (_n))
+#define RNPGBE_RX_FILTER_CTRL MUCSE_ETH_OFF(0x9110)
+#define RNPGBE_RX_MCAST_CTRL MUCSE_ETH_OFF(0x9114)
+#define RNPGBE_RX_RAR_VALID BIT(31)
+#define RNPGBE_RX_FILTER_BCAST BIT(10)
+#define RNPGBE_RX_FILTER_UCAST_ALL BIT(9)
+#define RNPGBE_RX_FILTER_MCAST_ALL BIT(8)
+#define RNPGBE_RX_MCAST_HASH_EN BIT(4)
+#define RNPGBE_RX_UCAST_TABLE_EN BIT(3)
+/* The final two of the 32 hardware RAR entries are reserved for NCSI. */
+#define RNPGBE_RX_RAR_ENTRIES 30
+#define RNPGBE_RX_MCAST_HASH_ENTRIES 128
#define RNPGBE_MAX_QUEUES 8
#endif /* _RNPGBE_HW_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
index e62366291b2a..817081e24bb5 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
@@ -43,7 +43,12 @@ static int rnpgbe_configure(struct mucse *mucse)
if (err)
return err;
- return rnpgbe_configure_rx(mucse);
+ err = rnpgbe_configure_rx(mucse);
+ if (err)
+ return err;
+
+ rnpgbe_set_rx_mode(mucse->netdev);
+ return 0;
}
/**
@@ -142,9 +147,10 @@ static netdev_tx_t rnpgbe_xmit_frame(struct sk_buff *skb,
}
static const struct net_device_ops rnpgbe_netdev_ops = {
- .ndo_open = rnpgbe_open,
- .ndo_stop = rnpgbe_close,
- .ndo_start_xmit = rnpgbe_xmit_frame,
+ .ndo_open = rnpgbe_open,
+ .ndo_stop = rnpgbe_close,
+ .ndo_start_xmit = rnpgbe_xmit_frame,
+ .ndo_set_rx_mode = rnpgbe_set_rx_mode,
.ndo_get_stats64 = rnpgbe_get_stats64,
};
--
2.50.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH net-next v10 4/5] net: rnpgbe: Add receive mode support
2026-08-31 7:36 ` [PATCH net-next v10 4/5] net: rnpgbe: Add receive mode support Dong Yibo
@ 2026-09-04 21:57 ` Jakub Kicinski
2026-09-08 7:46 ` Yibo Dong
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2026-09-04 21:57 UTC (permalink / raw)
To: dong100
Cc: Jakub Kicinski, andrew+netdev, davem, edumazet, pabeni,
vadim.fedorenko, u.kleine-koenig, netdev, linux-kernel, yaojun
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: rnpgbe: Add receive mode support
This adds chip-level receive filtering to rnpgbe along with the
ndo_set_rx_mode callback. Unicast addresses go into the 30 usable RAR
entries and multicast addresses into the 4096-bit hash table, with
IFF_PROMISC and IFF_ALLMULTI handled through the filter control register.
rnpgbe_configure() also calls the new function directly from the open path.
Two questions below.
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> index 291e77d573fe..c7d5b53f6209 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> @@ -149,3 +149,76 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type)
[ ... ]
> +void rnpgbe_set_rx_mode(struct net_device *netdev)
> +{
> + u32 mcast_hash[RNPGBE_RX_MCAST_HASH_ENTRIES] = {};
> + u32 mcast_ctrl = RNPGBE_RX_UCAST_TABLE_EN;
> + u32 filter_ctrl = RNPGBE_RX_FILTER_BCAST;
> + struct netdev_hw_addr *ha;
> + struct mucse_hw *hw;
> + struct mucse *mucse;
> + int rar = 1;
> + u16 hash;
> + int i;
> +
> + mucse = netdev_priv(netdev);
> + hw = &mucse->hw;
> +
> + /* RAR 0 always holds the interface's primary unicast address. */
> + rnpgbe_set_rar(hw, 0, netdev->dev_addr);
> +
> + netdev_for_each_uc_addr(ha, netdev) {
> + if (rar == RNPGBE_RX_RAR_ENTRIES) {
> + filter_ctrl |= RNPGBE_RX_FILTER_UCAST_ALL;
> + break;
> + }
> +
> + rnpgbe_set_rar(hw, rar, ha->addr);
> + rar++;
> + }
> +
> + for (; rar < RNPGBE_RX_RAR_ENTRIES; rar++)
> + rnpgbe_clear_rar(hw, rar);
> +
> + if (netdev->flags & IFF_PROMISC) {
> + filter_ctrl |= RNPGBE_RX_FILTER_UCAST_ALL |
> + RNPGBE_RX_FILTER_MCAST_ALL;
[Severity: High]
Can this secondary unicast RAR programming ever take effect? I could not
find netdev->priv_flags |= IFF_UNICAST_FLT anywhere under
drivers/net/ethernet/mucse/rnpgbe/, including rnpgbe_add_adapter() where
netdev_ops and features are set up before register_netdev().
Without that flag, the core forces promiscuous mode as soon as dev->uc is
non-empty, in netif_uc_promisc_update():
net/core/dev_addr_lists.c:netif_uc_promisc_update() {
if (dev->priv_flags & IFF_UNICAST_FLT)
return 0;
if (!netdev_uc_empty(dev) && !dev->uc_promisc) {
dev->uc_promisc = true;
return 1;
}
}
Both callers (netif_rx_mode_run() and __dev_set_rx_mode()) then call
__dev_set_promiscuity(dev, +1, false), which sets IFF_PROMISC in dev->flags
before ops->ndo_set_rx_mode() runs.
So for any configuration that adds a secondary unicast address (macvlan,
ipvlan, a bridge port with a different MAC, VM MAC passthrough),
rnpgbe_set_rx_mode() always sees IFF_PROMISC and takes the branch above.
The RAR entries just written become irrelevant, and since the else branch is
skipped the mcast_hash[] array stays all zero and RNPGBE_RX_MCAST_HASH_EN is
never set, so multicast filtering is off as well and the adapter accepts
everything on the wire.
That also seems to make this part of the changelog unobservable:
Fall back to accepting all unicast addresses when the RAR
table is exhausted
the fallback is pre-empted by the core at the first secondary address, long
before 30 RAR entries are used. Should the driver set IFF_UNICAST_FLT in
rnpgbe_add_adapter(), or is the exact unicast filter logic here not intended
to be used?
> + } else if (netdev->flags & IFF_ALLMULTI) {
> + filter_ctrl |= RNPGBE_RX_FILTER_MCAST_ALL;
> + } else {
> + /* The MTA index is the 12 address bits from 36 through 47. */
> + netdev_for_each_mc_addr(ha, netdev) {
> + hash = ((ha->addr[4] & 0xf) << 8) | ha->addr[5];
> + mcast_hash[hash >> 5] |= BIT(hash & 0x1f);
> + }
> +
> + if (!netdev_mc_empty(netdev))
> + mcast_ctrl |= RNPGBE_RX_MCAST_HASH_EN;
> + }
[Severity: High]
The comment directly above this line says the MTA index is the 12
address bits from 36 through 47, but the expression does not extract
those bits. With the usual MAC bit numbering (bit 0 is the LSB of
addr[0], so ha->addr[4] carries bits 32-39 and ha->addr[5] bits
40-47), ha->addr[4] & 0xf is bits 32-35. Bits 36-39 are the upper
nibble, i.e. ha->addr[4] >> 4. Compare ixgbe_mta_vector() in
drivers/net/ethernet/intel/ixgbe/ixgbe_common.c, which spells the
same field for "bits [47:36]" as
((mc_addr[4] >> 4) | (((u16)mc_addr[5]) << 4)).
The placement of the pieces differs too: here ha->addr[5] ends up in
the low 8 bits of hash and the nibble in bits 11:8, while the Intel
layout puts addr[5] in the high bits. The driver has to reproduce
exactly the index the hardware derives, so both the nibble that is
selected and where it lands matter.
If the index is wrong, mcast_hash[hash >> 5] |= BIT(hash & 0x1f) sets
the wrong bit and RNPGBE_RX_MCAST_HASH() ends up programmed for
groups nobody joined while the joined ones are filtered out. IPv6
neighbour discovery is the obvious casualty: solicited-node addresses
33:33:ff:xx:xx:xx differ only in the trailing bytes, so picking the
wrong nibble of ha->addr[4] mis-indexes essentially every entry, and
the failure is silent - traffic simply disappears unless something
puts the interface into IFF_ALLMULTI or IFF_PROMISC.
Could you confirm against the hardware documentation which address
bits the MTA index is built from, and then either fix the extraction
or correct the comment so the two agree?
[ ... ]
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index e62366291b2a..817081e24bb5 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> @@ -43,7 +43,12 @@ static int rnpgbe_configure(struct mucse *mucse)
> if (err)
> return err;
>
> - return rnpgbe_configure_rx(mucse);
> + err = rnpgbe_configure_rx(mucse);
> + if (err)
> + return err;
> +
> + rnpgbe_set_rx_mode(mucse->netdev);
> + return 0;
> }
[Severity: High]
Is this direct call missing the address list lock? rnpgbe_configure() runs
from the ndo_open path with only RTNL held, while every core invocation of
ndo_set_rx_mode wraps the callback in netif_addr_lock_bh() /
netif_addr_unlock_bh() (netif_rx_mode_run() and dev_set_rx_mode() in
net/core/dev_addr_lists.c).
rnpgbe_set_rx_mode() walks dev->uc and dev->mc with the plain, non-RCU
iterators:
netdev_for_each_uc_addr(ha, netdev) {
...
rnpgbe_set_rar(hw, rar, ha->addr);
...
netdev_for_each_mc_addr(ha, netdev) {
hash = ((ha->addr[4] & 0xf) << 8) | ha->addr[5];
Writers only take netif_addr_lock_bh(), not RTNL, and free entries with RCU:
net/core/dev_addr_lists.c:__hw_addr_del_entry() {
list_del_rcu(&ha->list);
kfree_rcu(ha, rcu_head);
}
Such a writer is reachable without RTNL from an ordinary socket:
setsockopt(IPV6_JOIN_GROUP) -> ipv6_sock_mc_join() ->
__ipv6_sock_mc_join() -> __ipv6_dev_mc_inc() -> igmp6_group_added() ->
dev_mc_add()
Can the walk above then dereference freed memory? The open path is
preemptible and holds no rcu_read_lock(), so if it is preempted while
holding ha, a concurrent list_del_rcu() + kfree_rcu() can complete its grace
period and the next ha->list.next dereference reads freed memory, possibly
programming a RAR from it.
Is the call needed at all? __dev_open() calls dev_set_rx_mode() right after
ndo_open returns, which invokes rnpgbe_set_rx_mode() with the address lock
held. If it is needed here, would netif_addr_lock_bh() around it be
appropriate?
[ ... ]
> @@ -149,3 +149,76 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type)
>
> return 0;
> }
> +
> +static void rnpgbe_set_rar(struct mucse_hw *hw, u32 index, const u8 *addr)
> +{
> + u32 rar_low, rar_high;
> +
> + /* The RAR stores the Ethernet address in reverse byte order. */
> + rar_low = (u32)addr[5] | ((u32)addr[4] << 8) |
> + ((u32)addr[3] << 16) | ((u32)addr[2] << 24);
> + rar_high = (u32)addr[1] | ((u32)addr[0] << 8) |
> + RNPGBE_RX_RAR_VALID;
> + mucse_hw_wr32(hw, RNPGBE_RX_RAR_LOW(index), rar_low);
> + mucse_hw_wr32(hw, RNPGBE_RX_RAR_HIGH(index), rar_high);
> +}
> +
> +static void rnpgbe_clear_rar(struct mucse_hw *hw, u32 index)
> +{
> + mucse_hw_wr32(hw, RNPGBE_RX_RAR_LOW(index), 0);
> + mucse_hw_wr32(hw, RNPGBE_RX_RAR_HIGH(index), 0);
> +}
> +
> +void rnpgbe_set_rx_mode(struct net_device *netdev)
> +{
> + u32 mcast_hash[RNPGBE_RX_MCAST_HASH_ENTRIES] = {};
> + u32 mcast_ctrl = RNPGBE_RX_UCAST_TABLE_EN;
> + u32 filter_ctrl = RNPGBE_RX_FILTER_BCAST;
> + struct netdev_hw_addr *ha;
> + struct mucse_hw *hw;
> + struct mucse *mucse;
> + int rar = 1;
> + u16 hash;
> + int i;
> +
> + mucse = netdev_priv(netdev);
> + hw = &mucse->hw;
> +
> + /* RAR 0 always holds the interface's primary unicast address. */
> + rnpgbe_set_rar(hw, 0, netdev->dev_addr);
> +
> + netdev_for_each_uc_addr(ha, netdev) {
> + if (rar == RNPGBE_RX_RAR_ENTRIES) {
> + filter_ctrl |= RNPGBE_RX_FILTER_UCAST_ALL;
> + break;
> + }
> +
> + rnpgbe_set_rar(hw, rar, ha->addr);
> + rar++;
> + }
> +
> + for (; rar < RNPGBE_RX_RAR_ENTRIES; rar++)
> + rnpgbe_clear_rar(hw, rar);
> +
> + if (netdev->flags & IFF_PROMISC) {
> + filter_ctrl |= RNPGBE_RX_FILTER_UCAST_ALL |
> + RNPGBE_RX_FILTER_MCAST_ALL;
> + } else if (netdev->flags & IFF_ALLMULTI) {
> + filter_ctrl |= RNPGBE_RX_FILTER_MCAST_ALL;
> + } else {
> + /* The MTA index is the 12 address bits from 36 through 47. */
> + netdev_for_each_mc_addr(ha, netdev) {
> + hash = ((ha->addr[4] & 0xf) << 8) | ha->addr[5];
> + mcast_hash[hash >> 5] |= BIT(hash & 0x1f);
> + }
> +
> + if (!netdev_mc_empty(netdev))
> + mcast_ctrl |= RNPGBE_RX_MCAST_HASH_EN;
> + }
> +
> + for (i = 0; i < RNPGBE_RX_MCAST_HASH_ENTRIES; i++)
> + mucse_hw_wr32(hw, RNPGBE_RX_MCAST_HASH(i), mcast_hash[i]);
> +
> + mucse_hw_wr32(hw, RNPGBE_RX_MCAST_CTRL, mcast_ctrl);
> + mucse_hw_wr32(hw, RNPGBE_RX_FILTER_CTRL, filter_ctrl);
> +}
[Severity: Medium]
RNPGBE_RX_RAR_VALID lives in the HIGH register, and both helpers
touch that register last. rnpgbe_clear_rar() zeroes
RNPGBE_RX_RAR_LOW(index) while the entry is still marked valid, so
between the two writes the filter matches the old addr[0], addr[1]
followed by four zero bytes. rnpgbe_set_rar() has the mirror image
of the same problem when it overwrites an entry that is already
valid: rar_low is programmed first, so for that window the entry
matches the new addr[2..5] against the stale addr[0..1] - an address
that was never configured on this interface.
This is not confined to teardown. rnpgbe_set_rx_mode() reprograms
the whole table from scratch on every invocation, walking RAR 0
through RNPGBE_RX_RAR_ENTRIES - 1 with rnpgbe_set_rar() and
rnpgbe_clear_rar(), and it is called on every unicast/multicast list
change as well as from rnpgbe_configure() after RX has been started,
so the window is hit routinely while the MAC is receiving.
The conventional ordering is to drop the valid bit before touching
the address bytes: see ixgbe_clear_rar(), which writes
IXGBE_RAH(index) with IXGBE_RAH_AV cleared first and only then sets
IXGBE_RAL(index) to 0.
Does this hardware latch the RAR pair on the HIGH write, so that the
LOW value has no effect until HIGH is written? If it does, a short
comment saying so would settle the question; if it does not, clearing
RNPGBE_RX_RAR_VALID first in both helpers looks like the right fix.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH net-next v10 4/5] net: rnpgbe: Add receive mode support
2026-09-04 21:57 ` Jakub Kicinski
@ 2026-09-08 7:46 ` Yibo Dong
0 siblings, 0 replies; 15+ messages in thread
From: Yibo Dong @ 2026-09-08 7:46 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, davem, edumazet, pabeni, vadim.fedorenko,
u.kleine-koenig, netdev, linux-kernel, yaojun
On Fri, Sep 04, 2026 at 02:57:17PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: rnpgbe: Add receive mode support
>
> This adds chip-level receive filtering to rnpgbe along with the
> ndo_set_rx_mode callback. Unicast addresses go into the 30 usable RAR
> entries and multicast addresses into the 4096-bit hash table, with
> IFF_PROMISC and IFF_ALLMULTI handled through the filter control register.
> rnpgbe_configure() also calls the new function directly from the open path.
>
> Two questions below.
>
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > index 291e77d573fe..c7d5b53f6209 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > @@ -149,3 +149,76 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type)
>
> [ ... ]
>
> > +void rnpgbe_set_rx_mode(struct net_device *netdev)
> > +{
> > + u32 mcast_hash[RNPGBE_RX_MCAST_HASH_ENTRIES] = {};
> > + u32 mcast_ctrl = RNPGBE_RX_UCAST_TABLE_EN;
> > + u32 filter_ctrl = RNPGBE_RX_FILTER_BCAST;
> > + struct netdev_hw_addr *ha;
> > + struct mucse_hw *hw;
> > + struct mucse *mucse;
> > + int rar = 1;
> > + u16 hash;
> > + int i;
> > +
> > + mucse = netdev_priv(netdev);
> > + hw = &mucse->hw;
> > +
> > + /* RAR 0 always holds the interface's primary unicast address. */
> > + rnpgbe_set_rar(hw, 0, netdev->dev_addr);
> > +
> > + netdev_for_each_uc_addr(ha, netdev) {
> > + if (rar == RNPGBE_RX_RAR_ENTRIES) {
> > + filter_ctrl |= RNPGBE_RX_FILTER_UCAST_ALL;
> > + break;
> > + }
> > +
> > + rnpgbe_set_rar(hw, rar, ha->addr);
> > + rar++;
> > + }
> > +
> > + for (; rar < RNPGBE_RX_RAR_ENTRIES; rar++)
> > + rnpgbe_clear_rar(hw, rar);
> > +
> > + if (netdev->flags & IFF_PROMISC) {
> > + filter_ctrl |= RNPGBE_RX_FILTER_UCAST_ALL |
> > + RNPGBE_RX_FILTER_MCAST_ALL;
>
> [Severity: High]
> Can this secondary unicast RAR programming ever take effect? I could not
> find netdev->priv_flags |= IFF_UNICAST_FLT anywhere under
> drivers/net/ethernet/mucse/rnpgbe/, including rnpgbe_add_adapter() where
> netdev_ops and features are set up before register_netdev().
>
> Without that flag, the core forces promiscuous mode as soon as dev->uc is
> non-empty, in netif_uc_promisc_update():
>
> net/core/dev_addr_lists.c:netif_uc_promisc_update() {
> if (dev->priv_flags & IFF_UNICAST_FLT)
> return 0;
>
> if (!netdev_uc_empty(dev) && !dev->uc_promisc) {
> dev->uc_promisc = true;
> return 1;
> }
> }
>
> Both callers (netif_rx_mode_run() and __dev_set_rx_mode()) then call
> __dev_set_promiscuity(dev, +1, false), which sets IFF_PROMISC in dev->flags
> before ops->ndo_set_rx_mode() runs.
>
> So for any configuration that adds a secondary unicast address (macvlan,
> ipvlan, a bridge port with a different MAC, VM MAC passthrough),
> rnpgbe_set_rx_mode() always sees IFF_PROMISC and takes the branch above.
> The RAR entries just written become irrelevant, and since the else branch is
> skipped the mcast_hash[] array stays all zero and RNPGBE_RX_MCAST_HASH_EN is
> never set, so multicast filtering is off as well and the adapter accepts
> everything on the wire.
>
> That also seems to make this part of the changelog unobservable:
>
> Fall back to accepting all unicast addresses when the RAR
> table is exhausted
>
> the fallback is pre-empted by the core at the first secondary address, long
> before 30 RAR entries are used. Should the driver set IFF_UNICAST_FLT in
> rnpgbe_add_adapter(), or is the exact unicast filter logic here not intended
> to be used?
>
Got it, I will set IFF_UNICAST_FLT before register_netdev().
> > + } else if (netdev->flags & IFF_ALLMULTI) {
> > + filter_ctrl |= RNPGBE_RX_FILTER_MCAST_ALL;
> > + } else {
> > + /* The MTA index is the 12 address bits from 36 through 47. */
> > + netdev_for_each_mc_addr(ha, netdev) {
> > + hash = ((ha->addr[4] & 0xf) << 8) | ha->addr[5];
> > + mcast_hash[hash >> 5] |= BIT(hash & 0x1f);
> > + }
> > +
> > + if (!netdev_mc_empty(netdev))
> > + mcast_ctrl |= RNPGBE_RX_MCAST_HASH_EN;
> > + }
>
> [Severity: High]
> The comment directly above this line says the MTA index is the 12
> address bits from 36 through 47, but the expression does not extract
> those bits. With the usual MAC bit numbering (bit 0 is the LSB of
> addr[0], so ha->addr[4] carries bits 32-39 and ha->addr[5] bits
> 40-47), ha->addr[4] & 0xf is bits 32-35. Bits 36-39 are the upper
> nibble, i.e. ha->addr[4] >> 4. Compare ixgbe_mta_vector() in
> drivers/net/ethernet/intel/ixgbe/ixgbe_common.c, which spells the
> same field for "bits [47:36]" as
> ((mc_addr[4] >> 4) | (((u16)mc_addr[5]) << 4)).
>
> The placement of the pieces differs too: here ha->addr[5] ends up in
> the low 8 bits of hash and the nibble in bits 11:8, while the Intel
> layout puts addr[5] in the high bits. The driver has to reproduce
> exactly the index the hardware derives, so both the nibble that is
> selected and where it lands matter.
>
> If the index is wrong, mcast_hash[hash >> 5] |= BIT(hash & 0x1f) sets
> the wrong bit and RNPGBE_RX_MCAST_HASH() ends up programmed for
> groups nobody joined while the joined ones are filtered out. IPv6
> neighbour discovery is the obvious casualty: solicited-node addresses
> 33:33:ff:xx:xx:xx differ only in the trailing bytes, so picking the
> wrong nibble of ha->addr[4] mis-indexes essentially every entry, and
> the failure is silent - traffic simply disappears unless something
> puts the interface into IFF_ALLMULTI or IFF_PROMISC.
>
> Could you confirm against the hardware documentation which address
> bits the MTA index is built from, and then either fix the extraction
> or correct the comment so the two agree?
>
> [ ... ]
>
The hardware defines the multicast filter vector as:
((addr[4] << 8) | addr[5]) & 0xfff
The upper 7 bits select one of the 128 MTA registers and the lower
5 bits select the bit within that register. The current calculation
implements this hardware-defined layout, so it intentionally differs
from ixgbe's multicast hash layout.
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> > index e62366291b2a..817081e24bb5 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> > @@ -43,7 +43,12 @@ static int rnpgbe_configure(struct mucse *mucse)
> > if (err)
> > return err;
> >
> > - return rnpgbe_configure_rx(mucse);
> > + err = rnpgbe_configure_rx(mucse);
> > + if (err)
> > + return err;
> > +
> > + rnpgbe_set_rx_mode(mucse->netdev);
> > + return 0;
> > }
>
> [Severity: High]
> Is this direct call missing the address list lock? rnpgbe_configure() runs
> from the ndo_open path with only RTNL held, while every core invocation of
> ndo_set_rx_mode wraps the callback in netif_addr_lock_bh() /
> netif_addr_unlock_bh() (netif_rx_mode_run() and dev_set_rx_mode() in
> net/core/dev_addr_lists.c).
>
> rnpgbe_set_rx_mode() walks dev->uc and dev->mc with the plain, non-RCU
> iterators:
>
> netdev_for_each_uc_addr(ha, netdev) {
> ...
> rnpgbe_set_rar(hw, rar, ha->addr);
> ...
> netdev_for_each_mc_addr(ha, netdev) {
> hash = ((ha->addr[4] & 0xf) << 8) | ha->addr[5];
>
> Writers only take netif_addr_lock_bh(), not RTNL, and free entries with RCU:
>
> net/core/dev_addr_lists.c:__hw_addr_del_entry() {
> list_del_rcu(&ha->list);
> kfree_rcu(ha, rcu_head);
> }
>
> Such a writer is reachable without RTNL from an ordinary socket:
>
> setsockopt(IPV6_JOIN_GROUP) -> ipv6_sock_mc_join() ->
> __ipv6_sock_mc_join() -> __ipv6_dev_mc_inc() -> igmp6_group_added() ->
> dev_mc_add()
>
> Can the walk above then dereference freed memory? The open path is
> preemptible and holds no rcu_read_lock(), so if it is preempted while
> holding ha, a concurrent list_del_rcu() + kfree_rcu() can complete its grace
> period and the next ha->list.next dereference reads freed memory, possibly
> programming a RAR from it.
>
> Is the call needed at all? __dev_open() calls dev_set_rx_mode() right after
> ndo_open returns, which invokes rnpgbe_set_rx_mode() with the address lock
> held. If it is needed here, would netif_addr_lock_bh() around it be
> appropriate?
>
The filter tables need to be configured before RX_START is enabled, so the
direct call is required. I will keep it and protect the address-list walk
with netif_addr_lock_bh()/netif_addr_unlock_bh().
> [ ... ]
>
> > @@ -149,3 +149,76 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type)
> >
> > return 0;
> > }
> > +
> > +static void rnpgbe_set_rar(struct mucse_hw *hw, u32 index, const u8 *addr)
> > +{
> > + u32 rar_low, rar_high;
> > +
> > + /* The RAR stores the Ethernet address in reverse byte order. */
> > + rar_low = (u32)addr[5] | ((u32)addr[4] << 8) |
> > + ((u32)addr[3] << 16) | ((u32)addr[2] << 24);
> > + rar_high = (u32)addr[1] | ((u32)addr[0] << 8) |
> > + RNPGBE_RX_RAR_VALID;
> > + mucse_hw_wr32(hw, RNPGBE_RX_RAR_LOW(index), rar_low);
> > + mucse_hw_wr32(hw, RNPGBE_RX_RAR_HIGH(index), rar_high);
> > +}
> > +
> > +static void rnpgbe_clear_rar(struct mucse_hw *hw, u32 index)
> > +{
> > + mucse_hw_wr32(hw, RNPGBE_RX_RAR_LOW(index), 0);
> > + mucse_hw_wr32(hw, RNPGBE_RX_RAR_HIGH(index), 0);
> > +}
> > +
> > +void rnpgbe_set_rx_mode(struct net_device *netdev)
> > +{
> > + u32 mcast_hash[RNPGBE_RX_MCAST_HASH_ENTRIES] = {};
> > + u32 mcast_ctrl = RNPGBE_RX_UCAST_TABLE_EN;
> > + u32 filter_ctrl = RNPGBE_RX_FILTER_BCAST;
> > + struct netdev_hw_addr *ha;
> > + struct mucse_hw *hw;
> > + struct mucse *mucse;
> > + int rar = 1;
> > + u16 hash;
> > + int i;
> > +
> > + mucse = netdev_priv(netdev);
> > + hw = &mucse->hw;
> > +
> > + /* RAR 0 always holds the interface's primary unicast address. */
> > + rnpgbe_set_rar(hw, 0, netdev->dev_addr);
> > +
> > + netdev_for_each_uc_addr(ha, netdev) {
> > + if (rar == RNPGBE_RX_RAR_ENTRIES) {
> > + filter_ctrl |= RNPGBE_RX_FILTER_UCAST_ALL;
> > + break;
> > + }
> > +
> > + rnpgbe_set_rar(hw, rar, ha->addr);
> > + rar++;
> > + }
> > +
> > + for (; rar < RNPGBE_RX_RAR_ENTRIES; rar++)
> > + rnpgbe_clear_rar(hw, rar);
> > +
> > + if (netdev->flags & IFF_PROMISC) {
> > + filter_ctrl |= RNPGBE_RX_FILTER_UCAST_ALL |
> > + RNPGBE_RX_FILTER_MCAST_ALL;
> > + } else if (netdev->flags & IFF_ALLMULTI) {
> > + filter_ctrl |= RNPGBE_RX_FILTER_MCAST_ALL;
> > + } else {
> > + /* The MTA index is the 12 address bits from 36 through 47. */
> > + netdev_for_each_mc_addr(ha, netdev) {
> > + hash = ((ha->addr[4] & 0xf) << 8) | ha->addr[5];
> > + mcast_hash[hash >> 5] |= BIT(hash & 0x1f);
> > + }
> > +
> > + if (!netdev_mc_empty(netdev))
> > + mcast_ctrl |= RNPGBE_RX_MCAST_HASH_EN;
> > + }
> > +
> > + for (i = 0; i < RNPGBE_RX_MCAST_HASH_ENTRIES; i++)
> > + mucse_hw_wr32(hw, RNPGBE_RX_MCAST_HASH(i), mcast_hash[i]);
> > +
> > + mucse_hw_wr32(hw, RNPGBE_RX_MCAST_CTRL, mcast_ctrl);
> > + mucse_hw_wr32(hw, RNPGBE_RX_FILTER_CTRL, filter_ctrl);
> > +}
>
> [Severity: Medium]
> RNPGBE_RX_RAR_VALID lives in the HIGH register, and both helpers
> touch that register last. rnpgbe_clear_rar() zeroes
> RNPGBE_RX_RAR_LOW(index) while the entry is still marked valid, so
> between the two writes the filter matches the old addr[0], addr[1]
> followed by four zero bytes. rnpgbe_set_rar() has the mirror image
> of the same problem when it overwrites an entry that is already
> valid: rar_low is programmed first, so for that window the entry
> matches the new addr[2..5] against the stale addr[0..1] - an address
> that was never configured on this interface.
>
> This is not confined to teardown. rnpgbe_set_rx_mode() reprograms
> the whole table from scratch on every invocation, walking RAR 0
> through RNPGBE_RX_RAR_ENTRIES - 1 with rnpgbe_set_rar() and
> rnpgbe_clear_rar(), and it is called on every unicast/multicast list
> change as well as from rnpgbe_configure() after RX has been started,
> so the window is hit routinely while the MAC is receiving.
>
> The conventional ordering is to drop the valid bit before touching
> the address bytes: see ixgbe_clear_rar(), which writes
> IXGBE_RAH(index) with IXGBE_RAH_AV cleared first and only then sets
> IXGBE_RAL(index) to 0.
>
> Does this hardware latch the RAR pair on the HIGH write, so that the
> LOW value has no effect until HIGH is written? If it does, a short
> comment saying so would settle the question; if it does not, clearing
> RNPGBE_RX_RAR_VALID first in both helpers looks like the right fix.
>
The RAR valid bit only controls address comparison; it does not latch the
RAR pair. I will clear VALID before updating or clearing the address fields,
and set it again only after the complete new address has been programmed
Thanks for your feedback.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v10 5/5] net: rnpgbe: Add link status handling support
[not found] <20260831073608.401988-1-dong100@mucse.com>
` (3 preceding siblings ...)
2026-08-31 7:36 ` [PATCH net-next v10 4/5] net: rnpgbe: Add receive mode support Dong Yibo
@ 2026-08-31 7:36 ` Dong Yibo
2026-09-04 21:57 ` Jakub Kicinski
4 siblings, 1 reply; 15+ messages in thread
From: Dong Yibo @ 2026-08-31 7:36 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, vadim.fedorenko,
u.kleine-koenig
Cc: netdev, linux-kernel, dong100, yaojun
Add link status handling for the RNPGBE driver.
Introduce link state data structures for speed, duplex and link state,
and process firmware link events delivered over the mailbox, since
firmware controls the link and phylink is not used. Add a service task
to handle pending events and manage carrier status with
netif_carrier_on()/netif_carrier_off().
Notify firmware when the interface is brought up and after the data path
is quiesced during teardown, and fail interface open if the firmware
port-up or link-report setup fails. Validate firmware link events before
updating the cached link state. Enable the GMAC receiver in receive-all
mode after the chip-level receive filter has been configured, and
document the driver-to-firmware link-state snapshot and reset semantics.
Signed-off-by: Dong Yibo <dong100@mucse.com>
---
drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 14 +-
.../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 35 ++-
drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 17 ++
.../net/ethernet/mucse/rnpgbe/rnpgbe_lib.c | 197 ++++++++++++++-
.../net/ethernet/mucse/rnpgbe/rnpgbe_lib.h | 3 +-
.../net/ethernet/mucse/rnpgbe/rnpgbe_main.c | 15 +-
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 20 ++
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 1 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c | 228 +++++++++++++++++-
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h | 42 ++++
10 files changed, 562 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 8d7c6f8301cd..22b224f97e8b 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -5,6 +5,7 @@
#define _RNPGBE_H
#include <linux/types.h>
+#include <linux/atomic.h>
#include <linux/mutex.h>
#include <linux/netdevice.h>
#include <linux/timer.h>
@@ -32,11 +33,10 @@ struct mucse_mbx_info {
u32 fwpf_ctrl_base;
};
-/* Enum for firmware notification modes,
- * more modes (e.g., portup, link_report) will be added in future
- **/
enum {
mucse_fw_powerup,
+ mucse_fw_portup,
+ mucse_fw_link_report_en,
};
struct mucse_hw {
@@ -45,8 +45,11 @@ struct mucse_hw {
struct pci_dev *pdev;
struct mucse_mbx_info mbx;
int port;
+ int speed;
+ bool link;
u16 cycles_per_us;
u8 pfvfnum;
+ u8 duplex;
};
struct rnpgbe_tx_desc {
@@ -224,7 +227,11 @@ struct mucse {
int num_rx_queues;
char mbx_name[32];
unsigned long state;
+ atomic_t link_pending;
+ atomic_t mbx_irq_seq;
struct work_struct mbx_work;
+ struct delayed_work serv_task;
+ spinlock_t link_lock; /* spinlock for link update */
};
int rnpgbe_get_permanent_mac(struct mucse_hw *hw, u8 *perm_addr);
@@ -234,6 +241,7 @@ int rnpgbe_send_notify(struct mucse_hw *hw,
int mode);
int rnpgbe_init_hw(struct mucse_hw *hw, int board_type);
void rnpgbe_set_rx_mode(struct net_device *netdev);
+void rnpgbe_set_link(struct mucse_hw *hw, bool linkup);
/* Device IDs */
#define PCI_VENDOR_ID_MUCSE 0x8848
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
index c7d5b53f6209..2a7f13e85e39 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
@@ -66,11 +66,17 @@ int rnpgbe_send_notify(struct mucse_hw *hw,
int mode)
{
int err;
- /* Keep switch struct to support more modes in the future */
+
switch (mode) {
case mucse_fw_powerup:
err = mucse_mbx_powerup(hw, enable);
break;
+ case mucse_fw_portup:
+ err = mucse_mbx_phyup(hw, enable);
+ break;
+ case mucse_fw_link_report_en:
+ err = mucse_mbx_link_report(hw, enable);
+ break;
default:
err = -EINVAL;
}
@@ -222,3 +228,30 @@ void rnpgbe_set_rx_mode(struct net_device *netdev)
mucse_hw_wr32(hw, RNPGBE_RX_MCAST_CTRL, mcast_ctrl);
mucse_hw_wr32(hw, RNPGBE_RX_FILTER_CTRL, filter_ctrl);
}
+
+/**
+ * rnpgbe_set_link - Set the hardware link state
+ * @hw: hw information structure
+ * @linkup: link on or not
+ *
+ * rnpgbe_set_link setup link status
+ *
+ **/
+void rnpgbe_set_link(struct mucse_hw *hw, bool linkup)
+{
+ u32 value = mucse_hw_rd32(hw, GMAC_CONTROL);
+
+ /* The chip-level filter is programmed by ndo_set_rx_mode(). Keep the
+ * GMAC in receive-all mode so it does not discard frames accepted by
+ * that filter.
+ */
+ if (linkup) {
+ mucse_hw_wr32(hw, GMAC_FRAME_FILTER, GMAC_RX_ALL);
+ value |= GMAC_CONTROL_RE;
+ mucse_hw_wr32(hw, GMAC_CONTROL, value);
+ } else {
+ value &= ~GMAC_CONTROL_RE;
+ mucse_hw_wr32(hw, GMAC_CONTROL, value);
+ mucse_hw_wr32(hw, GMAC_FRAME_FILTER, 0);
+ }
+}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
index feb84704d602..5659e91f3497 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
@@ -21,6 +21,17 @@
#define RNPGBE_DMA_TX_STATUS GENMASK_U32(23, 22)
#define RNPGBE_DMA_RX_STATUS GENMASK_U32(21, 20)
#define RX_AXI_RW_EN 0x03
+/* RNPGBE_LINK_ST is a driver-owned link-state snapshot consumed by firmware.
+ * Set the driver-state marker; all other driver status fields start clear.
+ */
+#define M_DEFAULT_ST 0xa0000000
+/* Firmware never reports 0xf in the speed field. */
+#define M_INVALID_ST (M_DEFAULT_ST | GENMASK_U32(11, 8))
+/* Driver-reported fields: 25:24 pause, 11:8 speed, 6 LLDP, 4 duplex,
+ * and 0 link up/down. M_DEFAULT_ST resets these fields so firmware reports
+ * the current hardware state again.
+ */
+#define RNPGBE_LINK_ST 0x000c
#define RNPGBE_DMA_AXI_EN 0x0010
#define RNPGBE_TX_MIN_PKT_LEN 33
@@ -39,5 +50,11 @@
/* The final two of the 32 hardware RAR entries are reserved for NCSI. */
#define RNPGBE_RX_RAR_ENTRIES 30
#define RNPGBE_RX_MCAST_HASH_ENTRIES 128
+
+#define MUCSE_GMAC_OFF(_n) (0x20000 + (_n))
+#define GMAC_CONTROL_RE 0x00000004
+#define GMAC_CONTROL MUCSE_GMAC_OFF(0)
+#define GMAC_RX_ALL (BIT(31) | BIT(0))
+#define GMAC_FRAME_FILTER MUCSE_GMAC_OFF(0x4)
#define RNPGBE_MAX_QUEUES 8
#endif /* _RNPGBE_HW_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
index 346e9a1cca8a..1c649ef7ee3a 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
@@ -16,8 +16,23 @@
static void rnpgbe_mbx_work(struct work_struct *work)
{
struct mucse *mucse = container_of(work, struct mucse, mbx_work);
+ int irq_seq;
+ irq_seq = atomic_read(&mucse->mbx_irq_seq);
mucse_fw_irq_handler(&mucse->hw);
+
+ /*
+ * Only multi-vector MSI-X has a dedicated mailbox interrupt, so its
+ * hard IRQ records a sequence number. With a shared MSI or MSI-X
+ * vector, all interrupts queue this work and it checks the mailbox
+ * request counter. Firmware posts one request at a time and waits for
+ * the PF ACK before posting the next one. A mailbox interrupt while
+ * this work runs queues a later invocation.
+ */
+ if ((mucse->flags & M_FLAG_MSIX_EN) &&
+ irq_seq != atomic_read(&mucse->mbx_irq_seq) &&
+ !test_bit(__MUCSE_DOWN, &mucse->state))
+ queue_work(system_percpu_wq, &mucse->mbx_work);
}
/**
@@ -31,6 +46,7 @@ static irqreturn_t rnpgbe_msix_other(int irq, void *data)
{
struct mucse *mucse = (struct mucse *)data;
+ atomic_inc(&mucse->mbx_irq_seq);
queue_work(system_percpu_wq, &mucse->mbx_work);
return IRQ_HANDLED;
@@ -675,6 +691,7 @@ int rnpgbe_request_mbx_irq(struct mucse *mucse)
snprintf(mucse->mbx_name, sizeof(mucse->mbx_name),
"rnpgbe-mbx:%s", pci_name(pdev));
INIT_WORK(&mucse->mbx_work, rnpgbe_mbx_work);
+ atomic_set(&mucse->mbx_irq_seq, 0);
if (mucse->flags & M_FLAG_MSIX_EN) {
err = request_irq(pci_irq_vector(pdev, 0),
@@ -1430,15 +1447,42 @@ static void rnpgbe_cancel_rx_retry_timers(struct mucse *mucse)
bool rnpgbe_down(struct mucse *mucse)
{
struct net_device *netdev = mucse->netdev;
+ struct mucse_hw *hw = &mucse->hw;
+ unsigned long flags;
+ int err;
if (test_and_set_bit(__MUCSE_DOWN, &mucse->state))
return false;
+ spin_lock_irqsave(&mucse->link_lock, flags);
+ WRITE_ONCE(hw->link, false);
+ WRITE_ONCE(hw->speed, 0);
+ WRITE_ONCE(hw->duplex, 0);
+ atomic_set(&mucse->link_pending, 0);
+ spin_unlock_irqrestore(&mucse->link_lock, flags);
+ cancel_delayed_work_sync(&mucse->serv_task);
+ rnpgbe_set_link(hw, false);
+
+ netif_carrier_off(netdev);
netif_tx_disable(netdev);
rnpgbe_napi_disable_all(mucse);
rnpgbe_irq_disable(mucse);
rnpgbe_stop_all_rx_rings(mucse);
rnpgbe_cancel_rx_retry_timers(mucse);
+
+ err = rnpgbe_send_notify(hw, false, mucse_fw_link_report_en);
+ if (err) {
+ dev_warn(&hw->pdev->dev, "Send link report to hw failed %d\n",
+ err);
+ dev_warn(&hw->pdev->dev, "Fw will still report link event\n");
+ }
+
+ err = rnpgbe_send_notify(hw, false, mucse_fw_portup);
+ if (err) {
+ dev_warn(&hw->pdev->dev, "Send port down to hw failed %d\n",
+ err);
+ dev_warn(&hw->pdev->dev, "Port is not truly down\n");
+ }
rnpgbe_clean_all_tx_rings(mucse);
rnpgbe_clean_all_rx_rings(mucse);
@@ -1448,10 +1492,15 @@ bool rnpgbe_down(struct mucse *mucse)
/**
* rnpgbe_up_complete - Final step for port up
* @mucse: pointer to private structure
+ *
+ * Return: 0 on success, negative errno if firmware setup fails
**/
-void rnpgbe_up_complete(struct mucse *mucse)
+int rnpgbe_up_complete(struct mucse *mucse)
{
struct net_device *netdev = mucse->netdev;
+ struct mucse_hw *hw = &mucse->hw;
+ unsigned long flags;
+ int err;
if (mucse->flags & (M_FLAG_MSIX_EN | M_FLAG_MSIX_SINGLE_EN))
rnpgbe_configure_msix(mucse);
@@ -1459,9 +1508,47 @@ void rnpgbe_up_complete(struct mucse *mucse)
rnpgbe_configure_msi(mucse);
rnpgbe_napi_enable_all(mucse);
+ /* Clear stale link state and tell firmware the driver hasn't
+ * seen any link yet. Firmware will re-assert LINK_CHANGE_EVT
+ * when link_report_en is sent below since RNPGBE_LINK_ST
+ * no longer matches the actual (possibly up) link state.
+ */
+ spin_lock_irqsave(&mucse->link_lock, flags);
+ WRITE_ONCE(hw->link, false);
+ WRITE_ONCE(hw->speed, 0);
+ WRITE_ONCE(hw->duplex, 0);
+ atomic_set(&mucse->link_pending, 0);
+ /* echo fw driver now in default state */
+ mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_DEFAULT_ST);
+ /* Keep the default snapshot and DOWN state transition atomic to
+ * mailbox link-event handling.
+ */
clear_bit(__MUCSE_DOWN, &mucse->state);
+ spin_unlock_irqrestore(&mucse->link_lock, flags);
rnpgbe_irq_enable(mucse);
netif_tx_start_all_queues(netdev);
+ err = rnpgbe_send_notify(hw, true, mucse_fw_portup);
+ if (err) {
+ dev_err(&hw->pdev->dev,
+ "Failed to notify firmware that port is up: %d\n", err);
+ return err;
+ }
+ /* Firmware checks RNPGBE_LINK_ST (driver's last-received link state)
+ * and only asserts LINK_CHANGE_EVT when it differs from the actual link
+ * state AND link_report_en is true.
+ */
+ err = rnpgbe_send_notify(hw, true, mucse_fw_link_report_en);
+ if (err) {
+ dev_err(&hw->pdev->dev,
+ "Failed to enable firmware link reporting: %d\n",
+ err);
+ return err;
+ }
+
+ queue_delayed_work(system_percpu_wq, &mucse->serv_task,
+ msecs_to_jiffies(500));
+
+ return 0;
}
/**
@@ -2120,3 +2207,111 @@ int rnpgbe_configure_rx(struct mucse *mucse)
return 0;
}
+
+/**
+ * rnpgbe_process_link_event - Consume a pending link event
+ * @mucse: pointer to the device private structure
+ * @link: link status snapshot
+ * @speed: link speed snapshot
+ * @duplex: link duplex snapshot
+ *
+ * Return: true if a link event was consumed, false otherwise
+ **/
+static bool rnpgbe_process_link_event(struct mucse *mucse, bool *link,
+ int *speed, u8 *duplex)
+{
+ struct mucse_hw *hw = &mucse->hw;
+ unsigned long flags;
+
+ if (!atomic_xchg(&mucse->link_pending, 0))
+ return false;
+
+ spin_lock_irqsave(&mucse->link_lock, flags);
+ *link = hw->link;
+ *speed = hw->speed;
+ *duplex = hw->duplex;
+ spin_unlock_irqrestore(&mucse->link_lock, flags);
+
+ return true;
+}
+
+/**
+ * rnpgbe_link_is_up - Update netif_carrier status and
+ * print link up message
+ * @mucse: pointer to the device private structure
+ * @speed: link speed snapshot
+ * @duplex: link duplex snapshot
+ **/
+static void rnpgbe_link_is_up(struct mucse *mucse, int speed, u8 duplex)
+{
+ struct net_device *netdev = mucse->netdev;
+ struct mucse_hw *hw = &mucse->hw;
+
+ /* Only continue if link was previously down */
+ if (netif_carrier_ok(netdev))
+ return;
+
+ netdev_info(netdev, "NIC Link is Up %d Mbps, %s Duplex\n",
+ speed, duplex ? "Full" : "Half");
+ rnpgbe_set_link(hw, true);
+ netif_carrier_on(netdev);
+}
+
+/**
+ * rnpgbe_link_is_down - Update netif_carrier status and
+ * print link down message
+ * @mucse: pointer to the private structure
+ **/
+static void rnpgbe_link_is_down(struct mucse *mucse)
+{
+ struct net_device *netdev = mucse->netdev;
+ struct mucse_hw *hw = &mucse->hw;
+
+ /* Only continue if link was up previously */
+ if (!netif_carrier_ok(netdev))
+ return;
+ netdev_info(netdev, "NIC Link is Down\n");
+ rnpgbe_set_link(hw, false);
+ netif_carrier_off(netdev);
+}
+
+/**
+ * rnpgbe_process_link_subtask - Process a link-state update
+ * @mucse: pointer to the device private structure
+ **/
+static void rnpgbe_process_link_subtask(struct mucse *mucse)
+{
+ bool link;
+ int speed;
+ u8 duplex;
+
+ /* if interface is down do nothing */
+ if (test_bit(__MUCSE_DOWN, &mucse->state))
+ return;
+
+ if (!rnpgbe_process_link_event(mucse, &link, &speed, &duplex))
+ return;
+
+ if (link)
+ rnpgbe_link_is_up(mucse, speed, duplex);
+ else
+ rnpgbe_link_is_down(mucse);
+}
+
+/**
+ * rnpgbe_service_task - Manages and runs subtasks
+ * @work: pointer to work_struct containing our data
+ **/
+void rnpgbe_service_task(struct work_struct *work)
+{
+ struct mucse *mucse = container_of(work, struct mucse, serv_task.work);
+
+ if (test_bit(__MUCSE_DOWN, &mucse->state))
+ return;
+
+ rnpgbe_process_link_subtask(mucse);
+
+ if (!test_bit(__MUCSE_DOWN, &mucse->state))
+ queue_delayed_work(system_percpu_wq, &mucse->serv_task,
+ msecs_to_jiffies(500));
+}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
index 234b3bff94c4..27c0acebb58f 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.h
@@ -68,7 +68,7 @@ int rnpgbe_request_irq(struct mucse *mucse);
void rnpgbe_free_irq(struct mucse *mucse);
void rnpgbe_irq_disable(struct mucse *mucse);
bool rnpgbe_down(struct mucse *mucse);
-void rnpgbe_up_complete(struct mucse *mucse);
+int rnpgbe_up_complete(struct mucse *mucse);
int rnpgbe_configure_tx(struct mucse *mucse);
int rnpgbe_configure_rx(struct mucse *mucse);
void rnpgbe_clean_all_tx_rings(struct mucse *mucse);
@@ -80,4 +80,5 @@ void rnpgbe_get_stats64(struct net_device *netdev,
struct rtnl_link_stats64 *stats);
int rnpgbe_setup_all_rx_resources(struct mucse *mucse);
void rnpgbe_free_all_rx_resources(struct mucse *mucse);
+void rnpgbe_service_task(struct work_struct *work);
#endif
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
index 817081e24bb5..46f636102824 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
@@ -68,6 +68,7 @@ static int rnpgbe_open(struct net_device *netdev)
if (test_bit(__MUCSE_AXI_FAULT, &mucse->state))
return -EIO;
+ netif_carrier_off(netdev);
err = rnpgbe_request_irq(mucse);
if (err)
return err;
@@ -87,9 +88,16 @@ static int rnpgbe_open(struct net_device *netdev)
err = rnpgbe_configure(mucse);
if (err)
goto err_free_rx;
- rnpgbe_up_complete(mucse);
+ err = rnpgbe_up_complete(mucse);
+ if (err)
+ goto err_down;
return 0;
+err_down:
+ rnpgbe_down(mucse);
+ rnpgbe_free_all_rx_resources(mucse);
+ rnpgbe_free_all_tx_resources(mucse);
+ goto err_free_irqs;
err_free_rx:
rnpgbe_free_all_rx_resources(mucse);
err_free_tx:
@@ -252,6 +260,10 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
goto err_powerdown;
}
+ INIT_DELAYED_WORK(&mucse->serv_task, rnpgbe_service_task);
+ spin_lock_init(&mucse->link_lock);
+ atomic_set(&mucse->link_pending, 0);
+
err = rnpgbe_init_interrupt_scheme(mucse);
if (err) {
dev_err(&pdev->dev, "init interrupt failed %d\n", err);
@@ -271,6 +283,7 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
netdev->hw_features |= NETIF_F_HIGHDMA;
}
+ netif_carrier_off(netdev);
err = register_netdev(netdev);
if (err)
goto err_remove_mbx;
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
index 2310f01df1f5..185465d4e0dc 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
@@ -252,6 +252,26 @@ int mucse_poll_and_read_mbx(struct mucse_hw *hw, __le32 *msg, u16 size)
return mucse_read_mbx_pf(hw, msg, size);
}
+/**
+ * mucse_check_and_read_mbx - check if there is notification and receive message
+ * @hw: pointer to the HW structure
+ * @msg: the message buffer
+ * @size: length of buffer
+ *
+ * Return: 0 if it successfully received a message notification and
+ * copied it into the receive buffer, negative errno on failure
+ **/
+int mucse_check_and_read_mbx(struct mucse_hw *hw, __le32 *msg, u16 size)
+{
+ int err;
+
+ err = mucse_check_for_msg_pf(hw);
+ if (err)
+ return err;
+
+ return mucse_read_mbx_pf(hw, msg, size);
+}
+
/**
* mucse_mbx_get_fwack - Read fw ack from reg
* @mbx: pointer to the MBX structure
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
index 75b88b18b04d..3af008a67fb1 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
@@ -18,4 +18,5 @@ int mucse_write_and_wait_ack_mbx(struct mucse_hw *hw,
const __le32 *msg, u16 size);
void mucse_init_mbx_params_pf(struct mucse_hw *hw);
int mucse_poll_and_read_mbx(struct mucse_hw *hw, __le32 *msg, u16 size);
+int mucse_check_and_read_mbx(struct mucse_hw *hw, __le32 *msg, u16 size);
#endif /* _RNPGBE_MBX_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
index e28f3193aa85..01bab789473c 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
@@ -3,6 +3,7 @@
#include <linux/if_ether.h>
#include <linux/bitfield.h>
+#include <linux/pci.h>
#include "rnpgbe.h"
#include "rnpgbe_mbx.h"
@@ -199,12 +200,233 @@ int mucse_mbx_get_macaddr(struct mucse_hw *hw, int pfvfnum,
}
/**
- * mucse_fw_irq_handler - Deferred firmware mailbox notification hook
+ * mucse_mbx_phyup - Request that firmware bring the PHY up or down
* @hw: pointer to the HW structure
+ * @is_phyup: true for up, false for down
*
- * No asynchronous firmware events are enabled until link status support is
- * added.
+ * mucse_mbx_phyup echo fw to change phy status
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_phyup(struct mucse_hw *hw, bool is_phyup)
+{
+ union mbx_fw_cmd_req_u req = {
+ .r = {
+ .datalen = cpu_to_le16(sizeof(req.r.phy_status) +
+ MUCSE_MBX_REQ_HDR_LEN),
+ .opcode = cpu_to_le16(SET_PHY_UP),
+ .phy_status = {
+ .port_mask = cpu_to_le32(BIT(hw->port)),
+ .status = cpu_to_le32(is_phyup ? 1 : 0),
+ },
+ },
+ };
+ int len, err;
+
+ len = le16_to_cpu(req.r.datalen);
+ mutex_lock(&hw->mbx.lock);
+ err = mucse_write_and_wait_ack_mbx(hw, req.dwords, len);
+ mutex_unlock(&hw->mbx.lock);
+
+ return err;
+}
+
+/**
+ * mucse_mbx_link_report - Configure firmware link-change event reporting
+ * @hw: pointer to the HW structure
+ * @is_report: true for report, false for no
+ *
+ * mucse_mbx_link_report echo fw to change event report state
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_link_report(struct mucse_hw *hw, bool is_report)
+{
+ union mbx_fw_cmd_req_u req = {
+ .r = {
+ .datalen = cpu_to_le16(sizeof(req.r.report_status) +
+ MUCSE_MBX_REQ_HDR_LEN),
+ .opcode = cpu_to_le16(LINK_REPORT_EN),
+ .report_status = {
+ .port_mask = cpu_to_le16(BIT(hw->port)),
+ .status = cpu_to_le16(is_report ? 1 : 0),
+ },
+ },
+ };
+ int len, err;
+
+ len = le16_to_cpu(req.r.datalen);
+ mutex_lock(&hw->mbx.lock);
+ err = mucse_write_and_wait_ack_mbx(hw, req.dwords, len);
+ mutex_unlock(&hw->mbx.lock);
+
+ return err;
+}
+
+static bool mucse_link_speed_valid(const struct mbx_fw_cmd_req *req)
+{
+ switch (le16_to_cpu(req->link_stat.st.speed)) {
+ case 10:
+ case 100:
+ case 1000:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool mucse_link_is_up(const struct mucse_hw *hw,
+ const struct mbx_fw_cmd_req *req)
+{
+ return le16_to_cpu(req->link_stat.port_status) & BIT(hw->port);
+}
+
+/**
+ * mucse_update_link_status_reg - update driver speed inf to reg
+ * @hw: pointer to the HW structure
+ * @req: pointer to req data
+ *
+ * Update the driver's link-state snapshot exported to firmware. Firmware
+ * sends a new event when this snapshot differs from the hardware state.
+ * The default snapshot clears the driver-reported fields;
+ * a valid event then repopulates them, including the LLDP status in bit 6.
+ *
+ **/
+static void mucse_update_link_status_reg(struct mucse_hw *hw,
+ struct mbx_fw_cmd_req *req)
+{
+ u16 status = le16_to_cpu(req->link_stat.st.status);
+ u16 speed = le16_to_cpu(req->link_stat.st.speed);
+ u32 value;
+
+ value = M_DEFAULT_ST;
+
+ if (mucse_link_is_up(hw, req)) {
+ value |= BIT(0);
+ switch (speed) {
+ case 10:
+ value |= (mucse_speed_10 << 8);
+ break;
+ case 100:
+ value |= (mucse_speed_100 << 8);
+ break;
+ case 1000:
+ value |= (mucse_speed_1000 << 8);
+ break;
+ default:
+ break;
+ }
+
+ value |= FIELD_PREP(BIT(4),
+ !!(req->link_stat.st.flags & DUPLEX_BIT));
+ value |= FIELD_PREP(GENMASK_U32(25, 24),
+ status & GENMASK(1, 0));
+ } else {
+ value &= ~BIT(0);
+ }
+
+ if (status & ST_STATUS_LLDP_STATUS_MASK)
+ value |= BIT(6);
+ else
+ value &= ~BIT(6);
+
+ mucse_hw_wr32(hw, RNPGBE_LINK_ST, value);
+}
+
+/**
+ * mucse_mbx_fw_req_handler - Handle fw req
+ * @hw: pointer to the HW structure
+ * @req: pointer to req data
+ *
+ * mucse_mbx_fw_req_handler handler fw req, such as a link event req.
+ **/
+static void mucse_mbx_fw_req_handler(struct mucse_hw *hw,
+ struct mbx_fw_cmd_req *req)
+{
+ struct mucse *mucse = container_of(hw, struct mucse, hw);
+ u32 magic = le32_to_cpu(req->link_stat.port_magic);
+ unsigned long flags;
+
+ if (le16_to_cpu(req->opcode) == LINK_CHANGE_EVT) {
+ spin_lock_irqsave(&mucse->link_lock, flags);
+ if (magic != ST_VALID_MAGIC) {
+ /* Do not change the cached state for an invalid event.
+ * Use an invalid speed encoding to make firmware report
+ * again.
+ */
+ mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_INVALID_ST);
+ spin_unlock_irqrestore(&mucse->link_lock, flags);
+ return;
+ }
+
+ if (mucse_link_is_up(hw, req) &&
+ !mucse_link_speed_valid(req)) {
+ /* Report link down so the snapshot differs from actual
+ * link-up state and firmware sends a new link event.
+ */
+ mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_DEFAULT_ST);
+ spin_unlock_irqrestore(&mucse->link_lock, flags);
+ dev_warn_ratelimited(&hw->pdev->dev,
+ "unsupported link speed %u Mbps\n",
+ le16_to_cpu(req->link_stat.st.speed));
+ return;
+ }
+
+ if (test_bit(__MUCSE_DOWN, &mucse->state)) {
+ mucse_update_link_status_reg(hw, req);
+ spin_unlock_irqrestore(&mucse->link_lock, flags);
+ return;
+ }
+
+ if (mucse_link_is_up(hw, req))
+ WRITE_ONCE(hw->link, true);
+ else
+ WRITE_ONCE(hw->link, false);
+
+ WRITE_ONCE(hw->speed, le16_to_cpu(req->link_stat.st.speed));
+ WRITE_ONCE(hw->duplex, req->link_stat.st.flags & DUPLEX_BIT);
+ /* update regs to notify link info is received */
+ mucse_update_link_status_reg(hw, req);
+ atomic_set_release(&mucse->link_pending, 1);
+ /* Run link handling immediately; the service task also remains
+ * periodically scheduled for future maintenance work.
+ */
+ mod_delayed_work(system_percpu_wq, &mucse->serv_task, 0);
+ spin_unlock_irqrestore(&mucse->link_lock, flags);
+ }
+}
+
+/**
+ * mucse_fw_handle_event - Handle one pending firmware event
+ * @hw: pointer to the hardware structure
+ *
+ * Return: true if an event was handled, false otherwise
+ **/
+static bool mucse_fw_handle_event(struct mucse_hw *hw)
+{
+ union mbx_fw_cmd_req_u msg = {};
+ int err;
+
+ /* try to check and read fw req */
+ mutex_lock(&hw->mbx.lock);
+ err = mucse_check_and_read_mbx(hw, msg.dwords, sizeof(msg));
+ mutex_unlock(&hw->mbx.lock);
+ if (err)
+ return false;
+
+ mucse_mbx_fw_req_handler(hw, &msg.r);
+
+ return true;
+}
+
+/**
+ * mucse_fw_irq_handler - Handle one pending firmware mailbox event
+ * @hw: pointer to the HW structure
+ *
+ * Process at most one event per work-item invocation. The caller requeues
+ * mailbox work when a dedicated mailbox interrupt arrives during handling.
**/
void mucse_fw_irq_handler(struct mucse_hw *hw)
{
+ mucse_fw_handle_event(hw);
}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
index a6bf5de55aa3..7480732c102c 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
@@ -14,6 +14,9 @@ enum MUCSE_FW_CMD {
GET_HW_INFO = 0x0601,
GET_MAC_ADDRESS = 0x0602,
RESET_HW = 0x0603,
+ LINK_CHANGE_EVT = 0x0608,
+ LINK_REPORT_EN = 0x0613,
+ SET_PHY_UP = 0x0800,
POWER_UP = 0x0803,
};
@@ -36,6 +39,16 @@ struct mucse_hw_info {
__le32 ext_info;
} __packed;
+#define ST_STATUS_LLDP_STATUS_MASK BIT(12)
+
+#define DUPLEX_BIT BIT(0)
+struct st_status {
+ u8 phyid;
+ u8 flags;
+ __le16 speed;
+ __le16 status;
+} __packed;
+
struct mbx_fw_cmd_req {
__le16 flags;
__le16 opcode;
@@ -55,6 +68,24 @@ struct mbx_fw_cmd_req {
__le32 port_mask;
__le32 pfvf_num;
} get_mac_addr;
+ struct {
+ __le32 port_mask;
+ __le32 status;
+ } phy_status;
+ struct {
+ /* LINK_REPORT_EN uses its own 16-bit payload layout:
+ * status precedes port_mask.
+ */
+ __le16 status;
+ __le16 port_mask;
+ } report_status;
+ struct {
+ __le16 changed_lanes;
+ __le16 port_status;
+ __le32 port_magic;
+#define ST_VALID_MAGIC 0xa4a6a8a9
+ struct st_status st;
+ } link_stat;
};
} __packed;
@@ -94,10 +125,21 @@ union mbx_fw_cmd_reply_u {
__le32 dwords[sizeof(struct mbx_fw_cmd_reply) / sizeof(__le32)];
};
+/* Firmware encoding for RNPGBE_LINK_ST[11:8]. Valid link events report
+ * only these three speeds.
+ */
+enum mucse_speed {
+ mucse_speed_10 = 0,
+ mucse_speed_100 = 1,
+ mucse_speed_1000 = 2,
+};
+
int mucse_mbx_sync_fw(struct mucse_hw *hw);
int mucse_mbx_powerup(struct mucse_hw *hw, bool is_powerup);
int mucse_mbx_reset_hw(struct mucse_hw *hw);
int mucse_mbx_get_macaddr(struct mucse_hw *hw, int pfvfnum,
u8 *mac_addr, int port);
+int mucse_mbx_phyup(struct mucse_hw *hw, bool is_phyup);
+int mucse_mbx_link_report(struct mucse_hw *hw, bool is_report);
void mucse_fw_irq_handler(struct mucse_hw *hw);
#endif /* _RNPGBE_MBX_FW_H */
--
2.50.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH net-next v10 5/5] net: rnpgbe: Add link status handling support
2026-08-31 7:36 ` [PATCH net-next v10 5/5] net: rnpgbe: Add link status handling support Dong Yibo
@ 2026-09-04 21:57 ` Jakub Kicinski
2026-09-08 11:03 ` Yibo Dong
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2026-09-04 21:57 UTC (permalink / raw)
To: dong100
Cc: Jakub Kicinski, andrew+netdev, davem, edumazet, pabeni,
vadim.fedorenko, u.kleine-koenig, netdev, linux-kernel, yaojun
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: rnpgbe: Add link status handling support
This patch adds firmware-driven link status handling to rnpgbe: link
state is cached from mailbox LINK_CHANGE_EVT events, a delayed service
task flips the carrier, and the driver notifies firmware of port up/down
around ndo_open and ndo_stop. A few questions about the new event and
teardown paths below.
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> index feb84704d6027..5659e91f34972 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> @@ -21,6 +21,17 @@
> #define RNPGBE_DMA_TX_STATUS GENMASK_U32(23, 22)
> #define RNPGBE_DMA_RX_STATUS GENMASK_U32(21, 20)
> #define RX_AXI_RW_EN 0x03
> +/* RNPGBE_LINK_ST is a driver-owned link-state snapshot consumed by firmware.
> + * Set the driver-state marker; all other driver status fields start clear.
> + */
> +#define M_DEFAULT_ST 0xa0000000
> +/* Firmware never reports 0xf in the speed field. */
> +#define M_INVALID_ST (M_DEFAULT_ST | GENMASK_U32(11, 8))
> +/* Driver-reported fields: 25:24 pause, 11:8 speed, 6 LLDP, 4 duplex,
> + * and 0 link up/down. M_DEFAULT_ST resets these fields so firmware reports
> + * the current hardware state again.
> + */
> +#define RNPGBE_LINK_ST 0x000c
[ ... ]
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> index 346e9a1cca8a9..1c649ef7ee3ac 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> @@ -16,8 +16,23 @@
> static void rnpgbe_mbx_work(struct work_struct *work)
> {
> struct mucse *mucse = container_of(work, struct mucse, mbx_work);
> + int irq_seq;
>
> + irq_seq = atomic_read(&mucse->mbx_irq_seq);
> mucse_fw_irq_handler(&mucse->hw);
> +
> + /*
> + * Only multi-vector MSI-X has a dedicated mailbox interrupt, so its
> + * hard IRQ records a sequence number. With a shared MSI or MSI-X
> + * vector, all interrupts queue this work and it checks the mailbox
> + * request counter. Firmware posts one request at a time and waits for
> + * the PF ACK before posting the next one. A mailbox interrupt while
> + * this work runs queues a later invocation.
> + */
> + if ((mucse->flags & M_FLAG_MSIX_EN) &&
> + irq_seq != atomic_read(&mucse->mbx_irq_seq) &&
> + !test_bit(__MUCSE_DOWN, &mucse->state))
> + queue_work(system_percpu_wq, &mucse->mbx_work);
> }
[Severity: Medium]
The requeue here is conditioned on a new mailbox interrupt arriving during
this invocation, not on the read having failed. Can a pending link event be
dropped when mucse_check_and_read_mbx() fails?
mucse_fw_handle_event() returns false on failure and mucse_fw_irq_handler()
discards that result:
mutex_lock(&hw->mbx.lock);
err = mucse_check_and_read_mbx(hw, msg.dwords, sizeof(msg));
mutex_unlock(&hw->mbx.lock);
if (err)
return false;
mucse_check_and_read_mbx() -> mucse_read_mbx_pf() can fail early in
mucse_obtain_mbx_lock_pf() (read_poll_timeout with mbx->timeout_us of four
seconds), and in that case neither hw->mbx.fw_req nor the PF ACK is updated:
err = mucse_obtain_mbx_lock_pf(hw);
if (err)
return err;
...
hw->mbx.fw_req = mucse_mbx_get_fwreq(mbx);
mucse_mbx_inc_pf_ack(hw);
With the dedicated MSI-X mailbox vector, rnpgbe_msix_other() is the only
source that queues mbx_work, and irq_seq is unchanged for the interrupt that
scheduled this very invocation, so no requeue happens. Per the comment above
("Firmware posts one request at a time and waits for the PF ACK before posting
the next one"), firmware would then also post nothing new.
rnpgbe_service_task() only consumes already-published events and never polls
the mailbox, so is there any path that retries the read? Would the carrier
stay off while the link is actually up until an administrative ifdown/ifup?
[ ... ]
> @@ -1430,15 +1447,42 @@ static void rnpgbe_cancel_rx_retry_timers(struct mucse *mucse)
> bool rnpgbe_down(struct mucse *mucse)
> {
> struct net_device *netdev = mucse->netdev;
> + struct mucse_hw *hw = &mucse->hw;
> + unsigned long flags;
> + int err;
>
> if (test_and_set_bit(__MUCSE_DOWN, &mucse->state))
> return false;
>
> + spin_lock_irqsave(&mucse->link_lock, flags);
> + WRITE_ONCE(hw->link, false);
> + WRITE_ONCE(hw->speed, 0);
> + WRITE_ONCE(hw->duplex, 0);
> + atomic_set(&mucse->link_pending, 0);
> + spin_unlock_irqrestore(&mucse->link_lock, flags);
> + cancel_delayed_work_sync(&mucse->serv_task);
> + rnpgbe_set_link(hw, false);
> +
> + netif_carrier_off(netdev);
> netif_tx_disable(netdev);
> rnpgbe_napi_disable_all(mucse);
> rnpgbe_irq_disable(mucse);
> rnpgbe_stop_all_rx_rings(mucse);
> rnpgbe_cancel_rx_retry_timers(mucse);
> +
> + err = rnpgbe_send_notify(hw, false, mucse_fw_link_report_en);
> + if (err) {
> + dev_warn(&hw->pdev->dev, "Send link report to hw failed %d\n",
> + err);
> + dev_warn(&hw->pdev->dev, "Fw will still report link event\n");
> + }
> +
> + err = rnpgbe_send_notify(hw, false, mucse_fw_portup);
> + if (err) {
> + dev_warn(&hw->pdev->dev, "Send port down to hw failed %d\n",
> + err);
> + dev_warn(&hw->pdev->dev, "Port is not truly down\n");
> + }
> rnpgbe_clean_all_tx_rings(mucse);
> rnpgbe_clean_all_rx_rings(mucse);
[Severity: Medium]
The commit message says the port-down notification is sent "after the data
path is quiesced during teardown". Is the TX side actually quiesced at this
point?
RX is stopped before the notification (rnpgbe_stop_all_rx_rings()), but
netif_tx_disable() only stops new software submissions. The TX hardware is
shut down afterwards, in rnpgbe_clean_all_tx_rings() ->
rnpgbe_stop_all_tx_rings(), which clears each ring's TX_START, waits for
RNPGBE_DMA_TX_STATUS to go idle and only then clears TX_AXI_RW_EN:
for (int i = 0; i < mucse->num_tx_queues; i++)
rnpgbe_stop_tx_ring(mucse->tx_ring[i]);
if (mucse->num_tx_queues && !test_bit(__MUCSE_AXI_FAULT, &mucse->state))
err = rnpgbe_wait_tx_dma_idle(mucse);
... dma_axi_ctl &= ~TX_AXI_RW_EN;
So firmware can take the PHY/port down while descriptors submitted earlier are
still being fetched and transmitted. If the MAC then stops draining, the
100 ms poll in rnpgbe_wait_tx_dma_idle() times out and sets a bit that nothing
ever clears:
if (err) {
set_bit(__MUCSE_AXI_FAULT, &mucse->state);
dev_err(&mucse->pdev->dev, "TX DMA failed to quiesce, status %#x\n", dma_status);
}
__MUCSE_AXI_FAULT is only ever set and tested, never cleared, and
rnpgbe_open() returns -EIO while it is set, so the interface would stay
unusable until the driver is unbound. Would it be safer to issue the
port-down mailbox command after rnpgbe_clean_all_tx_rings(), matching the RX
ordering?
[ ... ]
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> index e28f3193aa854..01bab789473cf 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> @@ -199,12 +200,233 @@ int mucse_mbx_get_macaddr(struct mucse_hw *hw, int pfvfnum,
[ ... ]
> +static void mucse_mbx_fw_req_handler(struct mucse_hw *hw,
> + struct mbx_fw_cmd_req *req)
> +{
> + struct mucse *mucse = container_of(hw, struct mucse, hw);
> + u32 magic = le32_to_cpu(req->link_stat.port_magic);
> + unsigned long flags;
> +
> + if (le16_to_cpu(req->opcode) == LINK_CHANGE_EVT) {
> + spin_lock_irqsave(&mucse->link_lock, flags);
> + if (magic != ST_VALID_MAGIC) {
> + /* Do not change the cached state for an invalid event.
> + * Use an invalid speed encoding to make firmware report
> + * again.
> + */
> + mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_INVALID_ST);
> + spin_unlock_irqrestore(&mucse->link_lock, flags);
> + return;
> + }
> +
> + if (mucse_link_is_up(hw, req) &&
> + !mucse_link_speed_valid(req)) {
> + /* Report link down so the snapshot differs from actual
> + * link-up state and firmware sends a new link event.
> + */
> + mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_DEFAULT_ST);
> + spin_unlock_irqrestore(&mucse->link_lock, flags);
> + dev_warn_ratelimited(&hw->pdev->dev,
> + "unsupported link speed %u Mbps\n",
> + le16_to_cpu(req->link_stat.st.speed));
> + return;
> + }
[Severity: Medium]
Both rejection paths write a snapshot that is deliberately guaranteed not to
match the real link state. Can this turn a rejected event into a
self-sustaining event source?
The documented contract in rnpgbe_hw.h and repeated in rnpgbe_up_complete()
is that firmware "only asserts LINK_CHANGE_EVT when it differs from the actual
link state AND link_report_en is true":
M_INVALID_ST is M_DEFAULT_ST plus a speed field of 0xf, described as
a value firmware never reports.
M_DEFAULT_ST is a link-down snapshot, written here while the link is
reported up.
Neither branch changes any cached driver state, keeps a retry count, applies a
backoff, or turns link_report_en off, so the same event looks like it can be
re-posted and re-rejected indefinitely. dev_warn_ratelimited() throttles the
log line but not the event churn.
Each iteration also takes hw->mbx.lock in mucse_fw_handle_event(), which is
the same mutex used by the port up/down and link-report commands issued from
rnpgbe_up_complete() and rnpgbe_down(), and those helpers poll with
mbx->timeout_us of four seconds. Would an older firmware revision reporting a
different port_magic, or a link up with a speed outside {10, 100, 1000}, keep
this loop running for as long as the link stays in that state?
[ ... ]
> +static bool mucse_fw_handle_event(struct mucse_hw *hw)
> +{
> + union mbx_fw_cmd_req_u msg = {};
> + int err;
> +
> + /* try to check and read fw req */
> + mutex_lock(&hw->mbx.lock);
> + err = mucse_check_and_read_mbx(hw, msg.dwords, sizeof(msg));
> + mutex_unlock(&hw->mbx.lock);
> + if (err)
> + return false;
> +
> + mucse_mbx_fw_req_handler(hw, &msg.r);
> +
> + return true;
> +}
> +
> +/**
> + * mucse_fw_irq_handler - Handle one pending firmware mailbox event
> + * @hw: pointer to the HW structure
> + *
> + * Process at most one event per work-item invocation. The caller requeues
> + * mailbox work when a dedicated mailbox interrupt arrives during handling.
> **/
> void mucse_fw_irq_handler(struct mucse_hw *hw)
> {
> + mucse_fw_handle_event(hw);
> }
The discarded return value here is the same concern raised on
rnpgbe_mbx_work() above: a failed mailbox read is indistinguishable from "no
event pending".
Thanks for considering these.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH net-next v10 5/5] net: rnpgbe: Add link status handling support
2026-09-04 21:57 ` Jakub Kicinski
@ 2026-09-08 11:03 ` Yibo Dong
0 siblings, 0 replies; 15+ messages in thread
From: Yibo Dong @ 2026-09-08 11:03 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, davem, edumazet, pabeni, vadim.fedorenko,
u.kleine-koenig, netdev, linux-kernel, yaojun
On Fri, Sep 04, 2026 at 02:57:19PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: rnpgbe: Add link status handling support
>
> This patch adds firmware-driven link status handling to rnpgbe: link
> state is cached from mailbox LINK_CHANGE_EVT events, a delayed service
> task flips the carrier, and the driver notifies firmware of port up/down
> around ndo_open and ndo_stop. A few questions about the new event and
> teardown paths below.
>
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> > index feb84704d6027..5659e91f34972 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> > @@ -21,6 +21,17 @@
> > #define RNPGBE_DMA_TX_STATUS GENMASK_U32(23, 22)
> > #define RNPGBE_DMA_RX_STATUS GENMASK_U32(21, 20)
> > #define RX_AXI_RW_EN 0x03
> > +/* RNPGBE_LINK_ST is a driver-owned link-state snapshot consumed by firmware.
> > + * Set the driver-state marker; all other driver status fields start clear.
> > + */
> > +#define M_DEFAULT_ST 0xa0000000
> > +/* Firmware never reports 0xf in the speed field. */
> > +#define M_INVALID_ST (M_DEFAULT_ST | GENMASK_U32(11, 8))
> > +/* Driver-reported fields: 25:24 pause, 11:8 speed, 6 LLDP, 4 duplex,
> > + * and 0 link up/down. M_DEFAULT_ST resets these fields so firmware reports
> > + * the current hardware state again.
> > + */
> > +#define RNPGBE_LINK_ST 0x000c
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> > index 346e9a1cca8a9..1c649ef7ee3ac 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> > @@ -16,8 +16,23 @@
> > static void rnpgbe_mbx_work(struct work_struct *work)
> > {
> > struct mucse *mucse = container_of(work, struct mucse, mbx_work);
> > + int irq_seq;
> >
> > + irq_seq = atomic_read(&mucse->mbx_irq_seq);
> > mucse_fw_irq_handler(&mucse->hw);
> > +
> > + /*
> > + * Only multi-vector MSI-X has a dedicated mailbox interrupt, so its
> > + * hard IRQ records a sequence number. With a shared MSI or MSI-X
> > + * vector, all interrupts queue this work and it checks the mailbox
> > + * request counter. Firmware posts one request at a time and waits for
> > + * the PF ACK before posting the next one. A mailbox interrupt while
> > + * this work runs queues a later invocation.
> > + */
> > + if ((mucse->flags & M_FLAG_MSIX_EN) &&
> > + irq_seq != atomic_read(&mucse->mbx_irq_seq) &&
> > + !test_bit(__MUCSE_DOWN, &mucse->state))
> > + queue_work(system_percpu_wq, &mucse->mbx_work);
> > }
>
> [Severity: Medium]
> The requeue here is conditioned on a new mailbox interrupt arriving during
> this invocation, not on the read having failed. Can a pending link event be
> dropped when mucse_check_and_read_mbx() fails?
>
> mucse_fw_handle_event() returns false on failure and mucse_fw_irq_handler()
> discards that result:
>
> mutex_lock(&hw->mbx.lock);
> err = mucse_check_and_read_mbx(hw, msg.dwords, sizeof(msg));
> mutex_unlock(&hw->mbx.lock);
> if (err)
> return false;
>
> mucse_check_and_read_mbx() -> mucse_read_mbx_pf() can fail early in
> mucse_obtain_mbx_lock_pf() (read_poll_timeout with mbx->timeout_us of four
> seconds), and in that case neither hw->mbx.fw_req nor the PF ACK is updated:
Not valid. The mailbox protocol does not leave the sender holding the hardware
lock after raising the interrupt. The sender releases the lock and triggers the
interrupt atomically, then waits for the peer ACK without owning the shared
mailbox. Both the driver and firmware follow this protocol.
Therefore the worker runs only after the firmware has released the lock.
>
> err = mucse_obtain_mbx_lock_pf(hw);
> if (err)
> return err;
> ...
> hw->mbx.fw_req = mucse_mbx_get_fwreq(mbx);
> mucse_mbx_inc_pf_ack(hw);
>
> With the dedicated MSI-X mailbox vector, rnpgbe_msix_other() is the only
> source that queues mbx_work, and irq_seq is unchanged for the interrupt that
> scheduled this very invocation, so no requeue happens. Per the comment above
> ("Firmware posts one request at a time and waits for the PF ACK before posting
> the next one"), firmware would then also post nothing new.
>
> rnpgbe_service_task() only consumes already-published events and never polls
> the mailbox, so is there any path that retries the read? Would the carrier
> stay off while the link is actually up until an administrative ifdown/ifup?
>
> [ ... ]
>
> > @@ -1430,15 +1447,42 @@ static void rnpgbe_cancel_rx_retry_timers(struct mucse *mucse)
> > bool rnpgbe_down(struct mucse *mucse)
> > {
> > struct net_device *netdev = mucse->netdev;
> > + struct mucse_hw *hw = &mucse->hw;
> > + unsigned long flags;
> > + int err;
> >
> > if (test_and_set_bit(__MUCSE_DOWN, &mucse->state))
> > return false;
> >
> > + spin_lock_irqsave(&mucse->link_lock, flags);
> > + WRITE_ONCE(hw->link, false);
> > + WRITE_ONCE(hw->speed, 0);
> > + WRITE_ONCE(hw->duplex, 0);
> > + atomic_set(&mucse->link_pending, 0);
> > + spin_unlock_irqrestore(&mucse->link_lock, flags);
> > + cancel_delayed_work_sync(&mucse->serv_task);
> > + rnpgbe_set_link(hw, false);
> > +
> > + netif_carrier_off(netdev);
> > netif_tx_disable(netdev);
> > rnpgbe_napi_disable_all(mucse);
> > rnpgbe_irq_disable(mucse);
> > rnpgbe_stop_all_rx_rings(mucse);
> > rnpgbe_cancel_rx_retry_timers(mucse);
> > +
> > + err = rnpgbe_send_notify(hw, false, mucse_fw_link_report_en);
> > + if (err) {
> > + dev_warn(&hw->pdev->dev, "Send link report to hw failed %d\n",
> > + err);
> > + dev_warn(&hw->pdev->dev, "Fw will still report link event\n");
> > + }
> > +
> > + err = rnpgbe_send_notify(hw, false, mucse_fw_portup);
> > + if (err) {
> > + dev_warn(&hw->pdev->dev, "Send port down to hw failed %d\n",
> > + err);
> > + dev_warn(&hw->pdev->dev, "Port is not truly down\n");
> > + }
> > rnpgbe_clean_all_tx_rings(mucse);
> > rnpgbe_clean_all_rx_rings(mucse);
>
> [Severity: Medium]
> The commit message says the port-down notification is sent "after the data
> path is quiesced during teardown". Is the TX side actually quiesced at this
> point?
>
> RX is stopped before the notification (rnpgbe_stop_all_rx_rings()), but
> netif_tx_disable() only stops new software submissions. The TX hardware is
> shut down afterwards, in rnpgbe_clean_all_tx_rings() ->
> rnpgbe_stop_all_tx_rings(), which clears each ring's TX_START, waits for
> RNPGBE_DMA_TX_STATUS to go idle and only then clears TX_AXI_RW_EN:
>
> for (int i = 0; i < mucse->num_tx_queues; i++)
> rnpgbe_stop_tx_ring(mucse->tx_ring[i]);
> if (mucse->num_tx_queues && !test_bit(__MUCSE_AXI_FAULT, &mucse->state))
> err = rnpgbe_wait_tx_dma_idle(mucse);
> ... dma_axi_ctl &= ~TX_AXI_RW_EN;
>
> So firmware can take the PHY/port down while descriptors submitted earlier are
> still being fetched and transmitted. If the MAC then stops draining, the
> 100 ms poll in rnpgbe_wait_tx_dma_idle() times out and sets a bit that nothing
> ever clears:
>
> if (err) {
> set_bit(__MUCSE_AXI_FAULT, &mucse->state);
> dev_err(&mucse->pdev->dev, "TX DMA failed to quiesce, status %#x\n", dma_status);
> }
>
> __MUCSE_AXI_FAULT is only ever set and tested, never cleared, and
> rnpgbe_open() returns -EIO while it is set, so the interface would stay
> unusable until the driver is unbound. Would it be safer to issue the
> port-down mailbox command after rnpgbe_clean_all_tx_rings(), matching the RX
> ordering?
>
Got it. I will move rnpgbe_clean_all_tx_rings() before port-down mailbox
command.
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> > index e28f3193aa854..01bab789473cf 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> > @@ -199,12 +200,233 @@ int mucse_mbx_get_macaddr(struct mucse_hw *hw, int pfvfnum,
>
> [ ... ]
>
> > +static void mucse_mbx_fw_req_handler(struct mucse_hw *hw,
> > + struct mbx_fw_cmd_req *req)
> > +{
> > + struct mucse *mucse = container_of(hw, struct mucse, hw);
> > + u32 magic = le32_to_cpu(req->link_stat.port_magic);
> > + unsigned long flags;
> > +
> > + if (le16_to_cpu(req->opcode) == LINK_CHANGE_EVT) {
> > + spin_lock_irqsave(&mucse->link_lock, flags);
> > + if (magic != ST_VALID_MAGIC) {
> > + /* Do not change the cached state for an invalid event.
> > + * Use an invalid speed encoding to make firmware report
> > + * again.
> > + */
> > + mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_INVALID_ST);
> > + spin_unlock_irqrestore(&mucse->link_lock, flags);
> > + return;
> > + }
> > +
> > + if (mucse_link_is_up(hw, req) &&
> > + !mucse_link_speed_valid(req)) {
> > + /* Report link down so the snapshot differs from actual
> > + * link-up state and firmware sends a new link event.
> > + */
> > + mucse_hw_wr32(hw, RNPGBE_LINK_ST, M_DEFAULT_ST);
> > + spin_unlock_irqrestore(&mucse->link_lock, flags);
> > + dev_warn_ratelimited(&hw->pdev->dev,
> > + "unsupported link speed %u Mbps\n",
> > + le16_to_cpu(req->link_stat.st.speed));
> > + return;
> > + }
>
> [Severity: Medium]
> Both rejection paths write a snapshot that is deliberately guaranteed not to
> match the real link state. Can this turn a rejected event into a
> self-sustaining event source?
>
> The documented contract in rnpgbe_hw.h and repeated in rnpgbe_up_complete()
> is that firmware "only asserts LINK_CHANGE_EVT when it differs from the actual
> link state AND link_report_en is true":
>
> M_INVALID_ST is M_DEFAULT_ST plus a speed field of 0xf, described as
> a value firmware never reports.
>
> M_DEFAULT_ST is a link-down snapshot, written here while the link is
> reported up.
>
> Neither branch changes any cached driver state, keeps a retry count, applies a
> backoff, or turns link_report_en off, so the same event looks like it can be
> re-posted and re-rejected indefinitely. dev_warn_ratelimited() throttles the
> log line but not the event churn.
>
> Each iteration also takes hw->mbx.lock in mucse_fw_handle_event(), which is
> the same mutex used by the port up/down and link-report commands issued from
> rnpgbe_up_complete() and rnpgbe_down(), and those helpers poll with
> mbx->timeout_us of four seconds. Would an older firmware revision reporting a
> different port_magic, or a link up with a speed outside {10, 100, 1000}, keep
> this loop running for as long as the link stays in that state?
>
Firmware normally reports only valid link events, will not continuously reports
invalid link events. These checks are defensive: they prevent a malformed event
from corrupting the driver's cached link state, for example by publishing an
unsupported speed. If such an event is rejected, the driver deliberately
invalidates its snapshot so that firmware can resynchronize it with the
current link state.
> [ ... ]
>
> > +static bool mucse_fw_handle_event(struct mucse_hw *hw)
> > +{
> > + union mbx_fw_cmd_req_u msg = {};
> > + int err;
> > +
> > + /* try to check and read fw req */
> > + mutex_lock(&hw->mbx.lock);
> > + err = mucse_check_and_read_mbx(hw, msg.dwords, sizeof(msg));
> > + mutex_unlock(&hw->mbx.lock);
> > + if (err)
> > + return false;
> > +
> > + mucse_mbx_fw_req_handler(hw, &msg.r);
> > +
> > + return true;
> > +}
> > +
> > +/**
> > + * mucse_fw_irq_handler - Handle one pending firmware mailbox event
> > + * @hw: pointer to the HW structure
> > + *
> > + * Process at most one event per work-item invocation. The caller requeues
> > + * mailbox work when a dedicated mailbox interrupt arrives during handling.
> > **/
> > void mucse_fw_irq_handler(struct mucse_hw *hw)
> > {
> > + mucse_fw_handle_event(hw);
> > }
>
> The discarded return value here is the same concern raised on
> rnpgbe_mbx_work() above: a failed mailbox read is indistinguishable from "no
> event pending".
>
Firmware writes the request, releases the hardware lock while raising the
mailbox interrupt, and then waits for the PF ACK without holding the
mailbox lock (same above). The firmware ACK wait is bounded. Thus this is not
an expected transient contention case where requeuing the same work would help.
If firmware sends another request after its ACK-timeout handling, it raises a
new mailbox interrupt and queues new work.
> Thanks for considering these.
>
Thanks for your feedback.
^ permalink raw reply [flat|nested] 15+ messages in thread