Re: [PATCH v5 01/18] net/r8169: add PMD driver skeleton

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

On 10/28/2024 7:30 AM, Howard Wang wrote:
> Meson build infrastructure, r8169_ethdev minimal skeleton,
> header with Realtek NIC device and vendor IDs.
> 
> Signed-off-by: Howard Wang <howard_wang@realsil.com.cn>
> ---
>  MAINTAINERS                      |   7 ++
>  drivers/net/meson.build          |   1 +
>  drivers/net/r8169/meson.build    |   6 ++
>  drivers/net/r8169/r8169_base.h   |  15 +++
>  drivers/net/r8169/r8169_ethdev.c | 178 +++++++++++++++++++++++++++++++
>  drivers/net/r8169/r8169_ethdev.h |  40 +++++++
>  6 files changed, 247 insertions(+)
>  create mode 100644 drivers/net/r8169/meson.build
>  create mode 100644 drivers/net/r8169/r8169_base.h
>  create mode 100644 drivers/net/r8169/r8169_ethdev.c
>  create mode 100644 drivers/net/r8169/r8169_ethdev.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c5a703b5c0..5f9eccc43f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1076,6 +1076,13 @@ F: drivers/net/memif/
>  F: doc/guides/nics/memif.rst
>  F: doc/guides/nics/features/memif.ini
>  
> +Realtek r8169
> +M: Howard Wang <howard_wang@realsil.com.cn>
> +M: ChunHao Lin <hau@realtek.com>
> +M: Xing Wang <xing_wang@realsil.com.cn>
> +M: Realtek NIC SW <pro_nic_dpdk@realtek.com>
>

We prefer authors as maintainer, instead of group/alias, and as you
already provide names, can this be removed?


> +F: drivers/net/r8169
> +
>

Please sort alphabetically for 'Realtek'

Can you also add driver documentation and update release notes in this
patch?


>  
>  Crypto Drivers
>  --------------
> diff --git a/drivers/net/meson.build b/drivers/net/meson.build
> index fb6d34b782..fddcf39655 100644
> --- a/drivers/net/meson.build
> +++ b/drivers/net/meson.build
> @@ -53,6 +53,7 @@ drivers = [
>          'pfe',
>          'qede',
>          'ring',
> +        'r8169',
>          'sfc',
>          'softnic',
>          'tap',
> diff --git a/drivers/net/r8169/meson.build b/drivers/net/r8169/meson.build
> new file mode 100644
> index 0000000000..f14d4ae8fb
> --- /dev/null
> +++ b/drivers/net/r8169/meson.build
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2024 Realtek Corporation. All rights reserved
> +
> +sources = files(
> +		'r8169_ethdev.c',
> +)
> diff --git a/drivers/net/r8169/r8169_base.h b/drivers/net/r8169/r8169_base.h
> new file mode 100644
> index 0000000000..6fc84592a6
> --- /dev/null
> +++ b/drivers/net/r8169/r8169_base.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2024 Realtek Corporation. All rights reserved
> + */
> +
> +#ifndef _R8169_BASE_H_
> +#define _R8169_BASE_H_
> +
> +typedef uint8_t   u8;
> +typedef uint16_t  u16;
> +typedef uint32_t  u32;
> +typedef uint64_t  u64;
>

most of the drivers has 'compat.h' or '<driver>_compat.h'
(r8169_compat.h) as compatibility layer for DPDK and define above like
structures there.
If there is no specific reason to have this file name, you can create a
compat.h for compatibility.

> +
> +#define PCI_VENDOR_ID_REALTEK 0x10EC
> +
> +#endif
> diff --git a/drivers/net/r8169/r8169_ethdev.c b/drivers/net/r8169/r8169_ethdev.c
> new file mode 100644
> index 0000000000..1f90a142c5
> --- /dev/null
> +++ b/drivers/net/r8169/r8169_ethdev.c
> @@ -0,0 +1,178 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2024 Realtek Corporation. All rights reserved
> + */
> +
> +#include <sys/queue.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <stdarg.h>
> +
> +#include <rte_eal.h>
> +
> +#include <rte_string_fns.h>
> +#include <rte_common.h>
> +#include <rte_interrupts.h>
> +#include <rte_byteorder.h>
> +#include <rte_log.h>
> +#include <rte_debug.h>
> +#include <rte_pci.h>
> +#include <bus_pci_driver.h>
> +#include <rte_ether.h>
> +#include <ethdev_driver.h>
> +#include <ethdev_pci.h>
> +#include <rte_memory.h>
> +#include <rte_malloc.h>
> +#include <dev_driver.h>
>

Do you really need all these headers here?
Please only include necessary ones and add them when you need them.


> +
> +#include "r8169_ethdev.h"
> +#include "r8169_base.h"
> +
> +static int rtl_dev_configure(struct rte_eth_dev *dev __rte_unused);
>

No need to have '__rte_unused' attribute for function decleration.

> +static int rtl_dev_start(struct rte_eth_dev *dev);
> +static int rtl_dev_stop(struct rte_eth_dev *dev);
> +static int rtl_dev_reset(struct rte_eth_dev *dev);
> +static int rtl_dev_close(struct rte_eth_dev *dev);
> +
> +/*
> + * The set of PCI devices this driver supports
> + */
> +static const struct rte_pci_id pci_id_r8169_map[] = {
> +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8125) },
> +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8162) },
> +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8126) },
> +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x5000) },
> +	{.vendor_id = 0, /* sentinel */ },
> +};
> +
> +static const struct eth_dev_ops rtl_eth_dev_ops = {
> +	.dev_configure	      = rtl_dev_configure,
> +	.dev_start	      = rtl_dev_start,
> +	.dev_stop	      = rtl_dev_stop,
> +	.dev_close	      = rtl_dev_close,
> +	.dev_reset	      = rtl_dev_reset,
> +};
> +
> +static int
> +rtl_dev_configure(struct rte_eth_dev *dev __rte_unused)
> +{
> +	return 0;
> +}
> +
> +/*
> + * Configure device link speed and setup link.
> + * It returns 0 on success.
> + */
> +static int
> +rtl_dev_start(struct rte_eth_dev *dev)
> +{
> +	struct rtl_adapter *adapter = RTL_DEV_PRIVATE(dev);
> +	struct rtl_hw *hw = &adapter->hw;
> +
> +	hw->adapter_stopped = 0;
> +
> +	return 0;
> +}
> +
> +/*
> + * Stop device: disable RX and TX functions to allow for reconfiguring.
> + */
> +static int
> +rtl_dev_stop(struct rte_eth_dev *dev)
> +{
> +	struct rtl_adapter *adapter = RTL_DEV_PRIVATE(dev);
> +	struct rtl_hw *hw = &adapter->hw;
> +
> +	if (hw->adapter_stopped)
> +		return 0;
>

There is 'dev->data->dev_started' variable for exact same reason, and it
is checked in 'rte_eth_dev_stop()' level, do you need this driver
version of same flag?

> +
> +	hw->adapter_stopped = 1;
> +	dev->data->dev_started = 0;
>

This flag is set in ethdev layer level, no need to set it here.

> +
> +	return 0;
> +}
> +
> +/*
> + * Reset and stop device.
> + */
> +static int
> +rtl_dev_close(struct rte_eth_dev *dev)
> +{
> +	int ret_stp;
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return 0;
> +
> +	ret_stp = rtl_dev_stop(dev);
> +
> +	return ret_stp;
> +}
> +
> +static int
> +rtl_dev_init(struct rte_eth_dev *dev)
> +{
> +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> +	struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
>

'intr_handle' not used yet, please add it when used.

> +	struct rtl_adapter *adapter = RTL_DEV_PRIVATE(dev);
> +	struct rtl_hw *hw = &adapter->hw;
> +
> +	dev->dev_ops = &rtl_eth_dev_ops;
> +
> +	/* For secondary processes, the primary process has done all the work */
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return 0;
> +
> +	rte_eth_copy_pci_info(dev, pci_dev);
>

This is already done as part of 'rte_eth_dev_pci_generic_probe()',
shouldn't need to do here. Can you please double check?

> +
> +	return 0;
> +}
> +
> +static int
> +rtl_dev_uninit(struct rte_eth_dev *dev)
> +{
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -EPERM;
> +
> +	rtl_dev_close(dev);
> +
> +	return 0;
> +}
> +
> +static int
> +rtl_dev_reset(struct rte_eth_dev *dev)
> +{
> +	int ret;
> +
> +	ret = rtl_dev_uninit(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = rtl_dev_init(dev);
> +
> +	return ret;
> +}
> +
> +static int
> +rtl_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> +	      struct rte_pci_device *pci_dev)
> +{
> +	return rte_eth_dev_pci_generic_probe(pci_dev, sizeof(struct rtl_adapter),
> +					     rtl_dev_init);
> +}
> +
> +static int
> +rtl_pci_remove(struct rte_pci_device *pci_dev)
> +{
> +	return rte_eth_dev_pci_generic_remove(pci_dev, rtl_dev_uninit);
> +}
> +
> +static struct rte_pci_driver rte_r8169_pmd = {
> +	.id_table  = pci_id_r8169_map,
> +	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
> +	.probe     = rtl_pci_probe,
> +	.remove    = rtl_pci_remove,
> +};
> +
> +RTE_PMD_REGISTER_PCI(net_r8169, rte_r8169_pmd);
> +RTE_PMD_REGISTER_PCI_TABLE(net_r8169, pci_id_r8169_map);
> +RTE_PMD_REGISTER_KMOD_DEP(net_r8169, "* igb_uio | uio_pci_generic | vfio-pci");
> diff --git a/drivers/net/r8169/r8169_ethdev.h b/drivers/net/r8169/r8169_ethdev.h
> new file mode 100644
> index 0000000000..e37f05c153
> --- /dev/null
> +++ b/drivers/net/r8169/r8169_ethdev.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2024 Realtek Corporation. All rights reserved
> + */
> +
> +#ifndef _R8169_ETHDEV_H_
> +#define _R8169_ETHDEV_H_
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +
> +#include <rte_ethdev.h>
> +#include <rte_ethdev_core.h>
> +
> +#include "r8169_base.h"
> +
> +struct rtl_hw {
> +	u8 adapter_stopped;
> +};
> +
> +struct rtl_sw_stats {
> +	u64 tx_packets;
> +	u64 tx_bytes;
> +	u64 tx_errors;
> +	u64 rx_packets;
> +	u64 rx_bytes;
> +	u64 rx_errors;
> +};
> +
> +struct rtl_adapter {
> +	struct rtl_hw       hw;
> +	struct rtl_sw_stats sw_stats;
> +};
> +
> +#define RTL_DEV_PRIVATE(eth_dev) \
> +	((struct rtl_adapter *)((eth_dev)->data->dev_private))
> +
> +uint16_t rtl_xmit_pkts(void *txq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts);
> +uint16_t rtl_recv_pkts(void *rxq, struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
>

These functions don't exists yet, please add the declarations when
functions added.

> +
> +#endif