On 9/1/2020 12:50 PM, Jiawen Wu wrote: > add basic PCIe ethdev probe and remove. > > Signed-off-by: Jiawen Wu <jiawe...@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, > + ð_da); > + if (retval) > + return retval; > + } else > + memset(ð_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.