[PATCH 0/6]

public inbox for dev@dpdk.org
 help / color / mirror / Atom feed

* [PATCH 0/6]
@ 2026-01-15 14:01 spinler
  2026-01-15 14:01 ` [PATCH 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
                   ` (10 more replies)
  0 siblings, 11 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:01 UTC (permalink / raw)
  To: spinler; +Cc: dev

From: Martin Spinler <spinler@cesnet.cz>

This patchset mainly cleans up the code and prepare it for another
quite large rework. Also it resolves some unpleasant behavior.

---

Martin Spinler (6):
  net/nfb: use constant values for max Rx/Tx queues count
  net/nfb: fix bad pointer access in queue stats
  net/nfb: update timestamp calculation to meaningful value
  net/nfb: use process private variable for internal data
  net/nfb: release allocated resources correctly
  net/nfb: stop only started queues in fail path

 doc/guides/nics/nfb.rst      |   6 +-
 drivers/net/nfb/nfb.h        |   5 ++
 drivers/net/nfb/nfb_ethdev.c | 163 +++++++++++++++++++++--------------
 drivers/net/nfb/nfb_rx.c     |   2 +-
 drivers/net/nfb/nfb_rx.h     |  13 +--
 drivers/net/nfb/nfb_rxmode.c |  12 +--
 drivers/net/nfb/nfb_stats.c  |  46 +++++-----
 drivers/net/nfb/nfb_tx.c     |   2 +-
 8 files changed, 141 insertions(+), 108 deletions(-)

--
2.52.0


^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH 1/6] net/nfb: use constant values for max Rx/Tx queues count
  2026-01-15 14:01 [PATCH 0/6] spinler
@ 2026-01-15 14:01 ` spinler
  2026-01-15 14:01 ` [PATCH 2/6] net/nfb: fix bad pointer access in queue stats spinler
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:01 UTC (permalink / raw)
  To: spinler; +Cc: dev, stable

From: Martin Spinler <spinler@cesnet.cz>

The nb_xx_queues values in the dev->data structure can be modified
dynamicaly by some apps. Use runtime-constant values from hardware
directly for max_rx_queues/max_tx_queues.

Fixes: 6435f9a0ac ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  3 +++
 drivers/net/nfb/nfb_ethdev.c | 10 ++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index a8b24ff822..8f8577860a 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -48,6 +48,9 @@ struct pmd_internals {
 	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
 	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
 	struct nfb_device *nfb;
+
+	int max_rx_queues;
+	int max_tx_queues;
 };
 
 #endif /* _NFB_H_ */
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 98119d70fd..7a472a5f1e 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -237,11 +237,13 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
+	struct pmd_internals *internals = dev->data->dev_private;
+
 	dev_info->max_mac_addrs = nfb_eth_get_max_mac_address_count(dev);
 
 	dev_info->max_rx_pktlen = (uint32_t)-1;
-	dev_info->max_rx_queues = dev->data->nb_rx_queues;
-	dev_info->max_tx_queues = dev->data->nb_tx_queues;
+	dev_info->max_rx_queues = internals->max_rx_queues;
+	dev_info->max_tx_queues = internals->max_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
@@ -538,8 +540,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
 		return -EINVAL;
 	}
-	data->nb_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
-	data->nb_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
+	internals->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
+	internals->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
 
 	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
 		data->nb_rx_queues, data->nb_tx_queues);
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH 2/6] net/nfb: fix bad pointer access in queue stats
  2026-01-15 14:01 [PATCH 0/6] spinler
  2026-01-15 14:01 ` [PATCH 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
@ 2026-01-15 14:01 ` spinler
  2026-01-15 14:01 ` [PATCH 3/6] net/nfb: update timestamp calculation to meaningful value spinler
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:01 UTC (permalink / raw)
  To: spinler; +Cc: dev, stable

From: Martin Spinler <spinler@cesnet.cz>

The driver code has dereferenced the dev->data->rx_queues pointer
without checking for its validity.
Pointer invalidation can occur when the eth_dev_rx_queue_config
is called with set to 0, for example.

Moreover, an array of pointers (to a structure) was used like array
of structures (which worked with early dereference just for one queue).

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_stats.c | 46 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/net/nfb/nfb_stats.c b/drivers/net/nfb/nfb_stats.c
index 4ea6b7be21..27a01c3160 100644
--- a/drivers/net/nfb/nfb_stats.c
+++ b/drivers/net/nfb/nfb_stats.c
@@ -20,28 +20,28 @@ nfb_eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
 	uint64_t rx_total_bytes = 0;
 	uint64_t tx_total_bytes = 0;
 
-	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
-		dev->data->rx_queues);
-	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
-		dev->data->tx_queues);
+	struct ndp_rx_queue *rx_queue;
+	struct ndp_tx_queue *tx_queue;
 
 	for (i = 0; i < nb_rx; i++) {
+		rx_queue = dev->data->rx_queues[i];
 		if (qstats && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			qstats->q_ipackets[i] = rx_queue[i].rx_pkts;
-			qstats->q_ibytes[i] = rx_queue[i].rx_bytes;
+			qstats->q_ipackets[i] = rx_queue->rx_pkts;
+			qstats->q_ibytes[i] = rx_queue->rx_bytes;
 		}
-		rx_total += rx_queue[i].rx_pkts;
-		rx_total_bytes += rx_queue[i].rx_bytes;
+		rx_total += rx_queue->rx_pkts;
+		rx_total_bytes += rx_queue->rx_bytes;
 	}
 
 	for (i = 0; i < nb_tx; i++) {
+		tx_queue = dev->data->tx_queues[i];
 		if (qstats && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			qstats->q_opackets[i] = tx_queue[i].tx_pkts;
-			qstats->q_obytes[i] = tx_queue[i].tx_bytes;
+			qstats->q_opackets[i] = tx_queue->tx_pkts;
+			qstats->q_obytes[i] = tx_queue->tx_bytes;
 		}
-		tx_total += tx_queue[i].tx_pkts;
-		tx_total_bytes += tx_queue[i].tx_bytes;
-		tx_err_total += tx_queue[i].err_pkts;
+		tx_total += tx_queue->tx_pkts;
+		tx_total_bytes += tx_queue->tx_bytes;
+		tx_err_total += tx_queue->err_pkts;
 	}
 
 	stats->ipackets = rx_total;
@@ -59,20 +59,20 @@ nfb_eth_stats_reset(struct rte_eth_dev *dev)
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 
-	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
-		dev->data->rx_queues);
-	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
-		dev->data->tx_queues);
+	struct ndp_rx_queue *rx_queue;
+	struct ndp_tx_queue *tx_queue;
 
 	for (i = 0; i < nb_rx; i++) {
-		rx_queue[i].rx_pkts = 0;
-		rx_queue[i].rx_bytes = 0;
-		rx_queue[i].err_pkts = 0;
+		rx_queue = dev->data->rx_queues[i];
+		rx_queue->rx_pkts = 0;
+		rx_queue->rx_bytes = 0;
+		rx_queue->err_pkts = 0;
 	}
 	for (i = 0; i < nb_tx; i++) {
-		tx_queue[i].tx_pkts = 0;
-		tx_queue[i].tx_bytes = 0;
-		tx_queue[i].err_pkts = 0;
+		tx_queue = dev->data->tx_queues[i];
+		tx_queue->tx_pkts = 0;
+		tx_queue->tx_bytes = 0;
+		tx_queue->err_pkts = 0;
 	}
 
 	return 0;
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH 3/6] net/nfb: update timestamp calculation to meaningful value
  2026-01-15 14:01 [PATCH 0/6] spinler
  2026-01-15 14:01 ` [PATCH 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
  2026-01-15 14:01 ` [PATCH 2/6] net/nfb: fix bad pointer access in queue stats spinler
@ 2026-01-15 14:01 ` spinler
  2026-01-15 14:01 ` [PATCH 4/6] net/nfb: use process private variable for internal data spinler
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:01 UTC (permalink / raw)
  To: spinler; +Cc: dev

From: Martin Spinler <spinler@cesnet.cz>

The resulting timestamp wasn't monotonically increasing value.

Now the timestamp is value in nanoseconds (Unix epoch).

Unfortunately there's currently no safe mechanism in nfb-framework
to get ticks from hardware to implement read_clock function.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 doc/guides/nics/nfb.rst  |  6 ++----
 drivers/net/nfb/nfb_rx.h | 13 +++++++------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/doc/guides/nics/nfb.rst b/doc/guides/nics/nfb.rst
index 96aaf64440..a9b4049654 100644
--- a/doc/guides/nics/nfb.rst
+++ b/doc/guides/nics/nfb.rst
@@ -64,10 +64,8 @@ products). The standard `RTE_ETH_RX_OFFLOAD_TIMESTAMP` flag can be used for this
 When the timestamps are enabled, a timestamp validity flag is set in the MBUFs
 containing received frames and timestamp is inserted into the `rte_mbuf` struct.
 
-The timestamp is an `uint64_t` field. Its lower 32 bits represent *seconds* portion of the timestamp
-(number of seconds elapsed since 1.1.1970 00:00:00 UTC) and its higher 32 bits represent
-*nanosecond* portion of the timestamp (number of nanoseconds elapsed since the beginning of the
-second in the *seconds* portion.
+The timestamp is an `uint64_t` field and holds the number of nanoseconds
+elapsed since 1.1.1970 00:00:00 UTC.
 
 
 Using the NFB PMD
diff --git a/drivers/net/nfb/nfb_rx.h b/drivers/net/nfb/nfb_rx.h
index 12769841ae..67b3b00e2a 100644
--- a/drivers/net/nfb/nfb_rx.h
+++ b/drivers/net/nfb/nfb_rx.h
@@ -13,6 +13,7 @@
 #include <rte_mbuf.h>
 #include <rte_mbuf_dyn.h>
 #include <rte_ethdev.h>
+#include <rte_time.h>
 
 #include "nfb.h"
 
@@ -202,15 +203,15 @@ nfb_eth_ndp_rx(void *queue,
 			if (nfb_timestamp_dynfield_offset >= 0) {
 				rte_mbuf_timestamp_t timestamp;
 
-				/* nanoseconds */
-				timestamp =
-					rte_le_to_cpu_32(*((uint32_t *)
-					(packets[i].header + 4)));
-				timestamp <<= 32;
 				/* seconds */
-				timestamp |=
+				timestamp =
 					rte_le_to_cpu_32(*((uint32_t *)
 					(packets[i].header + 8)));
+				timestamp *= NSEC_PER_SEC;
+				/* nanoseconds */
+				timestamp +=
+					rte_le_to_cpu_32(*((uint32_t *)
+					(packets[i].header + 4)));
 				*nfb_timestamp_dynfield(mbuf) = timestamp;
 				mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
 			}
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH 4/6] net/nfb: use process private variable for internal data
  2026-01-15 14:01 [PATCH 0/6] spinler
                   ` (2 preceding siblings ...)
  2026-01-15 14:01 ` [PATCH 3/6] net/nfb: update timestamp calculation to meaningful value spinler
@ 2026-01-15 14:01 ` spinler
  2026-01-15 14:01 ` [PATCH 5/6] net/nfb: release allocated resources correctly spinler
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:01 UTC (permalink / raw)
  To: spinler; +Cc: dev

From: Martin Spinler <spinler@cesnet.cz>

Internal structures of libnfb can't be shared between processes.
Move these structures from dev_private to process_private, which allows
secondary process to correctly initialize and uninitialize the eth_dev.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  2 +
 drivers/net/nfb/nfb_ethdev.c | 95 ++++++++++++++++++++----------------
 drivers/net/nfb/nfb_rx.c     |  2 +-
 drivers/net/nfb/nfb_rxmode.c | 12 ++---
 drivers/net/nfb/nfb_tx.c     |  2 +-
 5 files changed, 64 insertions(+), 49 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index 8f8577860a..d85c5fba62 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -48,7 +48,9 @@ struct pmd_internals {
 	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
 	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
 	struct nfb_device *nfb;
+};
 
+struct pmd_priv {
 	int max_rx_queues;
 	int max_tx_queues;
 };
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 7a472a5f1e..d73a5330ce 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -184,7 +184,7 @@ static int
 nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 {
 	int ret;
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
 
 	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
@@ -207,7 +207,7 @@ nfb_eth_get_max_mac_address_count(struct rte_eth_dev *dev)
 	uint16_t i;
 	uint32_t c;
 	uint32_t ret = (uint32_t)-1;
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	/*
 	 * Go through all RX MAC components in firmware and find
@@ -237,13 +237,13 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_priv *priv = dev->data->dev_private;
 
 	dev_info->max_mac_addrs = nfb_eth_get_max_mac_address_count(dev);
 
 	dev_info->max_rx_pktlen = (uint32_t)-1;
-	dev_info->max_rx_queues = internals->max_rx_queues;
-	dev_info->max_tx_queues = internals->max_tx_queues;
+	dev_info->max_rx_queues = priv->max_rx_queues;
+	dev_info->max_tx_queues = priv->max_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
@@ -262,20 +262,20 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 static int
 nfb_eth_dev_close(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	uint16_t i;
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 	int ret;
 
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
 	ret = nfb_eth_dev_stop(dev);
 
-	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
-	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
-
 	for (i = 0; i < nb_rx; i++) {
 		nfb_eth_rx_queue_release(dev, i);
 		dev->data->rx_queues[i] = NULL;
@@ -310,7 +310,7 @@ nfb_eth_link_update(struct rte_eth_dev *dev,
 	struct rte_eth_link link;
 	memset(&link, 0, sizeof(link));
 
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	status.speed = MAC_SPEED_UNKNOWN;
 
@@ -365,7 +365,7 @@ static int
 nfb_eth_dev_set_link_up(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -390,7 +390,7 @@ static int
 nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -431,9 +431,8 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
 {
 	unsigned int i;
 	uint64_t mac;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	mac = nfb_eth_mac_addr_conv(mac_addr);
 	/* Until no real multi-port support, configure all RX MACs the same */
@@ -449,9 +448,8 @@ nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
 {
 	unsigned int i;
 	uint64_t mac;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	mac = nfb_eth_mac_addr_conv(mac_addr);
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -464,9 +462,8 @@ static void
 nfb_eth_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
 {
 	unsigned int i;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	for (i = 0; i < internals->max_rxmac; ++i)
 		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
@@ -514,8 +511,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 {
 	uint32_t mac_count;
 	struct rte_eth_dev_data *data = dev->data;
-	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+	struct pmd_internals *internals;
+	struct pmd_priv *priv = data->dev_private;
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
 	struct rte_ether_addr eth_addr_init;
@@ -525,6 +522,15 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
 
+	internals = (struct pmd_internals *) rte_zmalloc_socket("nfb_internals",
+			sizeof(struct pmd_internals), RTE_CACHE_LINE_SIZE,
+			dev->device->numa_node);
+	if (internals == NULL) {
+		return -ENOMEM;
+	}
+
+	dev->process_private = internals;
+
 	snprintf(nfb_dev, sizeof(nfb_dev),
 		"/dev/nfb/by-pci-slot/" PCI_PRI_FMT,
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
@@ -538,10 +544,11 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	internals->nfb = nfb_open(nfb_dev);
 	if (internals->nfb == NULL) {
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
+		rte_free(internals);
 		return -EINVAL;
 	}
-	internals->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
-	internals->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
+	priv->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
+	priv->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
 
 	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
 		data->nb_rx_queues, data->nb_tx_queues);
@@ -563,28 +570,31 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	/* Get link state */
 	nfb_eth_link_update(dev, 0);
 
-	/* Allocate space for MAC addresses */
-	mac_count = nfb_eth_get_max_mac_address_count(dev);
-	data->mac_addrs = rte_zmalloc(data->name,
-		sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
-	if (data->mac_addrs == NULL) {
-		NFB_LOG(ERR, "Could not alloc space for MAC address");
-		nfb_close(internals->nfb);
-		return -EINVAL;
-	}
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		/* Allocate space for MAC addresses */
+		mac_count = nfb_eth_get_max_mac_address_count(dev);
+		data->mac_addrs = rte_zmalloc(data->name,
+			sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
+		if (data->mac_addrs == NULL) {
+			NFB_LOG(ERR, "Could not alloc space for MAC address");
+			nfb_close(internals->nfb);
+			rte_free(internals);
+			return -EINVAL;
+		}
 
-	rte_eth_random_addr(eth_addr_init.addr_bytes);
-	eth_addr_init.addr_bytes[0] = eth_addr.addr_bytes[0];
-	eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
-	eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
+		rte_eth_random_addr(eth_addr_init.addr_bytes);
+		eth_addr_init.addr_bytes[0] = eth_addr.addr_bytes[0];
+		eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
+		eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
 
-	nfb_eth_mac_addr_set(dev, &eth_addr_init);
-	rte_ether_addr_copy(&eth_addr_init, &dev->data->mac_addrs[0]);
+		nfb_eth_mac_addr_set(dev, &eth_addr_init);
+		rte_ether_addr_copy(&eth_addr_init, &data->mac_addrs[0]);
 
-	data->promiscuous = nfb_eth_promiscuous_get(dev);
-	data->all_multicast = nfb_eth_allmulticast_get(dev);
+		data->promiscuous = nfb_eth_promiscuous_get(dev);
+		data->all_multicast = nfb_eth_allmulticast_get(dev);
 
-	dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
+		data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
+	}
 
 	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully initialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
@@ -607,9 +617,12 @@ nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 {
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
+	struct pmd_internals *internals = dev->process_private;
 
 	nfb_eth_dev_close(dev);
 
+	rte_free(internals);
+
 	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully uninitialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
@@ -644,7 +657,7 @@ nfb_eth_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		struct rte_pci_device *pci_dev)
 {
 	return rte_eth_dev_pci_generic_probe(pci_dev,
-		sizeof(struct pmd_internals), nfb_eth_dev_init);
+		sizeof(struct pmd_priv), nfb_eth_dev_init);
 }
 
 /**
diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
index 462bc3b50d..413d275853 100644
--- a/drivers/net/nfb/nfb_rx.c
+++ b/drivers/net/nfb/nfb_rx.c
@@ -60,7 +60,7 @@ nfb_eth_rx_queue_setup(struct rte_eth_dev *dev,
 		const struct rte_eth_rxconf *rx_conf __rte_unused,
 		struct rte_mempool *mb_pool)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	struct ndp_rx_queue *rxq;
 	int ret;
diff --git a/drivers/net/nfb/nfb_rxmode.c b/drivers/net/nfb/nfb_rxmode.c
index ca6e4d5578..dc560d638b 100644
--- a/drivers/net/nfb/nfb_rxmode.c
+++ b/drivers/net/nfb/nfb_rxmode.c
@@ -11,7 +11,7 @@ int
 nfb_eth_promiscuous_enable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 	uint16_t i;
 
 	for (i = 0; i < internals->max_rxmac; ++i) {
@@ -26,7 +26,7 @@ int
 nfb_eth_promiscuous_disable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 	uint16_t i;
 	enum nc_rxmac_mac_filter filter = RXMAC_MAC_FILTER_TABLE_BCAST;
 
@@ -44,7 +44,7 @@ int
 nfb_eth_promiscuous_get(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	struct nc_rxmac_status status;
 	status.mac_filter = RXMAC_MAC_FILTER_PROMISCUOUS;
@@ -59,7 +59,7 @@ int
 nfb_eth_allmulticast_enable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	if (dev->data->promiscuous)
@@ -76,7 +76,7 @@ int
 nfb_eth_allmulticast_disable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 
@@ -95,7 +95,7 @@ int
 nfb_eth_allmulticast_get(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	struct nc_rxmac_status status;
 	status.mac_filter = RXMAC_MAC_FILTER_PROMISCUOUS;
diff --git a/drivers/net/nfb/nfb_tx.c b/drivers/net/nfb/nfb_tx.c
index cf99268c43..1f997ce22f 100644
--- a/drivers/net/nfb/nfb_tx.c
+++ b/drivers/net/nfb/nfb_tx.c
@@ -53,7 +53,7 @@ nfb_eth_tx_queue_setup(struct rte_eth_dev *dev,
 	unsigned int socket_id,
 	const struct rte_eth_txconf *tx_conf __rte_unused)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	int ret;
 	struct ndp_tx_queue *txq;
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH 5/6] net/nfb: release allocated resources correctly
  2026-01-15 14:01 [PATCH 0/6] spinler
                   ` (3 preceding siblings ...)
  2026-01-15 14:01 ` [PATCH 4/6] net/nfb: use process private variable for internal data spinler
@ 2026-01-15 14:01 ` spinler
  2026-01-15 14:01 ` [PATCH 6/6] net/nfb: stop only started queues in fail path spinler
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:01 UTC (permalink / raw)
  To: spinler; +Cc: dev

From: Martin Spinler <spinler@cesnet.cz>

Internal handles are initialized in eth_dev_init, so the cleanup
should be done in eth_dev_uninit.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 79 +++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 33 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index d73a5330ce..3bd9cab5fa 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -18,6 +18,9 @@
 #include "nfb_rxmode.h"
 #include "nfb.h"
 
+
+static int nfb_eth_dev_uninit(struct rte_eth_dev *dev);
+
 /**
  * Default MAC addr
  */
@@ -160,8 +163,6 @@ nfb_eth_dev_stop(struct rte_eth_dev *dev)
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 
-	dev->data->dev_started = 0;
-
 	for (i = 0; i < nb_tx; i++)
 		nfb_eth_tx_queue_stop(dev, i);
 
@@ -181,10 +182,9 @@ nfb_eth_dev_stop(struct rte_eth_dev *dev)
  *   0 on success, a negative errno value otherwise.
  */
 static int
-nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
+nfb_eth_dev_configure(struct rte_eth_dev *dev)
 {
 	int ret;
-	struct pmd_internals *internals = dev->process_private;
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
 
 	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
@@ -193,12 +193,16 @@ nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 				&nfb_timestamp_rx_dynflag);
 		if (ret != 0) {
 			NFB_LOG(ERR, "Cannot register Rx timestamp field/flag %d", ret);
-			nfb_close(internals->nfb);
-			return -rte_errno;
+			ret = -ENOMEM;
+			goto err_ts_register;
 		}
 	}
 
 	return 0;
+
+err_ts_register:
+	nfb_eth_dev_uninit(dev);
+	return ret;
 }
 
 static uint32_t
@@ -262,32 +266,28 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 static int
 nfb_eth_dev_close(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *internals = dev->process_private;
 	uint16_t i;
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
-	int ret;
 
-	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
-	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
-
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
-	ret = nfb_eth_dev_stop(dev);
-
-	for (i = 0; i < nb_rx; i++) {
-		nfb_eth_rx_queue_release(dev, i);
-		dev->data->rx_queues[i] = NULL;
-	}
-	dev->data->nb_rx_queues = 0;
-	for (i = 0; i < nb_tx; i++) {
-		nfb_eth_tx_queue_release(dev, i);
-		dev->data->tx_queues[i] = NULL;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		for (i = 0; i < nb_rx; i++) {
+			if (dev->data->rx_queues[i]) {
+				nfb_eth_rx_queue_release(dev, i);
+				dev->data->rx_queues[i] = NULL;
+			}
+		}
+		for (i = 0; i < nb_tx; i++) {
+			if (dev->data->tx_queues[i]) {
+				nfb_eth_tx_queue_release(dev, i);
+				dev->data->tx_queues[i] = NULL;
+			}
+		}
 	}
-	dev->data->nb_tx_queues = 0;
 
-	return ret;
+	nfb_eth_dev_uninit(dev);
+
+	return 0;
 }
 
 /**
@@ -509,6 +509,7 @@ static const struct eth_dev_ops ops = {
 static int
 nfb_eth_dev_init(struct rte_eth_dev *dev)
 {
+	int ret;
 	uint32_t mac_count;
 	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals;
@@ -526,7 +527,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 			sizeof(struct pmd_internals), RTE_CACHE_LINE_SIZE,
 			dev->device->numa_node);
 	if (internals == NULL) {
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_alloc_internals;
 	}
 
 	dev->process_private = internals;
@@ -544,8 +546,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	internals->nfb = nfb_open(nfb_dev);
 	if (internals->nfb == NULL) {
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
-		rte_free(internals);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_nfb_open;
 	}
 	priv->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
 	priv->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
@@ -577,9 +579,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 			sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
 		if (data->mac_addrs == NULL) {
 			NFB_LOG(ERR, "Could not alloc space for MAC address");
-			nfb_close(internals->nfb);
-			rte_free(internals);
-			return -EINVAL;
+			ret = -ENOMEM;
+			goto err_malloc_mac_addrs;
 		}
 
 		rte_eth_random_addr(eth_addr_init.addr_bytes);
@@ -601,6 +602,16 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		pci_addr->function);
 
 	return 0;
+
+err_malloc_mac_addrs:
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+	nfb_close(internals->nfb);
+
+err_nfb_open:
+	rte_free(internals);
+err_alloc_internals:
+	return ret;
 }
 
 /**
@@ -619,7 +630,9 @@ nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
 	struct pmd_internals *internals = dev->process_private;
 
-	nfb_eth_dev_close(dev);
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+	nfb_close(internals->nfb);
 
 	rte_free(internals);
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH 6/6] net/nfb: stop only started queues in fail path
  2026-01-15 14:01 [PATCH 0/6] spinler
                   ` (4 preceding siblings ...)
  2026-01-15 14:01 ` [PATCH 5/6] net/nfb: release allocated resources correctly spinler
@ 2026-01-15 14:01 ` spinler
  2026-01-15 14:40 ` [PATCH v2 0/6] net/nfb: code cleanup spinler
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:01 UTC (permalink / raw)
  To: spinler; +Cc: dev

From: Martin Spinler <spinler@cesnet.cz>

The driver should stop only running queues in case of error
inside eth_dev_start.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 3bd9cab5fa..2d36a0a7dd 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -140,11 +140,12 @@ nfb_eth_dev_start(struct rte_eth_dev *dev)
 	return 0;
 
 err_tx:
-	for (i = 0; i < nb_tx; i++)
-		nfb_eth_tx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_tx_queue_stop(dev, i-1);
+	i = nb_rx;
 err_rx:
-	for (i = 0; i < nb_rx; i++)
-		nfb_eth_rx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_rx_queue_stop(dev, i-1);
 	return ret;
 }
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v2 0/6] net/nfb: code cleanup
  2026-01-15 14:01 [PATCH 0/6] spinler
                   ` (5 preceding siblings ...)
  2026-01-15 14:01 ` [PATCH 6/6] net/nfb: stop only started queues in fail path spinler
@ 2026-01-15 14:40 ` spinler
  2026-01-15 14:40   ` [PATCH v2 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
                     ` (6 more replies)
  2026-01-16 15:20 ` spinler
                   ` (3 subsequent siblings)
  10 siblings, 7 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:40 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

This patchset mainly cleans up the code and prepare it for another
quite large rework. Also it resolves some unpleasant behavior.

---
v2:
* Fixed coding style issues

Martin Spinler (6):
  net/nfb: use constant values for max Rx/Tx queues count
  net/nfb: fix bad pointer access in queue stats
  net/nfb: update timestamp calculation to meaningful value
  net/nfb: use process private variable for internal data
  net/nfb: release allocated resources correctly
  net/nfb: stop only started queues in fail path

 doc/guides/nics/nfb.rst      |   6 +-
 drivers/net/nfb/nfb.h        |   5 ++
 drivers/net/nfb/nfb_ethdev.c | 165 ++++++++++++++++++++---------------
 drivers/net/nfb/nfb_rx.c     |   2 +-
 drivers/net/nfb/nfb_rx.h     |  13 +--
 drivers/net/nfb/nfb_rxmode.c |  12 +--
 drivers/net/nfb/nfb_stats.c  |  46 +++++-----
 drivers/net/nfb/nfb_tx.c     |   2 +-
 8 files changed, 142 insertions(+), 109 deletions(-)

--
2.52.0


^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v2 1/6] net/nfb: use constant values for max Rx/Tx queues count
  2026-01-15 14:40 ` [PATCH v2 0/6] net/nfb: code cleanup spinler
@ 2026-01-15 14:40   ` spinler
  2026-01-15 14:40   ` [PATCH v2 2/6] net/nfb: fix bad pointer access in queue stats spinler
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:40 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

The nb_xx_queues values in the dev->data structure can be modified
dynamically by some apps. Use runtime-constant values from hardware
directly for max_rx_queues/max_tx_queues.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  3 +++
 drivers/net/nfb/nfb_ethdev.c | 10 ++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index a8b24ff822..8f8577860a 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -48,6 +48,9 @@ struct pmd_internals {
 	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
 	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
 	struct nfb_device *nfb;
+
+	int max_rx_queues;
+	int max_tx_queues;
 };
 
 #endif /* _NFB_H_ */
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 98119d70fd..7a472a5f1e 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -237,11 +237,13 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
+	struct pmd_internals *internals = dev->data->dev_private;
+
 	dev_info->max_mac_addrs = nfb_eth_get_max_mac_address_count(dev);
 
 	dev_info->max_rx_pktlen = (uint32_t)-1;
-	dev_info->max_rx_queues = dev->data->nb_rx_queues;
-	dev_info->max_tx_queues = dev->data->nb_tx_queues;
+	dev_info->max_rx_queues = internals->max_rx_queues;
+	dev_info->max_tx_queues = internals->max_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
@@ -538,8 +540,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
 		return -EINVAL;
 	}
-	data->nb_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
-	data->nb_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
+	internals->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
+	internals->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
 
 	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
 		data->nb_rx_queues, data->nb_tx_queues);
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v2 2/6] net/nfb: fix bad pointer access in queue stats
  2026-01-15 14:40 ` [PATCH v2 0/6] net/nfb: code cleanup spinler
  2026-01-15 14:40   ` [PATCH v2 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
@ 2026-01-15 14:40   ` spinler
  2026-01-15 14:40   ` [PATCH v2 3/6] net/nfb: update timestamp calculation to meaningful value spinler
                     ` (4 subsequent siblings)
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:40 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

The driver code has dereferenced the dev->data->rx_queues pointer
without checking for its validity.
Pointer invalidation can occur when the eth_dev_rx_queue_config
is called with set to 0, for example.

Moreover, an array of pointers (to a structure) was used like array
of structures (which worked with early dereference just for one queue).

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_stats.c | 46 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/net/nfb/nfb_stats.c b/drivers/net/nfb/nfb_stats.c
index 4ea6b7be21..27a01c3160 100644
--- a/drivers/net/nfb/nfb_stats.c
+++ b/drivers/net/nfb/nfb_stats.c
@@ -20,28 +20,28 @@ nfb_eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
 	uint64_t rx_total_bytes = 0;
 	uint64_t tx_total_bytes = 0;
 
-	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
-		dev->data->rx_queues);
-	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
-		dev->data->tx_queues);
+	struct ndp_rx_queue *rx_queue;
+	struct ndp_tx_queue *tx_queue;
 
 	for (i = 0; i < nb_rx; i++) {
+		rx_queue = dev->data->rx_queues[i];
 		if (qstats && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			qstats->q_ipackets[i] = rx_queue[i].rx_pkts;
-			qstats->q_ibytes[i] = rx_queue[i].rx_bytes;
+			qstats->q_ipackets[i] = rx_queue->rx_pkts;
+			qstats->q_ibytes[i] = rx_queue->rx_bytes;
 		}
-		rx_total += rx_queue[i].rx_pkts;
-		rx_total_bytes += rx_queue[i].rx_bytes;
+		rx_total += rx_queue->rx_pkts;
+		rx_total_bytes += rx_queue->rx_bytes;
 	}
 
 	for (i = 0; i < nb_tx; i++) {
+		tx_queue = dev->data->tx_queues[i];
 		if (qstats && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			qstats->q_opackets[i] = tx_queue[i].tx_pkts;
-			qstats->q_obytes[i] = tx_queue[i].tx_bytes;
+			qstats->q_opackets[i] = tx_queue->tx_pkts;
+			qstats->q_obytes[i] = tx_queue->tx_bytes;
 		}
-		tx_total += tx_queue[i].tx_pkts;
-		tx_total_bytes += tx_queue[i].tx_bytes;
-		tx_err_total += tx_queue[i].err_pkts;
+		tx_total += tx_queue->tx_pkts;
+		tx_total_bytes += tx_queue->tx_bytes;
+		tx_err_total += tx_queue->err_pkts;
 	}
 
 	stats->ipackets = rx_total;
@@ -59,20 +59,20 @@ nfb_eth_stats_reset(struct rte_eth_dev *dev)
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 
-	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
-		dev->data->rx_queues);
-	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
-		dev->data->tx_queues);
+	struct ndp_rx_queue *rx_queue;
+	struct ndp_tx_queue *tx_queue;
 
 	for (i = 0; i < nb_rx; i++) {
-		rx_queue[i].rx_pkts = 0;
-		rx_queue[i].rx_bytes = 0;
-		rx_queue[i].err_pkts = 0;
+		rx_queue = dev->data->rx_queues[i];
+		rx_queue->rx_pkts = 0;
+		rx_queue->rx_bytes = 0;
+		rx_queue->err_pkts = 0;
 	}
 	for (i = 0; i < nb_tx; i++) {
-		tx_queue[i].tx_pkts = 0;
-		tx_queue[i].tx_bytes = 0;
-		tx_queue[i].err_pkts = 0;
+		tx_queue = dev->data->tx_queues[i];
+		tx_queue->tx_pkts = 0;
+		tx_queue->tx_bytes = 0;
+		tx_queue->err_pkts = 0;
 	}
 
 	return 0;
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v2 3/6] net/nfb: update timestamp calculation to meaningful value
  2026-01-15 14:40 ` [PATCH v2 0/6] net/nfb: code cleanup spinler
  2026-01-15 14:40   ` [PATCH v2 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
  2026-01-15 14:40   ` [PATCH v2 2/6] net/nfb: fix bad pointer access in queue stats spinler
@ 2026-01-15 14:40   ` spinler
  2026-01-15 14:40   ` [PATCH v2 4/6] net/nfb: use process private variable for internal data spinler
                     ` (3 subsequent siblings)
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:40 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

The resulting timestamp wasn't monotonically increasing value.

Now the timestamp is value in nanoseconds (Unix epoch).

Unfortunately there's currently no safe mechanism in nfb-framework
to get ticks from hardware to implement read_clock function.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 doc/guides/nics/nfb.rst  |  6 ++----
 drivers/net/nfb/nfb_rx.h | 13 +++++++------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/doc/guides/nics/nfb.rst b/doc/guides/nics/nfb.rst
index 96aaf64440..a9b4049654 100644
--- a/doc/guides/nics/nfb.rst
+++ b/doc/guides/nics/nfb.rst
@@ -64,10 +64,8 @@ products). The standard `RTE_ETH_RX_OFFLOAD_TIMESTAMP` flag can be used for this
 When the timestamps are enabled, a timestamp validity flag is set in the MBUFs
 containing received frames and timestamp is inserted into the `rte_mbuf` struct.
 
-The timestamp is an `uint64_t` field. Its lower 32 bits represent *seconds* portion of the timestamp
-(number of seconds elapsed since 1.1.1970 00:00:00 UTC) and its higher 32 bits represent
-*nanosecond* portion of the timestamp (number of nanoseconds elapsed since the beginning of the
-second in the *seconds* portion.
+The timestamp is an `uint64_t` field and holds the number of nanoseconds
+elapsed since 1.1.1970 00:00:00 UTC.
 
 
 Using the NFB PMD
diff --git a/drivers/net/nfb/nfb_rx.h b/drivers/net/nfb/nfb_rx.h
index 12769841ae..67b3b00e2a 100644
--- a/drivers/net/nfb/nfb_rx.h
+++ b/drivers/net/nfb/nfb_rx.h
@@ -13,6 +13,7 @@
 #include <rte_mbuf.h>
 #include <rte_mbuf_dyn.h>
 #include <rte_ethdev.h>
+#include <rte_time.h>
 
 #include "nfb.h"
 
@@ -202,15 +203,15 @@ nfb_eth_ndp_rx(void *queue,
 			if (nfb_timestamp_dynfield_offset >= 0) {
 				rte_mbuf_timestamp_t timestamp;
 
-				/* nanoseconds */
-				timestamp =
-					rte_le_to_cpu_32(*((uint32_t *)
-					(packets[i].header + 4)));
-				timestamp <<= 32;
 				/* seconds */
-				timestamp |=
+				timestamp =
 					rte_le_to_cpu_32(*((uint32_t *)
 					(packets[i].header + 8)));
+				timestamp *= NSEC_PER_SEC;
+				/* nanoseconds */
+				timestamp +=
+					rte_le_to_cpu_32(*((uint32_t *)
+					(packets[i].header + 4)));
 				*nfb_timestamp_dynfield(mbuf) = timestamp;
 				mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
 			}
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v2 4/6] net/nfb: use process private variable for internal data
  2026-01-15 14:40 ` [PATCH v2 0/6] net/nfb: code cleanup spinler
                     ` (2 preceding siblings ...)
  2026-01-15 14:40   ` [PATCH v2 3/6] net/nfb: update timestamp calculation to meaningful value spinler
@ 2026-01-15 14:40   ` spinler
  2026-01-15 14:40   ` [PATCH v2 5/6] net/nfb: release allocated resources correctly spinler
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:40 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

Internal structures of libnfb can't be shared between processes.
Move these structures from dev_private to process_private, which allows
secondary process to correctly initialize and uninitialize the eth_dev.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  2 +
 drivers/net/nfb/nfb_ethdev.c | 99 +++++++++++++++++++++---------------
 drivers/net/nfb/nfb_rx.c     |  2 +-
 drivers/net/nfb/nfb_rxmode.c | 12 ++---
 drivers/net/nfb/nfb_tx.c     |  2 +-
 5 files changed, 67 insertions(+), 50 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index 8f8577860a..d85c5fba62 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -48,7 +48,9 @@ struct pmd_internals {
 	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
 	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
 	struct nfb_device *nfb;
+};
 
+struct pmd_priv {
 	int max_rx_queues;
 	int max_tx_queues;
 };
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 7a472a5f1e..d7fa1444dd 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -145,7 +145,7 @@ nfb_eth_dev_start(struct rte_eth_dev *dev)
 	return ret;
 }
 
-/**
+/* *
  * DPDK callback to stop the device.
  *
  * Stop device by stopping all configured queues.
@@ -184,7 +184,7 @@ static int
 nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 {
 	int ret;
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
 
 	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
@@ -207,7 +207,7 @@ nfb_eth_get_max_mac_address_count(struct rte_eth_dev *dev)
 	uint16_t i;
 	uint32_t c;
 	uint32_t ret = (uint32_t)-1;
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	/*
 	 * Go through all RX MAC components in firmware and find
@@ -237,13 +237,13 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_priv *priv = dev->data->dev_private;
 
 	dev_info->max_mac_addrs = nfb_eth_get_max_mac_address_count(dev);
 
 	dev_info->max_rx_pktlen = (uint32_t)-1;
-	dev_info->max_rx_queues = internals->max_rx_queues;
-	dev_info->max_tx_queues = internals->max_tx_queues;
+	dev_info->max_rx_queues = priv->max_rx_queues;
+	dev_info->max_tx_queues = priv->max_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
@@ -262,20 +262,20 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 static int
 nfb_eth_dev_close(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	uint16_t i;
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 	int ret;
 
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
 	ret = nfb_eth_dev_stop(dev);
 
-	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
-	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
-
 	for (i = 0; i < nb_rx; i++) {
 		nfb_eth_rx_queue_release(dev, i);
 		dev->data->rx_queues[i] = NULL;
@@ -310,7 +310,7 @@ nfb_eth_link_update(struct rte_eth_dev *dev,
 	struct rte_eth_link link;
 	memset(&link, 0, sizeof(link));
 
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	status.speed = MAC_SPEED_UNKNOWN;
 
@@ -365,7 +365,7 @@ static int
 nfb_eth_dev_set_link_up(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -390,7 +390,7 @@ static int
 nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -431,9 +431,8 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
 {
 	unsigned int i;
 	uint64_t mac;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	mac = nfb_eth_mac_addr_conv(mac_addr);
 	/* Until no real multi-port support, configure all RX MACs the same */
@@ -449,9 +448,8 @@ nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
 {
 	unsigned int i;
 	uint64_t mac;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	mac = nfb_eth_mac_addr_conv(mac_addr);
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -464,9 +462,8 @@ static void
 nfb_eth_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
 {
 	unsigned int i;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	for (i = 0; i < internals->max_rxmac; ++i)
 		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
@@ -512,10 +509,11 @@ static const struct eth_dev_ops ops = {
 static int
 nfb_eth_dev_init(struct rte_eth_dev *dev)
 {
+	int ret;
 	uint32_t mac_count;
 	struct rte_eth_dev_data *data = dev->data;
-	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+	struct pmd_internals *internals;
+	struct pmd_priv *priv = data->dev_private;
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
 	struct rte_ether_addr eth_addr_init;
@@ -525,6 +523,16 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
 
+	internals = rte_zmalloc_socket("nfb_internals",
+			sizeof(struct pmd_internals), RTE_CACHE_LINE_SIZE,
+			dev->device->numa_node);
+	if (internals == NULL) {
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	dev->process_private = internals;
+
 	snprintf(nfb_dev, sizeof(nfb_dev),
 		"/dev/nfb/by-pci-slot/" PCI_PRI_FMT,
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
@@ -538,10 +546,11 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	internals->nfb = nfb_open(nfb_dev);
 	if (internals->nfb == NULL) {
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
+		rte_free(internals);
 		return -EINVAL;
 	}
-	internals->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
-	internals->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
+	priv->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
+	priv->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
 
 	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
 		data->nb_rx_queues, data->nb_tx_queues);
@@ -563,28 +572,31 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	/* Get link state */
 	nfb_eth_link_update(dev, 0);
 
-	/* Allocate space for MAC addresses */
-	mac_count = nfb_eth_get_max_mac_address_count(dev);
-	data->mac_addrs = rte_zmalloc(data->name,
-		sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
-	if (data->mac_addrs == NULL) {
-		NFB_LOG(ERR, "Could not alloc space for MAC address");
-		nfb_close(internals->nfb);
-		return -EINVAL;
-	}
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		/* Allocate space for MAC addresses */
+		mac_count = nfb_eth_get_max_mac_address_count(dev);
+		data->mac_addrs = rte_zmalloc(data->name,
+			sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
+		if (data->mac_addrs == NULL) {
+			NFB_LOG(ERR, "Could not alloc space for MAC address");
+			nfb_close(internals->nfb);
+			rte_free(internals);
+			return -EINVAL;
+		}
 
-	rte_eth_random_addr(eth_addr_init.addr_bytes);
-	eth_addr_init.addr_bytes[0] = eth_addr.addr_bytes[0];
-	eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
-	eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
+		rte_eth_random_addr(eth_addr_init.addr_bytes);
+		eth_addr_init.addr_bytes[0] = eth_addr.addr_bytes[0];
+		eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
+		eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
 
-	nfb_eth_mac_addr_set(dev, &eth_addr_init);
-	rte_ether_addr_copy(&eth_addr_init, &dev->data->mac_addrs[0]);
+		nfb_eth_mac_addr_set(dev, &eth_addr_init);
+		rte_ether_addr_copy(&eth_addr_init, &data->mac_addrs[0]);
 
-	data->promiscuous = nfb_eth_promiscuous_get(dev);
-	data->all_multicast = nfb_eth_allmulticast_get(dev);
+		data->promiscuous = nfb_eth_promiscuous_get(dev);
+		data->all_multicast = nfb_eth_allmulticast_get(dev);
 
-	dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
+		data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
+	}
 
 	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully initialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
@@ -607,9 +619,12 @@ nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 {
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
+	struct pmd_internals *internals = dev->process_private;
 
 	nfb_eth_dev_close(dev);
 
+	rte_free(internals);
+
 	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully uninitialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
@@ -644,7 +659,7 @@ nfb_eth_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		struct rte_pci_device *pci_dev)
 {
 	return rte_eth_dev_pci_generic_probe(pci_dev,
-		sizeof(struct pmd_internals), nfb_eth_dev_init);
+		sizeof(struct pmd_priv), nfb_eth_dev_init);
 }
 
 /**
diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
index 462bc3b50d..413d275853 100644
--- a/drivers/net/nfb/nfb_rx.c
+++ b/drivers/net/nfb/nfb_rx.c
@@ -60,7 +60,7 @@ nfb_eth_rx_queue_setup(struct rte_eth_dev *dev,
 		const struct rte_eth_rxconf *rx_conf __rte_unused,
 		struct rte_mempool *mb_pool)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	struct ndp_rx_queue *rxq;
 	int ret;
diff --git a/drivers/net/nfb/nfb_rxmode.c b/drivers/net/nfb/nfb_rxmode.c
index ca6e4d5578..dc560d638b 100644
--- a/drivers/net/nfb/nfb_rxmode.c
+++ b/drivers/net/nfb/nfb_rxmode.c
@@ -11,7 +11,7 @@ int
 nfb_eth_promiscuous_enable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 	uint16_t i;
 
 	for (i = 0; i < internals->max_rxmac; ++i) {
@@ -26,7 +26,7 @@ int
 nfb_eth_promiscuous_disable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 	uint16_t i;
 	enum nc_rxmac_mac_filter filter = RXMAC_MAC_FILTER_TABLE_BCAST;
 
@@ -44,7 +44,7 @@ int
 nfb_eth_promiscuous_get(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	struct nc_rxmac_status status;
 	status.mac_filter = RXMAC_MAC_FILTER_PROMISCUOUS;
@@ -59,7 +59,7 @@ int
 nfb_eth_allmulticast_enable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	if (dev->data->promiscuous)
@@ -76,7 +76,7 @@ int
 nfb_eth_allmulticast_disable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 
@@ -95,7 +95,7 @@ int
 nfb_eth_allmulticast_get(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	struct nc_rxmac_status status;
 	status.mac_filter = RXMAC_MAC_FILTER_PROMISCUOUS;
diff --git a/drivers/net/nfb/nfb_tx.c b/drivers/net/nfb/nfb_tx.c
index cf99268c43..1f997ce22f 100644
--- a/drivers/net/nfb/nfb_tx.c
+++ b/drivers/net/nfb/nfb_tx.c
@@ -53,7 +53,7 @@ nfb_eth_tx_queue_setup(struct rte_eth_dev *dev,
 	unsigned int socket_id,
 	const struct rte_eth_txconf *tx_conf __rte_unused)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	int ret;
 	struct ndp_tx_queue *txq;
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v2 5/6] net/nfb: release allocated resources correctly
  2026-01-15 14:40 ` [PATCH v2 0/6] net/nfb: code cleanup spinler
                     ` (3 preceding siblings ...)
  2026-01-15 14:40   ` [PATCH v2 4/6] net/nfb: use process private variable for internal data spinler
@ 2026-01-15 14:40   ` spinler
  2026-01-15 14:40   ` [PATCH v2 6/6] net/nfb: stop only started queues in fail path spinler
  2026-01-16  5:48   ` [PATCH v2 0/6] net/nfb: code cleanup Stephen Hemminger
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:40 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

Internal handles are initialized in eth_dev_init, so the cleanup
should be done in eth_dev_uninit.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 77 ++++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 33 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index d7fa1444dd..23f3896c36 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -18,6 +18,9 @@
 #include "nfb_rxmode.h"
 #include "nfb.h"
 
+
+static int nfb_eth_dev_uninit(struct rte_eth_dev *dev);
+
 /**
  * Default MAC addr
  */
@@ -160,8 +163,6 @@ nfb_eth_dev_stop(struct rte_eth_dev *dev)
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 
-	dev->data->dev_started = 0;
-
 	for (i = 0; i < nb_tx; i++)
 		nfb_eth_tx_queue_stop(dev, i);
 
@@ -181,10 +182,9 @@ nfb_eth_dev_stop(struct rte_eth_dev *dev)
  *   0 on success, a negative errno value otherwise.
  */
 static int
-nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
+nfb_eth_dev_configure(struct rte_eth_dev *dev)
 {
 	int ret;
-	struct pmd_internals *internals = dev->process_private;
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
 
 	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
@@ -193,12 +193,16 @@ nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 				&nfb_timestamp_rx_dynflag);
 		if (ret != 0) {
 			NFB_LOG(ERR, "Cannot register Rx timestamp field/flag %d", ret);
-			nfb_close(internals->nfb);
-			return -rte_errno;
+			ret = -ENOMEM;
+			goto err_ts_register;
 		}
 	}
 
 	return 0;
+
+err_ts_register:
+	nfb_eth_dev_uninit(dev);
+	return ret;
 }
 
 static uint32_t
@@ -262,32 +266,28 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 static int
 nfb_eth_dev_close(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *internals = dev->process_private;
 	uint16_t i;
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
-	int ret;
 
-	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
-	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
-
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
-	ret = nfb_eth_dev_stop(dev);
-
-	for (i = 0; i < nb_rx; i++) {
-		nfb_eth_rx_queue_release(dev, i);
-		dev->data->rx_queues[i] = NULL;
-	}
-	dev->data->nb_rx_queues = 0;
-	for (i = 0; i < nb_tx; i++) {
-		nfb_eth_tx_queue_release(dev, i);
-		dev->data->tx_queues[i] = NULL;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		for (i = 0; i < nb_rx; i++) {
+			if (dev->data->rx_queues[i]) {
+				nfb_eth_rx_queue_release(dev, i);
+				dev->data->rx_queues[i] = NULL;
+			}
+		}
+		for (i = 0; i < nb_tx; i++) {
+			if (dev->data->tx_queues[i]) {
+				nfb_eth_tx_queue_release(dev, i);
+				dev->data->tx_queues[i] = NULL;
+			}
+		}
 	}
-	dev->data->nb_tx_queues = 0;
 
-	return ret;
+	nfb_eth_dev_uninit(dev);
+
+	return 0;
 }
 
 /**
@@ -528,7 +528,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 			dev->device->numa_node);
 	if (internals == NULL) {
 		ret = -ENOMEM;
-		return ret;
+		goto err_alloc_internals;
 	}
 
 	dev->process_private = internals;
@@ -546,8 +546,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	internals->nfb = nfb_open(nfb_dev);
 	if (internals->nfb == NULL) {
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
-		rte_free(internals);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_nfb_open;
 	}
 	priv->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
 	priv->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
@@ -579,9 +579,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 			sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
 		if (data->mac_addrs == NULL) {
 			NFB_LOG(ERR, "Could not alloc space for MAC address");
-			nfb_close(internals->nfb);
-			rte_free(internals);
-			return -EINVAL;
+			ret = -ENOMEM;
+			goto err_malloc_mac_addrs;
 		}
 
 		rte_eth_random_addr(eth_addr_init.addr_bytes);
@@ -603,6 +602,16 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		pci_addr->function);
 
 	return 0;
+
+err_malloc_mac_addrs:
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+	nfb_close(internals->nfb);
+
+err_nfb_open:
+	rte_free(internals);
+err_alloc_internals:
+	return ret;
 }
 
 /**
@@ -621,7 +630,9 @@ nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
 	struct pmd_internals *internals = dev->process_private;
 
-	nfb_eth_dev_close(dev);
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+	nfb_close(internals->nfb);
 
 	rte_free(internals);
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v2 6/6] net/nfb: stop only started queues in fail path
  2026-01-15 14:40 ` [PATCH v2 0/6] net/nfb: code cleanup spinler
                     ` (4 preceding siblings ...)
  2026-01-15 14:40   ` [PATCH v2 5/6] net/nfb: release allocated resources correctly spinler
@ 2026-01-15 14:40   ` spinler
  2026-01-16  5:48   ` [PATCH v2 0/6] net/nfb: code cleanup Stephen Hemminger
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-15 14:40 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

The driver should stop only running queues in case of error
inside eth_dev_start.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 23f3896c36..74728f91dd 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -140,11 +140,12 @@ nfb_eth_dev_start(struct rte_eth_dev *dev)
 	return 0;
 
 err_tx:
-	for (i = 0; i < nb_tx; i++)
-		nfb_eth_tx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_tx_queue_stop(dev, i - 1);
+	i = nb_rx;
 err_rx:
-	for (i = 0; i < nb_rx; i++)
-		nfb_eth_rx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_rx_queue_stop(dev, i - 1);
 	return ret;
 }
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v2 0/6] net/nfb: code cleanup
  2026-01-15 14:40 ` [PATCH v2 0/6] net/nfb: code cleanup spinler
                     ` (5 preceding siblings ...)
  2026-01-15 14:40   ` [PATCH v2 6/6] net/nfb: stop only started queues in fail path spinler
@ 2026-01-16  5:48   ` Stephen Hemminger
  2026-01-16  9:42     ` Martin Spinler
  6 siblings, 1 reply; 58+ messages in thread
From: Stephen Hemminger @ 2026-01-16  5:48 UTC (permalink / raw)
  To: spinler; +Cc: dev

On Thu, 15 Jan 2026 15:40:34 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> This patchset mainly cleans up the code and prepare it for another
> quite large rework. Also it resolves some unpleasant behavior.
> 
> ---
> v2:
> * Fixed coding style issues
> 
> Martin Spinler (6):
>   net/nfb: use constant values for max Rx/Tx queues count
>   net/nfb: fix bad pointer access in queue stats
>   net/nfb: update timestamp calculation to meaningful value
>   net/nfb: use process private variable for internal data
>   net/nfb: release allocated resources correctly
>   net/nfb: stop only started queues in fail path
> 
>  doc/guides/nics/nfb.rst      |   6 +-
>  drivers/net/nfb/nfb.h        |   5 ++
>  drivers/net/nfb/nfb_ethdev.c | 165 ++++++++++++++++++++---------------
>  drivers/net/nfb/nfb_rx.c     |   2 +-
>  drivers/net/nfb/nfb_rx.h     |  13 +--
>  drivers/net/nfb/nfb_rxmode.c |  12 +--
>  drivers/net/nfb/nfb_stats.c  |  46 +++++-----
>  drivers/net/nfb/nfb_tx.c     |   2 +-
>  8 files changed, 142 insertions(+), 109 deletions(-)
> 
> --
> 2.52.0
> 

Looks ok but AI code review spotted somethings that need to be addressed.


---

## DPDK NFB Driver Patchset Review

**Series:** net/nfb cleanup (6 patches, v2)
**Submitter:** Martin Spinler <spinler@cesnet.cz>

---

### Patch 1/6: `net/nfb: use constant values for max Rx/Tx queues count`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 51 chars (≤60) |
| Subject format | ✅ PASS | Lowercase after colon, no trailing period |
| Component prefix | ✅ PASS | `net/nfb:` is correct |
| Signed-off-by | ✅ PASS | Present with valid name/email |
| Fixes tag | ✅ PASS | `Fixes: 6435f9a0ac22` (12-char SHA) |
| Cc: stable | ✅ PASS | Present |
| Tag order | ✅ PASS | Fixes before Cc, blank line before Signed-off-by |
| Body doesn't start with "It" | ✅ PASS | Starts with "The" |

**Code Review:**
- Clean addition of `max_rx_queues` and `max_tx_queues` to `pmd_internals` structure
- Proper initialization from hardware counts instead of mutable `nb_*_queues`

**Issue (Warning):** The log message at line 543 still references `data->nb_rx_queues` / `data->nb_tx_queues` which will be 0 at that point since they're no longer initialized. Should log `internals->max_rx_queues` and `internals->max_tx_queues` instead.

---

### Patch 2/6: `net/nfb: fix bad pointer access in queue stats`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 47 chars |
| Subject format | ✅ PASS | Correct format |
| Signed-off-by | ✅ PASS | Present |
| Fixes tag | ✅ PASS | Present with 12-char SHA |
| Cc: stable | ✅ PASS | Present |
| Body doesn't start with "It" | ✅ PASS | Starts with "The" |

**Code Review:**
- Correctly fixes the pointer dereference pattern from `rx_queue[i].rx_pkts` to `rx_queue->rx_pkts`
- The original code treated an array of pointers as an array of structures — this is a legitimate bug fix

**Issue (Warning):** Still missing NULL pointer checks before dereferencing `dev->data->rx_queues[i]` and `dev->data->tx_queues[i]`. The commit message mentions the pointer can be NULL, but the fix doesn't add the NULL check.

---

### Patch 3/6: `net/nfb: update timestamp calculation to meaningful value`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 55 chars |
| Subject format | ✅ PASS | Correct format |
| Signed-off-by | ✅ PASS | Present |
| Fixes tag | ⚠️ **MISSING** | This is a bug fix (non-monotonic timestamp) |
| Cc: stable | ⚠️ **MISSING** | Should be present for bug fixes |
| Body doesn't start with "It" | ✅ PASS | Starts with "The" |
| Doc + code atomic | ✅ PASS | Both `nfb.rst` and `nfb_rx.h` updated together |

**Recommendation:** Add:
```
Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org
```

---

### Patch 4/6: `net/nfb: use process private variable for internal data`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 56 chars |
| Subject format | ✅ PASS | Correct format |
| Signed-off-by | ✅ PASS | Present |
| Fixes tag | ⚠️ **MISSING** | This fixes multi-process support |
| Cc: stable | ⚠️ **MISSING** | Should be present for fixes |

**Code Issues:**

1. **Error (Style):** Line 148 changes a Doxygen comment to broken format:
   ```c
   -/**
   +/* *
   ```
   This breaks Doxygen documentation parsing. Should remain `/**` or be changed to `/*` (without the space after asterisk).

2. **Warning:** The new `struct pmd_priv` only contains `max_rx_queues` and `max_tx_queues` as `int` types. These should likely be `uint16_t` to match the DPDK convention for queue counts.

---

### Patch 5/6: `net/nfb: release allocated resources correctly`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 49 chars |
| Subject format | ✅ PASS | Correct format |
| Signed-off-by | ✅ PASS | Present |
| Fixes tag | ⚠️ **MISSING** | This fixes resource leaks |
| Cc: stable | ⚠️ **MISSING** | Should be present |

**Code Review:**
- Good refactoring of cleanup paths using `goto` error handling
- Properly separates primary vs secondary process cleanup

**Issue (Warning):** Forward declaration added at line 21:
```c
static int nfb_eth_dev_uninit(struct rte_eth_dev *dev);
```
This is fine, but the blank line before it creates two consecutive blank lines which is non-standard.

---

### Patch 6/6: `net/nfb: stop only started queues in fail path`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 47 chars |
| Subject format | ✅ PASS | Correct format |
| Signed-off-by | ✅ PASS | Present |
| Fixes tag | ⚠️ **MISSING** | This is a bug fix |
| Cc: stable | ⚠️ **MISSING** | Should be present |

**Code Review:**
- Correctly uses reverse iteration to only stop queues that were successfully started
- Loop logic is correct: `for (; i > 0; i--)` then `stop(dev, i - 1)`

---

## Summary

### Errors (Must Fix)

| Patch | Issue |
|-------|-------|
| 4/6 | Broken Doxygen comment: `/* *` should be `/**` or `/*` |

### Warnings (Should Fix)

| Patch | Issue |
|-------|-------|
| 1/6 | Log message references uninitialized `data->nb_*_queues` |
| 2/6 | Missing NULL checks for queue pointers (mentioned in commit but not implemented) |
| 3/6 | Missing `Fixes:` tag and `Cc: stable@dpdk.org` |
| 4/6 | Missing `Fixes:` tag and `Cc: stable@dpdk.org`; consider `uint16_t` for queue counts |
| 5/6 | Missing `Fixes:` tag and `Cc: stable@dpdk.org`; extra blank line before forward declaration |
| 6/6 | Missing `Fixes:` tag and `Cc: stable@dpdk.org` |

### Info (Consider)

- The series is well-structured with logical patch ordering
- Good that patches 1 and 2 have proper `Fixes:` tags — patches 3-6 should follow the same pattern
- Documentation is updated atomically with code in patch 3

### Verdict

**Needs revision.** The main blocker is the broken Doxygen comment in patch 4/6. Additionally, patches 3-6 should have `Fixes:` tags and `Cc: stable@dpdk.org` since they all fix bugs in the original driver.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v2 0/6] net/nfb: code cleanup
  2026-01-16  5:48   ` [PATCH v2 0/6] net/nfb: code cleanup Stephen Hemminger
@ 2026-01-16  9:42     ` Martin Spinler
  2026-01-16 17:39       ` Stephen Hemminger
  0 siblings, 1 reply; 58+ messages in thread
From: Martin Spinler @ 2026-01-16  9:42 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

Hi Stephen!

Thanks for quick review, my reaction to issues:

1/6: The log message at line 543: fixed
2/6: Still missing NULL pointer checks: misleading,
     dev->data->nb_rx_queues ensures the queue[i] was inited
4/6: uint16_t: applied, Doxygen comment: fixed
5/6: extra blank line: fixed
Those fixes will be in v3.

3,4,5,6/6: I think don't want to add Cc to stable, if you agree?

Besides that series and 'net-nfb/rework to real multiport', I have
another 3 series of a similar size that depends on those series: Native
Queues, Metadata+Offloads, and Ethernet. To make the work easier, would
you prefer to have it in one 19-commit series or should I to continue
in 3 small series?

Thanks! Martin

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v2 0/6] net/nfb: code cleanup
  2026-01-16  9:42     ` Martin Spinler
@ 2026-01-16 17:39       ` Stephen Hemminger
  0 siblings, 0 replies; 58+ messages in thread
From: Stephen Hemminger @ 2026-01-16 17:39 UTC (permalink / raw)
  To: Martin Spinler; +Cc: dev

On Fri, 16 Jan 2026 10:42:20 +0100
Martin Spinler <spinler@cesnet.cz> wrote:

> Hi Stephen!
> 
> Thanks for quick review, my reaction to issues:
> 
> 1/6: The log message at line 543: fixed
> 2/6: Still missing NULL pointer checks: misleading,
>      dev->data->nb_rx_queues ensures the queue[i] was inited
> 4/6: uint16_t: applied, Doxygen comment: fixed
> 5/6: extra blank line: fixed
> Those fixes will be in v3.
> 
> 3,4,5,6/6: I think don't want to add Cc to stable, if you agree?
> 
> 
> Besides that series and 'net-nfb/rework to real multiport', I have
> another 3 series of a similar size that depends on those series: Native
> Queues, Metadata+Offloads, and Ethernet. To make the work easier, would
> you prefer to have it in one 19-commit series or should I to continue
> in 3 small series?
> 
> Thanks! Martin

Only cc stable if the driver was released in a stable version and
it is something that could be triggerable by bad input

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v2 0/6] net/nfb: code cleanup
  2026-01-15 14:01 [PATCH 0/6] spinler
                   ` (6 preceding siblings ...)
  2026-01-15 14:40 ` [PATCH v2 0/6] net/nfb: code cleanup spinler
@ 2026-01-16 15:20 ` spinler
  2026-01-16 15:20   ` [PATCH v3 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
                     ` (5 more replies)
  2026-01-16 15:22 ` [PATCH v3 0/6] net/nfb: code cleanup spinler
                   ` (2 subsequent siblings)
  10 siblings, 6 replies; 58+ messages in thread
From: spinler @ 2026-01-16 15:20 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

This patchset mainly cleans up the code and prepare it for another
quite large rework. Also it resolves some unpleasant behavior.

---
v3:
* Changed type of queue count variable to uint16_t.
* Fixed wrong queue count variable usage in log.
* Fixed another coding style issues.

v2:
* Fixed coding style issues.


Martin Spinler (6):
  net/nfb: use constant values for max Rx/Tx queues count
  net/nfb: fix bad pointer access in queue stats
  net/nfb: update timestamp calculation to meaningful value
  net/nfb: use process private variable for internal data
  net/nfb: release allocated resources correctly
  net/nfb: stop only started queues in fail path

 doc/guides/nics/nfb.rst      |   6 +-
 drivers/net/nfb/nfb.h        |   5 ++
 drivers/net/nfb/nfb_ethdev.c | 164 ++++++++++++++++++++---------------
 drivers/net/nfb/nfb_rx.c     |   2 +-
 drivers/net/nfb/nfb_rx.h     |  13 +--
 drivers/net/nfb/nfb_rxmode.c |  12 +--
 drivers/net/nfb/nfb_stats.c  |  46 +++++-----
 drivers/net/nfb/nfb_tx.c     |   2 +-
 8 files changed, 141 insertions(+), 109 deletions(-)

--
2.52.0


^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v3 1/6] net/nfb: use constant values for max Rx/Tx queues count
  2026-01-16 15:20 ` spinler
@ 2026-01-16 15:20   ` spinler
  2026-02-02 17:47     ` Stephen Hemminger
  2026-01-16 15:20   ` [PATCH v3 2/6] net/nfb: fix bad pointer access in queue stats spinler
                     ` (4 subsequent siblings)
  5 siblings, 1 reply; 58+ messages in thread
From: spinler @ 2026-01-16 15:20 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

The nb_xx_queues values in the dev->data structure can be modified
dynamically by some apps. Use runtime-constant values from hardware
directly for max_rx_queues/max_tx_queues.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  3 +++
 drivers/net/nfb/nfb_ethdev.c | 12 +++++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index a8b24ff822..917b830283 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -48,6 +48,9 @@ struct pmd_internals {
 	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
 	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
 	struct nfb_device *nfb;
+
+	uint16_t max_rx_queues;
+	uint16_t max_tx_queues;
 };
 
 #endif /* _NFB_H_ */
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 98119d70fd..5b1df83b7f 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -237,11 +237,13 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
+	struct pmd_internals *internals = dev->data->dev_private;
+
 	dev_info->max_mac_addrs = nfb_eth_get_max_mac_address_count(dev);
 
 	dev_info->max_rx_pktlen = (uint32_t)-1;
-	dev_info->max_rx_queues = dev->data->nb_rx_queues;
-	dev_info->max_tx_queues = dev->data->nb_tx_queues;
+	dev_info->max_rx_queues = internals->max_rx_queues;
+	dev_info->max_tx_queues = internals->max_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
@@ -538,11 +540,11 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
 		return -EINVAL;
 	}
-	data->nb_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
-	data->nb_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
+	internals->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
+	internals->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
 
 	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
-		data->nb_rx_queues, data->nb_tx_queues);
+		internals->max_rx_queues, internals->max_tx_queues);
 
 	nfb_nc_rxmac_init(internals->nfb,
 		internals->rxmac,
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 1/6] net/nfb: use constant values for max Rx/Tx queues count
  2026-01-16 15:20   ` [PATCH v3 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
@ 2026-02-02 17:47     ` Stephen Hemminger
  2026-02-02 18:58       ` Martin Špinler
  0 siblings, 1 reply; 58+ messages in thread
From: Stephen Hemminger @ 2026-02-02 17:47 UTC (permalink / raw)
  To: spinler; +Cc: dev, stable

On Fri, 16 Jan 2026 16:20:52 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> The nb_xx_queues values in the dev->data structure can be modified
> dynamically by some apps. Use runtime-constant values from hardware
> directly for max_rx_queues/max_tx_queues.
> 
> Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---

This patch does not apply cleanly to main or next-net branch.
Please rebase.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 1/6] net/nfb: use constant values for max Rx/Tx queues count
  2026-02-02 17:47     ` Stephen Hemminger
@ 2026-02-02 18:58       ` Martin Špinler
  0 siblings, 0 replies; 58+ messages in thread
From: Martin Špinler @ 2026-02-02 18:58 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, stable

> This patch does not apply cleanly to main or next-net branch.
> Please rebase.

Oh, I'm sorry. My old patch was based on next-net-for-main, where is
included your commit: "net/nfb: remove unused attribute".

Rebasing on next-net-main

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v3 2/6] net/nfb: fix bad pointer access in queue stats
  2026-01-16 15:20 ` spinler
  2026-01-16 15:20   ` [PATCH v3 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
@ 2026-01-16 15:20   ` spinler
  2026-01-16 15:20   ` [PATCH v3 3/6] net/nfb: update timestamp calculation to meaningful value spinler
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-16 15:20 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

The driver code has dereferenced the dev->data->rx_queues pointer
without checking for its validity.
Pointer invalidation can occur when the eth_dev_rx_queue_config
is called with set to 0, for example.

Moreover, an array of pointers (to a structure) was used like array
of structures (which worked with early dereference just for one queue).

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_stats.c | 46 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/net/nfb/nfb_stats.c b/drivers/net/nfb/nfb_stats.c
index 4ea6b7be21..27a01c3160 100644
--- a/drivers/net/nfb/nfb_stats.c
+++ b/drivers/net/nfb/nfb_stats.c
@@ -20,28 +20,28 @@ nfb_eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
 	uint64_t rx_total_bytes = 0;
 	uint64_t tx_total_bytes = 0;
 
-	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
-		dev->data->rx_queues);
-	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
-		dev->data->tx_queues);
+	struct ndp_rx_queue *rx_queue;
+	struct ndp_tx_queue *tx_queue;
 
 	for (i = 0; i < nb_rx; i++) {
+		rx_queue = dev->data->rx_queues[i];
 		if (qstats && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			qstats->q_ipackets[i] = rx_queue[i].rx_pkts;
-			qstats->q_ibytes[i] = rx_queue[i].rx_bytes;
+			qstats->q_ipackets[i] = rx_queue->rx_pkts;
+			qstats->q_ibytes[i] = rx_queue->rx_bytes;
 		}
-		rx_total += rx_queue[i].rx_pkts;
-		rx_total_bytes += rx_queue[i].rx_bytes;
+		rx_total += rx_queue->rx_pkts;
+		rx_total_bytes += rx_queue->rx_bytes;
 	}
 
 	for (i = 0; i < nb_tx; i++) {
+		tx_queue = dev->data->tx_queues[i];
 		if (qstats && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			qstats->q_opackets[i] = tx_queue[i].tx_pkts;
-			qstats->q_obytes[i] = tx_queue[i].tx_bytes;
+			qstats->q_opackets[i] = tx_queue->tx_pkts;
+			qstats->q_obytes[i] = tx_queue->tx_bytes;
 		}
-		tx_total += tx_queue[i].tx_pkts;
-		tx_total_bytes += tx_queue[i].tx_bytes;
-		tx_err_total += tx_queue[i].err_pkts;
+		tx_total += tx_queue->tx_pkts;
+		tx_total_bytes += tx_queue->tx_bytes;
+		tx_err_total += tx_queue->err_pkts;
 	}
 
 	stats->ipackets = rx_total;
@@ -59,20 +59,20 @@ nfb_eth_stats_reset(struct rte_eth_dev *dev)
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 
-	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
-		dev->data->rx_queues);
-	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
-		dev->data->tx_queues);
+	struct ndp_rx_queue *rx_queue;
+	struct ndp_tx_queue *tx_queue;
 
 	for (i = 0; i < nb_rx; i++) {
-		rx_queue[i].rx_pkts = 0;
-		rx_queue[i].rx_bytes = 0;
-		rx_queue[i].err_pkts = 0;
+		rx_queue = dev->data->rx_queues[i];
+		rx_queue->rx_pkts = 0;
+		rx_queue->rx_bytes = 0;
+		rx_queue->err_pkts = 0;
 	}
 	for (i = 0; i < nb_tx; i++) {
-		tx_queue[i].tx_pkts = 0;
-		tx_queue[i].tx_bytes = 0;
-		tx_queue[i].err_pkts = 0;
+		tx_queue = dev->data->tx_queues[i];
+		tx_queue->tx_pkts = 0;
+		tx_queue->tx_bytes = 0;
+		tx_queue->err_pkts = 0;
 	}
 
 	return 0;
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 3/6] net/nfb: update timestamp calculation to meaningful value
  2026-01-16 15:20 ` spinler
  2026-01-16 15:20   ` [PATCH v3 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
  2026-01-16 15:20   ` [PATCH v3 2/6] net/nfb: fix bad pointer access in queue stats spinler
@ 2026-01-16 15:20   ` spinler
  2026-01-16 15:20   ` [PATCH v3 4/6] net/nfb: use process private variable for internal data spinler
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-16 15:20 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

The resulting timestamp wasn't monotonically increasing value.

Now the timestamp is value in nanoseconds (Unix epoch).

Unfortunately there's currently no safe mechanism in nfb-framework
to get ticks from hardware to implement read_clock function.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 doc/guides/nics/nfb.rst  |  6 ++----
 drivers/net/nfb/nfb_rx.h | 13 +++++++------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/doc/guides/nics/nfb.rst b/doc/guides/nics/nfb.rst
index 96aaf64440..a9b4049654 100644
--- a/doc/guides/nics/nfb.rst
+++ b/doc/guides/nics/nfb.rst
@@ -64,10 +64,8 @@ products). The standard `RTE_ETH_RX_OFFLOAD_TIMESTAMP` flag can be used for this
 When the timestamps are enabled, a timestamp validity flag is set in the MBUFs
 containing received frames and timestamp is inserted into the `rte_mbuf` struct.
 
-The timestamp is an `uint64_t` field. Its lower 32 bits represent *seconds* portion of the timestamp
-(number of seconds elapsed since 1.1.1970 00:00:00 UTC) and its higher 32 bits represent
-*nanosecond* portion of the timestamp (number of nanoseconds elapsed since the beginning of the
-second in the *seconds* portion.
+The timestamp is an `uint64_t` field and holds the number of nanoseconds
+elapsed since 1.1.1970 00:00:00 UTC.
 
 
 Using the NFB PMD
diff --git a/drivers/net/nfb/nfb_rx.h b/drivers/net/nfb/nfb_rx.h
index 12769841ae..67b3b00e2a 100644
--- a/drivers/net/nfb/nfb_rx.h
+++ b/drivers/net/nfb/nfb_rx.h
@@ -13,6 +13,7 @@
 #include <rte_mbuf.h>
 #include <rte_mbuf_dyn.h>
 #include <rte_ethdev.h>
+#include <rte_time.h>
 
 #include "nfb.h"
 
@@ -202,15 +203,15 @@ nfb_eth_ndp_rx(void *queue,
 			if (nfb_timestamp_dynfield_offset >= 0) {
 				rte_mbuf_timestamp_t timestamp;
 
-				/* nanoseconds */
-				timestamp =
-					rte_le_to_cpu_32(*((uint32_t *)
-					(packets[i].header + 4)));
-				timestamp <<= 32;
 				/* seconds */
-				timestamp |=
+				timestamp =
 					rte_le_to_cpu_32(*((uint32_t *)
 					(packets[i].header + 8)));
+				timestamp *= NSEC_PER_SEC;
+				/* nanoseconds */
+				timestamp +=
+					rte_le_to_cpu_32(*((uint32_t *)
+					(packets[i].header + 4)));
 				*nfb_timestamp_dynfield(mbuf) = timestamp;
 				mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
 			}
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 4/6] net/nfb: use process private variable for internal data
  2026-01-16 15:20 ` spinler
                     ` (2 preceding siblings ...)
  2026-01-16 15:20   ` [PATCH v3 3/6] net/nfb: update timestamp calculation to meaningful value spinler
@ 2026-01-16 15:20   ` spinler
  2026-01-20  0:13     ` Stephen Hemminger
  2026-01-16 15:20   ` [PATCH v3 5/6] net/nfb: release allocated resources correctly spinler
  2026-01-16 15:20   ` [PATCH v3 6/6] net/nfb: stop only started queues in fail path spinler
  5 siblings, 1 reply; 58+ messages in thread
From: spinler @ 2026-01-16 15:20 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

Internal structures of libnfb can't be shared between processes.
Move these structures from dev_private to process_private, which allows
secondary process to correctly initialize and uninitialize the eth_dev.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  2 +
 drivers/net/nfb/nfb_ethdev.c | 99 +++++++++++++++++++++---------------
 drivers/net/nfb/nfb_rx.c     |  2 +-
 drivers/net/nfb/nfb_rxmode.c | 12 ++---
 drivers/net/nfb/nfb_tx.c     |  2 +-
 5 files changed, 67 insertions(+), 50 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index 917b830283..09d4b7da5f 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -48,7 +48,9 @@ struct pmd_internals {
 	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
 	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
 	struct nfb_device *nfb;
+};
 
+struct pmd_priv {
 	uint16_t max_rx_queues;
 	uint16_t max_tx_queues;
 };
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 5b1df83b7f..4dcf6bde01 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -184,7 +184,7 @@ static int
 nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 {
 	int ret;
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
 
 	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
@@ -207,7 +207,7 @@ nfb_eth_get_max_mac_address_count(struct rte_eth_dev *dev)
 	uint16_t i;
 	uint32_t c;
 	uint32_t ret = (uint32_t)-1;
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	/*
 	 * Go through all RX MAC components in firmware and find
@@ -237,13 +237,13 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_priv *priv = dev->data->dev_private;
 
 	dev_info->max_mac_addrs = nfb_eth_get_max_mac_address_count(dev);
 
 	dev_info->max_rx_pktlen = (uint32_t)-1;
-	dev_info->max_rx_queues = internals->max_rx_queues;
-	dev_info->max_tx_queues = internals->max_tx_queues;
+	dev_info->max_rx_queues = priv->max_rx_queues;
+	dev_info->max_tx_queues = priv->max_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
@@ -262,20 +262,20 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 static int
 nfb_eth_dev_close(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	uint16_t i;
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 	int ret;
 
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
 	ret = nfb_eth_dev_stop(dev);
 
-	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
-	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
-
 	for (i = 0; i < nb_rx; i++) {
 		nfb_eth_rx_queue_release(dev, i);
 		dev->data->rx_queues[i] = NULL;
@@ -310,7 +310,7 @@ nfb_eth_link_update(struct rte_eth_dev *dev,
 	struct rte_eth_link link;
 	memset(&link, 0, sizeof(link));
 
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	status.speed = MAC_SPEED_UNKNOWN;
 
@@ -365,7 +365,7 @@ static int
 nfb_eth_dev_set_link_up(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -390,7 +390,7 @@ static int
 nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -431,9 +431,8 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
 {
 	unsigned int i;
 	uint64_t mac;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	mac = nfb_eth_mac_addr_conv(mac_addr);
 	/* Until no real multi-port support, configure all RX MACs the same */
@@ -449,9 +448,8 @@ nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
 {
 	unsigned int i;
 	uint64_t mac;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	mac = nfb_eth_mac_addr_conv(mac_addr);
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -464,9 +462,8 @@ static void
 nfb_eth_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
 {
 	unsigned int i;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	for (i = 0; i < internals->max_rxmac; ++i)
 		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
@@ -512,10 +509,11 @@ static const struct eth_dev_ops ops = {
 static int
 nfb_eth_dev_init(struct rte_eth_dev *dev)
 {
+	int ret;
 	uint32_t mac_count;
 	struct rte_eth_dev_data *data = dev->data;
-	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+	struct pmd_internals *internals;
+	struct pmd_priv *priv = data->dev_private;
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
 	struct rte_ether_addr eth_addr_init;
@@ -525,6 +523,16 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
 
+	internals = rte_zmalloc_socket("nfb_internals",
+			sizeof(struct pmd_internals), RTE_CACHE_LINE_SIZE,
+			dev->device->numa_node);
+	if (internals == NULL) {
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	dev->process_private = internals;
+
 	snprintf(nfb_dev, sizeof(nfb_dev),
 		"/dev/nfb/by-pci-slot/" PCI_PRI_FMT,
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
@@ -538,13 +546,14 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	internals->nfb = nfb_open(nfb_dev);
 	if (internals->nfb == NULL) {
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
+		rte_free(internals);
 		return -EINVAL;
 	}
-	internals->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
-	internals->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
+	priv->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
+	priv->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
 
 	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
-		internals->max_rx_queues, internals->max_tx_queues);
+		priv->max_rx_queues, priv->max_tx_queues);
 
 	nfb_nc_rxmac_init(internals->nfb,
 		internals->rxmac,
@@ -563,28 +572,31 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	/* Get link state */
 	nfb_eth_link_update(dev, 0);
 
-	/* Allocate space for MAC addresses */
-	mac_count = nfb_eth_get_max_mac_address_count(dev);
-	data->mac_addrs = rte_zmalloc(data->name,
-		sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
-	if (data->mac_addrs == NULL) {
-		NFB_LOG(ERR, "Could not alloc space for MAC address");
-		nfb_close(internals->nfb);
-		return -EINVAL;
-	}
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		/* Allocate space for MAC addresses */
+		mac_count = nfb_eth_get_max_mac_address_count(dev);
+		data->mac_addrs = rte_zmalloc(data->name,
+			sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
+		if (data->mac_addrs == NULL) {
+			NFB_LOG(ERR, "Could not alloc space for MAC address");
+			nfb_close(internals->nfb);
+			rte_free(internals);
+			return -EINVAL;
+		}
 
-	rte_eth_random_addr(eth_addr_init.addr_bytes);
-	eth_addr_init.addr_bytes[0] = eth_addr.addr_bytes[0];
-	eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
-	eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
+		rte_eth_random_addr(eth_addr_init.addr_bytes);
+		eth_addr_init.addr_bytes[0] = eth_addr.addr_bytes[0];
+		eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
+		eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
 
-	nfb_eth_mac_addr_set(dev, &eth_addr_init);
-	rte_ether_addr_copy(&eth_addr_init, &dev->data->mac_addrs[0]);
+		nfb_eth_mac_addr_set(dev, &eth_addr_init);
+		rte_ether_addr_copy(&eth_addr_init, &data->mac_addrs[0]);
 
-	data->promiscuous = nfb_eth_promiscuous_get(dev);
-	data->all_multicast = nfb_eth_allmulticast_get(dev);
+		data->promiscuous = nfb_eth_promiscuous_get(dev);
+		data->all_multicast = nfb_eth_allmulticast_get(dev);
 
-	dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
+		data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
+	}
 
 	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully initialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
@@ -607,9 +619,12 @@ nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 {
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
+	struct pmd_internals *internals = dev->process_private;
 
 	nfb_eth_dev_close(dev);
 
+	rte_free(internals);
+
 	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully uninitialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
@@ -644,7 +659,7 @@ nfb_eth_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		struct rte_pci_device *pci_dev)
 {
 	return rte_eth_dev_pci_generic_probe(pci_dev,
-		sizeof(struct pmd_internals), nfb_eth_dev_init);
+		sizeof(struct pmd_priv), nfb_eth_dev_init);
 }
 
 /**
diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
index 462bc3b50d..413d275853 100644
--- a/drivers/net/nfb/nfb_rx.c
+++ b/drivers/net/nfb/nfb_rx.c
@@ -60,7 +60,7 @@ nfb_eth_rx_queue_setup(struct rte_eth_dev *dev,
 		const struct rte_eth_rxconf *rx_conf __rte_unused,
 		struct rte_mempool *mb_pool)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	struct ndp_rx_queue *rxq;
 	int ret;
diff --git a/drivers/net/nfb/nfb_rxmode.c b/drivers/net/nfb/nfb_rxmode.c
index ca6e4d5578..dc560d638b 100644
--- a/drivers/net/nfb/nfb_rxmode.c
+++ b/drivers/net/nfb/nfb_rxmode.c
@@ -11,7 +11,7 @@ int
 nfb_eth_promiscuous_enable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 	uint16_t i;
 
 	for (i = 0; i < internals->max_rxmac; ++i) {
@@ -26,7 +26,7 @@ int
 nfb_eth_promiscuous_disable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 	uint16_t i;
 	enum nc_rxmac_mac_filter filter = RXMAC_MAC_FILTER_TABLE_BCAST;
 
@@ -44,7 +44,7 @@ int
 nfb_eth_promiscuous_get(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	struct nc_rxmac_status status;
 	status.mac_filter = RXMAC_MAC_FILTER_PROMISCUOUS;
@@ -59,7 +59,7 @@ int
 nfb_eth_allmulticast_enable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	if (dev->data->promiscuous)
@@ -76,7 +76,7 @@ int
 nfb_eth_allmulticast_disable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 
@@ -95,7 +95,7 @@ int
 nfb_eth_allmulticast_get(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	struct nc_rxmac_status status;
 	status.mac_filter = RXMAC_MAC_FILTER_PROMISCUOUS;
diff --git a/drivers/net/nfb/nfb_tx.c b/drivers/net/nfb/nfb_tx.c
index cf99268c43..1f997ce22f 100644
--- a/drivers/net/nfb/nfb_tx.c
+++ b/drivers/net/nfb/nfb_tx.c
@@ -53,7 +53,7 @@ nfb_eth_tx_queue_setup(struct rte_eth_dev *dev,
 	unsigned int socket_id,
 	const struct rte_eth_txconf *tx_conf __rte_unused)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	int ret;
 	struct ndp_tx_queue *txq;
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 4/6] net/nfb: use process private variable for internal data
  2026-01-16 15:20   ` [PATCH v3 4/6] net/nfb: use process private variable for internal data spinler
@ 2026-01-20  0:13     ` Stephen Hemminger
  2026-01-20 14:13       ` Martin Spinler
  0 siblings, 1 reply; 58+ messages in thread
From: Stephen Hemminger @ 2026-01-20  0:13 UTC (permalink / raw)
  To: spinler; +Cc: dev

On Fri, 16 Jan 2026 16:20:55 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> Internal structures of libnfb can't be shared between processes.
> Move these structures from dev_private to process_private, which allows
> secondary process to correctly initialize and uninitialize the eth_dev.
> 
> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---
>  drivers/net/nfb/nfb.h        |  2 +
>  drivers/net/nfb/nfb_ethdev.c | 99 +++++++++++++++++++++---------------
>  drivers/net/nfb/nfb_rx.c     |  2 +-
>  drivers/net/nfb/nfb_rxmode.c | 12 ++---
>  drivers/net/nfb/nfb_tx.c     |  2 +-
>  5 files changed, 67 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
> index 917b830283..09d4b7da5f 100644
> --- a/drivers/net/nfb/nfb.h
> +++ b/drivers/net/nfb/nfb.h
> @@ -48,7 +48,9 @@ struct pmd_internals {
>  	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
>  	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
>  	struct nfb_device *nfb;
> +};
>  
> +struct pmd_priv {
>  	uint16_t max_rx_queues;
>  	uint16_t max_tx_queues;
>  };

Where does max_rx_queues get populated in the secondary process?
What if either process calls configure to change number of queues?

Don't see other drivers splitting structure here.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 4/6] net/nfb: use process private variable for internal data
  2026-01-20  0:13     ` Stephen Hemminger
@ 2026-01-20 14:13       ` Martin Spinler
  2026-01-20 16:11         ` Stephen Hemminger
  0 siblings, 1 reply; 58+ messages in thread
From: Martin Spinler @ 2026-01-20 14:13 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

On Mon, 2026-01-19 at 16:13 -0800, Stephen Hemminger wrote:
> > Internal structures of libnfb can't be shared between processes.
> > Move these structures from dev_private to process_private, which allows
> > secondary process to correctly initialize and uninitialize the eth_dev.
> > 
> > 
> > diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
> > index 917b830283..09d4b7da5f 100644
> > --- a/drivers/net/nfb/nfb.h
> > +++ b/drivers/net/nfb/nfb.h
> > @@ -48,7 +48,9 @@ struct pmd_internals {
> >  	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
> >  	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
> >  	struct nfb_device *nfb;
> > +};
> >  
> > +struct pmd_priv {
> >  	uint16_t max_rx_queues;
> >  	uint16_t max_tx_queues;
> >  };
> 
> Where does max_rx_queues get populated in the secondary process?
> What if either process calls configure to change number of queues?
> 
> Don't see other drivers splitting structure here.

The NFB PMD must use separate handles for each process (each process
has its own FILE* and mmap handles from libnfb). However, some data,
such as max_rx_queues, is constant and can still be stored in shared
memory (and inited by RTE_PROC_PRIMARY).

The pmd_priv is stored in data->dev_private, so it is shared between
processes. However, your comment pointed at the opposite issue, that
the nfb_eth_dev_init() assigns to the (pmd_)priv->max_rx_queues in both
process types (duplicate assignment).
I will fix this by moving those assignments to the RTE_PROC_PRIMARY-
only block.

By the way, this partial diff shows the struct pmd_internals being
split into the new struct pmd_priv. Semantically, however, the struct
pmd_internals is the new one, is newly alocated and holds the
process_private data.

Does that make it better sense now?

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 4/6] net/nfb: use process private variable for internal data
  2026-01-20 14:13       ` Martin Spinler
@ 2026-01-20 16:11         ` Stephen Hemminger
  0 siblings, 0 replies; 58+ messages in thread
From: Stephen Hemminger @ 2026-01-20 16:11 UTC (permalink / raw)
  To: Martin Spinler; +Cc: dev

On Tue, 20 Jan 2026 15:13:58 +0100
Martin Spinler <spinler@cesnet.cz> wrote:

> On Mon, 2026-01-19 at 16:13 -0800, Stephen Hemminger wrote:
> > > Internal structures of libnfb can't be shared between processes.
> > > Move these structures from dev_private to process_private, which allows
> > > secondary process to correctly initialize and uninitialize the eth_dev.
> > > 
> > > 
> > > diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
> > > index 917b830283..09d4b7da5f 100644
> > > --- a/drivers/net/nfb/nfb.h
> > > +++ b/drivers/net/nfb/nfb.h
> > > @@ -48,7 +48,9 @@ struct pmd_internals {
> > >  	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
> > >  	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
> > >  	struct nfb_device *nfb;
> > > +};
> > >  
> > > +struct pmd_priv {
> > >  	uint16_t max_rx_queues;
> > >  	uint16_t max_tx_queues;
> > >  };  
> > 
> > Where does max_rx_queues get populated in the secondary process?
> > What if either process calls configure to change number of queues?
> > 
> > Don't see other drivers splitting structure here.  
> 
> The NFB PMD must use separate handles for each process (each process
> has its own FILE* and mmap handles from libnfb). However, some data,
> such as max_rx_queues, is constant and can still be stored in shared
> memory (and inited by RTE_PROC_PRIMARY).
> 
> The pmd_priv is stored in data->dev_private, so it is shared between
> processes. However, your comment pointed at the opposite issue, that
> the nfb_eth_dev_init() assigns to the (pmd_)priv->max_rx_queues in both
> process types (duplicate assignment).
> I will fix this by moving those assignments to the RTE_PROC_PRIMARY-
> only block.
> 
> By the way, this partial diff shows the struct pmd_internals being
> split into the new struct pmd_priv. Semantically, however, the struct
> pmd_internals is the new one, is newly alocated and holds the
> process_private data.
> 
> Does that make it better sense now?

Yes thanks, you might want to add a comment for future readers

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v3 5/6] net/nfb: release allocated resources correctly
  2026-01-16 15:20 ` spinler
                     ` (3 preceding siblings ...)
  2026-01-16 15:20   ` [PATCH v3 4/6] net/nfb: use process private variable for internal data spinler
@ 2026-01-16 15:20   ` spinler
  2026-01-20  0:10     ` Stephen Hemminger
  2026-01-16 15:20   ` [PATCH v3 6/6] net/nfb: stop only started queues in fail path spinler
  5 siblings, 1 reply; 58+ messages in thread
From: spinler @ 2026-01-16 15:20 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

Internal handles are initialized in eth_dev_init, so the cleanup
should be done in eth_dev_uninit.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 76 ++++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 33 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 4dcf6bde01..2f8a3813f7 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -18,6 +18,8 @@
 #include "nfb_rxmode.h"
 #include "nfb.h"
 
+static int nfb_eth_dev_uninit(struct rte_eth_dev *dev);
+
 /**
  * Default MAC addr
  */
@@ -160,8 +162,6 @@ nfb_eth_dev_stop(struct rte_eth_dev *dev)
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 
-	dev->data->dev_started = 0;
-
 	for (i = 0; i < nb_tx; i++)
 		nfb_eth_tx_queue_stop(dev, i);
 
@@ -181,10 +181,9 @@ nfb_eth_dev_stop(struct rte_eth_dev *dev)
  *   0 on success, a negative errno value otherwise.
  */
 static int
-nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
+nfb_eth_dev_configure(struct rte_eth_dev *dev)
 {
 	int ret;
-	struct pmd_internals *internals = dev->process_private;
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
 
 	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
@@ -193,12 +192,16 @@ nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 				&nfb_timestamp_rx_dynflag);
 		if (ret != 0) {
 			NFB_LOG(ERR, "Cannot register Rx timestamp field/flag %d", ret);
-			nfb_close(internals->nfb);
-			return -rte_errno;
+			ret = -ENOMEM;
+			goto err_ts_register;
 		}
 	}
 
 	return 0;
+
+err_ts_register:
+	nfb_eth_dev_uninit(dev);
+	return ret;
 }
 
 static uint32_t
@@ -262,32 +265,28 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 static int
 nfb_eth_dev_close(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *internals = dev->process_private;
 	uint16_t i;
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
-	int ret;
 
-	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
-	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
-
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
-	ret = nfb_eth_dev_stop(dev);
-
-	for (i = 0; i < nb_rx; i++) {
-		nfb_eth_rx_queue_release(dev, i);
-		dev->data->rx_queues[i] = NULL;
-	}
-	dev->data->nb_rx_queues = 0;
-	for (i = 0; i < nb_tx; i++) {
-		nfb_eth_tx_queue_release(dev, i);
-		dev->data->tx_queues[i] = NULL;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		for (i = 0; i < nb_rx; i++) {
+			if (dev->data->rx_queues[i]) {
+				nfb_eth_rx_queue_release(dev, i);
+				dev->data->rx_queues[i] = NULL;
+			}
+		}
+		for (i = 0; i < nb_tx; i++) {
+			if (dev->data->tx_queues[i]) {
+				nfb_eth_tx_queue_release(dev, i);
+				dev->data->tx_queues[i] = NULL;
+			}
+		}
 	}
-	dev->data->nb_tx_queues = 0;
 
-	return ret;
+	nfb_eth_dev_uninit(dev);
+
+	return 0;
 }
 
 /**
@@ -528,7 +527,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 			dev->device->numa_node);
 	if (internals == NULL) {
 		ret = -ENOMEM;
-		return ret;
+		goto err_alloc_internals;
 	}
 
 	dev->process_private = internals;
@@ -546,8 +545,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	internals->nfb = nfb_open(nfb_dev);
 	if (internals->nfb == NULL) {
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
-		rte_free(internals);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_nfb_open;
 	}
 	priv->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
 	priv->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
@@ -579,9 +578,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 			sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
 		if (data->mac_addrs == NULL) {
 			NFB_LOG(ERR, "Could not alloc space for MAC address");
-			nfb_close(internals->nfb);
-			rte_free(internals);
-			return -EINVAL;
+			ret = -ENOMEM;
+			goto err_malloc_mac_addrs;
 		}
 
 		rte_eth_random_addr(eth_addr_init.addr_bytes);
@@ -603,6 +601,16 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		pci_addr->function);
 
 	return 0;
+
+err_malloc_mac_addrs:
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+	nfb_close(internals->nfb);
+
+err_nfb_open:
+	rte_free(internals);
+err_alloc_internals:
+	return ret;
 }
 
 /**
@@ -621,7 +629,9 @@ nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
 	struct pmd_internals *internals = dev->process_private;
 
-	nfb_eth_dev_close(dev);
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+	nfb_close(internals->nfb);
 
 	rte_free(internals);
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 5/6] net/nfb: release allocated resources correctly
  2026-01-16 15:20   ` [PATCH v3 5/6] net/nfb: release allocated resources correctly spinler
@ 2026-01-20  0:10     ` Stephen Hemminger
  2026-01-20 14:14       ` Martin Spinler
  0 siblings, 1 reply; 58+ messages in thread
From: Stephen Hemminger @ 2026-01-20  0:10 UTC (permalink / raw)
  To: spinler; +Cc: dev

On Fri, 16 Jan 2026 16:20:56 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> Internal handles are initialized in eth_dev_init, so the cleanup
> should be done in eth_dev_uninit.
> 
> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---

Looks like a bugfix and should have Fixes tag.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 5/6] net/nfb: release allocated resources correctly
  2026-01-20  0:10     ` Stephen Hemminger
@ 2026-01-20 14:14       ` Martin Spinler
  0 siblings, 0 replies; 58+ messages in thread
From: Martin Spinler @ 2026-01-20 14:14 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

On Mon, 2026-01-19 at 16:10 -0800, Stephen Hemminger wrote:
> On Fri, 16 Jan 2026 16:20:56 +0100
> spinler@cesnet.cz wrote:
> 
> > From: Martin Spinler <spinler@cesnet.cz>
> > 
> > Internal handles are initialized in eth_dev_init, so the cleanup
> > should be done in eth_dev_uninit.
> > 
> > Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> > ---
> 
> Looks like a bugfix and should have Fixes tag.

Ack.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v3 6/6] net/nfb: stop only started queues in fail path
  2026-01-16 15:20 ` spinler
                     ` (4 preceding siblings ...)
  2026-01-16 15:20   ` [PATCH v3 5/6] net/nfb: release allocated resources correctly spinler
@ 2026-01-16 15:20   ` spinler
  2026-01-20  0:09     ` Stephen Hemminger
  5 siblings, 1 reply; 58+ messages in thread
From: spinler @ 2026-01-16 15:20 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

The driver should stop only running queues in case of error
inside eth_dev_start.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 2f8a3813f7..59d9987789 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -139,11 +139,12 @@ nfb_eth_dev_start(struct rte_eth_dev *dev)
 	return 0;
 
 err_tx:
-	for (i = 0; i < nb_tx; i++)
-		nfb_eth_tx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_tx_queue_stop(dev, i - 1);
+	i = nb_rx;
 err_rx:
-	for (i = 0; i < nb_rx; i++)
-		nfb_eth_rx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_rx_queue_stop(dev, i - 1);
 	return ret;
 }
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 6/6] net/nfb: stop only started queues in fail path
  2026-01-16 15:20   ` [PATCH v3 6/6] net/nfb: stop only started queues in fail path spinler
@ 2026-01-20  0:09     ` Stephen Hemminger
  2026-01-20 14:14       ` Martin Spinler
  0 siblings, 1 reply; 58+ messages in thread
From: Stephen Hemminger @ 2026-01-20  0:09 UTC (permalink / raw)
  To: spinler; +Cc: dev

On Fri, 16 Jan 2026 16:20:57 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> The driver should stop only running queues in case of error
> inside eth_dev_start.
> 
> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---

This is a bug fix, shouldn't have a Fixes and CC of stable?

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 6/6] net/nfb: stop only started queues in fail path
  2026-01-20  0:09     ` Stephen Hemminger
@ 2026-01-20 14:14       ` Martin Spinler
  0 siblings, 0 replies; 58+ messages in thread
From: Martin Spinler @ 2026-01-20 14:14 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

On Mon, 2026-01-19 at 16:09 -0800, Stephen Hemminger wrote:
> On Fri, 16 Jan 2026 16:20:57 +0100
> spinler@cesnet.cz wrote:
> 
> > From: Martin Spinler <spinler@cesnet.cz>
> > 
> > The driver should stop only running queues in case of error
> > inside eth_dev_start.
> > 
> > Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> > ---
> 
> This is a bug fix, shouldn't have a Fixes and CC of stable?

Well, the behaviour of the driver is exactly the same, but the code is
wrong. Ack.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v3 0/6] net/nfb: code cleanup
  2026-01-15 14:01 [PATCH 0/6] spinler
                   ` (7 preceding siblings ...)
  2026-01-16 15:20 ` spinler
@ 2026-01-16 15:22 ` spinler
  2026-01-21  4:57   ` Stephen Hemminger
  2026-01-21 17:01 ` [PATCH v4 " spinler
  2026-02-02 19:33 ` [PATCH v5 " spinler
  10 siblings, 1 reply; 58+ messages in thread
From: spinler @ 2026-01-16 15:22 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

This patchset mainly cleans up the code and prepare it for another
quite large rework. Also it resolves some unpleasant behavior.

---
v3:
* Changed type of queue count variable to uint16_t.
* Fixed wrong queue count variable usage in log.
* Fixed another coding style issues.

v2:
* Fixed coding style issues.


Martin Spinler (6):
  net/nfb: use constant values for max Rx/Tx queues count
  net/nfb: fix bad pointer access in queue stats
  net/nfb: update timestamp calculation to meaningful value
  net/nfb: use process private variable for internal data
  net/nfb: release allocated resources correctly
  net/nfb: stop only started queues in fail path

 doc/guides/nics/nfb.rst      |   6 +-
 drivers/net/nfb/nfb.h        |   5 ++
 drivers/net/nfb/nfb_ethdev.c | 164 ++++++++++++++++++++---------------
 drivers/net/nfb/nfb_rx.c     |   2 +-
 drivers/net/nfb/nfb_rx.h     |  13 +--
 drivers/net/nfb/nfb_rxmode.c |  12 +--
 drivers/net/nfb/nfb_stats.c  |  46 +++++-----
 drivers/net/nfb/nfb_tx.c     |   2 +-
 8 files changed, 141 insertions(+), 109 deletions(-)

--
2.52.0


^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 0/6] net/nfb: code cleanup
  2026-01-16 15:22 ` [PATCH v3 0/6] net/nfb: code cleanup spinler
@ 2026-01-21  4:57   ` Stephen Hemminger
  0 siblings, 0 replies; 58+ messages in thread
From: Stephen Hemminger @ 2026-01-21  4:57 UTC (permalink / raw)
  To: spinler; +Cc: dev

On Fri, 16 Jan 2026 16:22:51 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> This patchset mainly cleans up the code and prepare it for another
> quite large rework. Also it resolves some unpleasant behavior.
> 
> ---
> v3:
> * Changed type of queue count variable to uint16_t.
> * Fixed wrong queue count variable usage in log.
> * Fixed another coding style issues.
> 
> v2:
> * Fixed coding style issues.
> 
> 
> Martin Spinler (6):
>   net/nfb: use constant values for max Rx/Tx queues count
>   net/nfb: fix bad pointer access in queue stats
>   net/nfb: update timestamp calculation to meaningful value
>   net/nfb: use process private variable for internal data
>   net/nfb: release allocated resources correctly
>   net/nfb: stop only started queues in fail path
> 
>  doc/guides/nics/nfb.rst      |   6 +-
>  drivers/net/nfb/nfb.h        |   5 ++
>  drivers/net/nfb/nfb_ethdev.c | 164 ++++++++++++++++++++---------------
>  drivers/net/nfb/nfb_rx.c     |   2 +-
>  drivers/net/nfb/nfb_rx.h     |  13 +--
>  drivers/net/nfb/nfb_rxmode.c |  12 +--
>  drivers/net/nfb/nfb_stats.c  |  46 +++++-----
>  drivers/net/nfb/nfb_tx.c     |   2 +-
>  8 files changed, 141 insertions(+), 109 deletions(-)
> 
> --
> 2.52.0
> 

Send v4 with fixes and release note please.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v4 0/6] net/nfb: code cleanup
  2026-01-15 14:01 [PATCH 0/6] spinler
                   ` (8 preceding siblings ...)
  2026-01-16 15:22 ` [PATCH v3 0/6] net/nfb: code cleanup spinler
@ 2026-01-21 17:01 ` spinler
  2026-01-21 17:01   ` [PATCH v4 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
                     ` (6 more replies)
  2026-02-02 19:33 ` [PATCH v5 " spinler
  10 siblings, 7 replies; 58+ messages in thread
From: spinler @ 2026-01-21 17:01 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

This patchset mainly cleans up the code and prepare it for another
quite large rework. Also it resolves some unpleasant behavior.

---
v4:
* Added comment for process private/shared data.
* Added Fixes+Cc tag for patch 4,5,6
* Moved shared data assignments to RTE_PROC_PRIMARY block

v3:
* Changed type of queue count variable to uint16_t.
* Fixed wrong queue count variable usage in log.
* Fixed another coding style issues.

v2:
* Fixed coding style issues.


Martin Spinler (6):
  net/nfb: use constant values for max Rx/Tx queues count
  net/nfb: fix bad pointer access in queue stats
  net/nfb: update timestamp calculation to meaningful value
  net/nfb: use process private variable for internal data
  net/nfb: release allocated resources correctly
  net/nfb: stop only started queues in fail path

 doc/guides/nics/nfb.rst      |   6 +-
 drivers/net/nfb/nfb.h        |  14 ++-
 drivers/net/nfb/nfb_ethdev.c | 166 +++++++++++++++++++++--------------
 drivers/net/nfb/nfb_rx.c     |   2 +-
 drivers/net/nfb/nfb_rx.h     |  13 +--
 drivers/net/nfb/nfb_rxmode.c |  12 +--
 drivers/net/nfb/nfb_stats.c  |  46 +++++-----
 drivers/net/nfb/nfb_tx.c     |   2 +-
 8 files changed, 152 insertions(+), 109 deletions(-)

--
2.52.0


^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v4 1/6] net/nfb: use constant values for max Rx/Tx queues count
  2026-01-21 17:01 ` [PATCH v4 " spinler
@ 2026-01-21 17:01   ` spinler
  2026-01-21 17:01   ` [PATCH v4 2/6] net/nfb: fix bad pointer access in queue stats spinler
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-21 17:01 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

The nb_xx_queues values in the dev->data structure can be modified
dynamically by some apps. Use runtime-constant values from hardware
directly for max_rx_queues/max_tx_queues.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  3 +++
 drivers/net/nfb/nfb_ethdev.c | 12 +++++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index a8b24ff822..917b830283 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -48,6 +48,9 @@ struct pmd_internals {
 	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
 	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
 	struct nfb_device *nfb;
+
+	uint16_t max_rx_queues;
+	uint16_t max_tx_queues;
 };
 
 #endif /* _NFB_H_ */
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 5f1b11a961..feba06a291 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -237,11 +237,13 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
+	struct pmd_internals *internals = dev->data->dev_private;
+
 	dev_info->max_mac_addrs = nfb_eth_get_max_mac_address_count(dev);
 
 	dev_info->max_rx_pktlen = (uint32_t)-1;
-	dev_info->max_rx_queues = dev->data->nb_rx_queues;
-	dev_info->max_tx_queues = dev->data->nb_tx_queues;
+	dev_info->max_rx_queues = internals->max_rx_queues;
+	dev_info->max_tx_queues = internals->max_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
@@ -538,11 +540,11 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
 		return -EINVAL;
 	}
-	data->nb_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
-	data->nb_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
+	internals->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
+	internals->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
 
 	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
-		data->nb_rx_queues, data->nb_tx_queues);
+		internals->max_rx_queues, internals->max_tx_queues);
 
 	nfb_nc_rxmac_init(internals->nfb,
 		internals->rxmac,
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 2/6] net/nfb: fix bad pointer access in queue stats
  2026-01-21 17:01 ` [PATCH v4 " spinler
  2026-01-21 17:01   ` [PATCH v4 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
@ 2026-01-21 17:01   ` spinler
  2026-01-21 17:01   ` [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value spinler
                     ` (4 subsequent siblings)
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-21 17:01 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

The driver code has dereferenced the dev->data->rx_queues pointer
without checking for its validity.
Pointer invalidation can occur when the eth_dev_rx_queue_config
is called with set to 0, for example.

Moreover, an array of pointers (to a structure) was used like array
of structures (which worked with early dereference just for one queue).

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_stats.c | 46 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/net/nfb/nfb_stats.c b/drivers/net/nfb/nfb_stats.c
index 4ea6b7be21..27a01c3160 100644
--- a/drivers/net/nfb/nfb_stats.c
+++ b/drivers/net/nfb/nfb_stats.c
@@ -20,28 +20,28 @@ nfb_eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
 	uint64_t rx_total_bytes = 0;
 	uint64_t tx_total_bytes = 0;
 
-	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
-		dev->data->rx_queues);
-	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
-		dev->data->tx_queues);
+	struct ndp_rx_queue *rx_queue;
+	struct ndp_tx_queue *tx_queue;
 
 	for (i = 0; i < nb_rx; i++) {
+		rx_queue = dev->data->rx_queues[i];
 		if (qstats && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			qstats->q_ipackets[i] = rx_queue[i].rx_pkts;
-			qstats->q_ibytes[i] = rx_queue[i].rx_bytes;
+			qstats->q_ipackets[i] = rx_queue->rx_pkts;
+			qstats->q_ibytes[i] = rx_queue->rx_bytes;
 		}
-		rx_total += rx_queue[i].rx_pkts;
-		rx_total_bytes += rx_queue[i].rx_bytes;
+		rx_total += rx_queue->rx_pkts;
+		rx_total_bytes += rx_queue->rx_bytes;
 	}
 
 	for (i = 0; i < nb_tx; i++) {
+		tx_queue = dev->data->tx_queues[i];
 		if (qstats && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			qstats->q_opackets[i] = tx_queue[i].tx_pkts;
-			qstats->q_obytes[i] = tx_queue[i].tx_bytes;
+			qstats->q_opackets[i] = tx_queue->tx_pkts;
+			qstats->q_obytes[i] = tx_queue->tx_bytes;
 		}
-		tx_total += tx_queue[i].tx_pkts;
-		tx_total_bytes += tx_queue[i].tx_bytes;
-		tx_err_total += tx_queue[i].err_pkts;
+		tx_total += tx_queue->tx_pkts;
+		tx_total_bytes += tx_queue->tx_bytes;
+		tx_err_total += tx_queue->err_pkts;
 	}
 
 	stats->ipackets = rx_total;
@@ -59,20 +59,20 @@ nfb_eth_stats_reset(struct rte_eth_dev *dev)
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 
-	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
-		dev->data->rx_queues);
-	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
-		dev->data->tx_queues);
+	struct ndp_rx_queue *rx_queue;
+	struct ndp_tx_queue *tx_queue;
 
 	for (i = 0; i < nb_rx; i++) {
-		rx_queue[i].rx_pkts = 0;
-		rx_queue[i].rx_bytes = 0;
-		rx_queue[i].err_pkts = 0;
+		rx_queue = dev->data->rx_queues[i];
+		rx_queue->rx_pkts = 0;
+		rx_queue->rx_bytes = 0;
+		rx_queue->err_pkts = 0;
 	}
 	for (i = 0; i < nb_tx; i++) {
-		tx_queue[i].tx_pkts = 0;
-		tx_queue[i].tx_bytes = 0;
-		tx_queue[i].err_pkts = 0;
+		tx_queue = dev->data->tx_queues[i];
+		tx_queue->tx_pkts = 0;
+		tx_queue->tx_bytes = 0;
+		tx_queue->err_pkts = 0;
 	}
 
 	return 0;
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value
  2026-01-21 17:01 ` [PATCH v4 " spinler
  2026-01-21 17:01   ` [PATCH v4 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
  2026-01-21 17:01   ` [PATCH v4 2/6] net/nfb: fix bad pointer access in queue stats spinler
@ 2026-01-21 17:01   ` spinler
  2026-01-21 17:33     ` Stephen Hemminger
  2026-01-27  0:34     ` Stephen Hemminger
  2026-01-21 17:01   ` [PATCH v4 4/6] net/nfb: use process private variable for internal data spinler
                     ` (3 subsequent siblings)
  6 siblings, 2 replies; 58+ messages in thread
From: spinler @ 2026-01-21 17:01 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

The resulting timestamp wasn't monotonically increasing value.

Now the timestamp is value in nanoseconds (Unix epoch).

Unfortunately there's currently no safe mechanism in nfb-framework
to get ticks from hardware to implement read_clock function.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 doc/guides/nics/nfb.rst  |  6 ++----
 drivers/net/nfb/nfb_rx.h | 13 +++++++------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/doc/guides/nics/nfb.rst b/doc/guides/nics/nfb.rst
index 96aaf64440..a9b4049654 100644
--- a/doc/guides/nics/nfb.rst
+++ b/doc/guides/nics/nfb.rst
@@ -64,10 +64,8 @@ products). The standard `RTE_ETH_RX_OFFLOAD_TIMESTAMP` flag can be used for this
 When the timestamps are enabled, a timestamp validity flag is set in the MBUFs
 containing received frames and timestamp is inserted into the `rte_mbuf` struct.
 
-The timestamp is an `uint64_t` field. Its lower 32 bits represent *seconds* portion of the timestamp
-(number of seconds elapsed since 1.1.1970 00:00:00 UTC) and its higher 32 bits represent
-*nanosecond* portion of the timestamp (number of nanoseconds elapsed since the beginning of the
-second in the *seconds* portion.
+The timestamp is an `uint64_t` field and holds the number of nanoseconds
+elapsed since 1.1.1970 00:00:00 UTC.
 
 
 Using the NFB PMD
diff --git a/drivers/net/nfb/nfb_rx.h b/drivers/net/nfb/nfb_rx.h
index 12769841ae..67b3b00e2a 100644
--- a/drivers/net/nfb/nfb_rx.h
+++ b/drivers/net/nfb/nfb_rx.h
@@ -13,6 +13,7 @@
 #include <rte_mbuf.h>
 #include <rte_mbuf_dyn.h>
 #include <rte_ethdev.h>
+#include <rte_time.h>
 
 #include "nfb.h"
 
@@ -202,15 +203,15 @@ nfb_eth_ndp_rx(void *queue,
 			if (nfb_timestamp_dynfield_offset >= 0) {
 				rte_mbuf_timestamp_t timestamp;
 
-				/* nanoseconds */
-				timestamp =
-					rte_le_to_cpu_32(*((uint32_t *)
-					(packets[i].header + 4)));
-				timestamp <<= 32;
 				/* seconds */
-				timestamp |=
+				timestamp =
 					rte_le_to_cpu_32(*((uint32_t *)
 					(packets[i].header + 8)));
+				timestamp *= NSEC_PER_SEC;
+				/* nanoseconds */
+				timestamp +=
+					rte_le_to_cpu_32(*((uint32_t *)
+					(packets[i].header + 4)));
 				*nfb_timestamp_dynfield(mbuf) = timestamp;
 				mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
 			}
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value
  2026-01-21 17:01   ` [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value spinler
@ 2026-01-21 17:33     ` Stephen Hemminger
  2026-01-27  8:12       ` Martin Spinler
  2026-01-27  0:34     ` Stephen Hemminger
  1 sibling, 1 reply; 58+ messages in thread
From: Stephen Hemminger @ 2026-01-21 17:33 UTC (permalink / raw)
  To: spinler; +Cc: dev

On Wed, 21 Jan 2026 18:01:17 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> The resulting timestamp wasn't monotonically increasing value.
> 
> Now the timestamp is value in nanoseconds (Unix epoch).
> 
> Unfortunately there's currently no safe mechanism in nfb-framework
> to get ticks from hardware to implement read_clock function.
> 
> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---
>  doc/guides/nics/nfb.rst  |  6 ++----
>  drivers/net/nfb/nfb_rx.h | 13 +++++++------
>  2 files changed, 9 insertions(+), 10 deletions(-)

The patch is ok as far as it goes.

But the best practice in DPDK for drivers doing timestamps is for
the driver to only put a timestamp on if timestamp offload flag
was set when configuring that device. For the case of multiple devices
that means that one device may have timestamp and other not.

Also any driver that supports receive timestamp must provide
a read_clock operation. Otherwise the application has no way of knowing
what units the timestamp is in. I added that to the AI review
script so new drivers will get rejected with out it.

I suspect that in this drivers case it is only used in systems where:
 - only single instance of device is possible
 - your application is just assuming a common unit like nanosecond.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value
  2026-01-21 17:33     ` Stephen Hemminger
@ 2026-01-27  8:12       ` Martin Spinler
  0 siblings, 0 replies; 58+ messages in thread
From: Martin Spinler @ 2026-01-27  8:12 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

On Wed, 2026-01-21 at 09:33 -0800, Stephen Hemminger wrote:
> The patch is ok as far as it goes.
> 
> But the best practice in DPDK for drivers doing timestamps is for
> the driver to only put a timestamp on if timestamp offload flag
> was set when configuring that device. For the case of multiple devices
> that means that one device may have timestamp and other not.

Ack. I'll work it in.

> Also any driver that supports receive timestamp must provide
> a read_clock operation. Otherwise the application has no way of knowing
> what units the timestamp is in. I added that to the AI review
> script so new drivers will get rejected with out it.

Ack.

> I suspect that in this drivers case it is only used in systems where:
>  - only single instance of device is possible
>  - your application is just assuming a common unit like nanosecond.

You're right.


More responses in another email thread.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value
  2026-01-21 17:01   ` [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value spinler
  2026-01-21 17:33     ` Stephen Hemminger
@ 2026-01-27  0:34     ` Stephen Hemminger
  2026-01-27  8:16       ` Martin Spinler
  1 sibling, 1 reply; 58+ messages in thread
From: Stephen Hemminger @ 2026-01-27  0:34 UTC (permalink / raw)
  To: spinler; +Cc: dev

On Wed, 21 Jan 2026 18:01:17 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> The resulting timestamp wasn't monotonically increasing value.
> 
> Now the timestamp is value in nanoseconds (Unix epoch).
> 
> Unfortunately there's currently no safe mechanism in nfb-framework
> to get ticks from hardware to implement read_clock function.
> 
> Signed-off-by: Martin Spinler <spinler@cesnet.cz>

While review this code, noticed some things that are related.

1. Why is nfb_eth_ndp_rx in a header file, it is only called one place.
   C code should be in the .c file.

2. It is marked as always inline, but since it is called by an indirect
   function pointer, dev->rx_burst it is never going to be inlined anyway.

3. This could be simplified.

	/* returns either all or nothing */
	i = rte_pktmbuf_alloc_bulk(ndp->mb_pool, mbufs, nb_pkts);
	if (unlikely(i != 0))
		return 0;

	/* returns either all or nothing */
	if (unlikely(rte_pktmbuf_alloc_bulk(ndp->mb_pool, mbufs, nb_pkts) != 0))
		return 0;

4. You are fighting against line length here:
			if (nfb_timestamp_dynfield_offset >= 0) {
				rte_mbuf_timestamp_t timestamp;

				/* seconds */
				timestamp =
					rte_le_to_cpu_32(*((uint32_t *)
					(packets[i].header + 8)));
				timestamp *= NSEC_PER_SEC;
				/* nanoseconds */
				timestamp +=
					rte_le_to_cpu_32(*((uint32_t *)
					(packets[i].header + 4)));
				*nfb_timestamp_dynfield(mbuf) = timestamp;
				mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
			}
   Either add a helper function expand since current DPDK max line length is 100.
   It looks like you are using pointer math rather than a structure.
   Also does the driver just use timespec? I.e is it safe in 2038 with a 64 bit seconds?

Something like?

struct nfb_meta {
       rte_le32_t rx_hisecs;
       rte_le32_t rx_losecs;
       rte_le32_t rx_nsecs;
};

static inline void
nfb_set_timestamp(struct rte_mbuf *mbuf, const void *header)
{
	const struct nfb_meta *hdr = header;
	rte_mbuf_timestamp_t timestamp;

	timestamp = (((uint64_t) rte_le_to_cpu_32(hdr->rx_hisecs) << 32) 
		    + (uint64_t) rte_le_to_cpu_32(hdr->rx_losec)) * NS_PER_SEC;
	timestamp += rte_le_to_cpu_32(hdr->rx_nsecs);

	*nfb_timestamp_dynfield(mbuf) = timestamp;
	mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
}

5. Using volatile for statistics is unnecessary and slows down
   code. Since only one thread can update them there is no need.
   It seems that lots of drivers cut-paste this off old code.
   The one exception would be if the driver supported TX lockfree
   but in that case it would need real atomics.

6. Driver should consider getting rid of variable length arrays.
   VLA's are a bug trap and a GNU extension. It is perfectly reasonable
   to have an upper bound on burst size of something like 64 bytes.

7. Driver could use rte_pktmbuf_free_bulk instead of loops in partial rx case.

8. offload flags should already be set correctly when mbufs are allocated.
   doing mbuf->ol_flags = 0 is not needed (but harmless).

9. You might consider using rte_pktmbuf_append() which would standardize
   update to mbuf and avoid the current situation where if packet received
   is bigger than space in the mbuf pool, the driver will corrupt the mbuf.

10. The driver probably haven't been tested with multiple ports.
    But the expected behavior is that timestamp is configure per-port.
    Most drivers have a flag and only add timestamp if it has been
    configured per-port.

Since driver is using time since 1/1/1970, you can just use UTC time
for read_clock. Except if there is no time sync between CPU and NIC.
Maybe just compute offset once on first packet received?

Transmit has seem inline madness.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value
  2026-01-27  0:34     ` Stephen Hemminger
@ 2026-01-27  8:16       ` Martin Spinler
  0 siblings, 0 replies; 58+ messages in thread
From: Martin Spinler @ 2026-01-27  8:16 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

On Mon, 2026-01-26 at 16:34 -0800, Stephen Hemminger wrote:
> On Wed, 21 Jan 2026 18:01:17 +0100
> spinler@cesnet.cz wrote:
> 
> > From: Martin Spinler <spinler@cesnet.cz>
> > 
> > The resulting timestamp wasn't monotonically increasing value.
> > 
> > Now the timestamp is value in nanoseconds (Unix epoch).
> > 
> > Unfortunately there's currently no safe mechanism in nfb-framework
> > to get ticks from hardware to implement read_clock function.
> > 
> > Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> 

Together with the comment in another email thread

> 5. **Patch 8**: Release notes mention timestamp changes not present in this series
> Good to address.

it seems, that the "[v4,3/6] net/nfb: update timestamp calculation to
meaningful value" from "net/nfb: code cleanup" series was my rash act,
as it resolves just one of many issues.
Would it be possible to revert back this series to "Change Requested"
state? I would remove just this one patch from series and resubmit. Or
can you suggest a better solution?


> While review this code, noticed some things that are related.
> 
> 1. Why is nfb_eth_ndp_rx in a header file, it is only called one place.
>    C code should be in the .c file.

1. I completely agree, I usually don't touch code that I don't need to
modify (except comprehensive cleaning), but I will address this issue
in one of the next series (concerning queues), which I already have
semi-prepared; there is some cleaning up anyway.

> 2. It is marked as always inline, but since it is called by an indirect
>    function pointer, dev->rx_burst it is never going to be inlined anyway.

2. I see, perhaps the original author hoped, that always inline would
be faster this way? :)

> 3. This could be simplified.
> 
> 	/* returns either all or nothing */
> 	i = rte_pktmbuf_alloc_bulk(ndp->mb_pool, mbufs, nb_pkts);
> 	if (unlikely(i != 0))
> 		return 0;
> 
> 	/* returns either all or nothing */
> 	if (unlikely(rte_pktmbuf_alloc_bulk(ndp->mb_pool, mbufs, nb_pkts) != 0))
> 		return 0;
> 4. You are fighting against line length here:
> 			if (nfb_timestamp_dynfield_offset >= 0) {
> 				rte_mbuf_timestamp_t timestamp;
> 
> 				/* seconds */
> 				timestamp =
> 					rte_le_to_cpu_32(*((uint32_t *)
> 					(packets[i].header + 8)));
> 				timestamp *= NSEC_PER_SEC;
> 				/* nanoseconds */
> 				timestamp +=
> 					rte_le_to_cpu_32(*((uint32_t *)
> 					(packets[i].header + 4)));
> 				*nfb_timestamp_dynfield(mbuf) = timestamp;
> 				mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
> 			}
>    Either add a helper function expand since current DPDK max line length is 100.

4. Agree, I needed/have a shared function in my next series (offloads
and new/alternative queue driver) already.

>    It looks like you are using pointer math rather than a structure.
>    Also does the driver just use timespec? I.e is it safe in 2038 with a 64 bit seconds?

Unfortunately the firmware is not prepared to Y2038: needs more thought
in our team to find the solution.


> Something like?
> 
> struct nfb_meta {
>        rte_le32_t rx_hisecs;
>        rte_le32_t rx_losecs;
>        rte_le32_t rx_nsecs;
> };

Makes more sense.

> 
> static inline void
> nfb_set_timestamp(struct rte_mbuf *mbuf, const void *header)
> {
> 	const struct nfb_meta *hdr = header;
> 	rte_mbuf_timestamp_t timestamp;
> 
> 	timestamp = (((uint64_t) rte_le_to_cpu_32(hdr->rx_hisecs) << 32) 
> 		    + (uint64_t) rte_le_to_cpu_32(hdr->rx_losec)) * NS_PER_SEC;
> 	timestamp += rte_le_to_cpu_32(hdr->rx_nsecs);
> 	
> 	*nfb_timestamp_dynfield(mbuf) = timestamp;
> 	mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
> }
> 
> 5. Using volatile for statistics is unnecessary and slows down
>    code. Since only one thread can update them there is no need.
>    It seems that lots of drivers cut-paste this off old code.
>    The one exception would be if the driver supported TX lockfree
>    but in that case it would need real atomics.
> 6. Driver should consider getting rid of variable length arrays.
>    VLA's are a bug trap and a GNU extension. It is perfectly reasonable
>    to have an upper bound on burst size of something like 64 bytes.
> 
> 7. Driver could use rte_pktmbuf_free_bulk instead of loops in partial rx case.
> 
> 8. offload flags should already be set correctly when mbufs are allocated.
>    doing mbuf->ol_flags = 0 is not needed (but harmless).
> 
> 9. You might consider using rte_pktmbuf_append() which would standardize
>    update to mbuf and avoid the current situation where if packet received
>    is bigger than space in the mbuf pool, the driver will corrupt the mbuf.
> 
> 10. The driver probably haven't been tested with multiple ports.
>     But the expected behavior is that timestamp is configure per-port.
>     Most drivers have a flag and only add timestamp if it has been
>     configured per-port.

10. you are right. I'm addressing that in other series.

> 
> Since driver is using time since 1/1/1970, you can just use UTC time
> for read_clock. Except if there is no time sync between CPU and NIC.
> Maybe just compute offset once on first packet received?

UTC time is good hint for read_clock. I think it should work quite
well. But I need to think it through/test more.
(There is separate NFB tool/daemon, which configures/synchronises the
Timestamp Unit inside Firmware and also selects between external PPS
and CPU time. The usage varies for different applications, but the
values are still in nsecs.)

> 
> Transmit has seem inline madness.
> 	

Thank you very much for all the other comments (3, 5, 6, 7 ,8, 9), they
are all relevant and I will try to incorporate them.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v4 4/6] net/nfb: use process private variable for internal data
  2026-01-21 17:01 ` [PATCH v4 " spinler
                     ` (2 preceding siblings ...)
  2026-01-21 17:01   ` [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value spinler
@ 2026-01-21 17:01   ` spinler
  2026-01-21 17:01   ` [PATCH v4 5/6] net/nfb: release allocated resources correctly spinler
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-21 17:01 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

Internal structures of libnfb can't be shared between processes.
Move these structures from dev_private to process_private, which allows
secondary process to correctly initialize and uninitialize the eth_dev.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  11 +++-
 drivers/net/nfb/nfb_ethdev.c | 103 +++++++++++++++++++++--------------
 drivers/net/nfb/nfb_rx.c     |   2 +-
 drivers/net/nfb/nfb_rxmode.c |  12 ++--
 drivers/net/nfb/nfb_tx.c     |   2 +-
 5 files changed, 79 insertions(+), 51 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index 917b830283..90b04c6151 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -41,14 +41,23 @@ extern int nfb_logtype;
 
 #define RTE_NFB_DRIVER_NAME net_nfb
 
-
+/*
+ * Handles obtained from the libnfb: each process must use own instance.
+ * Stored inside dev->process_private.
+ */
 struct pmd_internals {
 	uint16_t         max_rxmac;
 	uint16_t         max_txmac;
 	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
 	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
 	struct nfb_device *nfb;
+};
 
+/*
+ * Common data, single instance usable in all processes.
+ * Inited in the RTE_PROC_PRIMARY, stored in dev->data->dev_private.
+ */
+struct pmd_priv {
 	uint16_t max_rx_queues;
 	uint16_t max_tx_queues;
 };
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index feba06a291..7dbdde2715 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -184,7 +184,7 @@ static int
 nfb_eth_dev_configure(struct rte_eth_dev *dev)
 {
 	int ret;
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
 
 	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
@@ -207,7 +207,7 @@ nfb_eth_get_max_mac_address_count(struct rte_eth_dev *dev)
 	uint16_t i;
 	uint32_t c;
 	uint32_t ret = (uint32_t)-1;
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	/*
 	 * Go through all RX MAC components in firmware and find
@@ -237,13 +237,13 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_priv *priv = dev->data->dev_private;
 
 	dev_info->max_mac_addrs = nfb_eth_get_max_mac_address_count(dev);
 
 	dev_info->max_rx_pktlen = (uint32_t)-1;
-	dev_info->max_rx_queues = internals->max_rx_queues;
-	dev_info->max_tx_queues = internals->max_tx_queues;
+	dev_info->max_rx_queues = priv->max_rx_queues;
+	dev_info->max_tx_queues = priv->max_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
@@ -262,20 +262,20 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 static int
 nfb_eth_dev_close(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	uint16_t i;
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 	int ret;
 
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
 	ret = nfb_eth_dev_stop(dev);
 
-	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
-	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
-
 	for (i = 0; i < nb_rx; i++) {
 		nfb_eth_rx_queue_release(dev, i);
 		dev->data->rx_queues[i] = NULL;
@@ -310,7 +310,7 @@ nfb_eth_link_update(struct rte_eth_dev *dev,
 	struct rte_eth_link link;
 	memset(&link, 0, sizeof(link));
 
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	status.speed = MAC_SPEED_UNKNOWN;
 
@@ -365,7 +365,7 @@ static int
 nfb_eth_dev_set_link_up(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -390,7 +390,7 @@ static int
 nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -431,9 +431,8 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
 {
 	unsigned int i;
 	uint64_t mac;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	mac = nfb_eth_mac_addr_conv(mac_addr);
 	/* Until no real multi-port support, configure all RX MACs the same */
@@ -449,9 +448,8 @@ nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
 {
 	unsigned int i;
 	uint64_t mac;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	mac = nfb_eth_mac_addr_conv(mac_addr);
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -464,9 +462,8 @@ static void
 nfb_eth_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
 {
 	unsigned int i;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	for (i = 0; i < internals->max_rxmac; ++i)
 		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
@@ -512,19 +509,31 @@ static const struct eth_dev_ops ops = {
 static int
 nfb_eth_dev_init(struct rte_eth_dev *dev)
 {
+	int ret;
 	uint32_t mac_count;
 	struct rte_eth_dev_data *data = dev->data;
-	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+	struct pmd_internals *internals;
+	struct pmd_priv *priv = data->dev_private;
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
 	struct rte_ether_addr eth_addr_init;
+	uint16_t max_rx_queues, max_tx_queues;
 	char nfb_dev[PATH_MAX];
 
 	NFB_LOG(INFO, "Initializing NFB device (" PCI_PRI_FMT ")",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
 
+	internals = rte_zmalloc_socket("nfb_internals",
+			sizeof(struct pmd_internals), RTE_CACHE_LINE_SIZE,
+			dev->device->numa_node);
+	if (internals == NULL) {
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	dev->process_private = internals;
+
 	snprintf(nfb_dev, sizeof(nfb_dev),
 		"/dev/nfb/by-pci-slot/" PCI_PRI_FMT,
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
@@ -538,13 +547,14 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	internals->nfb = nfb_open(nfb_dev);
 	if (internals->nfb == NULL) {
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
+		rte_free(internals);
 		return -EINVAL;
 	}
-	internals->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
-	internals->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
+	max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
+	max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
 
 	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
-		internals->max_rx_queues, internals->max_tx_queues);
+		max_rx_queues, max_tx_queues);
 
 	nfb_nc_rxmac_init(internals->nfb,
 		internals->rxmac,
@@ -563,28 +573,34 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	/* Get link state */
 	nfb_eth_link_update(dev, 0);
 
-	/* Allocate space for MAC addresses */
-	mac_count = nfb_eth_get_max_mac_address_count(dev);
-	data->mac_addrs = rte_zmalloc(data->name,
-		sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
-	if (data->mac_addrs == NULL) {
-		NFB_LOG(ERR, "Could not alloc space for MAC address");
-		nfb_close(internals->nfb);
-		return -EINVAL;
-	}
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		priv->max_rx_queues = max_rx_queues;
+		priv->max_tx_queues = max_tx_queues;
 
-	rte_eth_random_addr(eth_addr_init.addr_bytes);
-	eth_addr_init.addr_bytes[0] = eth_addr.addr_bytes[0];
-	eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
-	eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
+		/* Allocate space for MAC addresses */
+		mac_count = nfb_eth_get_max_mac_address_count(dev);
+		data->mac_addrs = rte_zmalloc(data->name,
+			sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
+		if (data->mac_addrs == NULL) {
+			NFB_LOG(ERR, "Could not alloc space for MAC address");
+			nfb_close(internals->nfb);
+			rte_free(internals);
+			return -EINVAL;
+		}
 
-	nfb_eth_mac_addr_set(dev, &eth_addr_init);
-	rte_ether_addr_copy(&eth_addr_init, &dev->data->mac_addrs[0]);
+		rte_eth_random_addr(eth_addr_init.addr_bytes);
+		eth_addr_init.addr_bytes[0] = eth_addr.addr_bytes[0];
+		eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
+		eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
 
-	data->promiscuous = nfb_eth_promiscuous_get(dev);
-	data->all_multicast = nfb_eth_allmulticast_get(dev);
+		nfb_eth_mac_addr_set(dev, &eth_addr_init);
+		rte_ether_addr_copy(&eth_addr_init, &data->mac_addrs[0]);
 
-	dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
+		data->promiscuous = nfb_eth_promiscuous_get(dev);
+		data->all_multicast = nfb_eth_allmulticast_get(dev);
+
+		data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
+	}
 
 	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully initialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
@@ -607,9 +623,12 @@ nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 {
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
+	struct pmd_internals *internals = dev->process_private;
 
 	nfb_eth_dev_close(dev);
 
+	rte_free(internals);
+
 	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully uninitialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
@@ -644,7 +663,7 @@ nfb_eth_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		struct rte_pci_device *pci_dev)
 {
 	return rte_eth_dev_pci_generic_probe(pci_dev,
-		sizeof(struct pmd_internals), nfb_eth_dev_init);
+		sizeof(struct pmd_priv), nfb_eth_dev_init);
 }
 
 /**
diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
index 462bc3b50d..413d275853 100644
--- a/drivers/net/nfb/nfb_rx.c
+++ b/drivers/net/nfb/nfb_rx.c
@@ -60,7 +60,7 @@ nfb_eth_rx_queue_setup(struct rte_eth_dev *dev,
 		const struct rte_eth_rxconf *rx_conf __rte_unused,
 		struct rte_mempool *mb_pool)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	struct ndp_rx_queue *rxq;
 	int ret;
diff --git a/drivers/net/nfb/nfb_rxmode.c b/drivers/net/nfb/nfb_rxmode.c
index ca6e4d5578..dc560d638b 100644
--- a/drivers/net/nfb/nfb_rxmode.c
+++ b/drivers/net/nfb/nfb_rxmode.c
@@ -11,7 +11,7 @@ int
 nfb_eth_promiscuous_enable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 	uint16_t i;
 
 	for (i = 0; i < internals->max_rxmac; ++i) {
@@ -26,7 +26,7 @@ int
 nfb_eth_promiscuous_disable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 	uint16_t i;
 	enum nc_rxmac_mac_filter filter = RXMAC_MAC_FILTER_TABLE_BCAST;
 
@@ -44,7 +44,7 @@ int
 nfb_eth_promiscuous_get(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	struct nc_rxmac_status status;
 	status.mac_filter = RXMAC_MAC_FILTER_PROMISCUOUS;
@@ -59,7 +59,7 @@ int
 nfb_eth_allmulticast_enable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	if (dev->data->promiscuous)
@@ -76,7 +76,7 @@ int
 nfb_eth_allmulticast_disable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 
@@ -95,7 +95,7 @@ int
 nfb_eth_allmulticast_get(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	struct nc_rxmac_status status;
 	status.mac_filter = RXMAC_MAC_FILTER_PROMISCUOUS;
diff --git a/drivers/net/nfb/nfb_tx.c b/drivers/net/nfb/nfb_tx.c
index cf99268c43..1f997ce22f 100644
--- a/drivers/net/nfb/nfb_tx.c
+++ b/drivers/net/nfb/nfb_tx.c
@@ -53,7 +53,7 @@ nfb_eth_tx_queue_setup(struct rte_eth_dev *dev,
 	unsigned int socket_id,
 	const struct rte_eth_txconf *tx_conf __rte_unused)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	int ret;
 	struct ndp_tx_queue *txq;
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 5/6] net/nfb: release allocated resources correctly
  2026-01-21 17:01 ` [PATCH v4 " spinler
                     ` (3 preceding siblings ...)
  2026-01-21 17:01   ` [PATCH v4 4/6] net/nfb: use process private variable for internal data spinler
@ 2026-01-21 17:01   ` spinler
  2026-01-21 17:01   ` [PATCH v4 6/6] net/nfb: stop only started queues in fail path spinler
  2026-01-21 17:35   ` [PATCH v4 0/6] net/nfb: code cleanup Stephen Hemminger
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-21 17:01 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

Internal handles are initialized in eth_dev_init, so the cleanup
should be done in eth_dev_uninit.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 74 ++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 32 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 7dbdde2715..abc5c12b1e 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -18,6 +18,8 @@
 #include "nfb_rxmode.h"
 #include "nfb.h"
 
+static int nfb_eth_dev_uninit(struct rte_eth_dev *dev);
+
 /**
  * Default MAC addr
  */
@@ -160,8 +162,6 @@ nfb_eth_dev_stop(struct rte_eth_dev *dev)
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 
-	dev->data->dev_started = 0;
-
 	for (i = 0; i < nb_tx; i++)
 		nfb_eth_tx_queue_stop(dev, i);
 
@@ -184,7 +184,6 @@ static int
 nfb_eth_dev_configure(struct rte_eth_dev *dev)
 {
 	int ret;
-	struct pmd_internals *internals = dev->process_private;
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
 
 	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
@@ -193,12 +192,16 @@ nfb_eth_dev_configure(struct rte_eth_dev *dev)
 				&nfb_timestamp_rx_dynflag);
 		if (ret != 0) {
 			NFB_LOG(ERR, "Cannot register Rx timestamp field/flag %d", ret);
-			nfb_close(internals->nfb);
-			return -rte_errno;
+			ret = -ENOMEM;
+			goto err_ts_register;
 		}
 	}
 
 	return 0;
+
+err_ts_register:
+	nfb_eth_dev_uninit(dev);
+	return ret;
 }
 
 static uint32_t
@@ -262,32 +265,28 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 static int
 nfb_eth_dev_close(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *internals = dev->process_private;
 	uint16_t i;
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
-	int ret;
 
-	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
-	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
-
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
-	ret = nfb_eth_dev_stop(dev);
-
-	for (i = 0; i < nb_rx; i++) {
-		nfb_eth_rx_queue_release(dev, i);
-		dev->data->rx_queues[i] = NULL;
-	}
-	dev->data->nb_rx_queues = 0;
-	for (i = 0; i < nb_tx; i++) {
-		nfb_eth_tx_queue_release(dev, i);
-		dev->data->tx_queues[i] = NULL;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		for (i = 0; i < nb_rx; i++) {
+			if (dev->data->rx_queues[i]) {
+				nfb_eth_rx_queue_release(dev, i);
+				dev->data->rx_queues[i] = NULL;
+			}
+		}
+		for (i = 0; i < nb_tx; i++) {
+			if (dev->data->tx_queues[i]) {
+				nfb_eth_tx_queue_release(dev, i);
+				dev->data->tx_queues[i] = NULL;
+			}
+		}
 	}
-	dev->data->nb_tx_queues = 0;
 
-	return ret;
+	nfb_eth_dev_uninit(dev);
+
+	return 0;
 }
 
 /**
@@ -529,7 +528,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 			dev->device->numa_node);
 	if (internals == NULL) {
 		ret = -ENOMEM;
-		return ret;
+		goto err_alloc_internals;
 	}
 
 	dev->process_private = internals;
@@ -547,8 +546,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	internals->nfb = nfb_open(nfb_dev);
 	if (internals->nfb == NULL) {
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
-		rte_free(internals);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_nfb_open;
 	}
 	max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
 	max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
@@ -583,9 +582,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 			sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
 		if (data->mac_addrs == NULL) {
 			NFB_LOG(ERR, "Could not alloc space for MAC address");
-			nfb_close(internals->nfb);
-			rte_free(internals);
-			return -EINVAL;
+			ret = -ENOMEM;
+			goto err_malloc_mac_addrs;
 		}
 
 		rte_eth_random_addr(eth_addr_init.addr_bytes);
@@ -607,6 +605,16 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		pci_addr->function);
 
 	return 0;
+
+err_malloc_mac_addrs:
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+	nfb_close(internals->nfb);
+
+err_nfb_open:
+	rte_free(internals);
+err_alloc_internals:
+	return ret;
 }
 
 /**
@@ -625,7 +633,9 @@ nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
 	struct pmd_internals *internals = dev->process_private;
 
-	nfb_eth_dev_close(dev);
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+	nfb_close(internals->nfb);
 
 	rte_free(internals);
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 6/6] net/nfb: stop only started queues in fail path
  2026-01-21 17:01 ` [PATCH v4 " spinler
                     ` (4 preceding siblings ...)
  2026-01-21 17:01   ` [PATCH v4 5/6] net/nfb: release allocated resources correctly spinler
@ 2026-01-21 17:01   ` spinler
  2026-01-21 17:35   ` [PATCH v4 0/6] net/nfb: code cleanup Stephen Hemminger
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-01-21 17:01 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

The driver should stop only running queues in case of error
inside eth_dev_start.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index abc5c12b1e..947ee9e21d 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -139,11 +139,12 @@ nfb_eth_dev_start(struct rte_eth_dev *dev)
 	return 0;
 
 err_tx:
-	for (i = 0; i < nb_tx; i++)
-		nfb_eth_tx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_tx_queue_stop(dev, i - 1);
+	i = nb_rx;
 err_rx:
-	for (i = 0; i < nb_rx; i++)
-		nfb_eth_rx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_rx_queue_stop(dev, i - 1);
 	return ret;
 }
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v4 0/6] net/nfb: code cleanup
  2026-01-21 17:01 ` [PATCH v4 " spinler
                     ` (5 preceding siblings ...)
  2026-01-21 17:01   ` [PATCH v4 6/6] net/nfb: stop only started queues in fail path spinler
@ 2026-01-21 17:35   ` Stephen Hemminger
  6 siblings, 0 replies; 58+ messages in thread
From: Stephen Hemminger @ 2026-01-21 17:35 UTC (permalink / raw)
  To: spinler; +Cc: dev

On Wed, 21 Jan 2026 18:01:14 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> This patchset mainly cleans up the code and prepare it for another
> quite large rework. Also it resolves some unpleasant behavior.
> 
> ---
> v4:
> * Added comment for process private/shared data.
> * Added Fixes+Cc tag for patch 4,5,6
> * Moved shared data assignments to RTE_PROC_PRIMARY block
> 
> v3:
> * Changed type of queue count variable to uint16_t.
> * Fixed wrong queue count variable usage in log.
> * Fixed another coding style issues.
> 
> v2:
> * Fixed coding style issues.
> 
> 
> Martin Spinler (6):
>   net/nfb: use constant values for max Rx/Tx queues count
>   net/nfb: fix bad pointer access in queue stats
>   net/nfb: update timestamp calculation to meaningful value
>   net/nfb: use process private variable for internal data
>   net/nfb: release allocated resources correctly
>   net/nfb: stop only started queues in fail path
> 
>  doc/guides/nics/nfb.rst      |   6 +-
>  drivers/net/nfb/nfb.h        |  14 ++-
>  drivers/net/nfb/nfb_ethdev.c | 166 +++++++++++++++++++++--------------
>  drivers/net/nfb/nfb_rx.c     |   2 +-
>  drivers/net/nfb/nfb_rx.h     |  13 +--
>  drivers/net/nfb/nfb_rxmode.c |  12 +--
>  drivers/net/nfb/nfb_stats.c  |  46 +++++-----
>  drivers/net/nfb/nfb_tx.c     |   2 +-
>  8 files changed, 152 insertions(+), 109 deletions(-)
> 
> --
> 2.52.0
> 

Queued to next-net

Verbose AI patch review (if you care) mostly it wants more Fixes: tags.


## NFB Driver Patch Series v4 Review

### Series Overview

This is a 6-patch series fixing multiple bugs in the NFB driver:
1. Use constant values for max Rx/Tx queues count
2. Fix bad pointer access in queue stats  
3. Update timestamp calculation to meaningful value
4. Use process private variable for internal data
5. Release allocated resources correctly
6. Stop only started queues in fail path

---

### Patch 1/6: `net/nfb: use constant values for max Rx/Tx queues count`

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 54 characters |
| Lowercase after colon | ✅ PASS | |
| Correct prefix (`net/nfb:`) | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| No trailing period | ✅ PASS | |
| Body ≤75 chars/line | ✅ PASS | |
| Body doesn't start with "It" | ⚠️ WARNING | Starts with "The" - acceptable but close |
| `Signed-off-by:` present | ✅ PASS | Valid name and email |
| `Fixes:` tag present | ✅ PASS | 12-char SHA with quoted subject |
| `Cc: stable@dpdk.org` | ✅ PASS | |
| Tag order | ✅ PASS | Fixes, then Cc, then blank, then Signed-off-by |

**Code Review:**
- Clean change, stores max queue counts in the `pmd_internals` structure
- Properly uses the hardware-reported counts instead of the dynamic `data->nb_*_queues` values
- No style issues detected

**Verdict: ✅ Ready for merge**

---

### Patch 2/6: `net/nfb: fix bad pointer access in queue stats`

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 46 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| `Signed-off-by:` present | ✅ PASS | |
| `Fixes:` tag | ✅ PASS | |
| `Cc: stable@dpdk.org` | ✅ PASS | |

**Code Review:**
- Fixes incorrect array-of-structures vs array-of-pointers dereference
- The fix correctly iterates through the queue pointers
- Moves queue pointer fetching inside the loop

**Verdict: ✅ Ready for merge**

---

### Patch 3/6: `net/nfb: update timestamp calculation to meaningful value`

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 53 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| `Signed-off-by:` present | ✅ PASS | |
| `Fixes:` tag | ❌ MISSING | This changes timestamp semantics - is it a bug fix? |
| `Cc: stable@dpdk.org` | ❌ MISSING | If this is a behavioral fix, it should have these |

**Code Review:**
- Changes timestamp from packed seconds:nanoseconds format to nanoseconds since epoch
- Documentation updated atomically - good
- Uses `NSEC_PER_SEC` from `<rte_time.h>` - appropriate

**Issue:** The commit body states "The resulting timestamp wasn't monotonically increasing value" which suggests this IS a bug fix. If so, it should have:
```
Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org
```

**Verdict: ⚠️ Needs revision** - Missing Fixes/Cc tags if this is indeed a bug fix

---

### Patch 4/6: `net/nfb: use process private variable for internal data`

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 56 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| Body ≤75 chars/line | ✅ PASS | |
| `Signed-off-by:` present | ✅ PASS | |
| `Fixes:` tag | ✅ PASS | |
| `Cc: stable@dpdk.org` | ✅ PASS | |

**Code Review:**
- Properly separates process-private (libnfb handles) from shared data
- Introduces new `struct pmd_priv` for shared data (`max_rx_queues`, `max_tx_queues`)
- `struct pmd_internals` now contains process-local handles
- Comments added explaining the purpose of each structure - good

**Style note:** The new struct comment style is good:
```c
/*
 * Handles obtained from the libnfb: each process must use own instance.
 * Stored inside dev->process_private.
 */
```

**Verdict: ✅ Ready for merge**

---

### Patch 5/6: `net/nfb: release allocated resources correctly`

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 47 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| `Signed-off-by:` present | ✅ PASS | |
| `Fixes:` tag | ✅ PASS | |
| `Cc: stable@dpdk.org` | ✅ PASS | |

**Code Review:**
- Refactors error handling in `nfb_eth_dev_init()` to use goto-style cleanup
- Properly sequences resource cleanup:
  - `err_ts_register` → calls `nfb_eth_dev_uninit()`
  - `err_malloc_mac_addrs` → deinits rxmac/txmac, closes nfb
  - `err_nfb_open` → frees internals
  - `err_alloc_internals` → returns error
- Moves rxmac/txmac deinit and nfb_close from `close()` to `uninit()`
- Adds forward declaration for `nfb_eth_dev_uninit()`

**Minor observation:** The refactored `nfb_eth_dev_close()` properly handles queue cleanup with NULL checks, which is good defensive programming.

**Verdict: ✅ Ready for merge**

---

### Patch 6/6: `net/nfb: stop only started queues in fail path`

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 48 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| `Signed-off-by:` present | ✅ PASS | |
| `Fixes:` tag | ✅ PASS | |
| `Cc: stable@dpdk.org` | ✅ PASS | |

**Code Review:**
```c
err_tx:
-	for (i = 0; i < nb_tx; i++)
-		nfb_eth_tx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_tx_queue_stop(dev, i - 1);
+	i = nb_rx;
err_rx:
-	for (i = 0; i < nb_rx; i++)
-		nfb_eth_rx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_rx_queue_stop(dev, i - 1);
```

- Properly stops only the queues that were successfully started
- Uses the loop counter `i` which indicates where the failure occurred
- The `i = nb_rx` assignment before `err_rx` ensures all Rx queues are stopped when coming from `err_tx`

**Verdict: ✅ Ready for merge**

---

## Summary

| Patch | Status | Action Required |
|-------|--------|-----------------|
| 1/6 | ✅ Ready | None |
| 2/6 | ✅ Ready | None |
| 3/6 | ⚠️ Warning | Add `Fixes:` and `Cc: stable@dpdk.org` if this is a bug fix |
| 4/6 | ✅ Ready | None |
| 5/6 | ✅ Ready | None |
| 6/6 | ✅ Ready | None |

### Recommendation

The series is well-structured with proper inter-patch dependencies. The v4 revision addresses the issue from earlier reviews by properly separating process-private and shared data.

**For patch 3/6:** The commit message describes non-monotonic timestamp behavior as a defect. Please either add the `Fixes:` and `Cc: stable@dpdk.org` tags, or clarify in the commit message that this is an intentional behavioral change rather than a bug fix.

With that clarification/fix, this series is ready for merge.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v5 0/6] net/nfb: code cleanup
  2026-01-15 14:01 [PATCH 0/6] spinler
                   ` (9 preceding siblings ...)
  2026-01-21 17:01 ` [PATCH v4 " spinler
@ 2026-02-02 19:33 ` spinler
  2026-02-02 19:33   ` [PATCH v5 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
                     ` (6 more replies)
  10 siblings, 7 replies; 58+ messages in thread
From: spinler @ 2026-02-02 19:33 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

This patchset mainly cleans up the code and prepare it for another
quite large rework. Also it resolves some unpleasant behavior.

---
v5:
* Rebased to next-net-main

Martin Spinler (6):
  net/nfb: use constant values for max Rx/Tx queues count
  net/nfb: fix bad pointer access in queue stats
  net/nfb: update timestamp calculation to meaningful value
  net/nfb: use process private variable for internal data
  net/nfb: release allocated resources correctly
  net/nfb: stop only started queues in fail path

 doc/guides/nics/nfb.rst      |   6 +-
 drivers/net/nfb/nfb.h        |  14 ++-
 drivers/net/nfb/nfb_ethdev.c | 166 +++++++++++++++++++++--------------
 drivers/net/nfb/nfb_rx.c     |   2 +-
 drivers/net/nfb/nfb_rx.h     |  13 +--
 drivers/net/nfb/nfb_rxmode.c |  12 +--
 drivers/net/nfb/nfb_stats.c  |  46 +++++-----
 drivers/net/nfb/nfb_tx.c     |   2 +-
 8 files changed, 152 insertions(+), 109 deletions(-)

-- 
2.52.0


^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v5 1/6] net/nfb: use constant values for max Rx/Tx queues count
  2026-02-02 19:33 ` [PATCH v5 " spinler
@ 2026-02-02 19:33   ` spinler
  2026-02-02 19:33   ` [PATCH v5 2/6] net/nfb: fix bad pointer access in queue stats spinler
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-02-02 19:33 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

The nb_xx_queues values in the dev->data structure can be modified
dynamically by some apps. Use runtime-constant values from hardware
directly for max_rx_queues/max_tx_queues.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  3 +++
 drivers/net/nfb/nfb_ethdev.c | 12 +++++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index a8b24ff822..917b830283 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -48,6 +48,9 @@ struct pmd_internals {
 	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
 	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
 	struct nfb_device *nfb;
+
+	uint16_t max_rx_queues;
+	uint16_t max_tx_queues;
 };
 
 #endif /* _NFB_H_ */
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 98119d70fd..5b1df83b7f 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -237,11 +237,13 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
+	struct pmd_internals *internals = dev->data->dev_private;
+
 	dev_info->max_mac_addrs = nfb_eth_get_max_mac_address_count(dev);
 
 	dev_info->max_rx_pktlen = (uint32_t)-1;
-	dev_info->max_rx_queues = dev->data->nb_rx_queues;
-	dev_info->max_tx_queues = dev->data->nb_tx_queues;
+	dev_info->max_rx_queues = internals->max_rx_queues;
+	dev_info->max_tx_queues = internals->max_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
@@ -538,11 +540,11 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
 		return -EINVAL;
 	}
-	data->nb_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
-	data->nb_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
+	internals->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
+	internals->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
 
 	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
-		data->nb_rx_queues, data->nb_tx_queues);
+		internals->max_rx_queues, internals->max_tx_queues);
 
 	nfb_nc_rxmac_init(internals->nfb,
 		internals->rxmac,
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v5 2/6] net/nfb: fix bad pointer access in queue stats
  2026-02-02 19:33 ` [PATCH v5 " spinler
  2026-02-02 19:33   ` [PATCH v5 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
@ 2026-02-02 19:33   ` spinler
  2026-02-10  0:51     ` Stephen Hemminger
  2026-02-02 19:33   ` [PATCH v5 3/6] net/nfb: update timestamp calculation to meaningful value spinler
                     ` (4 subsequent siblings)
  6 siblings, 1 reply; 58+ messages in thread
From: spinler @ 2026-02-02 19:33 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

The driver code has dereferenced the dev->data->rx_queues pointer
without checking for its validity.
Pointer invalidation can occur when the eth_dev_rx_queue_config
is called with set to 0, for example.

Moreover, an array of pointers (to a structure) was used like array
of structures (which worked with early dereference just for one queue).

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_stats.c | 46 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/net/nfb/nfb_stats.c b/drivers/net/nfb/nfb_stats.c
index 4ea6b7be21..27a01c3160 100644
--- a/drivers/net/nfb/nfb_stats.c
+++ b/drivers/net/nfb/nfb_stats.c
@@ -20,28 +20,28 @@ nfb_eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
 	uint64_t rx_total_bytes = 0;
 	uint64_t tx_total_bytes = 0;
 
-	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
-		dev->data->rx_queues);
-	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
-		dev->data->tx_queues);
+	struct ndp_rx_queue *rx_queue;
+	struct ndp_tx_queue *tx_queue;
 
 	for (i = 0; i < nb_rx; i++) {
+		rx_queue = dev->data->rx_queues[i];
 		if (qstats && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			qstats->q_ipackets[i] = rx_queue[i].rx_pkts;
-			qstats->q_ibytes[i] = rx_queue[i].rx_bytes;
+			qstats->q_ipackets[i] = rx_queue->rx_pkts;
+			qstats->q_ibytes[i] = rx_queue->rx_bytes;
 		}
-		rx_total += rx_queue[i].rx_pkts;
-		rx_total_bytes += rx_queue[i].rx_bytes;
+		rx_total += rx_queue->rx_pkts;
+		rx_total_bytes += rx_queue->rx_bytes;
 	}
 
 	for (i = 0; i < nb_tx; i++) {
+		tx_queue = dev->data->tx_queues[i];
 		if (qstats && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			qstats->q_opackets[i] = tx_queue[i].tx_pkts;
-			qstats->q_obytes[i] = tx_queue[i].tx_bytes;
+			qstats->q_opackets[i] = tx_queue->tx_pkts;
+			qstats->q_obytes[i] = tx_queue->tx_bytes;
 		}
-		tx_total += tx_queue[i].tx_pkts;
-		tx_total_bytes += tx_queue[i].tx_bytes;
-		tx_err_total += tx_queue[i].err_pkts;
+		tx_total += tx_queue->tx_pkts;
+		tx_total_bytes += tx_queue->tx_bytes;
+		tx_err_total += tx_queue->err_pkts;
 	}
 
 	stats->ipackets = rx_total;
@@ -59,20 +59,20 @@ nfb_eth_stats_reset(struct rte_eth_dev *dev)
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 
-	struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
-		dev->data->rx_queues);
-	struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
-		dev->data->tx_queues);
+	struct ndp_rx_queue *rx_queue;
+	struct ndp_tx_queue *tx_queue;
 
 	for (i = 0; i < nb_rx; i++) {
-		rx_queue[i].rx_pkts = 0;
-		rx_queue[i].rx_bytes = 0;
-		rx_queue[i].err_pkts = 0;
+		rx_queue = dev->data->rx_queues[i];
+		rx_queue->rx_pkts = 0;
+		rx_queue->rx_bytes = 0;
+		rx_queue->err_pkts = 0;
 	}
 	for (i = 0; i < nb_tx; i++) {
-		tx_queue[i].tx_pkts = 0;
-		tx_queue[i].tx_bytes = 0;
-		tx_queue[i].err_pkts = 0;
+		tx_queue = dev->data->tx_queues[i];
+		tx_queue->tx_pkts = 0;
+		tx_queue->tx_bytes = 0;
+		tx_queue->err_pkts = 0;
 	}
 
 	return 0;
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v5 2/6] net/nfb: fix bad pointer access in queue stats
  2026-02-02 19:33   ` [PATCH v5 2/6] net/nfb: fix bad pointer access in queue stats spinler
@ 2026-02-10  0:51     ` Stephen Hemminger
  0 siblings, 0 replies; 58+ messages in thread
From: Stephen Hemminger @ 2026-02-10  0:51 UTC (permalink / raw)
  To: spinler; +Cc: dev, stable

On Mon,  2 Feb 2026 20:33:26 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> The driver code has dereferenced the dev->data->rx_queues pointer
> without checking for its validity.
> Pointer invalidation can occur when the eth_dev_rx_queue_config
> is called with set to 0, for example.
> 
> Moreover, an array of pointers (to a structure) was used like array
> of structures (which worked with early dereference just for one queue).
> 
> Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---

AI found this potential issue:

ERRORS (Must Fix)
Patch 26: net/nfb: fix bad pointer access in queue stats

NULL pointer dereference risk

In nfb_eth_stats_get() and nfb_eth_stats_reset(), the patch correctly fixes the array-of-pointers vs array-of-structures bug, but introduces a new issue: it dereferences dev->data->rx_queues[i] and dev->data->tx_queues[i] without NULL checks.

The queues array can contain NULL pointers if a queue is not configured. The original buggy code had an early dereference that would have caught this, but the fixed version will crash on the first access to rx_queue->rx_pkts if the queue pointer is NULL.

I added a simple check, since fixing it takes less time than another patch cycle...

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v5 3/6] net/nfb: update timestamp calculation to meaningful value
  2026-02-02 19:33 ` [PATCH v5 " spinler
  2026-02-02 19:33   ` [PATCH v5 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
  2026-02-02 19:33   ` [PATCH v5 2/6] net/nfb: fix bad pointer access in queue stats spinler
@ 2026-02-02 19:33   ` spinler
  2026-02-02 19:33   ` [PATCH v5 4/6] net/nfb: use process private variable for internal data spinler
                     ` (3 subsequent siblings)
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-02-02 19:33 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler

From: Martin Spinler <spinler@cesnet.cz>

The resulting timestamp wasn't monotonically increasing value.

Now the timestamp is value in nanoseconds (Unix epoch).

Unfortunately there's currently no safe mechanism in nfb-framework
to get ticks from hardware to implement read_clock function.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 doc/guides/nics/nfb.rst  |  6 ++----
 drivers/net/nfb/nfb_rx.h | 13 +++++++------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/doc/guides/nics/nfb.rst b/doc/guides/nics/nfb.rst
index 96aaf64440..a9b4049654 100644
--- a/doc/guides/nics/nfb.rst
+++ b/doc/guides/nics/nfb.rst
@@ -64,10 +64,8 @@ products). The standard `RTE_ETH_RX_OFFLOAD_TIMESTAMP` flag can be used for this
 When the timestamps are enabled, a timestamp validity flag is set in the MBUFs
 containing received frames and timestamp is inserted into the `rte_mbuf` struct.
 
-The timestamp is an `uint64_t` field. Its lower 32 bits represent *seconds* portion of the timestamp
-(number of seconds elapsed since 1.1.1970 00:00:00 UTC) and its higher 32 bits represent
-*nanosecond* portion of the timestamp (number of nanoseconds elapsed since the beginning of the
-second in the *seconds* portion.
+The timestamp is an `uint64_t` field and holds the number of nanoseconds
+elapsed since 1.1.1970 00:00:00 UTC.
 
 
 Using the NFB PMD
diff --git a/drivers/net/nfb/nfb_rx.h b/drivers/net/nfb/nfb_rx.h
index 12769841ae..67b3b00e2a 100644
--- a/drivers/net/nfb/nfb_rx.h
+++ b/drivers/net/nfb/nfb_rx.h
@@ -13,6 +13,7 @@
 #include <rte_mbuf.h>
 #include <rte_mbuf_dyn.h>
 #include <rte_ethdev.h>
+#include <rte_time.h>
 
 #include "nfb.h"
 
@@ -202,15 +203,15 @@ nfb_eth_ndp_rx(void *queue,
 			if (nfb_timestamp_dynfield_offset >= 0) {
 				rte_mbuf_timestamp_t timestamp;
 
-				/* nanoseconds */
-				timestamp =
-					rte_le_to_cpu_32(*((uint32_t *)
-					(packets[i].header + 4)));
-				timestamp <<= 32;
 				/* seconds */
-				timestamp |=
+				timestamp =
 					rte_le_to_cpu_32(*((uint32_t *)
 					(packets[i].header + 8)));
+				timestamp *= NSEC_PER_SEC;
+				/* nanoseconds */
+				timestamp +=
+					rte_le_to_cpu_32(*((uint32_t *)
+					(packets[i].header + 4)));
 				*nfb_timestamp_dynfield(mbuf) = timestamp;
 				mbuf->ol_flags |= nfb_timestamp_rx_dynflag;
 			}
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v5 4/6] net/nfb: use process private variable for internal data
  2026-02-02 19:33 ` [PATCH v5 " spinler
                     ` (2 preceding siblings ...)
  2026-02-02 19:33   ` [PATCH v5 3/6] net/nfb: update timestamp calculation to meaningful value spinler
@ 2026-02-02 19:33   ` spinler
  2026-02-02 19:33   ` [PATCH v5 5/6] net/nfb: release allocated resources correctly spinler
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-02-02 19:33 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

Internal structures of libnfb can't be shared between processes.
Move these structures from dev_private to process_private, which allows
secondary process to correctly initialize and uninitialize the eth_dev.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  11 +++-
 drivers/net/nfb/nfb_ethdev.c | 103 +++++++++++++++++++++--------------
 drivers/net/nfb/nfb_rx.c     |   2 +-
 drivers/net/nfb/nfb_rxmode.c |  12 ++--
 drivers/net/nfb/nfb_tx.c     |   2 +-
 5 files changed, 79 insertions(+), 51 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index 917b830283..90b04c6151 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -41,14 +41,23 @@ extern int nfb_logtype;
 
 #define RTE_NFB_DRIVER_NAME net_nfb
 
-
+/*
+ * Handles obtained from the libnfb: each process must use own instance.
+ * Stored inside dev->process_private.
+ */
 struct pmd_internals {
 	uint16_t         max_rxmac;
 	uint16_t         max_txmac;
 	struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC];
 	struct nc_txmac *txmac[RTE_MAX_NC_TXMAC];
 	struct nfb_device *nfb;
+};
 
+/*
+ * Common data, single instance usable in all processes.
+ * Inited in the RTE_PROC_PRIMARY, stored in dev->data->dev_private.
+ */
+struct pmd_priv {
 	uint16_t max_rx_queues;
 	uint16_t max_tx_queues;
 };
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 5b1df83b7f..34949d6b1f 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -184,7 +184,7 @@ static int
 nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 {
 	int ret;
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
 
 	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
@@ -207,7 +207,7 @@ nfb_eth_get_max_mac_address_count(struct rte_eth_dev *dev)
 	uint16_t i;
 	uint32_t c;
 	uint32_t ret = (uint32_t)-1;
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	/*
 	 * Go through all RX MAC components in firmware and find
@@ -237,13 +237,13 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_priv *priv = dev->data->dev_private;
 
 	dev_info->max_mac_addrs = nfb_eth_get_max_mac_address_count(dev);
 
 	dev_info->max_rx_pktlen = (uint32_t)-1;
-	dev_info->max_rx_queues = internals->max_rx_queues;
-	dev_info->max_tx_queues = internals->max_tx_queues;
+	dev_info->max_rx_queues = priv->max_rx_queues;
+	dev_info->max_tx_queues = priv->max_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
 	dev_info->rx_offload_capa =
 		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
@@ -262,20 +262,20 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 static int
 nfb_eth_dev_close(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	uint16_t i;
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 	int ret;
 
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
 	ret = nfb_eth_dev_stop(dev);
 
-	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
-	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
-
 	for (i = 0; i < nb_rx; i++) {
 		nfb_eth_rx_queue_release(dev, i);
 		dev->data->rx_queues[i] = NULL;
@@ -310,7 +310,7 @@ nfb_eth_link_update(struct rte_eth_dev *dev,
 	struct rte_eth_link link;
 	memset(&link, 0, sizeof(link));
 
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	status.speed = MAC_SPEED_UNKNOWN;
 
@@ -365,7 +365,7 @@ static int
 nfb_eth_dev_set_link_up(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -390,7 +390,7 @@ static int
 nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -431,9 +431,8 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
 {
 	unsigned int i;
 	uint64_t mac;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	mac = nfb_eth_mac_addr_conv(mac_addr);
 	/* Until no real multi-port support, configure all RX MACs the same */
@@ -449,9 +448,8 @@ nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
 {
 	unsigned int i;
 	uint64_t mac;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	mac = nfb_eth_mac_addr_conv(mac_addr);
 	for (i = 0; i < internals->max_rxmac; ++i)
@@ -464,9 +462,8 @@ static void
 nfb_eth_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
 {
 	unsigned int i;
-	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+		dev->process_private;
 
 	for (i = 0; i < internals->max_rxmac; ++i)
 		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
@@ -512,19 +509,31 @@ static const struct eth_dev_ops ops = {
 static int
 nfb_eth_dev_init(struct rte_eth_dev *dev)
 {
+	int ret;
 	uint32_t mac_count;
 	struct rte_eth_dev_data *data = dev->data;
-	struct pmd_internals *internals = (struct pmd_internals *)
-		data->dev_private;
+	struct pmd_internals *internals;
+	struct pmd_priv *priv = data->dev_private;
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
 	struct rte_ether_addr eth_addr_init;
+	uint16_t max_rx_queues, max_tx_queues;
 	char nfb_dev[PATH_MAX];
 
 	NFB_LOG(INFO, "Initializing NFB device (" PCI_PRI_FMT ")",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
 
+	internals = rte_zmalloc_socket("nfb_internals",
+			sizeof(struct pmd_internals), RTE_CACHE_LINE_SIZE,
+			dev->device->numa_node);
+	if (internals == NULL) {
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	dev->process_private = internals;
+
 	snprintf(nfb_dev, sizeof(nfb_dev),
 		"/dev/nfb/by-pci-slot/" PCI_PRI_FMT,
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
@@ -538,13 +547,14 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	internals->nfb = nfb_open(nfb_dev);
 	if (internals->nfb == NULL) {
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
+		rte_free(internals);
 		return -EINVAL;
 	}
-	internals->max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
-	internals->max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
+	max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
+	max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
 
 	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
-		internals->max_rx_queues, internals->max_tx_queues);
+		max_rx_queues, max_tx_queues);
 
 	nfb_nc_rxmac_init(internals->nfb,
 		internals->rxmac,
@@ -563,28 +573,34 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	/* Get link state */
 	nfb_eth_link_update(dev, 0);
 
-	/* Allocate space for MAC addresses */
-	mac_count = nfb_eth_get_max_mac_address_count(dev);
-	data->mac_addrs = rte_zmalloc(data->name,
-		sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
-	if (data->mac_addrs == NULL) {
-		NFB_LOG(ERR, "Could not alloc space for MAC address");
-		nfb_close(internals->nfb);
-		return -EINVAL;
-	}
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		priv->max_rx_queues = max_rx_queues;
+		priv->max_tx_queues = max_tx_queues;
 
-	rte_eth_random_addr(eth_addr_init.addr_bytes);
-	eth_addr_init.addr_bytes[0] = eth_addr.addr_bytes[0];
-	eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
-	eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
+		/* Allocate space for MAC addresses */
+		mac_count = nfb_eth_get_max_mac_address_count(dev);
+		data->mac_addrs = rte_zmalloc(data->name,
+			sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
+		if (data->mac_addrs == NULL) {
+			NFB_LOG(ERR, "Could not alloc space for MAC address");
+			nfb_close(internals->nfb);
+			rte_free(internals);
+			return -EINVAL;
+		}
 
-	nfb_eth_mac_addr_set(dev, &eth_addr_init);
-	rte_ether_addr_copy(&eth_addr_init, &dev->data->mac_addrs[0]);
+		rte_eth_random_addr(eth_addr_init.addr_bytes);
+		eth_addr_init.addr_bytes[0] = eth_addr.addr_bytes[0];
+		eth_addr_init.addr_bytes[1] = eth_addr.addr_bytes[1];
+		eth_addr_init.addr_bytes[2] = eth_addr.addr_bytes[2];
 
-	data->promiscuous = nfb_eth_promiscuous_get(dev);
-	data->all_multicast = nfb_eth_allmulticast_get(dev);
+		nfb_eth_mac_addr_set(dev, &eth_addr_init);
+		rte_ether_addr_copy(&eth_addr_init, &data->mac_addrs[0]);
 
-	dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
+		data->promiscuous = nfb_eth_promiscuous_get(dev);
+		data->all_multicast = nfb_eth_allmulticast_get(dev);
+
+		data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
+	}
 
 	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully initialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
@@ -607,9 +623,12 @@ nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 {
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
+	struct pmd_internals *internals = dev->process_private;
 
 	nfb_eth_dev_close(dev);
 
+	rte_free(internals);
+
 	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully uninitialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
@@ -644,7 +663,7 @@ nfb_eth_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		struct rte_pci_device *pci_dev)
 {
 	return rte_eth_dev_pci_generic_probe(pci_dev,
-		sizeof(struct pmd_internals), nfb_eth_dev_init);
+		sizeof(struct pmd_priv), nfb_eth_dev_init);
 }
 
 /**
diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
index 462bc3b50d..413d275853 100644
--- a/drivers/net/nfb/nfb_rx.c
+++ b/drivers/net/nfb/nfb_rx.c
@@ -60,7 +60,7 @@ nfb_eth_rx_queue_setup(struct rte_eth_dev *dev,
 		const struct rte_eth_rxconf *rx_conf __rte_unused,
 		struct rte_mempool *mb_pool)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 
 	struct ndp_rx_queue *rxq;
 	int ret;
diff --git a/drivers/net/nfb/nfb_rxmode.c b/drivers/net/nfb/nfb_rxmode.c
index ca6e4d5578..dc560d638b 100644
--- a/drivers/net/nfb/nfb_rxmode.c
+++ b/drivers/net/nfb/nfb_rxmode.c
@@ -11,7 +11,7 @@ int
 nfb_eth_promiscuous_enable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 	uint16_t i;
 
 	for (i = 0; i < internals->max_rxmac; ++i) {
@@ -26,7 +26,7 @@ int
 nfb_eth_promiscuous_disable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 	uint16_t i;
 	enum nc_rxmac_mac_filter filter = RXMAC_MAC_FILTER_TABLE_BCAST;
 
@@ -44,7 +44,7 @@ int
 nfb_eth_promiscuous_get(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	struct nc_rxmac_status status;
 	status.mac_filter = RXMAC_MAC_FILTER_PROMISCUOUS;
@@ -59,7 +59,7 @@ int
 nfb_eth_allmulticast_enable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 	if (dev->data->promiscuous)
@@ -76,7 +76,7 @@ int
 nfb_eth_allmulticast_disable(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	uint16_t i;
 
@@ -95,7 +95,7 @@ int
 nfb_eth_allmulticast_get(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *internals = (struct pmd_internals *)
-		dev->data->dev_private;
+		dev->process_private;
 
 	struct nc_rxmac_status status;
 	status.mac_filter = RXMAC_MAC_FILTER_PROMISCUOUS;
diff --git a/drivers/net/nfb/nfb_tx.c b/drivers/net/nfb/nfb_tx.c
index cf99268c43..1f997ce22f 100644
--- a/drivers/net/nfb/nfb_tx.c
+++ b/drivers/net/nfb/nfb_tx.c
@@ -53,7 +53,7 @@ nfb_eth_tx_queue_setup(struct rte_eth_dev *dev,
 	unsigned int socket_id,
 	const struct rte_eth_txconf *tx_conf __rte_unused)
 {
-	struct pmd_internals *internals = dev->data->dev_private;
+	struct pmd_internals *internals = dev->process_private;
 	int ret;
 	struct ndp_tx_queue *txq;
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v5 5/6] net/nfb: release allocated resources correctly
  2026-02-02 19:33 ` [PATCH v5 " spinler
                     ` (3 preceding siblings ...)
  2026-02-02 19:33   ` [PATCH v5 4/6] net/nfb: use process private variable for internal data spinler
@ 2026-02-02 19:33   ` spinler
  2026-02-10  0:52     ` Stephen Hemminger
  2026-02-02 19:33   ` [PATCH v5 6/6] net/nfb: stop only started queues in fail path spinler
  2026-02-03  1:50   ` [PATCH v5 0/6] net/nfb: code cleanup Stephen Hemminger
  6 siblings, 1 reply; 58+ messages in thread
From: spinler @ 2026-02-02 19:33 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

Internal handles are initialized in eth_dev_init, so the cleanup
should be done in eth_dev_uninit.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 74 ++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 32 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 34949d6b1f..5197642f61 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -18,6 +18,8 @@
 #include "nfb_rxmode.h"
 #include "nfb.h"
 
+static int nfb_eth_dev_uninit(struct rte_eth_dev *dev);
+
 /**
  * Default MAC addr
  */
@@ -160,8 +162,6 @@ nfb_eth_dev_stop(struct rte_eth_dev *dev)
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
 
-	dev->data->dev_started = 0;
-
 	for (i = 0; i < nb_tx; i++)
 		nfb_eth_tx_queue_stop(dev, i);
 
@@ -184,7 +184,6 @@ static int
 nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 {
 	int ret;
-	struct pmd_internals *internals = dev->process_private;
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
 
 	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
@@ -193,12 +192,16 @@ nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 				&nfb_timestamp_rx_dynflag);
 		if (ret != 0) {
 			NFB_LOG(ERR, "Cannot register Rx timestamp field/flag %d", ret);
-			nfb_close(internals->nfb);
-			return -rte_errno;
+			ret = -ENOMEM;
+			goto err_ts_register;
 		}
 	}
 
 	return 0;
+
+err_ts_register:
+	nfb_eth_dev_uninit(dev);
+	return ret;
 }
 
 static uint32_t
@@ -262,32 +265,28 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 static int
 nfb_eth_dev_close(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *internals = dev->process_private;
 	uint16_t i;
 	uint16_t nb_rx = dev->data->nb_rx_queues;
 	uint16_t nb_tx = dev->data->nb_tx_queues;
-	int ret;
 
-	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
-	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
-
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
-	ret = nfb_eth_dev_stop(dev);
-
-	for (i = 0; i < nb_rx; i++) {
-		nfb_eth_rx_queue_release(dev, i);
-		dev->data->rx_queues[i] = NULL;
-	}
-	dev->data->nb_rx_queues = 0;
-	for (i = 0; i < nb_tx; i++) {
-		nfb_eth_tx_queue_release(dev, i);
-		dev->data->tx_queues[i] = NULL;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		for (i = 0; i < nb_rx; i++) {
+			if (dev->data->rx_queues[i]) {
+				nfb_eth_rx_queue_release(dev, i);
+				dev->data->rx_queues[i] = NULL;
+			}
+		}
+		for (i = 0; i < nb_tx; i++) {
+			if (dev->data->tx_queues[i]) {
+				nfb_eth_tx_queue_release(dev, i);
+				dev->data->tx_queues[i] = NULL;
+			}
+		}
 	}
-	dev->data->nb_tx_queues = 0;
 
-	return ret;
+	nfb_eth_dev_uninit(dev);
+
+	return 0;
 }
 
 /**
@@ -529,7 +528,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 			dev->device->numa_node);
 	if (internals == NULL) {
 		ret = -ENOMEM;
-		return ret;
+		goto err_alloc_internals;
 	}
 
 	dev->process_private = internals;
@@ -547,8 +546,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	internals->nfb = nfb_open(nfb_dev);
 	if (internals->nfb == NULL) {
 		NFB_LOG(ERR, "nfb_open(): failed to open %s", nfb_dev);
-		rte_free(internals);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_nfb_open;
 	}
 	max_rx_queues = ndp_get_rx_queue_available_count(internals->nfb);
 	max_tx_queues = ndp_get_tx_queue_available_count(internals->nfb);
@@ -583,9 +582,8 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 			sizeof(struct rte_ether_addr) * mac_count, RTE_CACHE_LINE_SIZE);
 		if (data->mac_addrs == NULL) {
 			NFB_LOG(ERR, "Could not alloc space for MAC address");
-			nfb_close(internals->nfb);
-			rte_free(internals);
-			return -EINVAL;
+			ret = -ENOMEM;
+			goto err_malloc_mac_addrs;
 		}
 
 		rte_eth_random_addr(eth_addr_init.addr_bytes);
@@ -607,6 +605,16 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 		pci_addr->function);
 
 	return 0;
+
+err_malloc_mac_addrs:
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+	nfb_close(internals->nfb);
+
+err_nfb_open:
+	rte_free(internals);
+err_alloc_internals:
+	return ret;
 }
 
 /**
@@ -625,7 +633,9 @@ nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 	struct rte_pci_addr *pci_addr = &pci_dev->addr;
 	struct pmd_internals *internals = dev->process_private;
 
-	nfb_eth_dev_close(dev);
+	nfb_nc_rxmac_deinit(internals->rxmac, internals->max_rxmac);
+	nfb_nc_txmac_deinit(internals->txmac, internals->max_txmac);
+	nfb_close(internals->nfb);
 
 	rte_free(internals);
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v5 5/6] net/nfb: release allocated resources correctly
  2026-02-02 19:33   ` [PATCH v5 5/6] net/nfb: release allocated resources correctly spinler
@ 2026-02-10  0:52     ` Stephen Hemminger
  0 siblings, 0 replies; 58+ messages in thread
From: Stephen Hemminger @ 2026-02-10  0:52 UTC (permalink / raw)
  To: spinler; +Cc: dev, stable

On Mon,  2 Feb 2026 20:33:29 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> Internal handles are initialized in eth_dev_init, so the cleanup
> should be done in eth_dev_uninit.
> 
> Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---

More AI review feedback. Will leave up to you to fix

Patch 29: net/nfb: release allocated resources correctly
Potential uninitialized access in error path

The patch adds err_ts_register: error label that calls
nfb_eth_dev_uninit(dev). This function accesses internals->rxmac,
internals->txmac, and internals->nfb. If timestamp registration fails,
these fields should all be initialized (since the error is late in the
configure flow), but this should be verified. The rxmac/txmac are
initialized in nfb_eth_dev_init() before this error can occur, so this
appears safe. However, calling uninit from configure is unusual -
typically configure errors would not tear down the entire device.
Recommendation: Verify that all error paths properly clean up only what
was allocated in that specific function, rather than calling the full
uninit.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v5 6/6] net/nfb: stop only started queues in fail path
  2026-02-02 19:33 ` [PATCH v5 " spinler
                     ` (4 preceding siblings ...)
  2026-02-02 19:33   ` [PATCH v5 5/6] net/nfb: release allocated resources correctly spinler
@ 2026-02-02 19:33   ` spinler
  2026-02-03  1:50   ` [PATCH v5 0/6] net/nfb: code cleanup Stephen Hemminger
  6 siblings, 0 replies; 58+ messages in thread
From: spinler @ 2026-02-02 19:33 UTC (permalink / raw)
  To: dev; +Cc: Martin Spinler, stable

From: Martin Spinler <spinler@cesnet.cz>

The driver should stop only running queues in case of error
inside eth_dev_start.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 5197642f61..967f127f40 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -139,11 +139,12 @@ nfb_eth_dev_start(struct rte_eth_dev *dev)
 	return 0;
 
 err_tx:
-	for (i = 0; i < nb_tx; i++)
-		nfb_eth_tx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_tx_queue_stop(dev, i - 1);
+	i = nb_rx;
 err_rx:
-	for (i = 0; i < nb_rx; i++)
-		nfb_eth_rx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_rx_queue_stop(dev, i - 1);
 	return ret;
 }
 
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v5 0/6] net/nfb: code cleanup
  2026-02-02 19:33 ` [PATCH v5 " spinler
                     ` (5 preceding siblings ...)
  2026-02-02 19:33   ` [PATCH v5 6/6] net/nfb: stop only started queues in fail path spinler
@ 2026-02-03  1:50   ` Stephen Hemminger
  2026-02-03  6:36     ` Martin Spinler
  6 siblings, 1 reply; 58+ messages in thread
From: Stephen Hemminger @ 2026-02-03  1:50 UTC (permalink / raw)
  To: spinler; +Cc: dev

On Mon,  2 Feb 2026 20:33:24 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> This patchset mainly cleans up the code and prepare it for another
> quite large rework. Also it resolves some unpleasant behavior.
> 
> ---
> v5:
> * Rebased to next-net-main
> 
> Martin Spinler (6):
>   net/nfb: use constant values for max Rx/Tx queues count
>   net/nfb: fix bad pointer access in queue stats
>   net/nfb: update timestamp calculation to meaningful value
>   net/nfb: use process private variable for internal data
>   net/nfb: release allocated resources correctly
>   net/nfb: stop only started queues in fail path
> 
>  doc/guides/nics/nfb.rst      |   6 +-
>  drivers/net/nfb/nfb.h        |  14 ++-
>  drivers/net/nfb/nfb_ethdev.c | 166 +++++++++++++++++++++--------------
>  drivers/net/nfb/nfb_rx.c     |   2 +-
>  drivers/net/nfb/nfb_rx.h     |  13 +--
>  drivers/net/nfb/nfb_rxmode.c |  12 +--
>  drivers/net/nfb/nfb_stats.c  |  46 +++++-----
>  drivers/net/nfb/nfb_tx.c     |   2 +-
>  8 files changed, 152 insertions(+), 109 deletions(-)
> 

AI patch review summary.  My comments afterward,

> Here's the review. The series is in good shape overall — no errors found across all six patches. The main items worth raising with Martin:
> Patch 2: The new queue iteration still doesn't NULL-check the per-queue pointer before dereferencing. Given the commit message specifically calls out the pointer validity problem, this seems like an oversight.
> Patch 4: rte_zmalloc_socket for a small control structure of file handles is using limited hugepage memory unnecessarily — plain calloc would suffice.
> Patch 5: The removal of dev_stop from close relies on the ethdev layer calling stop first. That's fine for modern DPDK but worth a sanity check. Similarly, the dev_started = 0 removal is correct if the minimum supported DPDK version manages that flag in the ethdev layer.
> Everything else — commit messages, tags, formatting, error paths, the timestamp arithmetic, the start error unwinding — looks clean.

Ignore the comment on 5.

Patch 4 raises a good point, file handles are per-process so doesn't need to be in hugepages.

Not sure about comment about Patch #2. The queues are set to null on close. So there might be small race with another process getting stats.

I needed to do some manual application to workaround patch fuzz.
Then queued to next-net.





^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v5 0/6] net/nfb: code cleanup
  2026-02-03  1:50   ` [PATCH v5 0/6] net/nfb: code cleanup Stephen Hemminger
@ 2026-02-03  6:36     ` Martin Spinler
  0 siblings, 0 replies; 58+ messages in thread
From: Martin Spinler @ 2026-02-03  6:36 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

> > Patch 2: The new queue iteration still doesn't NULL-check the per-queue pointer before dereferencing. Given the commit message specifically calls out the pointer validity problem, this seems like an oversight.
> > Patch 4: rte_zmalloc_socket for a small control structure of file handles is using limited hugepage memory unnecessarily — plain calloc would suffice.
> 
> Patch 4 raises a good point, file handles are per-process so doesn't need to be in hugepages.

I'll keep that in mind for future changes.

> Not sure about comment about Patch #2. The queues are set to null on close. So there might be small race with another process getting stats.

Oh, I see. The original problem (described in the commit message) has
been solved, but in the case you described, checking for null is still
not enough and some rte_spinlock_lock should also be used.

^ permalink raw reply	[flat|nested] 58+ messages in thread

end of thread, other threads:[~2026-02-10  0:52 UTC | newest]

Thread overview: 58+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-15 14:01 [PATCH 0/6] spinler
2026-01-15 14:01 ` [PATCH 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
2026-01-15 14:01 ` [PATCH 2/6] net/nfb: fix bad pointer access in queue stats spinler
2026-01-15 14:01 ` [PATCH 3/6] net/nfb: update timestamp calculation to meaningful value spinler
2026-01-15 14:01 ` [PATCH 4/6] net/nfb: use process private variable for internal data spinler
2026-01-15 14:01 ` [PATCH 5/6] net/nfb: release allocated resources correctly spinler
2026-01-15 14:01 ` [PATCH 6/6] net/nfb: stop only started queues in fail path spinler
2026-01-15 14:40 ` [PATCH v2 0/6] net/nfb: code cleanup spinler
2026-01-15 14:40   ` [PATCH v2 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
2026-01-15 14:40   ` [PATCH v2 2/6] net/nfb: fix bad pointer access in queue stats spinler
2026-01-15 14:40   ` [PATCH v2 3/6] net/nfb: update timestamp calculation to meaningful value spinler
2026-01-15 14:40   ` [PATCH v2 4/6] net/nfb: use process private variable for internal data spinler
2026-01-15 14:40   ` [PATCH v2 5/6] net/nfb: release allocated resources correctly spinler
2026-01-15 14:40   ` [PATCH v2 6/6] net/nfb: stop only started queues in fail path spinler
2026-01-16  5:48   ` [PATCH v2 0/6] net/nfb: code cleanup Stephen Hemminger
2026-01-16  9:42     ` Martin Spinler
2026-01-16 17:39       ` Stephen Hemminger
2026-01-16 15:20 ` spinler
2026-01-16 15:20   ` [PATCH v3 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
2026-02-02 17:47     ` Stephen Hemminger
2026-02-02 18:58       ` Martin Špinler
2026-01-16 15:20   ` [PATCH v3 2/6] net/nfb: fix bad pointer access in queue stats spinler
2026-01-16 15:20   ` [PATCH v3 3/6] net/nfb: update timestamp calculation to meaningful value spinler
2026-01-16 15:20   ` [PATCH v3 4/6] net/nfb: use process private variable for internal data spinler
2026-01-20  0:13     ` Stephen Hemminger
2026-01-20 14:13       ` Martin Spinler
2026-01-20 16:11         ` Stephen Hemminger
2026-01-16 15:20   ` [PATCH v3 5/6] net/nfb: release allocated resources correctly spinler
2026-01-20  0:10     ` Stephen Hemminger
2026-01-20 14:14       ` Martin Spinler
2026-01-16 15:20   ` [PATCH v3 6/6] net/nfb: stop only started queues in fail path spinler
2026-01-20  0:09     ` Stephen Hemminger
2026-01-20 14:14       ` Martin Spinler
2026-01-16 15:22 ` [PATCH v3 0/6] net/nfb: code cleanup spinler
2026-01-21  4:57   ` Stephen Hemminger
2026-01-21 17:01 ` [PATCH v4 " spinler
2026-01-21 17:01   ` [PATCH v4 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
2026-01-21 17:01   ` [PATCH v4 2/6] net/nfb: fix bad pointer access in queue stats spinler
2026-01-21 17:01   ` [PATCH v4 3/6] net/nfb: update timestamp calculation to meaningful value spinler
2026-01-21 17:33     ` Stephen Hemminger
2026-01-27  8:12       ` Martin Spinler
2026-01-27  0:34     ` Stephen Hemminger
2026-01-27  8:16       ` Martin Spinler
2026-01-21 17:01   ` [PATCH v4 4/6] net/nfb: use process private variable for internal data spinler
2026-01-21 17:01   ` [PATCH v4 5/6] net/nfb: release allocated resources correctly spinler
2026-01-21 17:01   ` [PATCH v4 6/6] net/nfb: stop only started queues in fail path spinler
2026-01-21 17:35   ` [PATCH v4 0/6] net/nfb: code cleanup Stephen Hemminger
2026-02-02 19:33 ` [PATCH v5 " spinler
2026-02-02 19:33   ` [PATCH v5 1/6] net/nfb: use constant values for max Rx/Tx queues count spinler
2026-02-02 19:33   ` [PATCH v5 2/6] net/nfb: fix bad pointer access in queue stats spinler
2026-02-10  0:51     ` Stephen Hemminger
2026-02-02 19:33   ` [PATCH v5 3/6] net/nfb: update timestamp calculation to meaningful value spinler
2026-02-02 19:33   ` [PATCH v5 4/6] net/nfb: use process private variable for internal data spinler
2026-02-02 19:33   ` [PATCH v5 5/6] net/nfb: release allocated resources correctly spinler
2026-02-10  0:52     ` Stephen Hemminger
2026-02-02 19:33   ` [PATCH v5 6/6] net/nfb: stop only started queues in fail path spinler
2026-02-03  1:50   ` [PATCH v5 0/6] net/nfb: code cleanup Stephen Hemminger
2026-02-03  6:36     ` Martin Spinler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox