[v3,01/18] net/r8169: add PMD driver skeleton

Message ID 20241023033328.191827-2-howard_wang@realsil.com.cn (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series Modify some code as suggested by the maintainer. |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Howard Wang Oct. 23, 2024, 3:33 a.m. UTC
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    |   7 ++
 drivers/net/r8169/r8169_base.h   |  16 +++
 drivers/net/r8169/r8169_ethdev.c | 179 +++++++++++++++++++++++++++++++
 drivers/net/r8169/r8169_ethdev.h |  41 +++++++
 6 files changed, 251 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
  

Comments

Stephen Hemminger Oct. 23, 2024, 5:17 a.m. UTC | #1
On Wed, 23 Oct 2024 11:33:11 +0800
Howard Wang <howard_wang@realsil.com.cn> 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>

This version is much better than the last.
Still has some issues:
  - git whitespace
  - braces
  - one spelling error

For example, doing git apply showed.

Applying: net/r8169: add support for hardware operations
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2381: trailing whitespace.
					  (BIT_15 | BIT_14 | BIT_13 | BIT_12 | 
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:460: new blank line at EOF.
+
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2052: new blank line at EOF.
+
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2073: new blank line at EOF.
+
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2470: new blank line at EOF.
+
warning: squelched 13 whitespace errors
warning: 18 lines add whitespace errors.


Some minor checkpatch warnings.

CHECK:BRACES: braces {} should be used on all arms of this statement
#268: FILE: drivers/net/r8169/r8169_hw.c:67:
+		if (len <= 4 - val_shift)
[...]
+		else {
[...]

CHECK:BRACES: Unbalanced braces around else statement
#270: FILE: drivers/net/r8169/r8169_hw.c:69:
+		else {

CHECK:BRACES: braces {} should be used on all arms of this statement
#334: FILE: drivers/net/r8169/r8169_hw.c:133:
+		if (len <= 4 - val_shift)
[...]
+		else {
[...]

CHECK:BRACES: Unbalanced braces around else statement
#336: FILE: drivers/net/r8169/r8169_hw.c:135:
+		else {

CHECK:BRACES: Blank lines aren't necessary before a close brace '}'
#433: FILE: drivers/net/r8169/r8169_hw.c:232:
+
+		}

WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#978: FILE: drivers/net/r8169/r8169_phy.c:2:
+ * Copyright(c) 2024 Realtek Corporation. All rights reserved */

CHECK:BRACES: braces {} should be used on all arms of this statement
#273: FILE: drivers/net/r8169/r8169_phy.c:536:
+	if (hw->hw_ram_code_ver == hw->sw_ram_code_ver) {
[...]
+	} else
[...]

### [PATCH] net/r8169: impelment MTU configuration

WARNING:TYPO_SPELLING: 'impelment' may be misspelled - perhaps 'implement'?
#4: 
Subject: [PATCH] net/r8169: impelment MTU configuration
                            ^^^^^^^^^
Suprised that you have to keep track of packets in SW.
The kernel driver is able to get them from the HW.

The kernel driver gets lots more counters from the HW which could go to xstats.
  
Howard Wang Oct. 23, 2024, 5:59 a.m. UTC | #2
Dear Stephen,

OK, I will fix these issues.
I would like to confirm that each document needs to have a blank line at the end, correct? 
Apologies, but we plan to maintain our current approach regarding stats this time.

Best Regards,
Howard Wang

-----邮件原件-----
发件人: Stephen Hemminger <stephen@networkplumber.org> 
发送时间: 2024年10月23日 13:18
收件人: 王颢 <howard_wang@realsil.com.cn>
抄送: dev@dpdk.org; pro_nic_dpdk@realtek.com
主题: Re: [PATCH v3 01/18] net/r8169: add PMD driver skeleton


External mail.



On Wed, 23 Oct 2024 11:33:11 +0800
Howard Wang <howard_wang@realsil.com.cn> 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>

This version is much better than the last.
Still has some issues:
  - git whitespace
  - braces
  - one spelling error

For example, doing git apply showed.

Applying: net/r8169: add support for hardware operations
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2381: trailing whitespace.
                                          (BIT_15 | BIT_14 | BIT_13 | BIT_12 |
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:460: new blank line at EOF.
+
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2052: new blank line at EOF.
+
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2073: new blank line at EOF.
+
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2470: new blank line at EOF.
+
warning: squelched 13 whitespace errors
warning: 18 lines add whitespace errors.


Some minor checkpatch warnings.

CHECK:BRACES: braces {} should be used on all arms of this statement
#268: FILE: drivers/net/r8169/r8169_hw.c:67:
+               if (len <= 4 - val_shift)
[...]
+               else {
[...]

CHECK:BRACES: Unbalanced braces around else statement
#270: FILE: drivers/net/r8169/r8169_hw.c:69:
+               else {

CHECK:BRACES: braces {} should be used on all arms of this statement
#334: FILE: drivers/net/r8169/r8169_hw.c:133:
+               if (len <= 4 - val_shift)
[...]
+               else {
[...]

CHECK:BRACES: Unbalanced braces around else statement
#336: FILE: drivers/net/r8169/r8169_hw.c:135:
+               else {

CHECK:BRACES: Blank lines aren't necessary before a close brace '}'
#433: FILE: drivers/net/r8169/r8169_hw.c:232:
+
+               }

WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#978: FILE: drivers/net/r8169/r8169_phy.c:2:
+ * Copyright(c) 2024 Realtek Corporation. All rights reserved */

CHECK:BRACES: braces {} should be used on all arms of this statement
#273: FILE: drivers/net/r8169/r8169_phy.c:536:
+       if (hw->hw_ram_code_ver == hw->sw_ram_code_ver) {
[...]
+       } else
[...]

### [PATCH] net/r8169: impelment MTU configuration

WARNING:TYPO_SPELLING: 'impelment' may be misspelled - perhaps 'implement'?
#4:
Subject: [PATCH] net/r8169: impelment MTU configuration
                            ^^^^^^^^^
Suprised that you have to keep track of packets in SW.
The kernel driver is able to get them from the HW.

The kernel driver gets lots more counters from the HW which could go to xstats.
  
Stephen Hemminger Oct. 23, 2024, 6:16 a.m. UTC | #3
On Wed, 23 Oct 2024 05:59:12 +0000
王颢 <howard_wang@realsil.com.cn> wrote:

> Dear Stephen,
> 
> OK, I will fix these issues.
> I would like to confirm that each document needs to have a blank line at the end, correct? 
> Apologies, but we plan to maintain our current approach regarding stats this time.
> 
> Best Regards,
> Howard Wang


Git documents this in 'git apply' man page. In this patchset, the errors appear
to be simple blank lines at the end.

       --whitespace=<action>
           When applying a patch, detect a new or modified line that has
           whitespace errors. What are considered whitespace errors is
           controlled by core.whitespace configuration. By default, trailing
           whitespaces (including lines that solely consist of whitespaces) and
           a space character that is immediately followed by a tab character
           inside the initial indent of the line are considered whitespace
           errors.

           By default, the command outputs warning messages but applies the
           patch. When git-apply is used for statistics and not applying a
           patch, it defaults to nowarn.

           You can use different <action> values to control this behavior:

           •   nowarn turns off the trailing whitespace warning.

           •   warn outputs warnings for a few such errors, but applies the
               patch as-is (default).

           •   fix outputs warnings for a few such errors, and applies the
               patch after fixing them (strip is a synonym — the tool used to
               consider only trailing whitespace characters as errors, and the
               fix involved stripping them, but modern Gits do more).

           •   error outputs warnings for a few such errors, and refuses to
               apply the patch.

           •   error-all is similar to error but shows all errors.
  
Howard Wang Oct. 31, 2024, 7:47 a.m. UTC | #4
Dear Stephen,

I have modified the code related to braces in the latest patch. Recently, I was reviewing the DPDK Coding Style and found the reason why I initially removed the redundant braces. If symmetrical braces are required, it would be better to update the DPDK Coding Style accordingly.

in https://doc.dpdk.org/guides/contributing/coding_style.html
Braces that are not necessary should be left out.

if (test)
        stmt;
else if (bar) {
        stmt;
        stmt;
} else
        stmt;

Best Regards,
Howard Wang

-----邮件原件-----
发件人: Stephen Hemminger <stephen@networkplumber.org> 
发送时间: 2024年10月23日 13:18
收件人: 王颢 <howard_wang@realsil.com.cn>
抄送: dev@dpdk.org; pro_nic_dpdk@realtek.com
主题: Re: [PATCH v3 01/18] net/r8169: add PMD driver skeleton


External mail.



On Wed, 23 Oct 2024 11:33:11 +0800
Howard Wang <howard_wang@realsil.com.cn> 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>

This version is much better than the last.
Still has some issues:
  - git whitespace
  - braces
  - one spelling error

For example, doing git apply showed.

Applying: net/r8169: add support for hardware operations
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2381: trailing whitespace.
                                          (BIT_15 | BIT_14 | BIT_13 | BIT_12 |
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:460: new blank line at EOF.
+
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2052: new blank line at EOF.
+
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2073: new blank line at EOF.
+
/home/shemminger/DPDK/main/.git/worktrees/r8169/rebase-apply/patch:2470: new blank line at EOF.
+
warning: squelched 13 whitespace errors
warning: 18 lines add whitespace errors.


Some minor checkpatch warnings.

CHECK:BRACES: braces {} should be used on all arms of this statement
#268: FILE: drivers/net/r8169/r8169_hw.c:67:
+               if (len <= 4 - val_shift)
[...]
+               else {
[...]

CHECK:BRACES: Unbalanced braces around else statement
#270: FILE: drivers/net/r8169/r8169_hw.c:69:
+               else {

CHECK:BRACES: braces {} should be used on all arms of this statement
#334: FILE: drivers/net/r8169/r8169_hw.c:133:
+               if (len <= 4 - val_shift)
[...]
+               else {
[...]

CHECK:BRACES: Unbalanced braces around else statement
#336: FILE: drivers/net/r8169/r8169_hw.c:135:
+               else {

CHECK:BRACES: Blank lines aren't necessary before a close brace '}'
#433: FILE: drivers/net/r8169/r8169_hw.c:232:
+
+               }

WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#978: FILE: drivers/net/r8169/r8169_phy.c:2:
+ * Copyright(c) 2024 Realtek Corporation. All rights reserved */

CHECK:BRACES: braces {} should be used on all arms of this statement
#273: FILE: drivers/net/r8169/r8169_phy.c:536:
+       if (hw->hw_ram_code_ver == hw->sw_ram_code_ver) {
[...]
+       } else
[...]

### [PATCH] net/r8169: impelment MTU configuration

WARNING:TYPO_SPELLING: 'impelment' may be misspelled - perhaps 'implement'?
#4:
Subject: [PATCH] net/r8169: impelment MTU configuration
                            ^^^^^^^^^
Suprised that you have to keep track of packets in SW.
The kernel driver is able to get them from the HW.

The kernel driver gets lots more counters from the HW which could go to xstats.
  
Stephen Hemminger Oct. 31, 2024, 3:49 p.m. UTC | #5
On Thu, 31 Oct 2024 07:47:51 +0000
王颢 <howard_wang@realsil.com.cn> wrote:

> Dear Stephen,
> 
> I have modified the code related to braces in the latest patch. Recently, I was reviewing the DPDK Coding Style and found the reason why I initially removed the redundant braces. If symmetrical braces are required, it would be better to update the DPDK Coding Style accordingly.
> 
> in https://doc.dpdk.org/guides/contributing/coding_style.html
> Braces that are not necessary should be left out.
> 
> if (test)
>         stmt;
> else if (bar) {
>         stmt;
>         stmt;
> } else
>         stmt;
> 
> Best Regards,
> Howard Wang

Thanks, the DPDK inherits DPDK from the Linux kernel, and the Linux kernel
community sometimes changes and adds things.
  

Patch

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>
+F: drivers/net/r8169
+
 
 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..e37b4fb237
--- /dev/null
+++ b/drivers/net/r8169/meson.build
@@ -0,0 +1,7 @@ 
+# 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..c3b0186daa
--- /dev/null
+++ b/drivers/net/r8169/r8169_base.h
@@ -0,0 +1,16 @@ 
+/* 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;
+
+#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..ec26bd2594
--- /dev/null
+++ b/drivers/net/r8169/r8169_ethdev.c
@@ -0,0 +1,179 @@ 
+/* 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>
+
+#include "r8169_ethdev.h"
+#include "r8169_base.h"
+
+static int rtl_dev_configure(struct rte_eth_dev *dev __rte_unused);
+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;
+
+	hw->adapter_stopped = 1;
+	dev->data->dev_started = 0;
+
+	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;
+	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);
+
+	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..5453832e04
--- /dev/null
+++ b/drivers/net/r8169/r8169_ethdev.h
@@ -0,0 +1,41 @@ 
+/* 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);
+
+#endif
+