Re: [dpdk-dev] [PATCH v1 02/42] net/txgbe: add ethdev probe and remove

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/1/2020 12:50 PM, Jiawen Wu wrote:
> add basic PCIe ethdev probe and remove.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

<...>

> +++ b/drivers/net/txgbe/base/meson.build
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2015-2020
> +
> +sources = [
> +
> +]
> +
> +error_cflags = ['-Wno-unused-value',
> +				'-Wno-unused-parameter',
> +				'-Wno-unused-but-set-variable']

Why these warnings are disabled, can't they fixed in the code? Can you please
remove them?

<...>

> --- a/drivers/net/txgbe/txgbe_ethdev.c
> +++ b/drivers/net/txgbe/txgbe_ethdev.c
> @@ -2,3 +2,164 @@
>   * Copyright(c) 2015-2020
>   */
>  
> +#include <sys/queue.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <stdarg.h>
> +#include <inttypes.h>
> +#include <netinet/in.h>
> +#include <rte_byteorder.h>
> +#include <rte_common.h>
> +#include <rte_cycles.h>
> +#include <rte_ethdev_driver.h>
> +#include <rte_ethdev_pci.h>
> +
> +#include <rte_interrupts.h>
> +#include <rte_log.h>
> +#include <rte_debug.h>
> +#include <rte_pci.h>
> +#include <rte_branch_prediction.h>
> +#include <rte_memory.h>
> +#include <rte_eal.h>
> +#include <rte_alarm.h>
> +#include <rte_ether.h>
> +#include <rte_malloc.h>
> +#include <rte_random.h>
> +#include <rte_dev.h>

Are all these headers needed at this stage? Can they be added as they needed?

<...>

> +static int
> +eth_txgbe_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> +		struct rte_pci_device *pci_dev)
> +{
> +	char name[RTE_ETH_NAME_MAX_LEN];
> +	struct rte_eth_dev *pf_ethdev;
> +	struct rte_eth_devargs eth_da;
> +	int i, retval;
> +
> +	if (pci_dev->device.devargs) {
> +		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> +				&eth_da);
> +		if (retval)
> +			return retval;
> +	} else
> +		memset(&eth_da, 0, sizeof(eth_da));
> +
> +	retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
> +		sizeof(struct txgbe_adapter),
> +		eth_dev_pci_specific_init, pci_dev,
> +		eth_txgbe_dev_init, NULL);

Better to indent with double tab to diffrenciate line continuation.

<...>

> +
> +		if (retval)
> +			PMD_DRV_LOG(ERR, "failed to create txgbe vf "
> +				"representor %s.", name);

You can join the splitted log message.
PMD_DRV_LOG(ERR,
	"failed to create txgbe vf representor %s.",
	name)


> +	}
> +
> +	return 0;
> +}
> +
> +static int eth_txgbe_pci_remove(struct rte_pci_device *pci_dev)
> +{
> +	struct rte_eth_dev *ethdev;
> +
> +	ethdev = rte_eth_dev_allocated(pci_dev->device.name);
> +	if (!ethdev)
> +		return -ENODEV;
> +
> +	if (ethdev->data->dev_flags & RTE_ETH_DEV_REPRESENTOR)

At least 'txgbe_vf_representor_init()' should set the 'RTE_ETH_DEV_REPRESENTOR'
in this patch for this check to make sense.

<...>

> +
> +#ifdef RTE_LIBRTE_TXGBE_DEBUG_INIT
> +#define PMD_TLOG_INIT(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, txgbe_logtype_init, \
> +		"%s(): " fmt, __func__, ##args)
> +#else
> +#define PMD_TLOG_INIT(level, fmt, args...)   do { } while (0)
> +#endif
> +
> +#ifdef RTE_LIBRTE_TXGBE_DEBUG_DRIVER
> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, txgbe_logtype_driver, \
> +		"%s(): " fmt, __func__, ##args)
> +#else
> +#define PMD_TLOG_DRIVER(level, fmt, args...) do { } while (0)
> +#endif

There is not config types for 'RTE_LIBRTE_TXGBE_DEBUG_INIT' &
'RTE_LIBRTE_TXGBE_DEBUG_DRIVER' and above there is already dynamic log types for
them, these look redundant.

> +
> +/*
> + * PMD_DEBUG_LOG: for debugger
> + */
> +#define TLOG_EMERG(fmt, args...)    PMD_TLOG_DRIVER(EMERG, fmt, ##args)
> +#define TLOG_ALERT(fmt, args...)    PMD_TLOG_DRIVER(ALERT, fmt, ##args)
> +#define TLOG_CRIT(fmt, args...)     PMD_TLOG_DRIVER(CRIT, fmt, ##args)
> +#define TLOG_ERR(fmt, args...)      PMD_TLOG_DRIVER(ERR, fmt, ##args)
> +#define TLOG_WARN(fmt, args...)     PMD_TLOG_DRIVER(WARNING, fmt, ##args)
> +#define TLOG_NOTICE(fmt, args...)   PMD_TLOG_DRIVER(NOTICE, fmt, ##args)
> +#define TLOG_INFO(fmt, args...)     PMD_TLOG_DRIVER(INFO, fmt, ##args)
> +#define TLOG_DEBUG(fmt, args...)    PMD_TLOG_DRIVER(DEBUG, fmt, ##args)

These can be dropped as well if 'PMD_TLOG_DRIVER' removed.

> +
> +/* to be deleted */
> +#define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
> +#define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
> +#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)

It looks like they are forgotten to be deleted.

> +
> +/*
> + * PMD_TEMP_LOG: for tester
> + */
> +#ifdef RTE_LIBRTE_TXGBE_DEBUG
> +#define wjmsg_line(fmt, ...) \
> +    do { \
> +	RTE_LOG(CRIT, PMD, "%s(%d): " fmt, \
> +	       __FUNCTION__, __LINE__, ## __VA_ARGS__); \
> +    } while (0)
> +#define wjmsg_stack(fmt, ...) \
> +    do { \
> +	wjmsg_line(fmt, ## __VA_ARGS__); \
> +	rte_dump_stack(); \
> +    } while (0)
> +#define wjmsg wjmsg_line
> +
> +#define wjdump(mb) { \
> +	int j; char buf[128] = ""; \
> +	wjmsg("data_len=%d pkt_len=%d vlan_tci=%d " \
> +		"packet_type=0x%08x ol_flags=0x%016lx " \
> +		"hash.rss=0x%08x hash.fdir.hash=0x%04x hash.fdir.id=%d\n", \
> +		mb->data_len, mb->pkt_len, mb->vlan_tci, \
> +		mb->packet_type, mb->ol_flags, \
> +		mb->hash.rss, mb->hash.fdir.hash, mb->hash.fdir.id); \
> +	for (j = 0; j < mb->data_len; j++) { \
> +		sprintf(buf + strlen(buf), "%02x ", \
> +			((uint8_t *)(mb->buf_addr) + mb->data_off)[j]); \
> +		if (j % 8 == 7) {\
> +			wjmsg("%s\n", buf); \
> +			buf[0] = '\0'; \
> +		} \
> +	} \
> +	wjmsg("%s\n", buf); \
> +}
> +#else /* RTE_LIBRTE_TXGBE_DEBUG */
> +#define wjmsg_line(fmt, args...) do {} while (0)
> +#define wjmsg_limit(fmt, args...) do {} while (0)
> +#define wjmsg_stack(fmt, args...) do {} while (0)
> +#define wjmsg(fmt, args...) do {} while (0)
> +#define wjdump(fmt, args...) do {} while (0)
> +#endif /* RTE_LIBRTE_TXGBE_DEBUG */

The 'RTE_LIBRTE_TXGBE_DEBUG' also doesn't exist.

<...>

> \ No newline at end of file

Can you please add the EOL.