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
