| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <b02be8ae-c904-4d67-9d21-87abf315cb7f@yunsilicon.com>
Date: Mon, 23 Dec 2024 11:43:37 +0800
From: "tianx" <tianx@...silicon.com>
To: "Przemek Kitszel" <przemyslaw.kitszel@...el.com>
Cc: <andrew+netdev@...n.ch>, <kuba@...nel.org>, <netdev@...r.kernel.org>,
<pabeni@...hat.com>, <edumazet@...gle.com>, <davem@...emloft.net>,
<jeff.johnson@....qualcomm.com>, <weihg@...silicon.com>,
<wanry@...silicon.com>
Subject: Re: [PATCH v1 01/16] net-next/yunsilicon: Add xsc driver basic framework
On 2024/12/18 21:58, Przemek Kitszel wrote:
> On 12/18/24 11:50, Xin Tian wrote:
>> Add yunsilicon xsc driver basic framework, including xsc_pci driver
>> and xsc_eth driver
>>
>> Co-developed-by: Honggang Wei <weihg@...silicon.com>
>> Co-developed-by: Lei Yan <Jacky@...silicon.com>
>
> Co-devs need to sign-off too, scripts/checkpatch.pl would catch that
> (and more)
>
got it
>> Signed-off-by: Xin Tian <tianx@...silicon.com>
>> ---
>> drivers/net/ethernet/Kconfig | 1 +
>> drivers/net/ethernet/Makefile | 1 +
>> drivers/net/ethernet/yunsilicon/Kconfig | 26 ++
>> drivers/net/ethernet/yunsilicon/Makefile | 8 +
>> .../ethernet/yunsilicon/xsc/common/xsc_core.h | 132 +++++++++
>> .../net/ethernet/yunsilicon/xsc/net/Kconfig | 16 ++
>> .../net/ethernet/yunsilicon/xsc/net/Makefile | 9 +
>> .../net/ethernet/yunsilicon/xsc/pci/Kconfig | 16 ++
>> .../net/ethernet/yunsilicon/xsc/pci/Makefile | 9 +
>> .../net/ethernet/yunsilicon/xsc/pci/main.c | 272 ++++++++++++++++++
>> 10 files changed, 490 insertions(+)
>> create mode 100644 drivers/net/ethernet/yunsilicon/Kconfig
>> create mode 100644 drivers/net/ethernet/yunsilicon/Makefile
>> create mode 100644
>> drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h
>> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/net/Kconfig
>> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/net/Makefile
>> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/Kconfig
>> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/Makefile
>> create mode 100644 drivers/net/ethernet/yunsilicon/xsc/pci/main.c
>>
>> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
>> index 0baac25db..aa6016597 100644
>> --- a/drivers/net/ethernet/Kconfig
>> +++ b/drivers/net/ethernet/Kconfig
>> @@ -82,6 +82,7 @@ source "drivers/net/ethernet/i825xx/Kconfig"
>> source "drivers/net/ethernet/ibm/Kconfig"
>> source "drivers/net/ethernet/intel/Kconfig"
>> source "drivers/net/ethernet/xscale/Kconfig"
>> +source "drivers/net/ethernet/yunsilicon/Kconfig"
>> config JME
>> tristate "JMicron(R) PCI-Express Gigabit Ethernet support"
>> diff --git a/drivers/net/ethernet/Makefile
>> b/drivers/net/ethernet/Makefile
>> index c03203439..c16c34d4b 100644
>> --- a/drivers/net/ethernet/Makefile
>> +++ b/drivers/net/ethernet/Makefile
>> @@ -51,6 +51,7 @@ obj-$(CONFIG_NET_VENDOR_INTEL) += intel/
>> obj-$(CONFIG_NET_VENDOR_I825XX) += i825xx/
>> obj-$(CONFIG_NET_VENDOR_MICROSOFT) += microsoft/
>> obj-$(CONFIG_NET_VENDOR_XSCALE) += xscale/
>> +obj-$(CONFIG_NET_VENDOR_YUNSILICON) += yunsilicon/
>> obj-$(CONFIG_JME) += jme.o
>> obj-$(CONFIG_KORINA) += korina.o
>> obj-$(CONFIG_LANTIQ_ETOP) += lantiq_etop.o
>> diff --git a/drivers/net/ethernet/yunsilicon/Kconfig
>> b/drivers/net/ethernet/yunsilicon/Kconfig
>> new file mode 100644
>> index 000000000..c766390b4
>> --- /dev/null
>> +++ b/drivers/net/ethernet/yunsilicon/Kconfig
>> @@ -0,0 +1,26 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
>> +# All rights reserved.
>> +# Yunsilicon driver configuration
>> +#
>> +
>> +config NET_VENDOR_YUNSILICON
>> + bool "Yunsilicon devices"
>> + default y
>> + depends on PCI || NET
>
> Would it work for you to have only one of the above enabled?
>
> I didn't noticed your response to the same question on your v0
> (BTW, versioning starts at v0, remember to add also links to previous
> versions (not needed for your v0, to don't bother you with 16 URLs :))
>
Will modify to PCI only, and move NET to xsc_eth Kconfig
>> + depends on ARM64 || X86_64
>> + help
>> + If you have a network (Ethernet) device 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 Yunsilicon cards. If you say Y, you will
>> be asked
>> + for your specific card in the following questions.
>> +
>> +if NET_VENDOR_YUNSILICON
>> +
>> +source "drivers/net/ethernet/yunsilicon/xsc/net/Kconfig"
>> +source "drivers/net/ethernet/yunsilicon/xsc/pci/Kconfig"
>> +
>> +endif # NET_VENDOR_YUNSILICON
>> diff --git a/drivers/net/ethernet/yunsilicon/Makefile
>> b/drivers/net/ethernet/yunsilicon/Makefile
>> new file mode 100644
>> index 000000000..6fc8259a7
>> --- /dev/null
>> +++ b/drivers/net/ethernet/yunsilicon/Makefile
>> @@ -0,0 +1,8 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
>> +# All rights reserved.
>> +# Makefile for the Yunsilicon device drivers.
>> +#
>> +
>> +# obj-$(CONFIG_YUNSILICON_XSC_ETH) += xsc/net/
>> +obj-$(CONFIG_YUNSILICON_XSC_PCI) += xsc/pci/
>> \ No newline at end of file
>> diff --git a/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h
>> b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h
>> new file mode 100644
>> index 000000000..5ed12760e
>> --- /dev/null
>> +++ b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h
>> @@ -0,0 +1,132 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
>> + * All rights reserved.
>> + */
>> +
>> +#ifndef XSC_CORE_H
>> +#define XSC_CORE_H
>
> typically there are two underscores in the header names
>
Got it.
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/pci.h>
>> +
>> +extern unsigned int xsc_log_level;
>> +
>> +#define XSC_PCI_VENDOR_ID 0x1f67
>> +
>> +#define XSC_MC_PF_DEV_ID 0x1011
>> +#define XSC_MC_VF_DEV_ID 0x1012
>> +#define XSC_MC_PF_DEV_ID_DIAMOND 0x1021
>> +
>> +#define XSC_MF_HOST_PF_DEV_ID 0x1051
>> +#define XSC_MF_HOST_VF_DEV_ID 0x1052
>> +#define XSC_MF_SOC_PF_DEV_ID 0x1053
>> +
>> +#define XSC_MS_PF_DEV_ID 0x1111
>> +#define XSC_MS_VF_DEV_ID 0x1112
>> +
>> +#define XSC_MV_HOST_PF_DEV_ID 0x1151
>> +#define XSC_MV_HOST_VF_DEV_ID 0x1152
>> +#define XSC_MV_SOC_PF_DEV_ID 0x1153
>> +
>> +enum {
>> + XSC_LOG_LEVEL_DBG = 0,
>> + XSC_LOG_LEVEL_INFO = 1,
>> + XSC_LOG_LEVEL_WARN = 2,
>> + XSC_LOG_LEVEL_ERR = 3,
>> +};
>> +
>> +#define xsc_dev_log(condition, level, dev, fmt, ...) \
>> +do { \
>> + if (condition) \
>> + dev_printk(level, dev, dev_fmt(fmt), ##__VA_ARGS__); \
>> +} while (0)
>> +
>> +#define xsc_core_dbg(__dev, format, ...) \
>> + xsc_dev_log(xsc_log_level <= XSC_LOG_LEVEL_DBG, KERN_DEBUG, \
>> + &(__dev)->pdev->dev, "%s:%d:(pid %d): " format, \
>> + __func__, __LINE__, current->pid, ##__VA_ARGS__)
>> +
>> +#define xsc_core_dbg_once(__dev, format, ...) \
>> + dev_dbg_once(&(__dev)->pdev->dev, "%s:%d:(pid %d): " format, \
>> + __func__, __LINE__, current->pid, \
>> + ##__VA_ARGS__)
>> +
>> +#define xsc_core_dbg_mask(__dev, mask, format, ...) \
>> +do { \
>> + if ((mask) & xsc_debug_mask) \
>> + xsc_core_dbg(__dev, format, ##__VA_ARGS__); \
>> +} while (0)
>> +
>> +#define xsc_core_err(__dev, format, ...) \
>> + xsc_dev_log(xsc_log_level <= XSC_LOG_LEVEL_ERR, KERN_ERR, \
>> + &(__dev)->pdev->dev, "%s:%d:(pid %d): " format, \
>> + __func__, __LINE__, current->pid, ##__VA_ARGS__)
>> +
>> +#define xsc_core_err_rl(__dev, format, ...) \
>> + dev_err_ratelimited(&(__dev)->pdev->dev, \
>> + "%s:%d:(pid %d): " format, \
>> + __func__, __LINE__, current->pid, \
>> + ##__VA_ARGS__)
>> +
>> +#define xsc_core_warn(__dev, format, ...) \
>> + xsc_dev_log(xsc_log_level <= XSC_LOG_LEVEL_WARN, KERN_WARNING, \
>> + &(__dev)->pdev->dev, "%s:%d:(pid %d): " format, \
>> + __func__, __LINE__, current->pid, ##__VA_ARGS__)
>> +
>> +#define xsc_core_info(__dev, format, ...) \
>> + xsc_dev_log(xsc_log_level <= XSC_LOG_LEVEL_INFO, KERN_INFO, \
>> + &(__dev)->pdev->dev, "%s:%d:(pid %d): " format, \
>> + __func__, __LINE__, current->pid, ##__VA_ARGS__)
>> +
>> +#define xsc_pr_debug(format, ...) \
>> +do { \
>> + if (xsc_log_level <= XSC_LOG_LEVEL_DBG) \
>> + pr_debug(format, ##__VA_ARGS__); \
>> +} while (0)
>> +
>> +#define assert(__dev, expr) \
>> +do { \
>> + if (!(expr)) { \
>> + dev_err(&(__dev)->pdev->dev, \
>> + "Assertion failed! %s, %s, %s, line %d\n", \
>> + #expr, __FILE__, __func__, __LINE__); \
>> + } \
>> +} while (0)
>
> as a rule of thumb, don't add functions/macros that you don't use in
> given patch
>
> have you seen WARN_ON() family?
>
Thank you, will use WARN_ON instead of assert.
>> +
>> +enum {
>> + XSC_MAX_NAME_LEN = 32,
>> +};
>> +
>> +struct xsc_dev_resource {
>> + struct mutex alloc_mutex; /* protect buffer alocation
>> according to numa node */
>> +};
>> +
>> +enum xsc_pci_state {
>> + XSC_PCI_STATE_DISABLED,
>> + XSC_PCI_STATE_ENABLED,
>> +};
>> +
>> +struct xsc_priv {
>> + char name[XSC_MAX_NAME_LEN];
>> + struct list_head dev_list;
>> + struct list_head ctx_list;
>> + spinlock_t ctx_lock; /* protect ctx_list */
>> + int numa_node;
>> +};
>> +
>> +struct xsc_core_device {
>> + struct pci_dev *pdev;
>> + struct device *device;
>> + struct xsc_priv priv;
>> + struct xsc_dev_resource *dev_res;
>> +
>> + void __iomem *bar;
>> + int bar_num;
>> +
>> + struct mutex pci_state_mutex; /* protect pci_state */
>> + enum xsc_pci_state pci_state;
>> + struct mutex intf_state_mutex; /* protect intf_state */
>> + unsigned long intf_state;
>> +};
>> +
>> +#endif
>> diff --git a/drivers/net/ethernet/yunsilicon/xsc/net/Kconfig
>> b/drivers/net/ethernet/yunsilicon/xsc/net/Kconfig
>> new file mode 100644
>> index 000000000..0d9a4ff8a
>> --- /dev/null
>> +++ b/drivers/net/ethernet/yunsilicon/xsc/net/Kconfig
>> @@ -0,0 +1,16 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
>> +# All rights reserved.
>> +# Yunsilicon driver configuration
>> +#
>> +
>> +config YUNSILICON_XSC_ETH
>> + tristate "Yunsilicon XSC ethernet driver"
>> + default n
>> + depends on YUNSILICON_XSC_PCI
>> + help
>> + This driver provides ethernet support for
>> + Yunsilicon XSC devices.
>> +
>> + To compile this driver as a module, choose M here. The module
>> + will be called xsc_eth.
>> diff --git a/drivers/net/ethernet/yunsilicon/xsc/net/Makefile
>> b/drivers/net/ethernet/yunsilicon/xsc/net/Makefile
>> new file mode 100644
>> index 000000000..2811433af
>> --- /dev/null
>> +++ b/drivers/net/ethernet/yunsilicon/xsc/net/Makefile
>> @@ -0,0 +1,9 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
>> +# All rights reserved.
>> +
>> +ccflags-y += -I$(srctree)/drivers/net/ethernet/yunsilicon/xsc
>> +
>> +obj-$(CONFIG_YUNSILICON_XSC_ETH) += xsc_eth.o
>> +
>> +xsc_eth-y := main.o
>> \ No newline at end of file
>> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/Kconfig
>> b/drivers/net/ethernet/yunsilicon/xsc/pci/Kconfig
>> new file mode 100644
>> index 000000000..2b6d79905
>> --- /dev/null
>> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/Kconfig
>> @@ -0,0 +1,16 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
>> +# All rights reserved.
>> +# Yunsilicon PCI configuration
>> +#
>> +
>> +config YUNSILICON_XSC_PCI
>> + tristate "Yunsilicon XSC PCI driver"
>> + default n
>> + select PAGE_POOL
>> + help
>> + This driver is common for Yunsilicon XSC
>> + ethernet and RDMA drivers.
>> +
>> + To compile this driver as a module, choose M here. The module
>> + will be called xsc_pci.
>> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile
>> b/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile
>> new file mode 100644
>> index 000000000..709270df8
>> --- /dev/null
>> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/Makefile
>> @@ -0,0 +1,9 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
>> +# All rights reserved.
>> +
>> +ccflags-y += -I$(srctree)/drivers/net/ethernet/yunsilicon/xsc
>> +
>> +obj-$(CONFIG_YUNSILICON_XSC_PCI) += xsc_pci.o
>> +
>> +xsc_pci-y := main.o
>> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/main.c
>> b/drivers/net/ethernet/yunsilicon/xsc/pci/main.c
>> new file mode 100644
>> index 000000000..cbe0bfbd1
>> --- /dev/null
>> +++ b/drivers/net/ethernet/yunsilicon/xsc/pci/main.c
>> @@ -0,0 +1,272 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (C) 2021-2025, Shanghai Yunsilicon Technology Co., Ltd.
>> + * All rights reserved.
>> + */
>> +
>> +#include "common/xsc_core.h"
>> +
>> +unsigned int xsc_log_level = XSC_LOG_LEVEL_WARN;
>> +module_param_named(log_level, xsc_log_level, uint, 0644);
>> +MODULE_PARM_DESC(log_level,
>> + "lowest log level to print: 0=debug, 1=info, 2=warning,
>> 3=error. Default=1");
>> +EXPORT_SYMBOL(xsc_log_level);
>> +
>> +#define XSC_PCI_DRV_DESC "Yunsilicon Xsc PCI driver"
>
> remove the define and just use the string inplace as desription
OK， will modify.
>
>> +
>> +static const struct pci_device_id xsc_pci_id_table[] = {
>> + { PCI_DEVICE(XSC_PCI_VENDOR_ID, XSC_MC_PF_DEV_ID) },
>> + { PCI_DEVICE(XSC_PCI_VENDOR_ID, XSC_MC_PF_DEV_ID_DIAMOND) },
>> + { PCI_DEVICE(XSC_PCI_VENDOR_ID, XSC_MF_HOST_PF_DEV_ID) },
>> + { PCI_DEVICE(XSC_PCI_VENDOR_ID, XSC_MF_SOC_PF_DEV_ID) },
>> + { PCI_DEVICE(XSC_PCI_VENDOR_ID, XSC_MS_PF_DEV_ID) },
>> + { PCI_DEVICE(XSC_PCI_VENDOR_ID, XSC_MV_HOST_PF_DEV_ID) },
>> + { PCI_DEVICE(XSC_PCI_VENDOR_ID, XSC_MV_SOC_PF_DEV_ID) },
>> + { 0 }
>> +};
>> +
>> +static int set_dma_caps(struct pci_dev *pdev)
>> +{
>> + int err;
>> +
>> + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
>> + if (err)
>> + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> + else
>> + err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
>> +
>> + if (!err)
>> + dma_set_max_seg_size(&pdev->dev, SZ_2G);
>> +
>> + return err;
>> +}
>> +
>> +static int xsc_pci_enable_device(struct xsc_core_device *xdev)
>> +{
>> + struct pci_dev *pdev = xdev->pdev;
>> + int err = 0;
>> +
>> + mutex_lock(&xdev->pci_state_mutex);
>> + if (xdev->pci_state == XSC_PCI_STATE_DISABLED) {
>> + err = pci_enable_device(pdev);
>> + if (!err)
>> + xdev->pci_state = XSC_PCI_STATE_ENABLED;
>> + }
>> + mutex_unlock(&xdev->pci_state_mutex);
>> +
>> + return err;
>> +}
>> +
>> +static void xsc_pci_disable_device(struct xsc_core_device *xdev)
>> +{
>> + struct pci_dev *pdev = xdev->pdev;
>> +
>> + mutex_lock(&xdev->pci_state_mutex);
>> + if (xdev->pci_state == XSC_PCI_STATE_ENABLED) {
>> + pci_disable_device(pdev);
>> + xdev->pci_state = XSC_PCI_STATE_DISABLED;
>> + }
>> + mutex_unlock(&xdev->pci_state_mutex);
>> +}
>> +
>> +static int xsc_pci_init(struct xsc_core_device *xdev, const struct
>> pci_device_id *id)
>> +{
>> + struct pci_dev *pdev = xdev->pdev;
>> + void __iomem *bar_base;
>> + int bar_num = 0;
>> + int err;
>> +
>> + mutex_init(&xdev->pci_state_mutex);
>> + xdev->priv.numa_node = dev_to_node(&pdev->dev);
>> +
>> + err = xsc_pci_enable_device(xdev);
>> + if (err) {
>> + xsc_core_err(xdev, "failed to enable PCI device: err=%d\n",
>> err);
>> + goto err_ret;
>> + }
>> +
>> + err = pci_request_region(pdev, bar_num, KBUILD_MODNAME);
>> + if (err) {
>> + xsc_core_err(xdev, "failed to request %s pci_region=%d:
>> err=%d\n",
>> + KBUILD_MODNAME, bar_num, err);
>> + goto err_disable;
>> + }
>> +
>> + pci_set_master(pdev);
>> +
>> + err = set_dma_caps(pdev);
>> + if (err) {
>> + xsc_core_err(xdev, "failed to set DMA capabilities mask:
>> err=%d\n", err);
>> + goto err_clr_master;
>> + }
>> +
>> + bar_base = pci_ioremap_bar(pdev, bar_num);
>> + if (!bar_base) {
>> + xsc_core_err(xdev, "failed to ioremap %s bar%d\n",
>> KBUILD_MODNAME, bar_num);
>> + goto err_clr_master;
>> + }
>> +
>> + err = pci_save_state(pdev);
>> + if (err) {
>> + xsc_core_err(xdev, "pci_save_state failed: err=%d\n", err);
>> + goto err_io_unmap;
>> + }
>> +
>> + xdev->bar_num = bar_num;
>> + xdev->bar = bar_base;
>> +
>> + return 0;
>> +
>> +err_io_unmap:
>> + pci_iounmap(pdev, bar_base);
>> +err_clr_master:
>> + pci_clear_master(pdev);
>> + pci_release_region(pdev, bar_num);
>> +err_disable:
>> + xsc_pci_disable_device(xdev);
>> +err_ret:
>> + return err;
>> +}
>> +
>> +static void xsc_pci_fini(struct xsc_core_device *xdev)
>> +{
>> + struct pci_dev *pdev = xdev->pdev;
>> +
>> + if (xdev->bar)
>> + pci_iounmap(pdev, xdev->bar);
>> + pci_clear_master(pdev);
>> + pci_release_region(pdev, xdev->bar_num);
>> + xsc_pci_disable_device(xdev);
>> +}
>> +
>> +static int xsc_priv_init(struct xsc_core_device *xdev)
>> +{
>> + struct xsc_priv *priv = &xdev->priv;
>> +
>> + strscpy(priv->name, dev_name(&xdev->pdev->dev), XSC_MAX_NAME_LEN);
>> +
>> + INIT_LIST_HEAD(&priv->ctx_list);
>> + spin_lock_init(&priv->ctx_lock);
>> + mutex_init(&xdev->intf_state_mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int xsc_dev_res_init(struct xsc_core_device *xdev)
>> +{
>> + struct xsc_dev_resource *dev_res;
>> +
>> + dev_res = kvzalloc(sizeof(*dev_res), GFP_KERNEL);
>> + if (!dev_res)
>> + return -ENOMEM;
>> +
>> + xdev->dev_res = dev_res;
>> + mutex_init(&dev_res->alloc_mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static void xsc_dev_res_cleanup(struct xsc_core_device *xdev)
>> +{
>> + kfree(xdev->dev_res);
>> +}
>> +
>> +static int xsc_core_dev_init(struct xsc_core_device *xdev)
>> +{
>> + int err;
>> +
>> + xsc_priv_init(xdev);
>> +
>> + err = xsc_dev_res_init(xdev);
>> + if (err) {
>> + xsc_core_err(xdev, "xsc dev res init failed %d\n", err);
>> + goto out;
>
> return err...
Thanks for the feedback. The |return err;| is retained to accommodate
additional error handling logic in subsequent patches. After those
patches are added, this structure will look OK.
>
>> + }
>> +
>> + return 0;
>> +out:
>> + return err;
>
> ...so you could remove last two lines
>
>> +}
>> +
>> +static void xsc_core_dev_cleanup(struct xsc_core_device *xdev)
>> +{
>> + xsc_dev_res_cleanup(xdev);
>> +}
>> +
>> +static int xsc_pci_probe(struct pci_dev *pci_dev,
>> + const struct pci_device_id *id)
>> +{
>> + struct xsc_core_device *xdev;
>> + int err;
>> +
>> + xdev = kzalloc(sizeof(*xdev), GFP_KERNEL);
>> + if (!xdev)
>> + return -ENOMEM;
>> +
>> + xdev->pdev = pci_dev;
>> + xdev->device = &pci_dev->dev;
>> +
>> + pci_set_drvdata(pci_dev, xdev);
>> + err = xsc_pci_init(xdev, id);
>> + if (err) {
>> + xsc_core_err(xdev, "xsc_pci_init failed %d\n", err);
>> + goto err_unset_pci_drvdata;
>> + }
>> +
>> + err = xsc_core_dev_init(xdev);
>> + if (err) {
>> + xsc_core_err(xdev, "xsc_core_dev_init failed %d\n", err);
>> + goto err_pci_fini;
>> + }
>> +
>> + return 0;
>> +err_pci_fini:
>> + xsc_pci_fini(xdev);
>> +err_unset_pci_drvdata:
>> + pci_set_drvdata(pci_dev, NULL);
>> + kfree(xdev);
>> +
>> + return err;
>> +}
>> +
>> +static void xsc_pci_remove(struct pci_dev *pci_dev)
>> +{
>> + struct xsc_core_device *xdev = pci_get_drvdata(pci_dev);
>> +
>> + xsc_core_dev_cleanup(xdev);
>> + xsc_pci_fini(xdev);
>> + pci_set_drvdata(pci_dev, NULL);
>> + kfree(xdev);
>> +}
>> +
>> +static struct pci_driver xsc_pci_driver = {
>> + .name = "xsc-pci",
>> + .id_table = xsc_pci_id_table,
>> + .probe = xsc_pci_probe,
>> + .remove = xsc_pci_remove,
>> +};
>> +
>> +static int __init xsc_init(void)
>> +{
>> + int err;
>> +
>> + err = pci_register_driver(&xsc_pci_driver);
>> + if (err) {
>> + pr_err("failed to register pci driver\n");
>> + goto out;
>
> ditto plain return
>
>> + }
>> + return 0;
>> +
>> +out:
>> + return err;
>> +}
>> +
>> +static void __exit xsc_fini(void)
>> +{
>> + pci_unregister_driver(&xsc_pci_driver);
>> +}
>> +
>> +module_init(xsc_init);
>> +module_exit(xsc_fini);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION(XSC_PCI_DRV_DESC);
>
Thank you，Przemek，for your thoughtful review and patient explanations.
Your clarification of some important rules is really valuable for
someone like me who is new to this. I will make sure to follow these
guidelines in my future changes. I also appreciate all the feedback
you’ve provided earlier.
Best regards,
Xin Tian
Powered by blists - more mailing lists