Re: [PATCH v12 00/28] [v12]drivers/net Add Support mucse N10 Pmd Driver

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 12 Feb 2025 23:32:20 +0800
Wenbo Cao <caowenbo@mucse.com> wrote:

> For This patchset just to support the basic chip init work
> and user can just found the eth_dev, but can't control more.
> For Now just support 2*10g nic,the chip can support
> 2*10g,4*10g,4*1g,8*1g,8*10g.
> The Feature rx side can support rx-cksum-offload,rss,vlan-filter
> flow_clow,uncast_filter,mcast_filter,1588,Jumbo-frame
> The Feature tx side can support tx-cksum-offload,tso,vxlan-tso 
> flow director base on ntuple pattern of tcp/udp/ip/ eth_hdr->type
> for sriov is also support.
> 
> Because of the chip design defect, for multiple-port mode
> one pci-bdf will have multiple-port (max can have four ports)
> so this code must be care of one bdf init multiple-port.

The driver is getting very close to being ready to merge but still see
some things. Mostly the issues are around the documentation now.

Review checklist for rnp v12 patches:

Mark items with:
    ✔ passed
    ✘ Failed

Basic hygiene
    ✔ Look at CI results in patchwork
    ✔ Merge cleanly with git am; look for missing newline at EOF etc
    ✘ Run checkpatches; warnings are ok, but look more carefully.
      Lots of warnings from base code (allowed but not preferred).

Fix these by making it an inline function?

#1143: FILE: drivers/net/rnp/rnp.h:62:
+#define RNP_PF_OWN_PORTS(id)	(((id) == 0) ? 1 : (((id) == 1) ? 2 : 4))

+#define RNP_LINK_SPEED_CODE(sp, n) \
+	(((sp) & RTE_GENMASK32((11) + ((4) * (n)), \
+		(8) + ((4) * (n)))) >> (8 + 4 * (n)))

This warning could be fixed with either a temporary variable or longer line

WARNING:MULTILINE_DEREFERENCE: Avoid multiple line dereference - prefer 'rte_eth_devices[rxq->attr.port_id].data->rx_mbuf_alloc_failed'
#63: FILE: drivers/net/rnp/rnp_rxtx.c:849:
+			rte_eth_devices[rxq->attr.port_id].data->
+				rx_mbuf_alloc_failed++;


    ✔ Run check-git-log
    ✔ Run check-symbol-maps.sh
    ✔ Run check-doc-vs-code
    ✔ Run check-spdk-tag

Builds
    ✔ Normal Gcc build
    ✔ Use latest experimental Gcc 15 to catch new warnings
    ✔ Clang build using current version (clang-19)
    ✔ Build for 32 bit x86
    ✔ Doc build; works but needs editing
The wording in rnp.rst is not grammatically correct.
Missing articles and some punctuation issues.
Need to mention big-endian limitations.
Windows limitation not mentioned.
Suggest using svg diagram (looks better) rather than ASCII art.

    ✘ Check feature matrix versus code
The driver doc says it supports queue start/stop but no
rx_queue_start or tx_queue_start operations are listed.

    ✔ Debug build
    ✔ Enable asserts
    ✔ Test meson builds

Experimental builds:
     Enable address sanitizer
     ✘ Enable extra warnings (edit meson.build) for
        -Wvla, -Wformat-truncation, -Waddress-of-packed-member

     The issue here is that name is limited to RTE_ETH_NAME_MAX_LEN
     and hw->device_name is RTE_DEV_NAME_MAX_LEN is also 64
     so adding a suffix could potentially overflow the name.

     One way to avoid compiler warning is a a check on length of
     hw->device_name in this code like:
        if (strlen(hw->device_name) + 4 > sizeof(name))
	   return -EINVAL


../drivers/net/rnp/rnp_ethdev.c: In function ‘rnp_eth_dev_init’:
../drivers/net/rnp/rnp_ethdev.c:1705:45: warning: ‘%d’ directive output may be truncated writing 1 byte into a region of size between 0 and 63 [-Wformat-truncation=]
 1705 |                                         "%s_%d", hw->device_name, p_id);
      |                                             ^~
../drivers/net/rnp/rnp_ethdev.c:1705:41: note: directive argument in the range [1, 4]
 1705 |                                         "%s_%d", hw->device_name, p_id);
      |                                         ^~~~~~~
../drivers/net/rnp/rnp_ethdev.c:1704:25: note: ‘snprintf’ output between 3 and 66 bytes into a destination of size 64
 1704 |                         snprintf(name, sizeof(name),
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1705 |                                         "%s_%d", hw->device_name, p_id);
      |                                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Look for anti-patterns:
    ✔ Driver must not disable warnings with compiler flags or pragma's
    ✔ Driver must not use thread and signal
    ✔ Apply coccinelle scripts; look that for example null free checks
    ✔ Review use of memcpy/rte_memcpy
    ✔ Review use of malloc
    ✔ Review locking
    ✘ Review use of memset
      Use of memset before snprintf is unnecessary.

    ✘ Handling of deferred start
      If driver supports deferred start then it has to have a
      queue start/stop operation. Otherwise ignore the flag.

    ✘ Review whitespace
      Unneeded indents in meson.build

sources = files(
                'rnp_ethdev.c',
...
vs.
sources = files(
        'ixgbe_82599_bypass.c',
...

Other
1. The xstat names are long and have spaces in them.
   I don't see this in other drivers.
   Suggest using similar naming pattern as virtio

2. Handling of queue counters
   This code would look much cleaner with temp variable.
   No need for useless init of i. and it is unsigned.
   Don't need to cast void * pointer

diff --git a/drivers/net/rnp/rnp_ethdev.c b/drivers/net/rnp/rnp_ethdev.c
index a295982780..1305d7e0dc 100644
--- a/drivers/net/rnp/rnp_ethdev.c
+++ b/drivers/net/rnp/rnp_ethdev.c
@@ -1254,48 +1254,36 @@ rnp_dev_stats_get(struct rte_eth_dev *dev,
 	struct rnp_eth_port *port = RNP_DEV_TO_PORT(dev);
 	struct rnp_hw_eth_stats *eth_stats = &port->eth_stats;
 	struct rte_eth_dev_data *data = dev->data;
-	int i = 0;
+	uint16_t i;

 	PMD_INIT_FUNC_TRACE();
 	rnp_get_hw_stats(dev);
+
 	for (i = 0; i < data->nb_rx_queues; i++) {
-		if (!data->rx_queues[i])
+		const struct rnp_rx_queue *rxq = dev->data->rx_queues[i];
+		if (!rxq)
 			continue;
+
+		stats->ipackets += rxq->stats.ipackets;
+		stats->ibytes += rxq->stats.ibytes;
 		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			stats->q_ipackets[i] = ((struct rnp_rx_queue **)
-					(data->rx_queues))[i]->stats.ipackets;
-			stats->q_ibytes[i] = ((struct rnp_rx_queue **)
-					(data->rx_queues))[i]->stats.ibytes;
-			stats->ipackets += stats->q_ipackets[i];
-			stats->ibytes += stats->q_ibytes[i];
-		} else {
-			stats->ipackets += ((struct rnp_rx_queue **)
-					(data->rx_queues))[i]->stats.ipackets;
-			stats->ibytes += ((struct rnp_rx_queue **)
-					(data->rx_queues))[i]->stats.ibytes;
+			stats->q_ipackets[i] = rxq->stats.ipackets;
+			stats->q_ibytes[i] = rxq->stats.ibytes;
 		}
 	}

 	for (i = 0; i < data->nb_tx_queues; i++) {
-		if (!data->tx_queues[i])
+		const struct rnp_tx_queue *txq = dev->data->tx_queues[i];
+		if (!txq)
 			continue;
-		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			stats->q_opackets[i] = ((struct rnp_tx_queue **)
-					(data->tx_queues))[i]->stats.opackets;
-			stats->q_obytes[i] = ((struct rnp_tx_queue **)
-					(data->tx_queues))[i]->stats.obytes;
-			stats->oerrors += ((struct rnp_tx_queue **)
-					(data->tx_queues))[i]->stats.errors;
-			stats->opackets += stats->q_opackets[i];
-			stats->obytes += stats->q_obytes[i];

-		} else {
-			stats->opackets += ((struct rnp_tx_queue **)
-					(data->tx_queues))[i]->stats.opackets;
-			stats->obytes += ((struct rnp_tx_queue **)
-					(data->tx_queues))[i]->stats.obytes;
-			stats->oerrors += ((struct rnp_tx_queue **)
-					(data->tx_queues))[i]->stats.errors;
+		stats->opackets += txq->stats.opackets;
+		stats->obytes += txq->stats.obytes;
+		stats->oerrors += txq->stats.errors;
+
+		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
+			stats->q_opackets[i] = txq->stats.opackets;
+			stats->q_obytes[i] = txq->stats.obytes;
 		}
 	}
 	stats->imissed = eth_stats->rx_trans_drop + eth_stats->rx_trunc_drop;
@@ -1308,22 +1296,18 @@ rnp_dev_stats_reset(struct rte_eth_dev *dev)
 {
 	struct rnp_eth_port *port = RNP_DEV_TO_PORT(dev);
 	struct rnp_hw_eth_stats *eth_stats = &port->eth_stats;
-	struct rnp_rx_queue *rxq;
-	struct rnp_tx_queue *txq;
 	uint16_t idx;

 	PMD_INIT_FUNC_TRACE();
 	memset(eth_stats, 0, sizeof(*eth_stats));
 	for (idx = 0; idx < dev->data->nb_rx_queues; idx++) {
-		rxq = ((struct rnp_rx_queue **)
-				(dev->data->rx_queues))[idx];
+		struct rnp_rx_queue *rxq = dev->data->rx_queues[i];
 		if (!rxq)
 			continue;
 		memset(&rxq->stats, 0, sizeof(struct rnp_queue_stats));
 	}
 	for (idx = 0; idx < dev->data->nb_tx_queues; idx++) {
-		txq = ((struct rnp_tx_queue **)
-				(dev->data->tx_queues))[idx];
+		struct rnp_tx_queue *txq = dev->data->tx_queues[idx];
 		if (!txq)
 			continue;
 		memset(&txq->stats, 0, sizeof(struct rnp_queue_stats));