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

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

On 11/4/2024 9:43 AM, 王颢 wrote:
> Dear Ferruh,
> 
> I am currently making the changes you mentioned. I would like to clarify a few questions with you.
> 
>> +F: drivers/net/r8169
>> +
>>
> 
> Please sort alphabetically for 'Realtek'
> 
> ===> As I can see in the MAINTAINERS file, each PMD in the Networking Drivers section is not sorted alphabetically. For example, Marvell appears in several places and is not grouped together. As a result, I am unsure where to place Realtek.
>

Please put just before 'Solarflare'.

You are right, there are some inconsistencies,
some Marvell ones are because company acquisitions, company name changed
but order of the file not changed etc.. But there are others too.

> 
> Can you also add driver documentation and update release notes in this patch?
> 
> ===> Do you mean that the 18th patch [PATCH v5 18/18] doc/guides/nics: add documents for r8169 pmd should be merged into the overall patch? Additionally, I don’t quite understand the “update release notes” part. Do you mean that a new release note file needs to be added in drivers/net/r8169 and updated with each patch? Or perhaps you mean that the r8169.ini and r8169.rst files should be created starting from the first patch and gradually improved as the patches are submitted?

Second one please, create documentation in this patch and improve it
gradually.
And same for the release notes (doc/guides/rel_notes/release_24_11.rst),
this patch may have the initial update announcing the new driver, if you
want to announce more updates in release notes, they can be added in
relevant patch in the series.

> 
> Best Regards,
> Howard Wang
> 
> -----邮件原件-----
> 发件人: Ferruh Yigit <ferruh.yigit@amd.com> 
> 发送时间: 2024年11月3日 10:17
> 收件人: 王颢 <howard_wang@realsil.com.cn>; dev@dpdk.org
> 抄送: pro_nic_dpdk@realtek.com
> 主题: Re: [PATCH v5 01/18] net/r8169: add PMD driver skeleton
> 
> 
> External mail.
> 
> 
> 
> 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
>