From: Greg KH <gregkh@linuxfoundation.org>
To: Gage Eads <gage.eads@intel.com>
Cc: linux-kernel@vger.kernel.org, arnd@arndb.de,
magnus.karlsson@intel.com, bjorn.topel@intel.com
Subject: Re: [PATCH v2 01/19] dlb2: add skeleton for DLB 2.0 driver
Date: Tue, 11 Aug 2020 19:05:09 +0200 [thread overview]
Message-ID: <20200811170509.GA765993@kroah.com> (raw)
In-Reply-To: <20200811162732.1369-2-gage.eads@intel.com>
On Tue, Aug 11, 2020 at 11:27:14AM -0500, Gage Eads wrote:
> diff --git a/drivers/misc/dlb2/dlb2_main.c b/drivers/misc/dlb2/dlb2_main.c
> new file mode 100644
> index 000000000000..ffd6df788e2e
> --- /dev/null
> +++ b/drivers/misc/dlb2/dlb2_main.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2018-2020 Intel Corporation */
> +
> +#include <linux/aer.h>
> +#include <linux/cdev.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/uaccess.h>
> +
> +#include "dlb2_main.h"
> +
> +static const char
> +dlb2_driver_copyright[] = "Copyright(c) 2018-2020 Intel Corporation";
Why do you have this static string that you never use?
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Copyright(c) 2018-2020 Intel Corporation");
> +MODULE_DESCRIPTION("Intel(R) Dynamic Load Balancer 2.0 Driver");
> +
> +/* The driver lock protects data structures that used by multiple devices. */
> +static DEFINE_MUTEX(dlb2_driver_lock);
> +static struct list_head dlb2_dev_list = LIST_HEAD_INIT(dlb2_dev_list);
A static list of devices that you constantly walk? Why? This isn't the
1990's anymore, that should not be needed anywhere at all.
> +
> +static struct class *dlb2_class;
> +static dev_t dlb2_dev_number_base;
static dev_t my_major; perhaps?
Why have "dlb2_" as a prefix for static variables?
> +
> +/*****************************/
> +/****** Devfs callbacks ******/
there is nothing in the kernel called "devfs", and has not been for 16+
years I think. Where did you get that term from???
> +/*****************************/
> +
> +static int dlb2_open(struct inode *i, struct file *f)
> +{
> + return 0;
> +}
> +
> +static int dlb2_close(struct inode *i, struct file *f)
> +{
> + return 0;
If you do not need an open/close function, do not include it.
> +}
> +
> +static const struct file_operations dlb2_fops = {
> + .owner = THIS_MODULE,
> + .open = dlb2_open,
> + .release = dlb2_close,
> +};
You never use this structure in this patch :(
> +
> +/**********************************/
> +/****** PCI driver callbacks ******/
> +/**********************************/
> +
> +static DEFINE_IDA(dlb2_ids);
This is not a pci driver callback :)
Why have comments if they instantly are wrong? :(
> +
> +static int dlb2_alloc_id(void)
> +{
> + return ida_alloc_max(&dlb2_ids, DLB2_MAX_NUM_DEVICES - 1, GFP_KERNEL);
> +}
> +
> +static void dlb2_free_id(int id)
> +{
> + ida_free(&dlb2_ids, id);
No locking needed for accessing the ida?
> +}
> +
> +static int dlb2_probe(struct pci_dev *pdev,
> + const struct pci_device_id *pdev_id)
> +{
> + struct dlb2_dev *dlb2_dev;
> + int ret;
> +
> + dlb2_dev = devm_kzalloc(&pdev->dev, sizeof(*dlb2_dev), GFP_KERNEL);
Why are you calling devm_kzalloc() if you manually call devm_kfree() for
this chunk of memory? Either rely on the devm api or don't use it at
all.
> + if (!dlb2_dev)
> + return -ENOMEM;
> +
> + pci_set_drvdata(pdev, dlb2_dev);
> +
> + dlb2_dev->pdev = pdev;
> +
> + dlb2_dev->id = dlb2_alloc_id();
> + if (dlb2_dev->id < 0) {
> + dev_err(&pdev->dev, "probe: device ID allocation failed\n");
> +
> + ret = dlb2_dev->id;
> + goto alloc_id_fail;
> + }
> +
> + ret = pci_enable_device(pdev);
> + if (ret != 0) {
> + dev_err(&pdev->dev, "pci_enable_device() returned %d\n", ret);
> +
> + goto pci_enable_device_fail;
> + }
> +
> + ret = pci_request_regions(pdev, dlb2_driver_name);
> + if (ret != 0) {
> + dev_err(&pdev->dev,
> + "pci_request_regions(): returned %d\n", ret);
> +
> + goto pci_request_regions_fail;
> + }
> +
> + pci_set_master(pdev);
> +
> + if (pci_enable_pcie_error_reporting(pdev))
> + dev_info(&pdev->dev, "[%s()] Failed to enable AER\n", __func__);
> +
> + mutex_lock(&dlb2_driver_lock);
> + list_add(&dlb2_dev->list, &dlb2_dev_list);
> + mutex_unlock(&dlb2_driver_lock);
> +
> + return 0;
> +
> +pci_request_regions_fail:
> + pci_disable_device(pdev);
> +pci_enable_device_fail:
> + dlb2_free_id(dlb2_dev->id);
> +alloc_id_fail:
> + devm_kfree(&pdev->dev, dlb2_dev);
> + return ret;
> +}
> +
> +static void dlb2_remove(struct pci_dev *pdev)
> +{
> + struct dlb2_dev *dlb2_dev;
> +
> + /* Undo all the dlb2_probe() operations */
> + dlb2_dev = pci_get_drvdata(pdev);
> +
> + mutex_lock(&dlb2_driver_lock);
> + list_del(&dlb2_dev->list);
> + mutex_unlock(&dlb2_driver_lock);
> +
> + pci_disable_pcie_error_reporting(pdev);
> +
> + pci_release_regions(pdev);
> +
> + pci_disable_device(pdev);
> +
> + dlb2_free_id(dlb2_dev->id);
> +
> + devm_kfree(&pdev->dev, dlb2_dev);
> +}
> +
> +static struct pci_device_id dlb2_id_table[] = {
> + { PCI_DEVICE_DATA(INTEL, DLB2_PF, NULL) },
> + { 0 }
> +};
> +MODULE_DEVICE_TABLE(pci, dlb2_id_table);
> +
> +static struct pci_driver dlb2_pci_driver = {
> + .name = (char *)dlb2_driver_name,
Why is this cast needed? That's a hint that something is wrong...
> + .id_table = dlb2_id_table,
> + .probe = dlb2_probe,
> + .remove = dlb2_remove,
> +};
> +
> +static int __init dlb2_init_module(void)
> +{
> + int err;
> +
> + dlb2_class = class_create(THIS_MODULE, dlb2_driver_name);
> +
> + if (IS_ERR(dlb2_class)) {
blank line was not needed.
> + pr_err("%s: class_create() returned %ld\n",
> + dlb2_driver_name, PTR_ERR(dlb2_class));
> +
> + return PTR_ERR(dlb2_class);
> + }
> +
> + /* Allocate one minor number per domain */
> + err = alloc_chrdev_region(&dlb2_dev_number_base,
> + 0,
> + DLB2_MAX_NUM_DEVICES,
> + dlb2_driver_name);
> +
> + if (err < 0) {
> + pr_err("%s: alloc_chrdev_region() returned %d\n",
> + dlb2_driver_name, err);
> +
> + return err;
> + }
> +
> + err = pci_register_driver(&dlb2_pci_driver);
> + if (err < 0) {
> + pr_err("%s: pci_register_driver() returned %d\n",
> + dlb2_driver_name, err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static void __exit dlb2_exit_module(void)
> +{
> + pci_unregister_driver(&dlb2_pci_driver);
> +
> + unregister_chrdev_region(dlb2_dev_number_base,
> + DLB2_MAX_NUM_DEVICES);
> +
> + if (dlb2_class) {
> + class_destroy(dlb2_class);
> + dlb2_class = NULL;
Why set this?
> + }
No freeing of allocated ida structures? Or is that not needed anymore,
I can never remember...
> +}
> +
> +module_init(dlb2_init_module);
> +module_exit(dlb2_exit_module);
> diff --git a/drivers/misc/dlb2/dlb2_main.h b/drivers/misc/dlb2/dlb2_main.h
> new file mode 100644
> index 000000000000..cc05546fba13
> --- /dev/null
> +++ b/drivers/misc/dlb2/dlb2_main.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-only
> + * Copyright(c) 2017-2020 Intel Corporation
> + */
> +
> +#ifndef __DLB2_MAIN_H
> +#define __DLB2_MAIN_H
> +
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/ktime.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/types.h>
> +
> +#include "dlb2_hw_types.h"
> +
> +static const char dlb2_driver_name[] = KBUILD_MODNAME;
> +
> +/*
> + * The dlb2 driver uses a different minor number for each device file, of which
> + * there are:
> + * - 33 per device (PF or VF/VDEV): 1 for the device, 32 for scheduling domains
> + * - Up to 17 devices per PF: 1 PF and up to 16 VFs/VDEVs
> + * - Up to 16 PFs per system
> + */
> +#define DLB2_MAX_NUM_PFS 16
> +#define DLB2_NUM_FUNCS_PER_DEVICE (1 + DLB2_MAX_NUM_VDEVS)
> +#define DLB2_MAX_NUM_DEVICES (DLB2_MAX_NUM_PFS * DLB2_NUM_FUNCS_PER_DEVICE)
> +
> +struct dlb2_dev {
> + struct pci_dev *pdev;
> + struct list_head list;
Why is list needed?
> + int id;
u32? u64? u8? Pick something :)
> +};
> +
> +#endif /* __DLB2_MAIN_H */
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 5c709a1450b1..eb865b4eb900 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2809,6 +2809,8 @@
> #define PCI_DEVICE_ID_INTEL_ESB2_14 0x2698
> #define PCI_DEVICE_ID_INTEL_ESB2_17 0x269b
> #define PCI_DEVICE_ID_INTEL_ESB2_18 0x269e
> +#define PCI_DEVICE_ID_INTEL_DLB2_PF 0x2710
> +#define PCI_DEVICE_ID_INTEL_DLB2_VF 0x2711
Did you read the top of this file? How does this patch justify the
request there?
{sigh}
I'm not reviewing beyond this patch, sorry.
greg k-h
next prev parent reply other threads:[~2020-08-11 17:05 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-11 16:27 [PATCH v2 00/19] dlb2: introduce DLB 2.0 device driver Gage Eads
2020-08-11 16:27 ` [PATCH v2 01/19] dlb2: add skeleton for DLB 2.0 driver Gage Eads
2020-08-11 17:05 ` Greg KH [this message]
2020-08-11 16:27 ` [PATCH v2 02/19] dlb2: initialize PF device Gage Eads
2020-08-11 17:06 ` Greg KH
2020-08-24 16:50 ` Eads, Gage
2020-08-11 16:27 ` [PATCH v2 03/19] dlb2: add resource and device initialization Gage Eads
2020-08-11 16:27 ` [PATCH v2 04/19] dlb2: add device ioctl layer and first three ioctls Gage Eads
2020-08-11 16:27 ` [PATCH v2 05/19] dlb2: add sched domain config and reset support Gage Eads
2020-08-11 16:27 ` [PATCH v2 06/19] dlb2: add runtime power-management support Gage Eads
2020-08-11 16:27 ` [PATCH v2 07/19] dlb2: add queue create and queue-depth-get ioctls Gage Eads
2020-08-11 16:27 ` [PATCH v2 08/19] dlb2: add ioctl to configure ports, query poll mode Gage Eads
2020-08-11 16:27 ` [PATCH v2 09/19] dlb2: add port mmap support Gage Eads
2020-08-11 16:27 ` [PATCH v2 10/19] dlb2: add start domain ioctl Gage Eads
2020-08-11 16:27 ` [PATCH v2 11/19] dlb2: add queue map and unmap ioctls Gage Eads
2020-08-11 16:27 ` [PATCH v2 12/19] dlb2: add port enable/disable ioctls Gage Eads
2020-08-11 16:27 ` [PATCH v2 13/19] dlb2: add CQ interrupt support Gage Eads
2020-08-11 16:27 ` [PATCH v2 14/19] dlb2: add domain alert support Gage Eads
2020-08-11 16:27 ` [PATCH v2 15/19] dlb2: add sequence-number management ioctls Gage Eads
2020-08-11 16:27 ` [PATCH v2 16/19] dlb2: add cos bandwidth get/set ioctls Gage Eads
2020-08-11 16:27 ` [PATCH v2 17/19] dlb2: add device FLR support Gage Eads
2020-08-11 16:27 ` [PATCH v2 18/19] dlb2: add basic PF sysfs interfaces Gage Eads
2020-08-11 16:27 ` [PATCH v2 19/19] dlb2: add ingress error handling Gage Eads
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200811170509.GA765993@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=arnd@arndb.de \
--cc=bjorn.topel@intel.com \
--cc=gage.eads@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=magnus.karlsson@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.