Re: [PATCH v1 1/2] net/macb: add new driver

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri,  1 Nov 2024 10:07:19 +0000
liwencheng <liwencheng@phytium.com.cn> wrote:

> add Phytium NIC MACB ethdev PMD driver.
> 
> Signed-off-by: liwencheng <liwencheng@phytium.com.cn>

Much better.

When submitting a new version of already submitted patch.
DPDK policy is to use version 2 (v2) and use in-reply-to
so that threads in mail client and patchwork archive stay
organized.

New drivers should be submitted in smaller chunks so that
comments are more organized. 

https://doc.dpdk.org/guides/contributing/new_driver.html

A saw a few things.

1. Don't put developer only printf's in under MACB_DEBUG.
   If you have to leave debugging around use the existing
   log macros with DEBUG as the level.

   This applies to all #ifdef's.
   Please don't use them because it leads to dead untested code.

2. Looks like driver is keeping per queue stats, but they are not filled
   in the get_stats code.

3. Unnecessary casts of void * pointer.

4. The code in interrupt handler is more complex than necessary.
   This does the same thing:

static void macb_interrupt_handler(void *param)
{
	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
	struct macb_priv *priv = dev->data->dev_private;
	struct rte_eth_link link;
	char status[128];

	if (priv->stopped)
		return;

	if (eth_macb_link_update(dev, 0) < 0)
		return;

	rte_eth_linkstatus_get(dev, &link);
	rte_eth_link_to_str(status, sizeof(status), &link);
	MACB_INFO("Port %u: %s", dev->data->port_id, status);

	macb_link_change(priv->bp);
	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL);
}

5. For files, the convention is to use PATH_MAX (you use MAX_FILE_LEN)

6. Use on stack variables where possible (avoids potential memory leaks).
   Example of where allocation can be avoided is in  macb_get_dev_pclk.

7. lots of extra includes.
   For example, why does macb_rxtx_vec_neon need arp, ioctl, socket and stat?

8. Don't use __builtin_ctz, instead use rte_ctz32()

9. Use __rte_cache_aligned not __rte_aligned(RTE_CACHE_LINE_SIZE)

10. Do not disable warnings it hides bugs

11. Take out this line, it bothers checkpatch

    meson.build:
	#allow_experimental_apis = true