[RFT] net/nfb: use dynamic logtype

Message ID 20231206175326.116375-1-stephen@networkplumber.org (mailing list archive)
State Superseded
Delegated to: Ferruh Yigit
Headers
Series [RFT] net/nfb: use dynamic logtype |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build success github build: passed
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS

Commit Message

Stephen Hemminger Dec. 6, 2023, 5:51 p.m. UTC
  All drivers should be using dynamic logtype.
This should have been caught during initial driver review.

Since this driver requires non-standard external library this
patch can not be tested by me.

Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/nfb/nfb_ethdev.c | 22 ++++++++++------------
 drivers/net/nfb/nfb_log.h    | 13 +++++++++++++
 drivers/net/nfb/nfb_rx.c     |  9 +++++----
 drivers/net/nfb/nfb_rx.h     |  2 +-
 drivers/net/nfb/nfb_tx.c     |  8 ++++----
 drivers/net/nfb/nfb_tx.h     |  2 +-
 6 files changed, 34 insertions(+), 22 deletions(-)
 create mode 100644 drivers/net/nfb/nfb_log.h
  

Comments

Martin Spinler Dec. 7, 2023, 10:37 a.m. UTC | #1
Unaddressed
Thanks for patch! There are some issues.

On Wed, 2023-12-06 at 09:51 -0800, Stephen Hemminger wrote:
> 
> diff --git a/drivers/net/nfb/nfb_log.h b/drivers/net/nfb/nfb_log.h
> new file mode 100644
> index 000000000000..fac66a38d4b3
> --- /dev/null
> +++ b/drivers/net/nfb/nfb_log.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + */
> +
> +#ifndef _NFB_STATS_H_
> +#define _NFB_STATS_H_

use the _NFB_LOG_H_ guards

> +
> +extern int nfb_logtype;
> +
> +#define NFB_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, nfb_logtype, "%s(): " fmt "\n", \
> +		__func__, ## args)
> +
> +#endif /* _NFB_STATS_H_ */

use the _NFB_LOG_H_ guard

> diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
> index 8a9b232305f2..e39592d04737 100644
> --- a/drivers/net/nfb/nfb_rx.c
> +++ b/drivers/net/nfb/nfb_rx.c
>  
> @@ -19,7 +20,7 @@ nfb_eth_rx_queue_start(struct rte_eth_dev *dev, uint16_t rxq_id)
>  	int ret;
>  
>  	if (rxq->queue == NULL) {
> -		RTE_LOG(ERR, PMD, "RX NDP queue is NULL!\n");
> +		NFP_LOG(ERR, "RX NDP queue is NULL");

typo, should be NFB_LOG instead of NFP_LOG

>  		return -EINVAL;
>  	}
>  
>  

Also, the nfb_rx.h and nfb_tx.h files use the macro NFB_LOG inside,
please add '#include "nfb_log.h"' into them (then the include in
nfb_rx.c will be duplicate). Otherwise, all .c sources, which include
main nfb.h, don't compile.

With these changes, the driver works.

Thank you!
Martin
  
Stephen Hemminger Dec. 7, 2023, 5:32 p.m. UTC | #2
On Thu, 07 Dec 2023 11:37:52 +0100
Martin Spinler <spinler@cesnet.cz> wrote:

> Also, the nfb_rx.h and nfb_tx.h files use the macro NFB_LOG inside,
> please add '#include "nfb_log.h"' into them (then the include in
> nfb_rx.c will be duplicate). Otherwise, all .c sources, which include
> main nfb.h, don't compile.
> 
> With these changes, the driver works.
> 
> Thank you!
> Martin

I found a few more leftover bits in nfb driver.
Will make a new patchset, and it looks like can build test in a
Redhat VM.
  
Ferruh Yigit Jan. 12, 2024, 12:16 p.m. UTC | #3
On 12/7/2023 6:56 PM, Stephen Hemminger wrote:
> Replace static logtype with dynamic logtype and
> remove dead code. Compile tested on Fedora.
> 
> Stephen Hemminger (3):
>   net/nfb: remove unused device args
>   net/nfb: make device path local to init function
>   net/nfb: use dynamic logtype
> 
>  

Hi Martin,

Can you please review the set?
  
Martin Spinler Jan. 12, 2024, 1:50 p.m. UTC | #4
Tested-by: Martin Spinler <spinler@cesnet.cz>
Acked-by: Martin Spinler <spinler@cesnet.cz>

---

Hi! Thanks for the cleanup. I've tested that patchset and works fine.

I'm just not sure, if the "net/nfb: use dynamic logtype" patch merges
with the "Remove uses of PMD logtype" series as they slightly differs
(both links below).
Stephen, would it make sense to remove the last patch from this series?

https://patchwork.dpdk.org/project/dpdk/patch/20231207185720.19913-4-stephen@networkplumber.org/
https://patchwork.dpdk.org/project/dpdk/patch/20231222171820.8778-9-stephen@networkplumber.org/


On Fri, 2024-01-12 at 12:16 +0000, Ferruh Yigit wrote:
> On 12/7/2023 6:56 PM, Stephen Hemminger wrote:
> > Replace static logtype with dynamic logtype and
> > remove dead code. Compile tested on Fedora.
> > 
> > Stephen Hemminger (3):
> >   net/nfb: remove unused device args
> >   net/nfb: make device path local to init function
> >   net/nfb: use dynamic logtype
> > 
> >  
> 
> Hi Martin,
> 
> Can you please review the set?
  
Ferruh Yigit Feb. 8, 2024, 1:02 a.m. UTC | #5
On 1/12/2024 1:50 PM, Martin Spinler wrote:
> Tested-by: Martin Spinler <spinler@cesnet.cz>
> Acked-by: Martin Spinler <spinler@cesnet.cz>
> 
> ---
> 
> Hi! Thanks for the cleanup. I've tested that patchset and works fine.
> 
> I'm just not sure, if the "net/nfb: use dynamic logtype" patch merges
> with the "Remove uses of PMD logtype" series as they slightly differs
> (both links below).
> Stephen, would it make sense to remove the last patch from this series?
> 
> https://patchwork.dpdk.org/project/dpdk/patch/20231207185720.19913-4-stephen@networkplumber.org/
> https://patchwork.dpdk.org/project/dpdk/patch/20231222171820.8778-9-stephen@networkplumber.org/
> 

Second one is larger set with multiple components involved, this one is
more specific, I will proceed with this one.

@Thomas may drop the nfp patch in that series.

> 
> On Fri, 2024-01-12 at 12:16 +0000, Ferruh Yigit wrote:
>> On 12/7/2023 6:56 PM, Stephen Hemminger wrote:
>>> Replace static logtype with dynamic logtype and
>>> remove dead code. Compile tested on Fedora.
>>>
>>> Stephen Hemminger (3):
>>>   net/nfb: remove unused device args
>>>   net/nfb: make device path local to init function
>>>   net/nfb: use dynamic logtype
>>>
>>>  
>>
>> Hi Martin,
>>
>> Can you please review the set?
>
  
Ferruh Yigit Feb. 8, 2024, 12:01 p.m. UTC | #6
On 12/7/2023 6:56 PM, Stephen Hemminger wrote:
> Replace static logtype with dynamic logtype and
> remove dead code. Compile tested on Fedora.
> 
> Stephen Hemminger (3):
>   net/nfb: remove unused device args
>   net/nfb: make device path local to init function
>   net/nfb: use dynamic logtype
> 

Moving ack from other thread:
Tested-by: Martin Spinler <spinler@cesnet.cz>
Acked-by: Martin Spinler <spinler@cesnet.cz>


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

Patch

diff --git a/drivers/net/nfb/nfb_ethdev.c b/drivers/net/nfb/nfb_ethdev.c
index defd118bd0ee..6c72ac101241 100644
--- a/drivers/net/nfb/nfb_ethdev.c
+++ b/drivers/net/nfb/nfb_ethdev.c
@@ -12,6 +12,7 @@ 
 #include <ethdev_pci.h>
 #include <rte_kvargs.h>
 
+#include "nfb_log.h"
 #include "nfb_stats.h"
 #include "nfb_rx.h"
 #include "nfb_tx.h"
@@ -192,8 +193,7 @@  nfb_eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 				(&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_LOG(ERR, "Cannot register Rx timestamp field/flag %d", ret);
 			nfb_close(internals->nfb);
 			return -rte_errno;
 		}
@@ -520,7 +520,7 @@  nfb_eth_dev_init(struct rte_eth_dev *dev)
 	struct rte_ether_addr eth_addr_init;
 	struct rte_kvargs *kvlist;
 
-	RTE_LOG(INFO, PMD, "Initializing NFB device (" PCI_PRI_FMT ")\n",
+	NFB_LOG(INFO, "Initializing NFB device (" PCI_PRI_FMT ")",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
 
@@ -536,7 +536,7 @@  nfb_eth_dev_init(struct rte_eth_dev *dev)
 		kvlist = rte_kvargs_parse(dev->device->devargs->args,
 						VALID_KEYS);
 		if (kvlist == NULL) {
-			RTE_LOG(ERR, PMD, "Failed to parse device arguments %s",
+			NFB_LOG(ERR, "Failed to parse device arguments %s",
 				dev->device->devargs->args);
 			rte_kvargs_free(kvlist);
 			return -EINVAL;
@@ -551,14 +551,13 @@  nfb_eth_dev_init(struct rte_eth_dev *dev)
 	 */
 	internals->nfb = nfb_open(internals->nfb_dev);
 	if (internals->nfb == NULL) {
-		RTE_LOG(ERR, PMD, "nfb_open(): failed to open %s",
-			internals->nfb_dev);
+		NFB_LOG(ERR, "nfb_open(): failed to open %s", internals->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);
 
-	RTE_LOG(INFO, PMD, "Available NDP queues RX: %u TX: %u\n",
+	NFB_LOG(INFO, "Available NDP queues RX: %u TX: %u",
 		data->nb_rx_queues, data->nb_tx_queues);
 
 	nfb_nc_rxmac_init(internals->nfb,
@@ -583,7 +582,7 @@  nfb_eth_dev_init(struct rte_eth_dev *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_LOG(ERR, "Could not alloc space for MAC address");
 		nfb_close(internals->nfb);
 		return -EINVAL;
 	}
@@ -601,8 +600,7 @@  nfb_eth_dev_init(struct rte_eth_dev *dev)
 
 	dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
 
-	RTE_LOG(INFO, PMD, "NFB device ("
-		PCI_PRI_FMT ") successfully initialized\n",
+	NFB_LOG(INFO, "NFB device (" PCI_PRI_FMT ") successfully initialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
 
@@ -626,8 +624,7 @@  nfb_eth_dev_uninit(struct rte_eth_dev *dev)
 
 	nfb_eth_dev_close(dev);
 
-	RTE_LOG(INFO, PMD, "NFB device ("
-		PCI_PRI_FMT ") successfully uninitialized\n",
+	NFB_LOG(INFO,"NFB device (" PCI_PRI_FMT ") successfully uninitialized",
 		pci_addr->domain, pci_addr->bus, pci_addr->devid,
 		pci_addr->function);
 
@@ -690,3 +687,4 @@  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_LOG_REGISTER_DEFAULT(nfb_logtype, NOTICE);
diff --git a/drivers/net/nfb/nfb_log.h b/drivers/net/nfb/nfb_log.h
new file mode 100644
index 000000000000..fac66a38d4b3
--- /dev/null
+++ b/drivers/net/nfb/nfb_log.h
@@ -0,0 +1,13 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ */
+
+#ifndef _NFB_STATS_H_
+#define _NFB_STATS_H_
+
+extern int nfb_logtype;
+
+#define NFB_LOG(level, fmt, args...) \
+	rte_log(RTE_LOG_ ## level, nfb_logtype, "%s(): " fmt "\n", \
+		__func__, ## args)
+
+#endif /* _NFB_STATS_H_ */
diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
index 8a9b232305f2..e39592d04737 100644
--- a/drivers/net/nfb/nfb_rx.c
+++ b/drivers/net/nfb/nfb_rx.c
@@ -6,6 +6,7 @@ 
 
 #include <rte_kvargs.h>
 
+#include "nfb_log.h"
 #include "nfb_rx.h"
 #include "nfb.h"
 
@@ -19,7 +20,7 @@  nfb_eth_rx_queue_start(struct rte_eth_dev *dev, uint16_t rxq_id)
 	int ret;
 
 	if (rxq->queue == NULL) {
-		RTE_LOG(ERR, PMD, "RX NDP queue is NULL!\n");
+		NFP_LOG(ERR, "RX NDP queue is NULL");
 		return -EINVAL;
 	}
 
@@ -40,7 +41,7 @@  nfb_eth_rx_queue_stop(struct rte_eth_dev *dev, uint16_t rxq_id)
 	int ret;
 
 	if (rxq->queue == NULL) {
-		RTE_LOG(ERR, PMD, "RX NDP queue is NULL!\n");
+		NFB_LOG(ERR, "RX NDP queue is NULL");
 		return -EINVAL;
 	}
 
@@ -70,8 +71,8 @@  nfb_eth_rx_queue_setup(struct rte_eth_dev *dev,
 			RTE_CACHE_LINE_SIZE, socket_id);
 
 	if (rxq == NULL) {
-		RTE_LOG(ERR, PMD, "rte_zmalloc_socket() failed for rx queue id "
-				"%" PRIu16 "!\n", rx_queue_id);
+		NFB_LOG(ERR, "rte_zmalloc_socket() failed for rx queue id %" PRIu16,
+			rx_queue_id);
 		return -ENOMEM;
 	}
 
diff --git a/drivers/net/nfb/nfb_rx.h b/drivers/net/nfb/nfb_rx.h
index b618682e1393..2802f17091a0 100644
--- a/drivers/net/nfb/nfb_rx.h
+++ b/drivers/net/nfb/nfb_rx.h
@@ -156,7 +156,7 @@  nfb_eth_ndp_rx(void *queue,
 	struct rte_mbuf *mbufs[nb_pkts];
 
 	if (unlikely(ndp->queue == NULL || nb_pkts == 0)) {
-		RTE_LOG(ERR, PMD, "RX invalid arguments!\n");
+		NFB_LOG(ERR, "RX invalid arguments");
 		return 0;
 	}
 
diff --git a/drivers/net/nfb/nfb_tx.c b/drivers/net/nfb/nfb_tx.c
index d49fc324e76b..3fdfb66a969e 100644
--- a/drivers/net/nfb/nfb_tx.c
+++ b/drivers/net/nfb/nfb_tx.c
@@ -14,7 +14,7 @@  nfb_eth_tx_queue_start(struct rte_eth_dev *dev, uint16_t txq_id)
 	int ret;
 
 	if (txq->queue == NULL) {
-		RTE_LOG(ERR, PMD, "RX NDP queue is NULL!\n");
+		NFB_LOG(ERR, "RX NDP queue is NULL");
 		return -EINVAL;
 	}
 
@@ -35,7 +35,7 @@  nfb_eth_tx_queue_stop(struct rte_eth_dev *dev, uint16_t txq_id)
 	int ret;
 
 	if (txq->queue == NULL) {
-		RTE_LOG(ERR, PMD, "TX NDP queue is NULL!\n");
+		NFB_LOG(ERR, "TX NDP queue is NULL");
 		return -EINVAL;
 	}
 
@@ -62,8 +62,8 @@  nfb_eth_tx_queue_setup(struct rte_eth_dev *dev,
 		RTE_CACHE_LINE_SIZE, socket_id);
 
 	if (txq == NULL) {
-		RTE_LOG(ERR, PMD, "rte_zmalloc_socket() failed for tx queue id "
-			"%" PRIu16 "!\n", tx_queue_id);
+		NFB_LOG(ERR, "rte_zmalloc_socket() failed for tx queue id %" PRIu16,
+			tx_queue_id);
 		return -ENOMEM;
 	}
 
diff --git a/drivers/net/nfb/nfb_tx.h b/drivers/net/nfb/nfb_tx.h
index 910020e9e96f..f107cf914bbd 100644
--- a/drivers/net/nfb/nfb_tx.h
+++ b/drivers/net/nfb/nfb_tx.h
@@ -140,7 +140,7 @@  nfb_eth_ndp_tx(void *queue,
 		return 0;
 
 	if (unlikely(ndp->queue == NULL)) {
-		RTE_LOG(ERR, PMD, "TX invalid arguments!\n");
+		NFB_LOG(ERR, "TX invalid arguments");
 		return 0;
 	}