* [PATCH net-next v2] net: txgbe: Add build support for txgbe
@ 2022-05-17 9:21 Jiawen Wu
2022-05-17 21:28 ` kernel test robot
2022-05-18 3:12 ` Andrew Lunn
0 siblings, 2 replies; 4+ messages in thread
From: Jiawen Wu @ 2022-05-17 9:21 UTC (permalink / raw)
To: netdev; +Cc: Jiawen Wu
Add doc build infrastructure for txgbe driver.
Initialize PCI memory space for WangXun 10 Gigabit Ethernet devices.
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
.../device_drivers/ethernet/index.rst | 1 +
.../device_drivers/ethernet/wangxun/txgbe.rst | 19 +
MAINTAINERS | 7 +
drivers/net/ethernet/Kconfig | 1 +
drivers/net/ethernet/Makefile | 1 +
drivers/net/ethernet/wangxun/Kconfig | 34 ++
drivers/net/ethernet/wangxun/Makefile | 6 +
drivers/net/ethernet/wangxun/txgbe/Makefile | 9 +
drivers/net/ethernet/wangxun/txgbe/txgbe.h | 76 ++++
.../net/ethernet/wangxun/txgbe/txgbe_main.c | 332 ++++++++++++++++++
.../net/ethernet/wangxun/txgbe/txgbe_type.h | 87 +++++
11 files changed, 573 insertions(+)
create mode 100644 Documentation/networking/device_drivers/ethernet/wangxun/txgbe.rst
create mode 100644 drivers/net/ethernet/wangxun/Kconfig
create mode 100644 drivers/net/ethernet/wangxun/Makefile
create mode 100644 drivers/net/ethernet/wangxun/txgbe/Makefile
create mode 100644 drivers/net/ethernet/wangxun/txgbe/txgbe.h
create mode 100644 drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
create mode 100644 drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
diff --git a/Documentation/networking/device_drivers/ethernet/index.rst b/Documentation/networking/device_drivers/ethernet/index.rst
index 6b5dc203da2b..4766ac9d260e 100644
--- a/Documentation/networking/device_drivers/ethernet/index.rst
+++ b/Documentation/networking/device_drivers/ethernet/index.rst
@@ -52,6 +52,7 @@ Contents:
ti/am65_nuss_cpsw_switchdev
ti/tlan
toshiba/spider_net
+ wangxun/txgbe
.. only:: subproject and html
diff --git a/Documentation/networking/device_drivers/ethernet/wangxun/txgbe.rst b/Documentation/networking/device_drivers/ethernet/wangxun/txgbe.rst
new file mode 100644
index 000000000000..de5661ba0603
--- /dev/null
+++ b/Documentation/networking/device_drivers/ethernet/wangxun/txgbe.rst
@@ -0,0 +1,19 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+================================================================
+Linux Base Driver for WangXun(R) 10 Gigabit PCI Express Adapters
+================================================================
+
+WangXun 10 Gigabit Linux driver.
+Copyright (c) 2015 - 2022 Beijing WangXun Technology Co., Ltd.
+
+
+Contents
+========
+
+- Support
+
+
+Support
+=======
+If you got any problem, contact Wangxun support team via support@trustnetic.com
diff --git a/MAINTAINERS b/MAINTAINERS
index b7b1dfba707c..37d1043d1926 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21207,6 +21207,13 @@ L: linux-input@vger.kernel.org
S: Maintained
F: drivers/input/tablet/wacom_serial4.c
+WANGXUN ETHERNET DRIVER
+M: Jiawen Wu <jiawenwu@trustnetic.com>
+L: netdev@vger.kernel.org
+S: Maintained
+F: Documentation/networking/device_drivers/ethernet/wangxun/txgbe.rst
+F: drivers/net/ethernet/wangxun/
+
WATCHDOG DEVICE DRIVERS
M: Wim Van Sebroeck <wim@linux-watchdog.org>
M: Guenter Roeck <linux@roeck-us.net>
diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 827993022386..e505cb1c171b 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -84,6 +84,7 @@ source "drivers/net/ethernet/huawei/Kconfig"
source "drivers/net/ethernet/i825xx/Kconfig"
source "drivers/net/ethernet/ibm/Kconfig"
source "drivers/net/ethernet/intel/Kconfig"
+source "drivers/net/ethernet/wangxun/Kconfig"
source "drivers/net/ethernet/xscale/Kconfig"
config JME
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index 8ef43e0c33c0..82db3b15e421 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_NET_VENDOR_TOSHIBA) += toshiba/
obj-$(CONFIG_NET_VENDOR_TUNDRA) += tundra/
obj-$(CONFIG_NET_VENDOR_VERTEXCOM) += vertexcom/
obj-$(CONFIG_NET_VENDOR_VIA) += via/
+obj-$(CONFIG_NET_VENDOR_WANGXUN) += wangxun/
obj-$(CONFIG_NET_VENDOR_WIZNET) += wiznet/
obj-$(CONFIG_NET_VENDOR_XILINX) += xilinx/
obj-$(CONFIG_NET_VENDOR_XIRCOM) += xircom/
diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig
new file mode 100644
index 000000000000..251f222f84db
--- /dev/null
+++ b/drivers/net/ethernet/wangxun/Kconfig
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Wangxun network device configuration
+#
+
+config NET_VENDOR_WANGXUN
+ bool "Wangxun devices"
+ default y
+ help
+ If you have a network (Ethernet) card belonging to this class, say Y.
+
+ Note that the answer to this question doesn't directly affect the
+ kernel: saying N will just cause the configurator to skip all
+ the questions about Intel cards. If you say Y, you will be asked for
+ your specific card in the following questions.
+
+if NET_VENDOR_WANGXUN
+
+config TXGBE
+ tristate "Wangxun(R) 10GbE PCI Express adapters support"
+ depends on PCI
+ depends on PTP_1588_CLOCK_OPTIONAL
+ select PHYLIB
+ help
+ This driver supports Wangxun(R) 10GbE PCI Express family of
+ adapters.
+
+ More specific information on configuring the driver is in
+ <file:Documentation/networking/device_drivers/ethernet/wangxun/txgbe.rst>.
+
+ To compile this driver as a module, choose M here. The module
+ will be called txgbe.
+
+endif # NET_VENDOR_WANGXUN
diff --git a/drivers/net/ethernet/wangxun/Makefile b/drivers/net/ethernet/wangxun/Makefile
new file mode 100644
index 000000000000..c34db1bead25
--- /dev/null
+++ b/drivers/net/ethernet/wangxun/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for the Wangxun network device drivers.
+#
+
+obj-$(CONFIG_TXGBE) += txgbe/
diff --git a/drivers/net/ethernet/wangxun/txgbe/Makefile b/drivers/net/ethernet/wangxun/txgbe/Makefile
new file mode 100644
index 000000000000..725aa1f721f6
--- /dev/null
+++ b/drivers/net/ethernet/wangxun/txgbe/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2015 - 2017 Beijing WangXun Technology Co., Ltd.
+#
+# Makefile for the Wangxun(R) 10GbE PCI Express ethernet driver
+#
+
+obj-$(CONFIG_TXGBE) += txgbe.o
+
+txgbe-objs := txgbe_main.o
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe.h b/drivers/net/ethernet/wangxun/txgbe/txgbe.h
new file mode 100644
index 000000000000..d51b5a2c5356
--- /dev/null
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe.h
@@ -0,0 +1,76 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2015 - 2017 Beijing WangXun Technology Co., Ltd. */
+
+#ifndef _TXGBE_H_
+#define _TXGBE_H_
+
+#include "txgbe_type.h"
+
+#ifndef MAX_REQUEST_SIZE
+#define MAX_REQUEST_SIZE 256
+#endif
+
+#define TXGBE_MAX_FDIR_INDICES 63
+
+#define MAX_TX_QUEUES (TXGBE_MAX_FDIR_INDICES + 1)
+
+/* board specific private data structure */
+struct txgbe_adapter {
+ /* OS defined structs */
+ struct net_device *netdev;
+ struct pci_dev *pdev;
+
+ unsigned long state;
+
+ /* structs defined in txgbe_hw.h */
+ struct txgbe_hw hw;
+ u16 msg_enable;
+
+ u8 __iomem *io_addr; /* Mainly for iounmap use */
+};
+
+enum txgbe_state_t {
+ __TXGBE_TESTING,
+ __TXGBE_RESETTING,
+ __TXGBE_DOWN,
+ __TXGBE_HANGING,
+ __TXGBE_DISABLED,
+ __TXGBE_REMOVING,
+ __TXGBE_SERVICE_SCHED,
+ __TXGBE_SERVICE_INITED,
+ __TXGBE_IN_SFP_INIT,
+ __TXGBE_PTP_RUNNING,
+ __TXGBE_PTP_TX_IN_PROGRESS,
+};
+
+#define TXGBE_NAME "txgbe"
+
+static inline struct device *pci_dev_to_dev(struct pci_dev *pdev)
+{
+ return &pdev->dev;
+}
+
+#define txgbe_dev_info(format, arg...) \
+ dev_info(&adapter->pdev->dev, format, ## arg)
+#define txgbe_dev_warn(format, arg...) \
+ dev_warn(&adapter->pdev->dev, format, ## arg)
+#define txgbe_dev_err(format, arg...) \
+ dev_err(&adapter->pdev->dev, format, ## arg)
+#define txgbe_dev_notice(format, arg...) \
+ dev_notice(&adapter->pdev->dev, format, ## arg)
+#define txgbe_dbg(msglvl, format, arg...) \
+ netif_dbg(adapter, msglvl, adapter->netdev, format, ## arg)
+#define txgbe_info(msglvl, format, arg...) \
+ netif_info(adapter, msglvl, adapter->netdev, format, ## arg)
+#define txgbe_err(msglvl, format, arg...) \
+ netif_err(adapter, msglvl, adapter->netdev, format, ## arg)
+#define txgbe_warn(msglvl, format, arg...) \
+ netif_warn(adapter, msglvl, adapter->netdev, format, ## arg)
+#define txgbe_crit(msglvl, format, arg...) \
+ netif_crit(adapter, msglvl, adapter->netdev, format, ## arg)
+
+#define TXGBE_FAILED_READ_CFG_DWORD 0xffffffffU
+#define TXGBE_FAILED_READ_CFG_WORD 0xffffU
+#define TXGBE_FAILED_READ_CFG_BYTE 0xffU
+
+#endif /* _TXGBE_H_ */
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
new file mode 100644
index 000000000000..17a30629f76a
--- /dev/null
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -0,0 +1,332 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2015 - 2017 Beijing WangXun Technology Co., Ltd. */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/netdevice.h>
+#include <linux/string.h>
+#include <linux/aer.h>
+#include <linux/etherdevice.h>
+
+#include "txgbe.h"
+
+char txgbe_driver_name[32] = TXGBE_NAME;
+static const char txgbe_driver_string[] =
+ "WangXun 10 Gigabit PCI Express Network Driver";
+
+static const char txgbe_copyright[] =
+ "Copyright (c) 2015 -2017 Beijing WangXun Technology Co., Ltd";
+
+/* txgbe_pci_tbl - PCI Device ID Table
+ *
+ * Wildcard entries (PCI_ANY_ID) should come last
+ * Last entry must be all 0s
+ *
+ * { Vendor ID, Device ID, SubVendor ID, SubDevice ID,
+ * Class, Class Mask, private data (not used) }
+ */
+static const struct pci_device_id txgbe_pci_tbl[] = {
+ { PCI_VDEVICE(TRUSTNETIC, TXGBE_DEV_ID_SP1000), 0},
+ { PCI_VDEVICE(TRUSTNETIC, TXGBE_DEV_ID_WX1820), 0},
+ /* required last entry */
+ { .device = 0 }
+};
+MODULE_DEVICE_TABLE(pci, txgbe_pci_tbl);
+
+MODULE_AUTHOR("Beijing WangXun Technology Co., Ltd, <software@trustnetic.com>");
+MODULE_DESCRIPTION("WangXun(R) 10 Gigabit PCI Express Network Driver");
+MODULE_LICENSE("GPL");
+
+#define DEFAULT_DEBUG_LEVEL_SHIFT 3
+
+static struct workqueue_struct *txgbe_wq;
+
+static bool txgbe_check_cfg_remove(struct txgbe_hw *hw, struct pci_dev *pdev);
+
+static void txgbe_remove_adapter(struct txgbe_hw *hw)
+{
+ struct txgbe_adapter *adapter = hw->back;
+
+ if (!hw->hw_addr)
+ return;
+ hw->hw_addr = NULL;
+ txgbe_dev_err("Adapter removed\n");
+}
+
+/**
+ * txgbe_sw_init - Initialize general software structures (struct txgbe_adapter)
+ * @adapter: board private structure to initialize
+ *
+ * txgbe_sw_init initializes the Adapter private data structure.
+ * Fields are initialized based on PCI device information and
+ * OS network device settings (MTU size).
+ **/
+static int txgbe_sw_init(struct txgbe_adapter *adapter)
+{
+ struct txgbe_hw *hw = &adapter->hw;
+ struct pci_dev *pdev = adapter->pdev;
+ int err = 0;
+
+ /* PCI config space info */
+ hw->vendor_id = pdev->vendor;
+ hw->device_id = pdev->device;
+ pci_read_config_byte(pdev, PCI_REVISION_ID, &hw->revision_id);
+ if (hw->revision_id == TXGBE_FAILED_READ_CFG_BYTE &&
+ txgbe_check_cfg_remove(hw, pdev)) {
+ txgbe_err(probe, "read of revision id failed\n");
+ err = -ENODEV;
+ goto out;
+ }
+ hw->subsystem_vendor_id = pdev->subsystem_vendor;
+ hw->subsystem_device_id = pdev->subsystem_device;
+
+ pci_read_config_word(pdev, PCI_SUBSYSTEM_ID, &hw->subsystem_id);
+ if (hw->subsystem_id == TXGBE_FAILED_READ_CFG_WORD) {
+ txgbe_err(probe, "read of subsystem id failed\n");
+ err = -ENODEV;
+ goto out;
+ }
+
+out:
+ return err;
+}
+
+static int __txgbe_shutdown(struct pci_dev *pdev, bool *enable_wake)
+{
+ struct txgbe_adapter *adapter = pci_get_drvdata(pdev);
+ struct net_device *netdev = adapter->netdev;
+
+ netif_device_detach(netdev);
+
+ if (!test_and_set_bit(__TXGBE_DISABLED, &adapter->state))
+ pci_disable_device(pdev);
+
+ return 0;
+}
+
+static void txgbe_shutdown(struct pci_dev *pdev)
+{
+ bool wake;
+
+ __txgbe_shutdown(pdev, &wake);
+
+ if (system_state == SYSTEM_POWER_OFF) {
+ pci_wake_from_d3(pdev, wake);
+ pci_set_power_state(pdev, PCI_D3hot);
+ }
+}
+
+/**
+ * txgbe_probe - Device Initialization Routine
+ * @pdev: PCI device information struct
+ * @ent: entry in txgbe_pci_tbl
+ *
+ * Returns 0 on success, negative on failure
+ *
+ * txgbe_probe initializes an adapter identified by a pci_dev structure.
+ * The OS initialization, configuring of the adapter private structure,
+ * and a hardware reset occur.
+ **/
+static int txgbe_probe(struct pci_dev *pdev,
+ const struct pci_device_id __always_unused *ent)
+{
+ struct net_device *netdev;
+ struct txgbe_adapter *adapter = NULL;
+ struct txgbe_hw *hw = NULL;
+ int err, pci_using_dac;
+ unsigned int indices = MAX_TX_QUEUES;
+ bool disable_dev = false;
+
+ err = pci_enable_device_mem(pdev);
+ if (err)
+ return err;
+
+ if (!dma_set_mask(pci_dev_to_dev(pdev), DMA_BIT_MASK(64)) &&
+ !dma_set_coherent_mask(pci_dev_to_dev(pdev), DMA_BIT_MASK(64))) {
+ pci_using_dac = 1;
+ } else {
+ err = dma_set_mask(pci_dev_to_dev(pdev), DMA_BIT_MASK(32));
+ if (err) {
+ err = dma_set_coherent_mask(pci_dev_to_dev(pdev),
+ DMA_BIT_MASK(32));
+ if (err) {
+ dev_err(pci_dev_to_dev(pdev),
+ "No usable DMA configuration, aborting\n");
+ goto err_dma;
+ }
+ }
+ pci_using_dac = 0;
+ }
+
+ err = pci_request_selected_regions(pdev,
+ pci_select_bars(pdev, IORESOURCE_MEM),
+ txgbe_driver_name);
+ if (err) {
+ dev_err(pci_dev_to_dev(pdev),
+ "pci_request_selected_regions failed 0x%x\n", err);
+ goto err_pci_reg;
+ }
+
+ hw = vmalloc(sizeof(*hw));
+ if (!hw)
+ return -ENOMEM;
+
+ hw->vendor_id = pdev->vendor;
+ hw->device_id = pdev->device;
+ vfree(hw);
+
+ pci_enable_pcie_error_reporting(pdev);
+ pci_set_master(pdev);
+ /* errata 16 */
+ if (MAX_REQUEST_SIZE == 512) {
+ pcie_capability_clear_and_set_word(pdev, PCI_EXP_DEVCTL,
+ PCI_EXP_DEVCTL_READRQ,
+ 0x2000);
+ } else {
+ pcie_capability_clear_and_set_word(pdev, PCI_EXP_DEVCTL,
+ PCI_EXP_DEVCTL_READRQ,
+ 0x1000);
+ }
+
+ netdev = alloc_etherdev_mq(sizeof(struct txgbe_adapter), indices);
+ if (!netdev) {
+ err = -ENOMEM;
+ goto err_alloc_etherdev;
+ }
+
+ SET_NETDEV_DEV(netdev, pci_dev_to_dev(pdev));
+
+ adapter = netdev_priv(netdev);
+ adapter->netdev = netdev;
+ adapter->pdev = pdev;
+ hw = &adapter->hw;
+ hw->back = adapter;
+ adapter->msg_enable = (1 << DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
+
+ hw->hw_addr = ioremap(pci_resource_start(pdev, 0),
+ pci_resource_len(pdev, 0));
+ adapter->io_addr = hw->hw_addr;
+ if (!hw->hw_addr) {
+ err = -EIO;
+ goto err_ioremap;
+ }
+
+ strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
+
+ /* setup the private structure */
+ err = txgbe_sw_init(adapter);
+ if (err)
+ goto err_sw_init;
+
+ if (pci_using_dac)
+ netdev->features |= NETIF_F_HIGHDMA;
+
+err_sw_init:
+ iounmap(adapter->io_addr);
+err_ioremap:
+ disable_dev = !test_and_set_bit(__TXGBE_DISABLED, &adapter->state);
+ free_netdev(netdev);
+err_alloc_etherdev:
+ pci_release_selected_regions(pdev,
+ pci_select_bars(pdev, IORESOURCE_MEM));
+err_pci_reg:
+err_dma:
+ if (!adapter || disable_dev)
+ pci_disable_device(pdev);
+ return err;
+}
+
+/**
+ * txgbe_remove - Device Removal Routine
+ * @pdev: PCI device information struct
+ *
+ * txgbe_remove is called by the PCI subsystem to alert the driver
+ * that it should release a PCI device. The could be caused by a
+ * Hot-Plug event, or because the driver is going to be removed from
+ * memory.
+ **/
+static void txgbe_remove(struct pci_dev *pdev)
+{
+ struct txgbe_adapter *adapter = pci_get_drvdata(pdev);
+ struct net_device *netdev;
+ bool disable_dev;
+
+ /* if !adapter then we already cleaned up in probe */
+ if (!adapter)
+ return;
+
+ netdev = adapter->netdev;
+
+ iounmap(adapter->io_addr);
+ pci_release_selected_regions(pdev,
+ pci_select_bars(pdev, IORESOURCE_MEM));
+
+ disable_dev = !test_and_set_bit(__TXGBE_DISABLED, &adapter->state);
+ free_netdev(netdev);
+
+ pci_disable_pcie_error_reporting(pdev);
+
+ if (disable_dev)
+ pci_disable_device(pdev);
+}
+
+static bool txgbe_check_cfg_remove(struct txgbe_hw *hw, struct pci_dev *pdev)
+{
+ u16 value;
+
+ pci_read_config_word(pdev, PCI_VENDOR_ID, &value);
+ if (value == TXGBE_FAILED_READ_CFG_WORD) {
+ txgbe_remove_adapter(hw);
+ return true;
+ }
+ return false;
+}
+
+static struct pci_driver txgbe_driver = {
+ .name = txgbe_driver_name,
+ .id_table = txgbe_pci_tbl,
+ .probe = txgbe_probe,
+ .remove = txgbe_remove,
+ .shutdown = txgbe_shutdown,
+};
+
+/**
+ * txgbe_init_module - Driver Registration Routine
+ *
+ * txgbe_init_module is the first routine called when the driver is
+ * loaded. All it does is register with the PCI subsystem.
+ **/
+static int __init txgbe_init_module(void)
+{
+ int ret;
+
+ pr_info("%s\n", txgbe_driver_string);
+ pr_info("%s\n", txgbe_copyright);
+
+ txgbe_wq = create_singlethread_workqueue(txgbe_driver_name);
+ if (!txgbe_wq) {
+ pr_err("%s: Failed to create workqueue\n", txgbe_driver_name);
+ return -ENOMEM;
+ }
+
+ ret = pci_register_driver(&txgbe_driver);
+ return ret;
+}
+
+module_init(txgbe_init_module);
+
+/**
+ * txgbe_exit_module - Driver Exit Cleanup Routine
+ *
+ * txgbe_exit_module is called just before the driver is removed
+ * from memory.
+ **/
+static void __exit txgbe_exit_module(void)
+{
+ pci_unregister_driver(&txgbe_driver);
+ if (txgbe_wq)
+ destroy_workqueue(txgbe_wq);
+}
+
+module_exit(txgbe_exit_module);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
new file mode 100644
index 000000000000..ba9306982317
--- /dev/null
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -0,0 +1,87 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2015 - 2017 Beijing WangXun Technology Co., Ltd. */
+
+#ifndef _TXGBE_TYPE_H_
+#define _TXGBE_TYPE_H_
+
+#include <linux/types.h>
+#include <linux/netdevice.h>
+
+/* Little Endian defines */
+#ifndef __le16
+#define __le16 u16
+#endif
+#ifndef __le32
+#define __le32 u32
+#endif
+#ifndef __le64
+#define __le64 u64
+
+#endif
+#ifndef __be16
+/* Big Endian defines */
+#define __be16 u16
+#define __be32 u32
+#define __be64 u64
+
+#endif
+
+/************ txgbe_register.h ************/
+/* Vendor ID */
+#ifndef PCI_VENDOR_ID_TRUSTNETIC
+#define PCI_VENDOR_ID_TRUSTNETIC 0x8088
+#endif
+
+/* Device IDs */
+#define TXGBE_DEV_ID_SP1000 0x1001
+#define TXGBE_DEV_ID_WX1820 0x2001
+
+/* Subsystem IDs */
+/* SFP */
+#define TXGBE_ID_SP1000_SFP 0x0000
+#define TXGBE_ID_WX1820_SFP 0x2000
+#define TXGBE_ID_SFP 0x00
+
+/* copper */
+#define TXGBE_ID_SP1000_XAUI 0x1010
+#define TXGBE_ID_WX1820_XAUI 0x2010
+#define TXGBE_ID_XAUI 0x10
+#define TXGBE_ID_SP1000_SGMII 0x1020
+#define TXGBE_ID_WX1820_SGMII 0x2020
+#define TXGBE_ID_SGMII 0x20
+/* backplane */
+#define TXGBE_ID_SP1000_KR_KX_KX4 0x1030
+#define TXGBE_ID_WX1820_KR_KX_KX4 0x2030
+#define TXGBE_ID_KR_KX_KX4 0x30
+/* MAC Interface */
+#define TXGBE_ID_SP1000_MAC_XAUI 0x1040
+#define TXGBE_ID_WX1820_MAC_XAUI 0x2040
+#define TXGBE_ID_MAC_XAUI 0x40
+#define TXGBE_ID_SP1000_MAC_SGMII 0x1060
+#define TXGBE_ID_WX1820_MAC_SGMII 0x2060
+#define TXGBE_ID_MAC_SGMII 0x60
+
+#define TXGBE_NCSI_SUP 0x8000
+#define TXGBE_NCSI_MASK 0x8000
+#define TXGBE_WOL_SUP 0x4000
+#define TXGBE_WOL_MASK 0x4000
+#define TXGBE_DEV_MASK 0xf0
+
+/* Combined interface*/
+#define TXGBE_ID_SFI_XAUI 0x50
+
+/* Revision ID */
+#define TXGBE_SP_MPW 1
+
+struct txgbe_hw {
+ u8 __iomem *hw_addr;
+ void *back;
+ u16 device_id;
+ u16 vendor_id;
+ u16 subsystem_device_id;
+ u16 subsystem_vendor_id;
+ u8 revision_id;
+ u16 subsystem_id;
+};
+
+#endif /* _TXGBE_TYPE_H_ */
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next v2] net: txgbe: Add build support for txgbe
2022-05-17 9:21 [PATCH net-next v2] net: txgbe: Add build support for txgbe Jiawen Wu
@ 2022-05-17 21:28 ` kernel test robot
2022-05-18 3:12 ` Andrew Lunn
1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2022-05-17 21:28 UTC (permalink / raw)
To: Jiawen Wu, netdev; +Cc: kbuild-all, Jiawen Wu
Hi Jiawen,
I love your patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Jiawen-Wu/net-txgbe-Add-build-support-for-txgbe/20220517-171540
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 6251264fedde83ade6f0f1f7049037469dd4de0b
config: mips-allmodconfig (https://download.01.org/0day-ci/archive/20220518/202205180531.6Hzhl9KN-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/bbd3d7eec488dc00f9b366f24e12bcb793cdd0bc
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jiawen-Wu/net-txgbe-Add-build-support-for-txgbe/20220517-171540
git checkout bbd3d7eec488dc00f9b366f24e12bcb793cdd0bc
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/net/ethernet/wangxun/txgbe/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
drivers/net/ethernet/wangxun/txgbe/txgbe_main.c: In function 'txgbe_probe':
>> drivers/net/ethernet/wangxun/txgbe/txgbe_main.c:171:14: error: implicit declaration of function 'vmalloc'; did you mean 'kvmalloc'? [-Werror=implicit-function-declaration]
171 | hw = vmalloc(sizeof(*hw));
| ^~~~~~~
| kvmalloc
>> drivers/net/ethernet/wangxun/txgbe/txgbe_main.c:171:12: warning: assignment to 'struct txgbe_hw *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
171 | hw = vmalloc(sizeof(*hw));
| ^
>> drivers/net/ethernet/wangxun/txgbe/txgbe_main.c:177:9: error: implicit declaration of function 'vfree'; did you mean 'kvfree'? [-Werror=implicit-function-declaration]
177 | vfree(hw);
| ^~~~~
| kvfree
cc1: some warnings being treated as errors
vim +171 drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
119
120 /**
121 * txgbe_probe - Device Initialization Routine
122 * @pdev: PCI device information struct
123 * @ent: entry in txgbe_pci_tbl
124 *
125 * Returns 0 on success, negative on failure
126 *
127 * txgbe_probe initializes an adapter identified by a pci_dev structure.
128 * The OS initialization, configuring of the adapter private structure,
129 * and a hardware reset occur.
130 **/
131 static int txgbe_probe(struct pci_dev *pdev,
132 const struct pci_device_id __always_unused *ent)
133 {
134 struct net_device *netdev;
135 struct txgbe_adapter *adapter = NULL;
136 struct txgbe_hw *hw = NULL;
137 int err, pci_using_dac;
138 unsigned int indices = MAX_TX_QUEUES;
139 bool disable_dev = false;
140
141 err = pci_enable_device_mem(pdev);
142 if (err)
143 return err;
144
145 if (!dma_set_mask(pci_dev_to_dev(pdev), DMA_BIT_MASK(64)) &&
146 !dma_set_coherent_mask(pci_dev_to_dev(pdev), DMA_BIT_MASK(64))) {
147 pci_using_dac = 1;
148 } else {
149 err = dma_set_mask(pci_dev_to_dev(pdev), DMA_BIT_MASK(32));
150 if (err) {
151 err = dma_set_coherent_mask(pci_dev_to_dev(pdev),
152 DMA_BIT_MASK(32));
153 if (err) {
154 dev_err(pci_dev_to_dev(pdev),
155 "No usable DMA configuration, aborting\n");
156 goto err_dma;
157 }
158 }
159 pci_using_dac = 0;
160 }
161
162 err = pci_request_selected_regions(pdev,
163 pci_select_bars(pdev, IORESOURCE_MEM),
164 txgbe_driver_name);
165 if (err) {
166 dev_err(pci_dev_to_dev(pdev),
167 "pci_request_selected_regions failed 0x%x\n", err);
168 goto err_pci_reg;
169 }
170
> 171 hw = vmalloc(sizeof(*hw));
172 if (!hw)
173 return -ENOMEM;
174
175 hw->vendor_id = pdev->vendor;
176 hw->device_id = pdev->device;
> 177 vfree(hw);
178
179 pci_enable_pcie_error_reporting(pdev);
180 pci_set_master(pdev);
181 /* errata 16 */
182 if (MAX_REQUEST_SIZE == 512) {
183 pcie_capability_clear_and_set_word(pdev, PCI_EXP_DEVCTL,
184 PCI_EXP_DEVCTL_READRQ,
185 0x2000);
186 } else {
187 pcie_capability_clear_and_set_word(pdev, PCI_EXP_DEVCTL,
188 PCI_EXP_DEVCTL_READRQ,
189 0x1000);
190 }
191
192 netdev = alloc_etherdev_mq(sizeof(struct txgbe_adapter), indices);
193 if (!netdev) {
194 err = -ENOMEM;
195 goto err_alloc_etherdev;
196 }
197
198 SET_NETDEV_DEV(netdev, pci_dev_to_dev(pdev));
199
200 adapter = netdev_priv(netdev);
201 adapter->netdev = netdev;
202 adapter->pdev = pdev;
203 hw = &adapter->hw;
204 hw->back = adapter;
205 adapter->msg_enable = (1 << DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
206
207 hw->hw_addr = ioremap(pci_resource_start(pdev, 0),
208 pci_resource_len(pdev, 0));
209 adapter->io_addr = hw->hw_addr;
210 if (!hw->hw_addr) {
211 err = -EIO;
212 goto err_ioremap;
213 }
214
215 strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
216
217 /* setup the private structure */
218 err = txgbe_sw_init(adapter);
219 if (err)
220 goto err_sw_init;
221
222 if (pci_using_dac)
223 netdev->features |= NETIF_F_HIGHDMA;
224
225 err_sw_init:
226 iounmap(adapter->io_addr);
227 err_ioremap:
228 disable_dev = !test_and_set_bit(__TXGBE_DISABLED, &adapter->state);
229 free_netdev(netdev);
230 err_alloc_etherdev:
231 pci_release_selected_regions(pdev,
232 pci_select_bars(pdev, IORESOURCE_MEM));
233 err_pci_reg:
234 err_dma:
235 if (!adapter || disable_dev)
236 pci_disable_device(pdev);
237 return err;
238 }
239
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next v2] net: txgbe: Add build support for txgbe
2022-05-17 9:21 [PATCH net-next v2] net: txgbe: Add build support for txgbe Jiawen Wu
2022-05-17 21:28 ` kernel test robot
@ 2022-05-18 3:12 ` Andrew Lunn
[not found] ` <01d001d86fe7$a4f4ba20$eede2e60$@trustnetic.com>
1 sibling, 1 reply; 4+ messages in thread
From: Andrew Lunn @ 2022-05-18 3:12 UTC (permalink / raw)
To: Jiawen Wu; +Cc: netdev
> +Support
> +=======
> +If you got any problem, contact Wangxun support team via support@trustnetic.com
Since this is now a mainline driver, you should be doing support out
in the open. So indicate your should also Cc: netdev, so other members
of the networking community using this hardware can learn as well from
peoples questions.
> +config TXGBE
> + tristate "Wangxun(R) 10GbE PCI Express adapters support"
> + depends on PCI
> + depends on PTP_1588_CLOCK_OPTIONAL
> + select PHYLIB
The current driver does not depend on PTP nor need PHYLIB. Please add
these when they are actually needed.
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2015 - 2017 Beijing WangXun Technology Co., Ltd. */
> +
> +#ifndef _TXGBE_H_
> +#define _TXGBE_H_
> +
> +#include "txgbe_type.h"
> +
> +#ifndef MAX_REQUEST_SIZE
> +#define MAX_REQUEST_SIZE 256
> +#endif
Why the #ifndef? What could be setting it?
A TXGBE_ prefix would also be good.
> +
> +#define TXGBE_MAX_FDIR_INDICES 63
> +
> +#define MAX_TX_QUEUES (TXGBE_MAX_FDIR_INDICES + 1)
Prefix here as well.
> +
> +/* board specific private data structure */
> +struct txgbe_adapter {
> + /* OS defined structs */
> + struct net_device *netdev;
> + struct pci_dev *pdev;
> +
> + unsigned long state;
> +
> + /* structs defined in txgbe_hw.h */
> + struct txgbe_hw hw;
> + u16 msg_enable;
> +
> + u8 __iomem *io_addr; /* Mainly for iounmap use */
> +};
> +
> +enum txgbe_state_t {
> + __TXGBE_TESTING,
> + __TXGBE_RESETTING,
> + __TXGBE_DOWN,
> + __TXGBE_HANGING,
> + __TXGBE_DISABLED,
> + __TXGBE_REMOVING,
> + __TXGBE_SERVICE_SCHED,
> + __TXGBE_SERVICE_INITED,
> + __TXGBE_IN_SFP_INIT,
> + __TXGBE_PTP_RUNNING,
> + __TXGBE_PTP_TX_IN_PROGRESS,
> +};
> +
> +#define TXGBE_NAME "txgbe"
> +
> +static inline struct device *pci_dev_to_dev(struct pci_dev *pdev)
> +{
> + return &pdev->dev;
> +}
Does not have any value. &pdev->dev; is shorter than
pci_dev_to_dev(pdev), there are no casts here, etc.
> +#define txgbe_dev_info(format, arg...) \
> + dev_info(&adapter->pdev->dev, format, ## arg)
> +#define txgbe_dev_warn(format, arg...) \
> + dev_warn(&adapter->pdev->dev, format, ## arg)
> +#define txgbe_dev_err(format, arg...) \
> + dev_err(&adapter->pdev->dev, format, ## arg)
> +#define txgbe_dev_notice(format, arg...) \
> + dev_notice(&adapter->pdev->dev, format, ## arg)
> +#define txgbe_dbg(msglvl, format, arg...) \
> + netif_dbg(adapter, msglvl, adapter->netdev, format, ## arg)
> +#define txgbe_info(msglvl, format, arg...) \
> + netif_info(adapter, msglvl, adapter->netdev, format, ## arg)
> +#define txgbe_err(msglvl, format, arg...) \
> + netif_err(adapter, msglvl, adapter->netdev, format, ## arg)
> +#define txgbe_warn(msglvl, format, arg...) \
> + netif_warn(adapter, msglvl, adapter->netdev, format, ## arg)
> +#define txgbe_crit(msglvl, format, arg...) \
> + netif_crit(adapter, msglvl, adapter->netdev, format, ## arg)
It is pretty unusual to use wrappers like this. It is also bad
practice for a macro to access something which is not passed to it as
a parameter. I suggest you remove all these.
> +
> +#define TXGBE_FAILED_READ_CFG_DWORD 0xffffffffU
> +#define TXGBE_FAILED_READ_CFG_WORD 0xffffU
> +#define TXGBE_FAILED_READ_CFG_BYTE 0xffU
> +
> +#endif /* _TXGBE_H_ */
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> new file mode 100644
> index 000000000000..17a30629f76a
> --- /dev/null
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -0,0 +1,332 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2015 - 2017 Beijing WangXun Technology Co., Ltd. */
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/netdevice.h>
> +#include <linux/string.h>
> +#include <linux/aer.h>
> +#include <linux/etherdevice.h>
> +
> +#include "txgbe.h"
> +
> +char txgbe_driver_name[32] = TXGBE_NAME;
> +static const char txgbe_driver_string[] =
> + "WangXun 10 Gigabit PCI Express Network Driver";
> +
> +static const char txgbe_copyright[] =
> + "Copyright (c) 2015 -2017 Beijing WangXun Technology Co., Ltd";
Only until 2017? You don't need this anyway, you have the copyright on
the top of each file.
> +
> +/* txgbe_pci_tbl - PCI Device ID Table
> + *
> + * Wildcard entries (PCI_ANY_ID) should come last
> + * Last entry must be all 0s
> + *
> + * { Vendor ID, Device ID, SubVendor ID, SubDevice ID,
> + * Class, Class Mask, private data (not used) }
> + */
> +static const struct pci_device_id txgbe_pci_tbl[] = {
> + { PCI_VDEVICE(TRUSTNETIC, TXGBE_DEV_ID_SP1000), 0},
> + { PCI_VDEVICE(TRUSTNETIC, TXGBE_DEV_ID_WX1820), 0},
> + /* required last entry */
> + { .device = 0 }
> +};
> +MODULE_DEVICE_TABLE(pci, txgbe_pci_tbl);
> +
> +MODULE_AUTHOR("Beijing WangXun Technology Co., Ltd, <software@trustnetic.com>");
> +MODULE_DESCRIPTION("WangXun(R) 10 Gigabit PCI Express Network Driver");
> +MODULE_LICENSE("GPL");
Traditionally, all MODULE_* things come at the end.
> +#define DEFAULT_DEBUG_LEVEL_SHIFT 3
> +
> +static struct workqueue_struct *txgbe_wq;
No globals.
> +
> +static bool txgbe_check_cfg_remove(struct txgbe_hw *hw, struct pci_dev *pdev);
Forwards references should only be needed if you have mutually
recursive functions. For anything else, move the code around to avoid
them.
> +
> +static void txgbe_remove_adapter(struct txgbe_hw *hw)
> +{
> + struct txgbe_adapter *adapter = hw->back;
> +
> + if (!hw->hw_addr)
> + return;
> + hw->hw_addr = NULL;
> + txgbe_dev_err("Adapter removed\n");
It is not an error, modules can be unloaded, drivers unbound etc.
> +}
> +
> +/**
> + * txgbe_sw_init - Initialize general software structures (struct txgbe_adapter)
> + * @adapter: board private structure to initialize
> + *
> + * txgbe_sw_init initializes the Adapter private data structure.
> + * Fields are initialized based on PCI device information and
> + * OS network device settings (MTU size).
> + **/
> +static int txgbe_sw_init(struct txgbe_adapter *adapter)
> +{
> + struct txgbe_hw *hw = &adapter->hw;
> + struct pci_dev *pdev = adapter->pdev;
> + int err = 0;
> +
> + /* PCI config space info */
> + hw->vendor_id = pdev->vendor;
> + hw->device_id = pdev->device;
> + pci_read_config_byte(pdev, PCI_REVISION_ID, &hw->revision_id);
> + if (hw->revision_id == TXGBE_FAILED_READ_CFG_BYTE &&
> + txgbe_check_cfg_remove(hw, pdev)) {
> + txgbe_err(probe, "read of revision id failed\n");
> + err = -ENODEV;
> + goto out;
goto out is used when you have something to cleanup on error. If there
is no cleanup needed, just return -ENODEV.
> + }
> + hw->subsystem_vendor_id = pdev->subsystem_vendor;
> + hw->subsystem_device_id = pdev->subsystem_device;
> +
> + pci_read_config_word(pdev, PCI_SUBSYSTEM_ID, &hw->subsystem_id);
> + if (hw->subsystem_id == TXGBE_FAILED_READ_CFG_WORD) {
> + txgbe_err(probe, "read of subsystem id failed\n");
> + err = -ENODEV;
> + goto out;
And this goto is pointless.
> + }
> +
> +out:
> + return err;
> +}
> +
> +static int __txgbe_shutdown(struct pci_dev *pdev, bool *enable_wake)
Please avoid __foo, unless you already have a _foo. And if you do have
foo, _foo and __foo, you should probably think about better names.
> +{
> + struct txgbe_adapter *adapter = pci_get_drvdata(pdev);
> + struct net_device *netdev = adapter->netdev;
> +
> + netif_device_detach(netdev);
> +
> + if (!test_and_set_bit(__TXGBE_DISABLED, &adapter->state))
> + pci_disable_device(pdev);
> +
> + return 0;
Looks like this should be a void function.
> +}
> +
> +static void txgbe_shutdown(struct pci_dev *pdev)
> +{
> + bool wake;
> +
> + __txgbe_shutdown(pdev, &wake);
> +
> + if (system_state == SYSTEM_POWER_OFF) {
> + pci_wake_from_d3(pdev, wake);
> + pci_set_power_state(pdev, PCI_D3hot);
> + }
> +}
> +
> +/**
> + * txgbe_probe - Device Initialization Routine
> + * @pdev: PCI device information struct
> + * @ent: entry in txgbe_pci_tbl
> + *
> + * Returns 0 on success, negative on failure
> + *
> + * txgbe_probe initializes an adapter identified by a pci_dev structure.
> + * The OS initialization, configuring of the adapter private structure,
> + * and a hardware reset occur.
> + **/
> +static int txgbe_probe(struct pci_dev *pdev,
> + const struct pci_device_id __always_unused *ent)
> +{
> + struct net_device *netdev;
> + struct txgbe_adapter *adapter = NULL;
> + struct txgbe_hw *hw = NULL;
> + int err, pci_using_dac;
> + unsigned int indices = MAX_TX_QUEUES;
> + bool disable_dev = false;
Reverse christmas tree. That is, sort these lines longest to shortest.
> +
> + err = pci_enable_device_mem(pdev);
> + if (err)
> + return err;
> +
> + if (!dma_set_mask(pci_dev_to_dev(pdev), DMA_BIT_MASK(64)) &&
> + !dma_set_coherent_mask(pci_dev_to_dev(pdev), DMA_BIT_MASK(64))) {
> + pci_using_dac = 1;
> + } else {
> + err = dma_set_mask(pci_dev_to_dev(pdev), DMA_BIT_MASK(32));
> + if (err) {
> + err = dma_set_coherent_mask(pci_dev_to_dev(pdev),
> + DMA_BIT_MASK(32));
> + if (err) {
> + dev_err(pci_dev_to_dev(pdev),
> + "No usable DMA configuration, aborting\n");
> + goto err_dma;
> + }
> + }
> + pci_using_dac = 0;
> + }
> +
> + err = pci_request_selected_regions(pdev,
> + pci_select_bars(pdev, IORESOURCE_MEM),
> + txgbe_driver_name);
> + if (err) {
> + dev_err(pci_dev_to_dev(pdev),
> + "pci_request_selected_regions failed 0x%x\n", err);
> + goto err_pci_reg;
> + }
> +
> + hw = vmalloc(sizeof(*hw));
Why vmalloc? Is *hw very big?
> + if (!hw)
> + return -ENOMEM;
This should probably by a goto, to unwind what you have done above.
> +
> + hw->vendor_id = pdev->vendor;
> + hw->device_id = pdev->device;
> + vfree(hw);
??? You just allocated it?
> + pci_enable_pcie_error_reporting(pdev);
> + pci_set_master(pdev);
> + /* errata 16 */
> + if (MAX_REQUEST_SIZE == 512) {
So this probably has something to do with my question above. Please
explain.
> + pcie_capability_clear_and_set_word(pdev, PCI_EXP_DEVCTL,
> + PCI_EXP_DEVCTL_READRQ,
> + 0x2000);
> + } else {
> + pcie_capability_clear_and_set_word(pdev, PCI_EXP_DEVCTL,
> + PCI_EXP_DEVCTL_READRQ,
> + 0x1000);
> + }
> +
> + netdev = alloc_etherdev_mq(sizeof(struct txgbe_adapter), indices);
devm_alloc_etherdev_mqs(). Using devm makes your cleanup code simpler
and so less buggy.
> + if (!netdev) {
> + err = -ENOMEM;
> + goto err_alloc_etherdev;
> + }
> +
> + SET_NETDEV_DEV(netdev, pci_dev_to_dev(pdev));
> +
> + adapter = netdev_priv(netdev);
> + adapter->netdev = netdev;
> + adapter->pdev = pdev;
> + hw = &adapter->hw;
> + hw->back = adapter;
You should not need this. container_of() will get you from hw to adapter.
> + adapter->msg_enable = (1 << DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
> +
> + hw->hw_addr = ioremap(pci_resource_start(pdev, 0),
> + pci_resource_len(pdev, 0));
devm_ioremap()
> + adapter->io_addr = hw->hw_addr;
Suggests you don't actually have a clean separation. So why have hw?
> + if (!hw->hw_addr) {
> + err = -EIO;
> + goto err_ioremap;
> + }
> +
> + strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
The device gets a name when you register it. It is very unusual to do
this. It needs an explanation.
> +
> + /* setup the private structure */
> + err = txgbe_sw_init(adapter);
> + if (err)
> + goto err_sw_init;
> +
> + if (pci_using_dac)
> + netdev->features |= NETIF_F_HIGHDMA;
There should probably be a return 0; here, so the probe is
successful. Without that, you cannot test the remove function.
> +
> +err_sw_init:
> + iounmap(adapter->io_addr);
> +err_ioremap:
> + disable_dev = !test_and_set_bit(__TXGBE_DISABLED, &adapter->state);
> + free_netdev(netdev);
> +err_alloc_etherdev:
> + pci_release_selected_regions(pdev,
> + pci_select_bars(pdev, IORESOURCE_MEM));
> +err_pci_reg:
> +err_dma:
> + if (!adapter || disable_dev)
> + pci_disable_device(pdev);
Having an if in unwind code like this is very unusual. Is it really
needed?
> + return err;
> +}
> +
> +/**
> + * txgbe_remove - Device Removal Routine
> + * @pdev: PCI device information struct
> + *
> + * txgbe_remove is called by the PCI subsystem to alert the driver
> + * that it should release a PCI device. The could be caused by a
> + * Hot-Plug event, or because the driver is going to be removed from
> + * memory.
> + **/
> +static void txgbe_remove(struct pci_dev *pdev)
> +{
> + struct txgbe_adapter *adapter = pci_get_drvdata(pdev);
> + struct net_device *netdev;
> + bool disable_dev;
> +
> + /* if !adapter then we already cleaned up in probe */
> + if (!adapter)
> + return;
Remove is only called if the probe was success. So adapter is valid,
no test needed.
> + netdev = adapter->netdev;
> +
> + iounmap(adapter->io_addr);
> + pci_release_selected_regions(pdev,
> + pci_select_bars(pdev, IORESOURCE_MEM));
> +
> + disable_dev = !test_and_set_bit(__TXGBE_DISABLED, &adapter->state);
> + free_netdev(netdev);
> +
> + pci_disable_pcie_error_reporting(pdev);
> +
> + if (disable_dev)
> + pci_disable_device(pdev);
And this test is probably not needed.
> +}
> +
> +static bool txgbe_check_cfg_remove(struct txgbe_hw *hw, struct pci_dev *pdev)
> +{
> + u16 value;
> +
> + pci_read_config_word(pdev, PCI_VENDOR_ID, &value);
> + if (value == TXGBE_FAILED_READ_CFG_WORD) {
> + txgbe_remove_adapter(hw);
> + return true;
> + }
> + return false;
This needs a comment to explain what is happening here, because it is
not clear to me.
> +}
> +
> +static struct pci_driver txgbe_driver = {
> + .name = txgbe_driver_name,
> + .id_table = txgbe_pci_tbl,
> + .probe = txgbe_probe,
> + .remove = txgbe_remove,
> + .shutdown = txgbe_shutdown,
> +};
> +
> +/**
> + * txgbe_init_module - Driver Registration Routine
> + *
> + * txgbe_init_module is the first routine called when the driver is
> + * loaded. All it does is register with the PCI subsystem.
> + **/
> +static int __init txgbe_init_module(void)
> +{
> + int ret;
> +
> + pr_info("%s\n", txgbe_driver_string);
> + pr_info("%s\n", txgbe_copyright);
Don't spam the kernel log with useless information.
> +
> + txgbe_wq = create_singlethread_workqueue(txgbe_driver_name);
> + if (!txgbe_wq) {
> + pr_err("%s: Failed to create workqueue\n", txgbe_driver_name);
> + return -ENOMEM;
> + }
Why do you need a global work queue? I suggest you start with a plain
PCI device, no __init and __exit functions. You can add this work
queue along with the code which uses it. It will then be clear why it
is needed.
> +/* Little Endian defines */
> +#ifndef __le16
> +#define __le16 u16
> +#endif
> +#ifndef __le32
> +#define __le32 u32
> +#endif
> +#ifndef __le64
> +#define __le64 u64
> +
> +#endif
> +#ifndef __be16
> +/* Big Endian defines */
> +#define __be16 u16
> +#define __be32 u32
> +#define __be64 u64
The kernel provides these. No need for your own.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next v2] net: txgbe: Add build support for txgbe
[not found] ` <01d001d86fe7$a4f4ba20$eede2e60$@trustnetic.com>
@ 2022-05-25 12:56 ` Andrew Lunn
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Lunn @ 2022-05-25 12:56 UTC (permalink / raw)
To: Jiawen Wu; +Cc: netdev
> > > + /* setup the private structure */
> > > + err = txgbe_sw_init(adapter);
> > > + if (err)
> > > + goto err_sw_init;
> > > +
> > > + if (pci_using_dac)
> > > + netdev->features |= NETIF_F_HIGHDMA;
> >
> > There should probably be a return 0; here, so the probe is successful.
> Without
> > that, you cannot test the remove function.
> >
>
> I find that when I execute 'rmmod txgbe', it causes a segmentation fault
> which prints 'iounmap: bad address'.
> But when I try to do 'iounmap' before 'return 0' in the probe function,
> there is no error.
> Could you please tell me the reason for this?
I'm assuming it is this code which is doing the print:
https://elixir.bootlin.com/linux/v5.18/source/arch/x86/mm/ioremap.c#L469
Which suggests the area you are trying to unmap is not actually
mapped.
Your code is a bit confusing:
in probe you have:
+ hw->hw_addr = ioremap(pci_resource_start(pdev, 0),
+ pci_resource_len(pdev, 0));
and remove:
+ iounmap(adapter->io_addr);
There is an assignment adapter->io_addr = hw->hw_addr; but this is
enough suggestion your structure of adapter and hw is not correct.
What i also notice is that release would normally things in the
opposite order to probe. That is not the case for your code.
> > > +static bool txgbe_check_cfg_remove(struct txgbe_hw *hw, struct
> > > +pci_dev *pdev) {
> > > + u16 value;
> > > +
> > > + pci_read_config_word(pdev, PCI_VENDOR_ID, &value);
> > > + if (value == TXGBE_FAILED_READ_CFG_WORD) {
> > > + txgbe_remove_adapter(hw);
> > > + return true;
> > > + }
> > > + return false;
> >
> > This needs a comment to explain what is happening here, because it is not
> > clear to me.
> >
>
> It means some kind of problem occur on PCI.
Which does not explain what this function is doing.
It seems like you have two cases to cover:
A PCI problem during probe. This is probably the more likely case. You
just fail the probe.
A PCI problem during run time. What sort of recovery are you going to
do? Just print a warning and keep going, hope for the best?
Andrew
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-25 12:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-17 9:21 [PATCH net-next v2] net: txgbe: Add build support for txgbe Jiawen Wu
2022-05-17 21:28 ` kernel test robot
2022-05-18 3:12 ` Andrew Lunn
[not found] ` <01d001d86fe7$a4f4ba20$eede2e60$@trustnetic.com>
2022-05-25 12:56 ` Andrew Lunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).