[PATCH 1/6] net/nfb: add missing libfdt dependency for build

[PATCH 1/6] net/nfb: add missing libfdt dependency for build

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

The driver uses some FDT manipulation functions from libfdt.
Let the build system check for libfdt package.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/meson.build | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/nfb/meson.build b/drivers/net/nfb/meson.build
index bb5f66a09a..c080c06bf9 100644
--- a/drivers/net/nfb/meson.build
+++ b/drivers/net/nfb/meson.build
@@ -9,6 +9,12 @@ if is_windows
     subdir_done()
 endif
 
+if has_libfdt == 0
+    build = false
+    reason = 'missing dependency, "libfdt"'
+    subdir_done()
+endif
+
 dep = dependency('netcope-common', required: false, method: 'pkg-config')
 reason = 'missing dependency, "libnfb"'
 build = dep.found()
-- 
2.35.1

[PATCH 2/6] drivers/nfb: fix array indexes in deinit functions

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

The indexes in the for cycle were wrongly used and
the code accessed outside of the rxmac/txmac array.

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

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 3c39937816..0b27fe78cc 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -77,9 +77,10 @@ static void
 nfb_nc_rxmac_deinit(struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC],
 	uint16_t max_rxmac)
 {
-	for (; max_rxmac > 0; --max_rxmac) {
-		nc_rxmac_close(rxmac[max_rxmac]);
-		rxmac[max_rxmac] = NULL;
+	uint16_t i;
+	for (i = 0; i < max_rxmac; i++) {
+		nc_rxmac_close(rxmac[i]);
+		rxmac[i] = NULL;
 	}
 }
 
@@ -95,9 +96,10 @@ static void
 nfb_nc_txmac_deinit(struct nc_txmac *txmac[RTE_MAX_NC_TXMAC],
 	uint16_t max_txmac)
 {
-	for (; max_txmac > 0; --max_txmac) {
-		nc_txmac_close(txmac[max_txmac]);
-		txmac[max_txmac] = NULL;
+	uint16_t i;
+	for (i = 0; i < max_txmac; i++) {
+		nc_txmac_close(txmac[i]);
+		txmac[i] = NULL;
 	}
 }
 
-- 
2.35.1

[PATCH 3/6] drivers/nfb: do not report zero-sized TX burst

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

Zero-sized TX burst floods the log no more.

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

diff --git a/drivers/net/nfb/nfb_tx.h b/drivers/net/nfb/nfb_tx.h
index d3cbe3e6b3..910020e9e9 100644
--- a/drivers/net/nfb/nfb_tx.h
+++ b/drivers/net/nfb/nfb_tx.h
@@ -136,7 +136,10 @@ nfb_eth_ndp_tx(void *queue,
 
 	struct ndp_packet packets[nb_pkts];
 
-	if (unlikely(ndp->queue == NULL || nb_pkts == 0)) {
+	if (unlikely(nb_pkts == 0))
+		return 0;
+
+	if (unlikely(ndp->queue == NULL)) {
 		RTE_LOG(ERR, PMD, "TX invalid arguments!\n");
 		return 0;
 	}
-- 
2.35.1

[PATCH 4/6] drivers/nfb: use RTE_ETH_RX_OFFLOAD_TIMESTAMP flag

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

Rewrite the RX timestamp setup code to use standard offload flag.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  3 +-
 drivers/net/nfb/nfb_ethdev.c | 19 ++++++++++++-
 drivers/net/nfb/nfb_rx.c     | 53 ------------------------------------
 drivers/net/nfb/nfb_rx.h     |  7 +----
 4 files changed, 20 insertions(+), 62 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index 59d3ab4986..4de9006ac0 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -37,8 +37,7 @@
 #define RTE_NFB_DRIVER_NAME net_nfb
 
 /* Device arguments */
-#define TIMESTAMP_ARG  "timestamp"
-static const char * const VALID_KEYS[] = {TIMESTAMP_ARG, NULL};
+static const char * const VALID_KEYS[] = {NULL};
 
 struct pmd_internals {
 	uint16_t         max_rxmac;
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 0b27fe78cc..53a98642b3 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -183,6 +183,22 @@ nfb_eth_dev_stop(struct rte_eth_dev *dev)
 static int
 nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 {
+	int ret;
+	struct pmd_internals *internals = dev->data->dev_private;
+	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
+
+	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
+		ret = rte_mbuf_dyn_rx_timestamp_register
+				(&nfb_timestamp_dynfield_offset,
+				&nfb_timestamp_rx_dynflag);
+		if (ret != 0) {
+			RTE_LOG(ERR, PMD, "Cannot register Rx timestamp"
+					" field/flag %d\n", ret);
+			nfb_close(internals->nfb);
+			return -rte_errno;
+		}
+	}
+
 	return 0;
 }
 
@@ -203,6 +219,8 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 	dev_info->max_rx_queues = dev->data->nb_rx_queues;
 	dev_info->max_tx_queues = dev->data->nb_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
+	dev_info->rx_offload_capa =
+		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
 
 	return 0;
 }
@@ -609,4 +627,3 @@ static struct rte_pci_driver nfb_eth_driver = {
 RTE_PMD_REGISTER_PCI(RTE_NFB_DRIVER_NAME, nfb_eth_driver);
 RTE_PMD_REGISTER_PCI_TABLE(RTE_NFB_DRIVER_NAME, nfb_pci_id_table);
 RTE_PMD_REGISTER_KMOD_DEP(RTE_NFB_DRIVER_NAME, "* nfb");
-RTE_PMD_REGISTER_PARAM_STRING(RTE_NFB_DRIVER_NAME, TIMESTAMP_ARG "=<0|1>");
diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
index f76e2ba646..8a9b232305 100644
--- a/drivers/net/nfb/nfb_rx.c
+++ b/drivers/net/nfb/nfb_rx.c
@@ -12,56 +12,6 @@
 uint64_t nfb_timestamp_rx_dynflag;
 int nfb_timestamp_dynfield_offset = -1;
 
-static int
-timestamp_check_handler(__rte_unused const char *key,
-	const char *value, __rte_unused void *opaque)
-{
-	if (strcmp(value, "1"))
-		return -1;
-
-	return 0;
-}
-
-
-static int
-nfb_check_timestamp(struct rte_devargs *devargs)
-{
-	struct rte_kvargs *kvlist;
-	int ret;
-
-	if (devargs == NULL)
-		return 0;
-
-	kvlist = rte_kvargs_parse(devargs->args, NULL);
-	if (kvlist == NULL)
-		return 0;
-
-	if (!rte_kvargs_count(kvlist, TIMESTAMP_ARG)) {
-		rte_kvargs_free(kvlist);
-		return 0;
-	}
-	/* Timestamps are enabled when there is
-	 * key-value pair: enable_timestamp=1
-	 * TODO: timestamp should be enabled with RTE_ETH_RX_OFFLOAD_TIMESTAMP
-	 */
-	if (rte_kvargs_process(kvlist, TIMESTAMP_ARG,
-		timestamp_check_handler, NULL) < 0) {
-		rte_kvargs_free(kvlist);
-		return 0;
-	}
-	rte_kvargs_free(kvlist);
-
-	ret = rte_mbuf_dyn_rx_timestamp_register(
-			&nfb_timestamp_dynfield_offset,
-			&nfb_timestamp_rx_dynflag);
-	if (ret != 0) {
-		RTE_LOG(ERR, PMD, "Cannot register Rx timestamp field/flag\n");
-		return -rte_errno;
-	}
-
-	return 1;
-}
-
 int
 nfb_eth_rx_queue_start(struct rte_eth_dev *dev, uint16_t rxq_id)
 {
@@ -138,9 +88,6 @@ nfb_eth_rx_queue_setup(struct rte_eth_dev *dev,
 	else
 		rte_free(rxq);
 
-	if (nfb_check_timestamp(dev->device->devargs) > 0)
-		rxq->flags |= NFB_TIMESTAMP_FLAG;
-
 	return ret;
 }
 
diff --git a/drivers/net/nfb/nfb_rx.h b/drivers/net/nfb/nfb_rx.h
index 638205d53c..b618682e13 100644
--- a/drivers/net/nfb/nfb_rx.h
+++ b/drivers/net/nfb/nfb_rx.h
@@ -14,8 +14,6 @@
 #include <rte_mbuf_dyn.h>
 #include <rte_ethdev.h>
 
-#define NFB_TIMESTAMP_FLAG (1 << 0)
-
 extern uint64_t nfb_timestamp_rx_dynflag;
 extern int nfb_timestamp_dynfield_offset;
 
@@ -145,7 +143,6 @@ nfb_eth_ndp_rx(void *queue,
 	uint16_t nb_pkts)
 {
 	struct ndp_rx_queue *ndp = queue;
-	uint8_t timestamping_enabled;
 	uint16_t packet_size;
 	uint64_t num_bytes = 0;
 	uint16_t num_rx;
@@ -163,8 +160,6 @@ nfb_eth_ndp_rx(void *queue,
 		return 0;
 	}
 
-	timestamping_enabled = ndp->flags & NFB_TIMESTAMP_FLAG;
-
 	/* returns either all or nothing */
 	i = rte_pktmbuf_alloc_bulk(ndp->mb_pool, mbufs, nb_pkts);
 	if (unlikely(i != 0))
@@ -202,7 +197,7 @@ nfb_eth_ndp_rx(void *queue,
 			mbuf->port = ndp->in_port;
 			mbuf->ol_flags = 0;
 
-			if (timestamping_enabled) {
+			if (nfb_timestamp_dynfield_offset >= 0) {
 				rte_mbuf_timestamp_t timestamp;
 
 				/* nanoseconds */
-- 
2.35.1

[PATCH 5/6] drivers/nfb: fix multicast/promiscuous mode switching

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

In the firmware, the promisc mode overrides the multicast mode.
So when the promisc mode is turned off, driver must check if the
multicast mode was active before and conditionally reactivate it.

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

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index 4de9006ac0..7dc5bd29e4 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -47,10 +47,6 @@ struct pmd_internals {
 
 	char             nfb_dev[PATH_MAX];
 	struct nfb_device *nfb;
-	/* Place to remember if filter was promiscuous or filtering by table,
-	 * when disabling allmulticast
-	 */
-	enum nc_rxmac_mac_filter rx_filter_original;
 };
 
 #endif /* _NFB_H_ */
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 53a98642b3..5d503e131a 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -534,7 +534,6 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 
 	data->promiscuous = nfb_eth_promiscuous_get(dev);
 	data->all_multicast = nfb_eth_allmulticast_get(dev);
-	internals->rx_filter_original = data->promiscuous;
 
 	dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
 
diff --git a/drivers/net/nfb/nfb_rxmode.c b/drivers/net/nfb/nfb_rxmode.c
index 2d0b613d21..ca6e4d5578 100644
--- a/drivers/net/nfb/nfb_rxmode.c
+++ b/drivers/net/nfb/nfb_rxmode.c
@@ -14,8 +14,6 @@ nfb_eth_promiscuous_enable(struct rte_eth_dev *dev)
 		dev->data->dev_private;
 	uint16_t i;
 
-	internals->rx_filter_original = RXMAC_MAC_FILTER_PROMISCUOUS;
-
 	for (i = 0; i < internals->max_rxmac; ++i) {
 		nc_rxmac_mac_filter_enable(internals->rxmac[i],
 			RXMAC_MAC_FILTER_PROMISCUOUS);
@@ -30,16 +28,13 @@ nfb_eth_promiscuous_disable(struct rte_eth_dev *dev)
 	struct pmd_internals *internals = (struct pmd_internals *)
 		dev->data->dev_private;
 	uint16_t i;
+	enum nc_rxmac_mac_filter filter = RXMAC_MAC_FILTER_TABLE_BCAST;
 
-	internals->rx_filter_original = RXMAC_MAC_FILTER_TABLE;
-
-	/* if promisc is not enabled, do nothing */
-	if (!nfb_eth_promiscuous_get(dev))
-		return 0;
+	if (dev->data->all_multicast)
+		filter = RXMAC_MAC_FILTER_TABLE_BCAST_MCAST;
 
 	for (i = 0; i < internals->max_rxmac; ++i) {
-		nc_rxmac_mac_filter_enable(internals->rxmac[i],
-			RXMAC_MAC_FILTER_TABLE);
+		nc_rxmac_mac_filter_enable(internals->rxmac[i], filter);
 	}
 
 	return 0;
@@ -67,6 +62,8 @@ nfb_eth_allmulticast_enable(struct rte_eth_dev *dev)
 		dev->data->dev_private;
 
 	uint16_t i;
+	if (dev->data->promiscuous)
+		return 0;
 	for (i = 0; i < internals->max_rxmac; ++i) {
 		nc_rxmac_mac_filter_enable(internals->rxmac[i],
 			RXMAC_MAC_FILTER_TABLE_BCAST_MCAST);
@@ -83,13 +80,12 @@ nfb_eth_allmulticast_disable(struct rte_eth_dev *dev)
 
 	uint16_t i;
 
-	/* if multicast is not enabled do nothing */
-	if (!nfb_eth_allmulticast_get(dev))
+	if (dev->data->promiscuous)
 		return 0;
 
 	for (i = 0; i < internals->max_rxmac; ++i) {
 		nc_rxmac_mac_filter_enable(internals->rxmac[i],
-			internals->rx_filter_original);
+			RXMAC_MAC_FILTER_TABLE_BCAST);
 	}
 
 	return 0;
-- 
2.35.1

[PATCH 6/6] drivers/nfb: add support for more MAC addresses

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

Extend the eth_dev_ops by add/remove MAC address functions.

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

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 5d503e131a..7dec8e022e 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -214,7 +214,19 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
-	dev_info->max_mac_addrs = 1;
+	uint16_t i;
+	uint32_t max_mac_addrs;
+	struct pmd_internals *internals = dev->data->dev_private;
+
+	dev_info->max_mac_addrs = (uint32_t)-1;
+	for (i = 0; i < internals->max_rxmac; i++) {
+		max_mac_addrs = nc_rxmac_mac_address_count(internals->rxmac[i]);
+		dev_info->max_mac_addrs = RTE_MIN(max_mac_addrs,
+				dev_info->max_mac_addrs);
+	}
+	if (internals->max_rxmac == 0)
+		dev_info->max_mac_addrs = 0;
+
 	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;
@@ -376,6 +388,18 @@ nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
 	return 0;
 }
 
+static uint64_t
+nfb_eth_mac_addr_conv(struct rte_ether_addr *mac_addr)
+{
+	int i;
+	uint64_t res = 0;
+	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
+		res <<= 8;
+		res |= mac_addr->addr_bytes[i] & 0xFF;
+	}
+	return res;
+}
+
 /**
  * DPDK callback to set primary MAC address.
  *
@@ -392,26 +416,47 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
 	struct rte_ether_addr *mac_addr)
 {
 	unsigned int i;
-	uint64_t mac = 0;
+	uint64_t mac;
 	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
 		data->dev_private;
 
-	if (!rte_is_valid_assigned_ether_addr(mac_addr))
-		return -EINVAL;
+	mac = nfb_eth_mac_addr_conv(mac_addr);
+	for (i = 0; i < internals->max_rxmac; ++i)
+		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
 
-	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
-		mac <<= 8;
-		mac |= mac_addr->addr_bytes[i] & 0xFF;
-	}
+	return 0;
+}
 
+static int
+nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
+	struct rte_ether_addr *mac_addr, uint32_t index, uint32_t pool __rte_unused)
+{
+	unsigned int i;
+	uint64_t mac = 0;
+	struct rte_eth_dev_data *data = dev->data;
+	struct pmd_internals *internals = (struct pmd_internals *)
+		data->dev_private;
+
+	mac = nfb_eth_mac_addr_conv(mac_addr);
 	for (i = 0; i < internals->max_rxmac; ++i)
-		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
+		nc_rxmac_set_mac(internals->rxmac[i], index, mac, 1);
 
-	rte_ether_addr_copy(mac_addr, data->mac_addrs);
 	return 0;
 }
 
+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;
+
+	for (i = 0; i < internals->max_rxmac; ++i)
+		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
+}
+
 static const struct eth_dev_ops ops = {
 	.dev_start = nfb_eth_dev_start,
 	.dev_stop = nfb_eth_dev_stop,
@@ -436,6 +481,8 @@ static const struct eth_dev_ops ops = {
 	.stats_get = nfb_eth_stats_get,
 	.stats_reset = nfb_eth_stats_reset,
 	.mac_addr_set = nfb_eth_mac_addr_set,
+	.mac_addr_add = nfb_eth_mac_addr_add,
+	.mac_addr_remove = nfb_eth_mac_addr_remove,
 };
 
 /**
@@ -530,7 +577,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	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_eth_dev_default_mac_addr_set(dev->data->port_id, &eth_addr_init);
 
 	data->promiscuous = nfb_eth_promiscuous_get(dev);
 	data->all_multicast = nfb_eth_allmulticast_get(dev);
-- 
2.35.1

Re: [PATCH 2/6] drivers/nfb: fix array indexes in deinit functions

[Ferruh Yigit]

On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> From: Martin Spinler <spinler@cesnet.cz>
> 
> The indexes in the for cycle were wrongly used and
> the code accessed outside of the rxmac/txmac array.
> 

can you please add fixes tag, to help backport.
Also please add stable tag to request backport.

> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---
>   drivers/net/nfb/nfb_ethdev.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
> index 3c39937816..0b27fe78cc 100644
> --- a/drivers/net/nfb/nfb_ethdev.c
> +++ b/drivers/net/nfb/nfb_ethdev.c
> @@ -77,9 +77,10 @@ static void
>   nfb_nc_rxmac_deinit(struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC],
>   	uint16_t max_rxmac)
>   {
> -	for (; max_rxmac > 0; --max_rxmac) {
> -		nc_rxmac_close(rxmac[max_rxmac]);
> -		rxmac[max_rxmac] = NULL;
> +	uint16_t i;
> +	for (i = 0; i < max_rxmac; i++) {
> +		nc_rxmac_close(rxmac[i]);
> +		rxmac[i] = NULL;
>   	}
>   }
>   
> @@ -95,9 +96,10 @@ static void
>   nfb_nc_txmac_deinit(struct nc_txmac *txmac[RTE_MAX_NC_TXMAC],
>   	uint16_t max_txmac)
>   {
> -	for (; max_txmac > 0; --max_txmac) {
> -		nc_txmac_close(txmac[max_txmac]);
> -		txmac[max_txmac] = NULL;
> +	uint16_t i;
> +	for (i = 0; i < max_txmac; i++) {
> +		nc_txmac_close(txmac[i]);
> +		txmac[i] = NULL;
>   	}
>   }
>   

Re: [PATCH 3/6] drivers/nfb: do not report zero-sized TX burst

[Ferruh Yigit]

On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> From: Martin Spinler <spinler@cesnet.cz>
> 
> Zero-sized TX burst floods the log no more.
> 

If you want this patch to be backported, you can update
commit log as fix and add fixes & stable tags.

> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---
>   drivers/net/nfb/nfb_tx.h | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/nfb/nfb_tx.h b/drivers/net/nfb/nfb_tx.h
> index d3cbe3e6b3..910020e9e9 100644
> --- a/drivers/net/nfb/nfb_tx.h
> +++ b/drivers/net/nfb/nfb_tx.h
> @@ -136,7 +136,10 @@ nfb_eth_ndp_tx(void *queue,
>   
>   	struct ndp_packet packets[nb_pkts];
>   
> -	if (unlikely(ndp->queue == NULL || nb_pkts == 0)) {
> +	if (unlikely(nb_pkts == 0))
> +		return 0;
> +
> +	if (unlikely(ndp->queue == NULL)) {
>   		RTE_LOG(ERR, PMD, "TX invalid arguments!\n");
>   		return 0;
>   	}

Re: [PATCH 5/6] drivers/nfb: fix multicast/promiscuous mode switching

[Ferruh Yigit]

On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> From: Martin Spinler <spinler@cesnet.cz>
> 
> In the firmware, the promisc mode overrides the multicast mode.
> So when the promisc mode is turned off, driver must check if the
> multicast mode was active before and conditionally reactivate it.
> 

Can you please add fixes & stable tags.

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

<...>

Re: [PATCH 1/6] net/nfb: add missing libfdt dependency for build

[Ferruh Yigit]

On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> From: Martin Spinler <spinler@cesnet.cz>
> 
> The driver uses some FDT manipulation functions from libfdt.
> Let the build system check for libfdt package.
> 

I don't see 'libfdt.h' included by the driver, where/how libfdt
is used?

Also what do you think to document this external dependency
and its usage in the driver documentation?

> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---
>   drivers/net/nfb/meson.build | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/nfb/meson.build b/drivers/net/nfb/meson.build
> index bb5f66a09a..c080c06bf9 100644
> --- a/drivers/net/nfb/meson.build
> +++ b/drivers/net/nfb/meson.build
> @@ -9,6 +9,12 @@ if is_windows
>       subdir_done()
>   endif
>   
> +if has_libfdt == 0
> +    build = false
> +    reason = 'missing dependency, "libfdt"'
> +    subdir_done()
> +endif
> +
>   dep = dependency('netcope-common', required: false, method: 'pkg-config')
>   reason = 'missing dependency, "libnfb"'
>   build = dep.found()

Re: [PATCH 6/6] drivers/nfb: add support for more MAC addresses

[Ferruh Yigit]

On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> From: Martin Spinler <spinler@cesnet.cz>
> 
> Extend the eth_dev_ops by add/remove MAC address functions.
> 
> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> ---
>   drivers/net/nfb/nfb_ethdev.c | 69 ++++++++++++++++++++++++++++++------
>   1 file changed, 58 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
> index 5d503e131a..7dec8e022e 100644
> --- a/drivers/net/nfb/nfb_ethdev.c
> +++ b/drivers/net/nfb/nfb_ethdev.c
> @@ -214,7 +214,19 @@ static int
>   nfb_eth_dev_info(struct rte_eth_dev *dev,
>   	struct rte_eth_dev_info *dev_info)
>   {
> -	dev_info->max_mac_addrs = 1;
> +	uint16_t i;
> +	uint32_t max_mac_addrs;
> +	struct pmd_internals *internals = dev->data->dev_private;
> +
> +	dev_info->max_mac_addrs = (uint32_t)-1;
> +	for (i = 0; i < internals->max_rxmac; i++) {
> +		max_mac_addrs = nc_rxmac_mac_address_count(internals->rxmac[i]);
> +		dev_info->max_mac_addrs = RTE_MIN(max_mac_addrs,
> +				dev_info->max_mac_addrs);
> +	}
> +	if (internals->max_rxmac == 0)
> +		dev_info->max_mac_addrs = 0;
> +

Not sure if 'max_mac_addrs = 0' is valid value, as far as I can see
driver allocates memory for at least one MAC address, so there is no
case that NIC doesn't support any MAC address.

It looks like max_mac_addrs usage is not clear, what is exactly the
'max_rxmac' & 'rxmac[]' variables are?

>   	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;
> @@ -376,6 +388,18 @@ nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
>   	return 0;
>   }
>   
> +static uint64_t
> +nfb_eth_mac_addr_conv(struct rte_ether_addr *mac_addr)
> +{
> +	int i;
> +	uint64_t res = 0;
> +	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
> +		res <<= 8;
> +		res |= mac_addr->addr_bytes[i] & 0xFF;
> +	}
> +	return res;
> +}
> +
>   /**
>    * DPDK callback to set primary MAC address.
>    *
> @@ -392,26 +416,47 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
>   	struct rte_ether_addr *mac_addr)
>   {
>   	unsigned int i;
> -	uint64_t mac = 0;
> +	uint64_t mac;
>   	struct rte_eth_dev_data *data = dev->data;
>   	struct pmd_internals *internals = (struct pmd_internals *)
>   		data->dev_private;
>   
> -	if (!rte_is_valid_assigned_ether_addr(mac_addr))
> -		return -EINVAL;
> +	mac = nfb_eth_mac_addr_conv(mac_addr);
> +	for (i = 0; i < internals->max_rxmac; ++i)
> +		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);

This functions is to set default MAC address 'data->mac_addrs[0]',
why same MAC set multiple times in a loop?

>   
> -	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
> -		mac <<= 8;
> -		mac |= mac_addr->addr_bytes[i] & 0xFF;
> -	}
> +	return 0;
> +}
>   
> +static int
> +nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
> +	struct rte_ether_addr *mac_addr, uint32_t index, uint32_t pool __rte_unused)
> +{
> +	unsigned int i;
> +	uint64_t mac = 0;
> +	struct rte_eth_dev_data *data = dev->data;
> +	struct pmd_internals *internals = (struct pmd_internals *)
> +		data->dev_private;
> +
> +	mac = nfb_eth_mac_addr_conv(mac_addr);
>   	for (i = 0; i < internals->max_rxmac; ++i)
> -		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
> +		nc_rxmac_set_mac(internals->rxmac[i], index, mac, 1);
>   
> -	rte_ether_addr_copy(mac_addr, data->mac_addrs);
>   	return 0;
>   }
>   
> +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;
> +
> +	for (i = 0; i < internals->max_rxmac; ++i)
> +		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
> +}
> +
>   static const struct eth_dev_ops ops = {
>   	.dev_start = nfb_eth_dev_start,
>   	.dev_stop = nfb_eth_dev_stop,
> @@ -436,6 +481,8 @@ static const struct eth_dev_ops ops = {
>   	.stats_get = nfb_eth_stats_get,
>   	.stats_reset = nfb_eth_stats_reset,
>   	.mac_addr_set = nfb_eth_mac_addr_set,
> +	.mac_addr_add = nfb_eth_mac_addr_add,
> +	.mac_addr_remove = nfb_eth_mac_addr_remove,
>   };
>   
>   /**
> @@ -530,7 +577,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
>   	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_eth_dev_default_mac_addr_set(dev->data->port_id, &eth_addr_init);
>   

I didn't get this change, why calling the API from the driver,
instead of calling the dev_ops directly as original code did?

>   	data->promiscuous = nfb_eth_promiscuous_get(dev);
>   	data->all_multicast = nfb_eth_allmulticast_get(dev);

Re: [PATCH 2/6] drivers/nfb: fix array indexes in deinit functions

[Martin Spinler]

Hi Ferruh,
thanks for all comments in this series.

On Mon, 2022-02-14 at 13:34 +0000, Ferruh Yigit wrote:
> On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> > From: Martin Spinler <spinler@cesnet.cz>
> > 
> > The indexes in the for cycle were wrongly used and
> > the code accessed outside of the rxmac/txmac array.
> > 
> 
> can you please add fixes tag, to help backport.
> Also please add stable tag to request backport.

Tags will be added in v2.

> 
> > Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> > ---
> >   drivers/net/nfb/nfb_ethdev.c | 14 ++++++++------
> >   1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
> > index 3c39937816..0b27fe78cc 100644
> > --- a/drivers/net/nfb/nfb_ethdev.c
> > +++ b/drivers/net/nfb/nfb_ethdev.c
> > @@ -77,9 +77,10 @@ static void
> >   nfb_nc_rxmac_deinit(struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC],
> >   	uint16_t max_rxmac)
> >   {
> > -	for (; max_rxmac > 0; --max_rxmac) {
> > -		nc_rxmac_close(rxmac[max_rxmac]);
> > -		rxmac[max_rxmac] = NULL;
> > +	uint16_t i;
> > +	for (i = 0; i < max_rxmac; i++) {
> > +		nc_rxmac_close(rxmac[i]);
> > +		rxmac[i] = NULL;
> >   	}
> >   }
> >   
> > @@ -95,9 +96,10 @@ static void
> >   nfb_nc_txmac_deinit(struct nc_txmac *txmac[RTE_MAX_NC_TXMAC],
> >   	uint16_t max_txmac)
> >   {
> > -	for (; max_txmac > 0; --max_txmac) {
> > -		nc_txmac_close(txmac[max_txmac]);
> > -		txmac[max_txmac] = NULL;
> > +	uint16_t i;
> > +	for (i = 0; i < max_txmac; i++) {
> > +		nc_txmac_close(txmac[i]);
> > +		txmac[i] = NULL;
> >   	}
> >   }
> >   
> 

Re: [PATCH 3/6] drivers/nfb: do not report zero-sized TX burst

[Martin Spinler]

On Mon, 2022-02-14 at 13:35 +0000, Ferruh Yigit wrote:
> On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> > From: Martin Spinler <spinler@cesnet.cz>
> > 
> > Zero-sized TX burst floods the log no more.
> > 
> 
> If you want this patch to be backported, you can update
> commit log as fix and add fixes & stable tags.

I don't think it is a fix, rather an unplesant behaviour in a specific
application. It doesn't happen in the test-pmd app.

> 
> > Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> > ---
> >   drivers/net/nfb/nfb_tx.h | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/nfb/nfb_tx.h b/drivers/net/nfb/nfb_tx.h
> > index d3cbe3e6b3..910020e9e9 100644
> > --- a/drivers/net/nfb/nfb_tx.h
> > +++ b/drivers/net/nfb/nfb_tx.h
> > @@ -136,7 +136,10 @@ nfb_eth_ndp_tx(void *queue,
> >   
> >   	struct ndp_packet packets[nb_pkts];
> >   
> > -	if (unlikely(ndp->queue == NULL || nb_pkts == 0)) {
> > +	if (unlikely(nb_pkts == 0))
> > +		return 0;
> > +
> > +	if (unlikely(ndp->queue == NULL)) {
> >   		RTE_LOG(ERR, PMD, "TX invalid arguments!\n");
> >   		return 0;
> >   	}
> 

Re: [PATCH 1/6] net/nfb: add missing libfdt dependency for build

[Martin Spinler]

On Mon, 2022-02-14 at 13:36 +0000, Ferruh Yigit wrote:
> On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> > From: Martin Spinler <spinler@cesnet.cz>
> > 
> > The driver uses some FDT manipulation functions from libfdt.
> > Let the build system check for libfdt package.
> > 
> 
> I don't see 'libfdt.h' included by the driver, where/how libfdt
> is used?

I've prepared this dependency for the future commit (fw_version_get
feature), but i've omit it from this series for now due to internal
discussion about versioning (can take a week or two) and doesn't
realise that this 'libfdt' commit is now needless. 

The second fact is, it will not pass even in the current form without
libfdt, because the netcope-common package (precisely the libnetcope
headers included in DPDK driver) doesn't specify the libfdt-devel
dependency. But this is a defect in the netcope-common package and
should be fixed there.

Anyway, I can remove this patch from series, but not sure if it will
not confuse the Patchwork series/version matching as this is the first
patch.

Does it make sense to you to remove this patch from this series
completely?

> 
> Also what do you think to document this external dependency
> and its usage in the driver documentation?
> 
> > Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> > ---
> >   drivers/net/nfb/meson.build | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/nfb/meson.build b/drivers/net/nfb/meson.build
> > index bb5f66a09a..c080c06bf9 100644
> > --- a/drivers/net/nfb/meson.build
> > +++ b/drivers/net/nfb/meson.build
> > @@ -9,6 +9,12 @@ if is_windows
> >       subdir_done()
> >   endif
> >   
> > +if has_libfdt == 0
> > +    build = false
> > +    reason = 'missing dependency, "libfdt"'
> > +    subdir_done()
> > +endif
> > +
> >   dep = dependency('netcope-common', required: false, method: 'pkg-config')
> >   reason = 'missing dependency, "libnfb"'
> >   build = dep.found()
> 

Re: [PATCH 5/6] drivers/nfb: fix multicast/promiscuous mode switching

[Martin Spinler]

On Mon, 2022-02-14 at 13:36 +0000, Ferruh Yigit wrote:
> On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> > From: Martin Spinler <spinler@cesnet.cz>
> > 
> > In the firmware, the promisc mode overrides the multicast mode.
> > So when the promisc mode is turned off, driver must check if the
> > multicast mode was active before and conditionally reactivate it.
> > 
> 
> Can you please add fixes & stable tags.

Tags will be added in v2.

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

Re: [PATCH 6/6] drivers/nfb: add support for more MAC addresses

[Martin Spinler]

On Mon, 2022-02-14 at 13:37 +0000, Ferruh Yigit wrote:
> On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
> > From: Martin Spinler <spinler@cesnet.cz>
> > 
> > Extend the eth_dev_ops by add/remove MAC address functions.
> > 
> > Signed-off-by: Martin Spinler <spinler@cesnet.cz>
> > ---
> >   drivers/net/nfb/nfb_ethdev.c | 69 ++++++++++++++++++++++++++++++------
> >   1 file changed, 58 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
> > index 5d503e131a..7dec8e022e 100644
> > --- a/drivers/net/nfb/nfb_ethdev.c
> > +++ b/drivers/net/nfb/nfb_ethdev.c
> > @@ -214,7 +214,19 @@ static int
> >   nfb_eth_dev_info(struct rte_eth_dev *dev,
> >   	struct rte_eth_dev_info *dev_info)
> >   {
> > -	dev_info->max_mac_addrs = 1;
> > +	uint16_t i;
> > +	uint32_t max_mac_addrs;
> > +	struct pmd_internals *internals = dev->data->dev_private;
> > +
> > +	dev_info->max_mac_addrs = (uint32_t)-1;
> > +	for (i = 0; i < internals->max_rxmac; i++) {
> > +		max_mac_addrs = nc_rxmac_mac_address_count(internals->rxmac[i]);
> > +		dev_info->max_mac_addrs = RTE_MIN(max_mac_addrs,
> > +				dev_info->max_mac_addrs);
> > +	}
> > +	if (internals->max_rxmac == 0)
> > +		dev_info->max_mac_addrs = 0;
> > +
> 
> Not sure if 'max_mac_addrs = 0' is valid value, as far as I can see
> driver allocates memory for at least one MAC address, so there is no
> case that NIC doesn't support any MAC address.

Oh, sorry, I missed that, will be fixed in v2.

> 
> It looks like max_mac_addrs usage is not clear, what is exactly the
> 'max_rxmac' & 'rxmac[]' variables are?

rxmac is a firmware unit which represents the RX side of one physical
Ethernet port (more precisely one logical Eth channel on port) and
handles stats+CRC check+MAC filtering. So this goes through all those
units and finds the rxmac with minimal memory space for MAC addresses
and uses this value.

I'll add few comments in the code to v2.

> 
> >   	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;
> > @@ -376,6 +388,18 @@ nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
> >   	return 0;
> >   }
> >   
> > +static uint64_t
> > +nfb_eth_mac_addr_conv(struct rte_ether_addr *mac_addr)
> > +{
> > +	int i;
> > +	uint64_t res = 0;
> > +	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
> > +		res <<= 8;
> > +		res |= mac_addr->addr_bytes[i] & 0xFF;
> > +	}
> > +	return res;
> > +}
> > +
> >   /**
> >    * DPDK callback to set primary MAC address.
> >    *
> > @@ -392,26 +416,47 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
> >   	struct rte_ether_addr *mac_addr)
> >   {
> >   	unsigned int i;
> > -	uint64_t mac = 0;
> > +	uint64_t mac;
> >   	struct rte_eth_dev_data *data = dev->data;
> >   	struct pmd_internals *internals = (struct pmd_internals *)
> >   		data->dev_private;
> >   
> > -	if (!rte_is_valid_assigned_ether_addr(mac_addr))
> > -		return -EINVAL;
> > +	mac = nfb_eth_mac_addr_conv(mac_addr);
> > +	for (i = 0; i < internals->max_rxmac; ++i)
> > +		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
> 
> This functions is to set default MAC address 'data->mac_addrs[0]',
> why same MAC set multiple times in a loop?

Unfortunately, currently we don't have firmware support for proper
separate port configuration, because DMA queues aren't bound to
specific rxmac and traffic can be mixed between them. So I think the
best way in this case is to write the same MAC address into all of the
rxmacs inside firmware.

> 
> >   
> > -	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
> > -		mac <<= 8;
> > -		mac |= mac_addr->addr_bytes[i] & 0xFF;
> > -	}
> > +	return 0;
> > +}
> >   
> > +static int
> > +nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
> > +	struct rte_ether_addr *mac_addr, uint32_t index, uint32_t pool __rte_unused)
> > +{
> > +	unsigned int i;
> > +	uint64_t mac = 0;
> > +	struct rte_eth_dev_data *data = dev->data;
> > +	struct pmd_internals *internals = (struct pmd_internals *)
> > +		data->dev_private;
> > +
> > +	mac = nfb_eth_mac_addr_conv(mac_addr);
> >   	for (i = 0; i < internals->max_rxmac; ++i)
> > -		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
> > +		nc_rxmac_set_mac(internals->rxmac[i], index, mac, 1);
> >   
> > -	rte_ether_addr_copy(mac_addr, data->mac_addrs);
> >   	return 0;
> >   }
> >   
> > +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;
> > +
> > +	for (i = 0; i < internals->max_rxmac; ++i)
> > +		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
> > +}
> > +
> >   static const struct eth_dev_ops ops = {
> >   	.dev_start = nfb_eth_dev_start,
> >   	.dev_stop = nfb_eth_dev_stop,
> > @@ -436,6 +481,8 @@ static const struct eth_dev_ops ops = {
> >   	.stats_get = nfb_eth_stats_get,
> >   	.stats_reset = nfb_eth_stats_reset,
> >   	.mac_addr_set = nfb_eth_mac_addr_set,
> > +	.mac_addr_add = nfb_eth_mac_addr_add,
> > +	.mac_addr_remove = nfb_eth_mac_addr_remove,
> >   };
> >   
> >   /**
> > @@ -530,7 +577,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
> >   	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_eth_dev_default_mac_addr_set(dev->data->port_id, &eth_addr_init);
> >   
> 
> I didn't get this change, why calling the API from the driver,
> instead of calling the dev_ops directly as original code did?

The API does all the checks and copies the MAC value into internal data
struct, so our code was duplicit. I didn't realize that calling the API
could be problem. I assume it should be reverted to the prev form?

> 
> >   	data->promiscuous = nfb_eth_promiscuous_get(dev);
> >   	data->all_multicast = nfb_eth_allmulticast_get(dev);
> 

Re: [PATCH 3/6] drivers/nfb: do not report zero-sized TX burst

[Ferruh Yigit]

On 2/14/2022 4:53 PM, Martin Spinler wrote:
> On Mon, 2022-02-14 at 13:35 +0000, Ferruh Yigit wrote:
>> On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
>>> From: Martin Spinler <spinler@cesnet.cz>
>>>
>>> Zero-sized TX burst floods the log no more.
>>>
>>
>> If you want this patch to be backported, you can update
>> commit log as fix and add fixes & stable tags.
> 
> I don't think it is a fix, rather an unplesant behaviour in a specific
> application. It doesn't happen in the test-pmd app.
> 

If you don't want to backport, OK to keep as it is.

>>
>>> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
>>> ---
>>>    drivers/net/nfb/nfb_tx.h | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/nfb/nfb_tx.h b/drivers/net/nfb/nfb_tx.h
>>> index d3cbe3e6b3..910020e9e9 100644
>>> --- a/drivers/net/nfb/nfb_tx.h
>>> +++ b/drivers/net/nfb/nfb_tx.h
>>> @@ -136,7 +136,10 @@ nfb_eth_ndp_tx(void *queue,
>>>    
>>>    	struct ndp_packet packets[nb_pkts];
>>>    
>>> -	if (unlikely(ndp->queue == NULL || nb_pkts == 0)) {
>>> +	if (unlikely(nb_pkts == 0))
>>> +		return 0;
>>> +
>>> +	if (unlikely(ndp->queue == NULL)) {
>>>    		RTE_LOG(ERR, PMD, "TX invalid arguments!\n");
>>>    		return 0;
>>>    	}
>>
> 

Re: [PATCH 6/6] drivers/nfb: add support for more MAC addresses

[Ferruh Yigit]

On 2/14/2022 4:53 PM, Martin Spinler wrote:
> On Mon, 2022-02-14 at 13:37 +0000, Ferruh Yigit wrote:
>> On 2/14/2022 11:25 AM, spinler@cesnet.cz wrote:
>>> From: Martin Spinler <spinler@cesnet.cz>
>>>
>>> Extend the eth_dev_ops by add/remove MAC address functions.
>>>
>>> Signed-off-by: Martin Spinler <spinler@cesnet.cz>
>>> ---
>>>    drivers/net/nfb/nfb_ethdev.c | 69 ++++++++++++++++++++++++++++++------
>>>    1 file changed, 58 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
>>> index 5d503e131a..7dec8e022e 100644
>>> --- a/drivers/net/nfb/nfb_ethdev.c
>>> +++ b/drivers/net/nfb/nfb_ethdev.c
>>> @@ -214,7 +214,19 @@ static int
>>>    nfb_eth_dev_info(struct rte_eth_dev *dev,
>>>    	struct rte_eth_dev_info *dev_info)
>>>    {
>>> -	dev_info->max_mac_addrs = 1;
>>> +	uint16_t i;
>>> +	uint32_t max_mac_addrs;
>>> +	struct pmd_internals *internals = dev->data->dev_private;
>>> +
>>> +	dev_info->max_mac_addrs = (uint32_t)-1;
>>> +	for (i = 0; i < internals->max_rxmac; i++) {
>>> +		max_mac_addrs = nc_rxmac_mac_address_count(internals->rxmac[i]);
>>> +		dev_info->max_mac_addrs = RTE_MIN(max_mac_addrs,
>>> +				dev_info->max_mac_addrs);
>>> +	}
>>> +	if (internals->max_rxmac == 0)
>>> +		dev_info->max_mac_addrs = 0;
>>> +
>>
>> Not sure if 'max_mac_addrs = 0' is valid value, as far as I can see
>> driver allocates memory for at least one MAC address, so there is no
>> case that NIC doesn't support any MAC address.
> 
> Oh, sorry, I missed that, will be fixed in v2.
> 
>>
>> It looks like max_mac_addrs usage is not clear, what is exactly the
>> 'max_rxmac' & 'rxmac[]' variables are?
> 
> rxmac is a firmware unit which represents the RX side of one physical
> Ethernet port (more precisely one logical Eth channel on port) and
> handles stats+CRC check+MAC filtering. So this goes through all those
> units and finds the rxmac with minimal memory space for MAC addresses
> and uses this value.
> 

Got it.

> I'll add few comments in the code to v2.
> 

Thanks.

>>
>>>    	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;
>>> @@ -376,6 +388,18 @@ nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
>>>    	return 0;
>>>    }
>>>    
>>> +static uint64_t
>>> +nfb_eth_mac_addr_conv(struct rte_ether_addr *mac_addr)
>>> +{
>>> +	int i;
>>> +	uint64_t res = 0;
>>> +	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
>>> +		res <<= 8;
>>> +		res |= mac_addr->addr_bytes[i] & 0xFF;
>>> +	}
>>> +	return res;
>>> +}
>>> +
>>>    /**
>>>     * DPDK callback to set primary MAC address.
>>>     *
>>> @@ -392,26 +416,47 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
>>>    	struct rte_ether_addr *mac_addr)
>>>    {
>>>    	unsigned int i;
>>> -	uint64_t mac = 0;
>>> +	uint64_t mac;
>>>    	struct rte_eth_dev_data *data = dev->data;
>>>    	struct pmd_internals *internals = (struct pmd_internals *)
>>>    		data->dev_private;
>>>    
>>> -	if (!rte_is_valid_assigned_ether_addr(mac_addr))
>>> -		return -EINVAL;
>>> +	mac = nfb_eth_mac_addr_conv(mac_addr);
>>> +	for (i = 0; i < internals->max_rxmac; ++i)
>>> +		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
>>
>> This functions is to set default MAC address 'data->mac_addrs[0]',
>> why same MAC set multiple times in a loop?
> 
> Unfortunately, currently we don't have firmware support for proper
> separate port configuration, because DMA queues aren't bound to
> specific rxmac and traffic can be mixed between them. So I think the
> best way in this case is to write the same MAC address into all of the
> rxmacs inside firmware.
> 

I see the usage now, that looks OK.

>>
>>>    
>>> -	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
>>> -		mac <<= 8;
>>> -		mac |= mac_addr->addr_bytes[i] & 0xFF;
>>> -	}
>>> +	return 0;
>>> +}
>>>    
>>> +static int
>>> +nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
>>> +	struct rte_ether_addr *mac_addr, uint32_t index, uint32_t pool __rte_unused)
>>> +{
>>> +	unsigned int i;
>>> +	uint64_t mac = 0;
>>> +	struct rte_eth_dev_data *data = dev->data;
>>> +	struct pmd_internals *internals = (struct pmd_internals *)
>>> +		data->dev_private;
>>> +
>>> +	mac = nfb_eth_mac_addr_conv(mac_addr);
>>>    	for (i = 0; i < internals->max_rxmac; ++i)
>>> -		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
>>> +		nc_rxmac_set_mac(internals->rxmac[i], index, mac, 1);
>>>    
>>> -	rte_ether_addr_copy(mac_addr, data->mac_addrs);
>>>    	return 0;
>>>    }
>>>    
>>> +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;
>>> +
>>> +	for (i = 0; i < internals->max_rxmac; ++i)
>>> +		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
>>> +}
>>> +
>>>    static const struct eth_dev_ops ops = {
>>>    	.dev_start = nfb_eth_dev_start,
>>>    	.dev_stop = nfb_eth_dev_stop,
>>> @@ -436,6 +481,8 @@ static const struct eth_dev_ops ops = {
>>>    	.stats_get = nfb_eth_stats_get,
>>>    	.stats_reset = nfb_eth_stats_reset,
>>>    	.mac_addr_set = nfb_eth_mac_addr_set,
>>> +	.mac_addr_add = nfb_eth_mac_addr_add,
>>> +	.mac_addr_remove = nfb_eth_mac_addr_remove,
>>>    };
>>>    
>>>    /**
>>> @@ -530,7 +577,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
>>>    	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_eth_dev_default_mac_addr_set(dev->data->port_id, &eth_addr_init);
>>>    
>>
>> I didn't get this change, why calling the API from the driver,
>> instead of calling the dev_ops directly as original code did?
> 
> The API does all the checks and copies the MAC value into internal data
> struct, so our code was duplicit. I didn't realize that calling the API
> could be problem. I assume it should be reverted to the prev form?
> 

It is not a problem, and APIs are used because of the reason you
mentioned in a few places, but for this case I didn't get it,
what API check is required?
Is it 'rte_is_valid_assigned_ether_addr()' check? The mac in this
function ('nfb_eth_dev_init()') is a hardcoded one, instead of
validity check, why not select a valid MAC at this stage?

I mean still drop the 'rte_is_valid_assigned_ether_addr()' check
from 'nfb_eth_mac_addr_set()', but be sure 'eth_addr_init' is
valid MAC, will it work?

>>
>>>    	data->promiscuous = nfb_eth_promiscuous_get(dev);
>>>    	data->all_multicast = nfb_eth_allmulticast_get(dev);
>>
> 

[PATCH v2 0/5] net/nfb: add missing libfdt dependency for build

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

This series introduces standard RX timestamp offload flag support
together with multiple MAC addresses support for NFB devices.
Also fixes a promisc/multicast switch issue and minor annoyances.
---

v2:
 * Removed libfdt dependency patch from series, is needless.
 * Added the Fixes/CC-stable tags to 2 patches.
 * Reworked support for more MAC addresses: extracted
   nfb_eth_get_max_mac_address_count, fixed allocation, added
   comments.
 * Removed documentation reference to custom timestamp parameter.

 doc/guides/nics/nfb.rst      |   8 +--
 drivers/net/nfb/nfb.h        |   7 +-
 drivers/net/nfb/nfb_ethdev.c | 124 +++++++++++++++++++++++++++++------
 drivers/net/nfb/nfb_rx.c     |  53 ---------------
 drivers/net/nfb/nfb_rx.h     |   7 +-
 drivers/net/nfb/nfb_rxmode.c |  20 +++---
 drivers/net/nfb/nfb_tx.h     |   5 +-
 7 files changed, 119 insertions(+), 105 deletions(-)

-- 
2.35.1

[PATCH v2 1/5] net/nfb: fix array indexes in deinit functions

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

The indexes in the for cycle were wrongly used and
the code accessed outside of the rxmac/txmac array.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: cernay@netcope.com
Cc: stable@dpdk.org
Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb_ethdev.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 3c39937816..0b27fe78cc 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -77,9 +77,10 @@ static void
 nfb_nc_rxmac_deinit(struct nc_rxmac *rxmac[RTE_MAX_NC_RXMAC],
 	uint16_t max_rxmac)
 {
-	for (; max_rxmac > 0; --max_rxmac) {
-		nc_rxmac_close(rxmac[max_rxmac]);
-		rxmac[max_rxmac] = NULL;
+	uint16_t i;
+	for (i = 0; i < max_rxmac; i++) {
+		nc_rxmac_close(rxmac[i]);
+		rxmac[i] = NULL;
 	}
 }
 
@@ -95,9 +96,10 @@ static void
 nfb_nc_txmac_deinit(struct nc_txmac *txmac[RTE_MAX_NC_TXMAC],
 	uint16_t max_txmac)
 {
-	for (; max_txmac > 0; --max_txmac) {
-		nc_txmac_close(txmac[max_txmac]);
-		txmac[max_txmac] = NULL;
+	uint16_t i;
+	for (i = 0; i < max_txmac; i++) {
+		nc_txmac_close(txmac[i]);
+		txmac[i] = NULL;
 	}
 }
 
-- 
2.35.1

[PATCH v2 2/5] net/nfb: do not report zero-sized TX burst

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

Zero-sized TX burst floods the log no more.

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

diff --git a/drivers/net/nfb/nfb_tx.h b/drivers/net/nfb/nfb_tx.h
index d3cbe3e6b3..910020e9e9 100644
--- a/drivers/net/nfb/nfb_tx.h
+++ b/drivers/net/nfb/nfb_tx.h
@@ -136,7 +136,10 @@ nfb_eth_ndp_tx(void *queue,
 
 	struct ndp_packet packets[nb_pkts];
 
-	if (unlikely(ndp->queue == NULL || nb_pkts == 0)) {
+	if (unlikely(nb_pkts == 0))
+		return 0;
+
+	if (unlikely(ndp->queue == NULL)) {
 		RTE_LOG(ERR, PMD, "TX invalid arguments!\n");
 		return 0;
 	}
-- 
2.35.1

[PATCH v2 3/5] net/nfb: use RTE_ETH_RX_OFFLOAD_TIMESTAMP flag

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

Rewrite the RX timestamp setup code to use standard offload flag.

Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 doc/guides/nics/nfb.rst      |  8 ++----
 drivers/net/nfb/nfb.h        |  3 +-
 drivers/net/nfb/nfb_ethdev.c | 19 ++++++++++++-
 drivers/net/nfb/nfb_rx.c     | 53 ------------------------------------
 drivers/net/nfb/nfb_rx.h     |  7 +----
 5 files changed, 22 insertions(+), 68 deletions(-)

diff --git a/doc/guides/nics/nfb.rst b/doc/guides/nics/nfb.rst
index 14560d38e4..96aaf64440 100644
--- a/doc/guides/nics/nfb.rst
+++ b/doc/guides/nics/nfb.rst
@@ -59,13 +59,9 @@ Timestamps
 
 The PMD supports hardware timestamps of frame receipt on physical network interface. In order to use
 the timestamps, the hardware timestamping unit must be enabled (follow the documentation of the NFB
-products) and the device argument `timestamp=1` must be used.
+products). The standard `RTE_ETH_RX_OFFLOAD_TIMESTAMP` flag can be used for this feature.
 
-.. code-block:: console
-
-    ./<build_dir>/app/dpdk-testpmd -a b3:00.0,timestamp=1 <other EAL params> -- <testpmd params>
-
-When the timestamps are enabled with the *devarg*, a timestamp validity flag is set in the MBUFs
+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
diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index 59d3ab4986..4de9006ac0 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -37,8 +37,7 @@
 #define RTE_NFB_DRIVER_NAME net_nfb
 
 /* Device arguments */
-#define TIMESTAMP_ARG  "timestamp"
-static const char * const VALID_KEYS[] = {TIMESTAMP_ARG, NULL};
+static const char * const VALID_KEYS[] = {NULL};
 
 struct pmd_internals {
 	uint16_t         max_rxmac;
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 0b27fe78cc..53a98642b3 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -183,6 +183,22 @@ nfb_eth_dev_stop(struct rte_eth_dev *dev)
 static int
 nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 {
+	int ret;
+	struct pmd_internals *internals = dev->data->dev_private;
+	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
+
+	if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
+		ret = rte_mbuf_dyn_rx_timestamp_register
+				(&nfb_timestamp_dynfield_offset,
+				&nfb_timestamp_rx_dynflag);
+		if (ret != 0) {
+			RTE_LOG(ERR, PMD, "Cannot register Rx timestamp"
+					" field/flag %d\n", ret);
+			nfb_close(internals->nfb);
+			return -rte_errno;
+		}
+	}
+
 	return 0;
 }
 
@@ -203,6 +219,8 @@ nfb_eth_dev_info(struct rte_eth_dev *dev,
 	dev_info->max_rx_queues = dev->data->nb_rx_queues;
 	dev_info->max_tx_queues = dev->data->nb_tx_queues;
 	dev_info->speed_capa = RTE_ETH_LINK_SPEED_100G;
+	dev_info->rx_offload_capa =
+		RTE_ETH_RX_OFFLOAD_TIMESTAMP;
 
 	return 0;
 }
@@ -609,4 +627,3 @@ static struct rte_pci_driver nfb_eth_driver = {
 RTE_PMD_REGISTER_PCI(RTE_NFB_DRIVER_NAME, nfb_eth_driver);
 RTE_PMD_REGISTER_PCI_TABLE(RTE_NFB_DRIVER_NAME, nfb_pci_id_table);
 RTE_PMD_REGISTER_KMOD_DEP(RTE_NFB_DRIVER_NAME, "* nfb");
-RTE_PMD_REGISTER_PARAM_STRING(RTE_NFB_DRIVER_NAME, TIMESTAMP_ARG "=<0|1>");
diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
index f76e2ba646..8a9b232305 100644
--- a/drivers/net/nfb/nfb_rx.c
+++ b/drivers/net/nfb/nfb_rx.c
@@ -12,56 +12,6 @@
 uint64_t nfb_timestamp_rx_dynflag;
 int nfb_timestamp_dynfield_offset = -1;
 
-static int
-timestamp_check_handler(__rte_unused const char *key,
-	const char *value, __rte_unused void *opaque)
-{
-	if (strcmp(value, "1"))
-		return -1;
-
-	return 0;
-}
-
-
-static int
-nfb_check_timestamp(struct rte_devargs *devargs)
-{
-	struct rte_kvargs *kvlist;
-	int ret;
-
-	if (devargs == NULL)
-		return 0;
-
-	kvlist = rte_kvargs_parse(devargs->args, NULL);
-	if (kvlist == NULL)
-		return 0;
-
-	if (!rte_kvargs_count(kvlist, TIMESTAMP_ARG)) {
-		rte_kvargs_free(kvlist);
-		return 0;
-	}
-	/* Timestamps are enabled when there is
-	 * key-value pair: enable_timestamp=1
-	 * TODO: timestamp should be enabled with RTE_ETH_RX_OFFLOAD_TIMESTAMP
-	 */
-	if (rte_kvargs_process(kvlist, TIMESTAMP_ARG,
-		timestamp_check_handler, NULL) < 0) {
-		rte_kvargs_free(kvlist);
-		return 0;
-	}
-	rte_kvargs_free(kvlist);
-
-	ret = rte_mbuf_dyn_rx_timestamp_register(
-			&nfb_timestamp_dynfield_offset,
-			&nfb_timestamp_rx_dynflag);
-	if (ret != 0) {
-		RTE_LOG(ERR, PMD, "Cannot register Rx timestamp field/flag\n");
-		return -rte_errno;
-	}
-
-	return 1;
-}
-
 int
 nfb_eth_rx_queue_start(struct rte_eth_dev *dev, uint16_t rxq_id)
 {
@@ -138,9 +88,6 @@ nfb_eth_rx_queue_setup(struct rte_eth_dev *dev,
 	else
 		rte_free(rxq);
 
-	if (nfb_check_timestamp(dev->device->devargs) > 0)
-		rxq->flags |= NFB_TIMESTAMP_FLAG;
-
 	return ret;
 }
 
diff --git a/drivers/net/nfb/nfb_rx.h b/drivers/net/nfb/nfb_rx.h
index 638205d53c..b618682e13 100644
--- a/drivers/net/nfb/nfb_rx.h
+++ b/drivers/net/nfb/nfb_rx.h
@@ -14,8 +14,6 @@
 #include <rte_mbuf_dyn.h>
 #include <rte_ethdev.h>
 
-#define NFB_TIMESTAMP_FLAG (1 << 0)
-
 extern uint64_t nfb_timestamp_rx_dynflag;
 extern int nfb_timestamp_dynfield_offset;
 
@@ -145,7 +143,6 @@ nfb_eth_ndp_rx(void *queue,
 	uint16_t nb_pkts)
 {
 	struct ndp_rx_queue *ndp = queue;
-	uint8_t timestamping_enabled;
 	uint16_t packet_size;
 	uint64_t num_bytes = 0;
 	uint16_t num_rx;
@@ -163,8 +160,6 @@ nfb_eth_ndp_rx(void *queue,
 		return 0;
 	}
 
-	timestamping_enabled = ndp->flags & NFB_TIMESTAMP_FLAG;
-
 	/* returns either all or nothing */
 	i = rte_pktmbuf_alloc_bulk(ndp->mb_pool, mbufs, nb_pkts);
 	if (unlikely(i != 0))
@@ -202,7 +197,7 @@ nfb_eth_ndp_rx(void *queue,
 			mbuf->port = ndp->in_port;
 			mbuf->ol_flags = 0;
 
-			if (timestamping_enabled) {
+			if (nfb_timestamp_dynfield_offset >= 0) {
 				rte_mbuf_timestamp_t timestamp;
 
 				/* nanoseconds */
-- 
2.35.1

[PATCH v2 4/5] net/nfb: fix multicast/promiscuous mode switching

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

In the firmware, the promisc mode overrides the multicast mode.
So when the promisc mode is turned off, driver must check if the
multicast mode was active before and conditionally reactivate it.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: cernay@netcope.com
Cc: stable@dpdk.org
Signed-off-by: Martin Spinler <spinler@cesnet.cz>
---
 drivers/net/nfb/nfb.h        |  4 ----
 drivers/net/nfb/nfb_ethdev.c |  1 -
 drivers/net/nfb/nfb_rxmode.c | 20 ++++++++------------
 3 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/net/nfb/nfb.h b/drivers/net/nfb/nfb.h
index 4de9006ac0..7dc5bd29e4 100644
--- a/drivers/net/nfb/nfb.h
+++ b/drivers/net/nfb/nfb.h
@@ -47,10 +47,6 @@ struct pmd_internals {
 
 	char             nfb_dev[PATH_MAX];
 	struct nfb_device *nfb;
-	/* Place to remember if filter was promiscuous or filtering by table,
-	 * when disabling allmulticast
-	 */
-	enum nc_rxmac_mac_filter rx_filter_original;
 };
 
 #endif /* _NFB_H_ */
diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 53a98642b3..5d503e131a 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -534,7 +534,6 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 
 	data->promiscuous = nfb_eth_promiscuous_get(dev);
 	data->all_multicast = nfb_eth_allmulticast_get(dev);
-	internals->rx_filter_original = data->promiscuous;
 
 	dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
 
diff --git a/drivers/net/nfb/nfb_rxmode.c b/drivers/net/nfb/nfb_rxmode.c
index 2d0b613d21..ca6e4d5578 100644
--- a/drivers/net/nfb/nfb_rxmode.c
+++ b/drivers/net/nfb/nfb_rxmode.c
@@ -14,8 +14,6 @@ nfb_eth_promiscuous_enable(struct rte_eth_dev *dev)
 		dev->data->dev_private;
 	uint16_t i;
 
-	internals->rx_filter_original = RXMAC_MAC_FILTER_PROMISCUOUS;
-
 	for (i = 0; i < internals->max_rxmac; ++i) {
 		nc_rxmac_mac_filter_enable(internals->rxmac[i],
 			RXMAC_MAC_FILTER_PROMISCUOUS);
@@ -30,16 +28,13 @@ nfb_eth_promiscuous_disable(struct rte_eth_dev *dev)
 	struct pmd_internals *internals = (struct pmd_internals *)
 		dev->data->dev_private;
 	uint16_t i;
+	enum nc_rxmac_mac_filter filter = RXMAC_MAC_FILTER_TABLE_BCAST;
 
-	internals->rx_filter_original = RXMAC_MAC_FILTER_TABLE;
-
-	/* if promisc is not enabled, do nothing */
-	if (!nfb_eth_promiscuous_get(dev))
-		return 0;
+	if (dev->data->all_multicast)
+		filter = RXMAC_MAC_FILTER_TABLE_BCAST_MCAST;
 
 	for (i = 0; i < internals->max_rxmac; ++i) {
-		nc_rxmac_mac_filter_enable(internals->rxmac[i],
-			RXMAC_MAC_FILTER_TABLE);
+		nc_rxmac_mac_filter_enable(internals->rxmac[i], filter);
 	}
 
 	return 0;
@@ -67,6 +62,8 @@ nfb_eth_allmulticast_enable(struct rte_eth_dev *dev)
 		dev->data->dev_private;
 
 	uint16_t i;
+	if (dev->data->promiscuous)
+		return 0;
 	for (i = 0; i < internals->max_rxmac; ++i) {
 		nc_rxmac_mac_filter_enable(internals->rxmac[i],
 			RXMAC_MAC_FILTER_TABLE_BCAST_MCAST);
@@ -83,13 +80,12 @@ nfb_eth_allmulticast_disable(struct rte_eth_dev *dev)
 
 	uint16_t i;
 
-	/* if multicast is not enabled do nothing */
-	if (!nfb_eth_allmulticast_get(dev))
+	if (dev->data->promiscuous)
 		return 0;
 
 	for (i = 0; i < internals->max_rxmac; ++i) {
 		nc_rxmac_mac_filter_enable(internals->rxmac[i],
-			internals->rx_filter_original);
+			RXMAC_MAC_FILTER_TABLE_BCAST);
 	}
 
 	return 0;
-- 
2.35.1

[PATCH v2 5/5] net/nfb: add support for more MAC addresses

[spinler]

From: Martin Spinler <spinler@cesnet.cz>

Extend the eth_dev_ops by add/remove MAC address functions.

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

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index 5d503e131a..defd118bd0 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -202,6 +202,30 @@ nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 	return 0;
 }
 
+static uint32_t
+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;
+
+	/*
+	 * Go through all RX MAC components in firmware and find
+	 * the minimal indicated space size for MAC addresses.
+	 */
+	for (i = 0; i < internals->max_rxmac; i++) {
+		c = nc_rxmac_mac_address_count(internals->rxmac[i]);
+		ret = RTE_MIN(c, ret);
+	}
+
+	/* The driver must support at least 1 MAC address, pretend that */
+	if (internals->max_rxmac == 0 || ret == 0)
+		ret = 1;
+
+	return ret;
+}
+
 /**
  * DPDK callback to get information about the device.
  *
@@ -214,7 +238,8 @@ static int
 nfb_eth_dev_info(struct rte_eth_dev *dev,
 	struct rte_eth_dev_info *dev_info)
 {
-	dev_info->max_mac_addrs = 1;
+	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;
@@ -376,6 +401,18 @@ nfb_eth_dev_set_link_down(struct rte_eth_dev *dev)
 	return 0;
 }
 
+static uint64_t
+nfb_eth_mac_addr_conv(struct rte_ether_addr *mac_addr)
+{
+	int i;
+	uint64_t res = 0;
+	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
+		res <<= 8;
+		res |= mac_addr->addr_bytes[i] & 0xFF;
+	}
+	return res;
+}
+
 /**
  * DPDK callback to set primary MAC address.
  *
@@ -392,26 +429,48 @@ nfb_eth_mac_addr_set(struct rte_eth_dev *dev,
 	struct rte_ether_addr *mac_addr)
 {
 	unsigned int i;
-	uint64_t mac = 0;
+	uint64_t mac;
 	struct rte_eth_dev_data *data = dev->data;
 	struct pmd_internals *internals = (struct pmd_internals *)
 		data->dev_private;
 
-	if (!rte_is_valid_assigned_ether_addr(mac_addr))
-		return -EINVAL;
+	mac = nfb_eth_mac_addr_conv(mac_addr);
+	/* Until no real multi-port support, configure all RX MACs the same */
+	for (i = 0; i < internals->max_rxmac; ++i)
+		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
 
-	for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
-		mac <<= 8;
-		mac |= mac_addr->addr_bytes[i] & 0xFF;
-	}
+	return 0;
+}
 
+static int
+nfb_eth_mac_addr_add(struct rte_eth_dev *dev,
+	struct rte_ether_addr *mac_addr, uint32_t index, uint32_t pool __rte_unused)
+{
+	unsigned int i;
+	uint64_t mac;
+	struct rte_eth_dev_data *data = dev->data;
+	struct pmd_internals *internals = (struct pmd_internals *)
+		data->dev_private;
+
+	mac = nfb_eth_mac_addr_conv(mac_addr);
 	for (i = 0; i < internals->max_rxmac; ++i)
-		nc_rxmac_set_mac(internals->rxmac[i], 0, mac, 1);
+		nc_rxmac_set_mac(internals->rxmac[i], index, mac, 1);
 
-	rte_ether_addr_copy(mac_addr, data->mac_addrs);
 	return 0;
 }
 
+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;
+
+	for (i = 0; i < internals->max_rxmac; ++i)
+		nc_rxmac_set_mac(internals->rxmac[i], index, 0, 0);
+}
+
 static const struct eth_dev_ops ops = {
 	.dev_start = nfb_eth_dev_start,
 	.dev_stop = nfb_eth_dev_stop,
@@ -436,6 +495,8 @@ static const struct eth_dev_ops ops = {
 	.stats_get = nfb_eth_stats_get,
 	.stats_reset = nfb_eth_stats_reset,
 	.mac_addr_set = nfb_eth_mac_addr_set,
+	.mac_addr_add = nfb_eth_mac_addr_add,
+	.mac_addr_remove = nfb_eth_mac_addr_remove,
 };
 
 /**
@@ -450,6 +511,7 @@ static const struct eth_dev_ops ops = {
 static int
 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;
@@ -516,9 +578,10 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	/* Get link state */
 	nfb_eth_link_update(dev, 0);
 
-	/* Allocate space for one mac address */
-	data->mac_addrs = rte_zmalloc(data->name, sizeof(struct rte_ether_addr),
-		RTE_CACHE_LINE_SIZE);
+	/* 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) {
 		RTE_LOG(ERR, PMD, "Could not alloc space for MAC address!\n");
 		nfb_close(internals->nfb);
@@ -531,6 +594,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
 	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]);
 
 	data->promiscuous = nfb_eth_promiscuous_get(dev);
 	data->all_multicast = nfb_eth_allmulticast_get(dev);
-- 
2.35.1

Re: [PATCH 6/6] drivers/nfb: add support for more MAC addresses

[Martin Spinler]

On Mon, 2022-02-14 at 17:54 +0000, Ferruh Yigit wrote:
> > > > @@ -530,7 +577,7 @@ nfb_eth_dev_init(struct rte_eth_dev *dev)
> > > >     	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_eth_dev_default_mac_addr_set(dev->data->port_id,
> > > > &eth_addr_init);
> > > >     
> > > 
> > > I didn't get this change, why calling the API from the driver,
> > > instead of calling the dev_ops directly as original code did?
> > 
> > The API does all the checks and copies the MAC value into internal
> > data
> > struct, so our code was duplicit. I didn't realize that calling the
> > API
> > could be problem. I assume it should be reverted to the prev form?
> > 
> 
> It is not a problem, and APIs are used because of the reason you
> mentioned in a few places, but for this case I didn't get it,
> what API check is required?
> Is it 'rte_is_valid_assigned_ether_addr()' check? The mac in this
> function ('nfb_eth_dev_init()') is a hardcoded one, instead of
> validity check, why not select a valid MAC at this stage?
> 
> I mean still drop the 'rte_is_valid_assigned_ether_addr()' check
> from 'nfb_eth_mac_addr_set()', but be sure 'eth_addr_init' is
> valid MAC, will it work?

Yes, definitely a better way. Just rte_ether_addr_copy remains.

> 
> > > 
> > > >     	data->promiscuous = nfb_eth_promiscuous_get(dev);
> > > >     	data->all_multicast = nfb_eth_allmulticast_get(dev);
> > > 
> > 

Re: [PATCH v2 0/5] net/nfb: add missing libfdt dependency for build

[Ferruh Yigit]

On 2/15/2022 12:55 PM, spinler@cesnet.cz wrote:
> From: Martin Spinler <spinler@cesnet.cz>
> 
> This series introduces standard RX timestamp offload flag support
> together with multiple MAC addresses support for NFB devices.
> Also fixes a promisc/multicast switch issue and minor annoyances.
> ---
> 
> v2:
>   * Removed libfdt dependency patch from series, is needless.
>   * Added the Fixes/CC-stable tags to 2 patches.
>   * Reworked support for more MAC addresses: extracted
>     nfb_eth_get_max_mac_address_count, fixed allocation, added
>     comments.
>   * Removed documentation reference to custom timestamp parameter.
> 

Series applied to dpdk-next-net/main, thanks.

Re: [PATCH v2 0/5] net/nfb: add missing libfdt dependency for build

[Martin Spinler]

Thank you for the review!

On Tue, 2022-02-15 at 13:55 +0000, Ferruh Yigit wrote:
> On 2/15/2022 12:55 PM, spinler@cesnet.cz wrote:
> > From: Martin Spinler <spinler@cesnet.cz>
> > 
> > This series introduces standard RX timestamp offload flag support
> > together with multiple MAC addresses support for NFB devices.
> > Also fixes a promisc/multicast switch issue and minor annoyances.
> > ---
> > 
> > v2:
> >   * Removed libfdt dependency patch from series, is needless.
> >   * Added the Fixes/CC-stable tags to 2 patches.
> >   * Reworked support for more MAC addresses: extracted
> >     nfb_eth_get_max_mac_address_count, fixed allocation, added
> >     comments.
> >   * Removed documentation reference to custom timestamp parameter.
> > 
> 
> Series applied to dpdk-next-net/main, thanks.