Re: [PATCH v2 01/19] dlb2: add skeleton for DLB 2.0 driver

[Greg KH]

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