Re: [PATCH v1 01/12] gna: add driver module
From: Andy Shevchenko
Date: Tue Feb 16 2021 - 11:56:03 EST
On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
<maciej.kwapulinski@xxxxxxxxxxxxxxx> wrote:
>
> From: Tomasz Jankowski <tomasz1.jankowski@xxxxxxxxx>
>
> Add a new PCI driver for Intel(R) Gaussian & Neural Accelerator
> with basic support like module loading and unloading. The full
> function of the driver will be added by further changes.
...
> +config INTEL_GNA
> + tristate "Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)"
Intel (R) Intel (R) RRR!
> + depends on X86_64 && PCI
> + help
> + This option enables the Intel(R) Gaussian & Neural Accelerator
> + (Intel(R) GNA) driver.
> + User space interface is defined in include/uapi/misc/gna.h, while
> + information about functionality is in
> + Documentation/misc-devices/gna.rst
No module name?
...
> +/* Reverse gna_dev_init() */
> +static void gna_dev_deinit(struct gna_private *gna_priv)
> +{
> + pci_set_drvdata(gna_priv->pdev, NULL);
> +}
This is done by device core. Why do you need it?
...
> + ret = pcim_enable_device(pcidev);
> + if (ret) {
> + dev_err(&pcidev->dev, "pci device can't be enabled\n");
> + goto end;
Useless label. Return here.
> + }
...
> + ret = pci_request_regions(pcidev, GNA_DRV_NAME);
> + if (ret)
> + goto end;
Why? Can't you use pcim_iomap_regions() directly?
...
> + ret = pci_set_dma_mask(pcidev, DMA_BIT_MASK(64));
No way. This is an obsoleted API.
> + if (ret) {
> + dev_err(&pcidev->dev, "pci_set_dma_mask returned error %d\n", ret);
> + goto err_release_regions;
> + }
...
> + /* init gna device */
Useless comments here and there.
...
> + gna_priv = devm_kzalloc(&pcidev->dev, sizeof(*gna_priv), GFP_KERNEL);
> + if (!gna_priv) {
> + ret = -ENOMEM;
> + goto err_clear_master;
What? You have used pciM_enabled_device(). Please, read documentation.
> + }
...
> + gna_priv->bar0.iostart = pci_resource_start(pcidev, 0);
> + gna_priv->bar0.iosize = pci_resource_len(pcidev, 0);
> + gna_priv->bar0.mem_addr = pcim_iomap(pcidev, 0, 0);
> + if (!gna_priv->bar0.mem_addr) {
> + dev_err(&pcidev->dev, "could not map BAR 0\n");
> + ret = -EINVAL;
> + goto err_clear_master;
> + }
Why do you need all these?!
...
> + dev_dbg(&pcidev->dev, "bar0 io start: 0x%llx\n", (unsigned long long)gna_priv->bar0.iostart);
> + dev_dbg(&pcidev->dev, "bar0 io size: %llu\n", (unsigned long long)gna_priv->bar0.iosize);
> + dev_dbg(&pcidev->dev, "bar0 memory address: %p\n", gna_priv->bar0.mem_addr);
No, please read printk-formats.rst.
...
> +err_clear_master:
> + pci_clear_master(pcidev);
> +err_release_regions:
> + pci_release_regions(pcidev);
> +end:
> + dev_err(&pcidev->dev, "gna probe failed with %d\n", ret);
> + return ret;
These are all completely redundant.
> +}
...
> +void gna_remove(struct pci_dev *pcidev)
> +{
> + struct gna_private *gna_priv;
> +
> + gna_priv = pci_get_drvdata(pcidev);
> +
> + gna_dev_deinit(gna_priv);
> +
> + pci_clear_master(pcidev);
> + pci_release_regions(pcidev);
> +}
Redundant entire function.
...
> +#include <linux/pci.h>
Haven't noticed how this header is used here.
...
> + struct device dev;
Missed linux/device.h.
...
> +static int __init gna_drv_init(void)
> +{
> + int ret;
> +
> + mutex_init(&gna_drv_priv.lock);
> +
> + gna_class = class_create(THIS_MODULE, "gna");
> + if (IS_ERR(gna_class)) {
> + pr_err("class device create failed\n");
> + return PTR_ERR(gna_class);
> + }
> + gna_class->devnode = gna_devnode;
> +
> + ret = pci_register_driver(&gna_driver);
Is it possible to decouple a PCI glue driver from the class as many
other existing examples are doing?
> + if (ret) {
> + pr_err("pci register driver failed\n");
> + class_destroy(gna_class);
> + return ret;
> + }
> +
> + return 0;
> +}
...
> +#include <linux/kernel.h>
Why do you need this header?
...
> +#define GNA_DRV_VER "1.2.0"
Nowadays the version is the Git SHA sum.
...
> + struct file *fd;
Missed forward declaration.
...
> + struct list_head memory_list;
Missed linux/list.h
> + struct list_head flist;
...
> +extern struct gna_driver_private gna_drv_priv;
> +extern int recovery_timeout;
Global?!
...
> +#define GNA_STS_SCORE_COMPLETED (1 << 0)
> +#define GNA_STS_STATISTICS_VALID (1 << 3)
> +#define GNA_STS_PCI_MMU_ERR (1 << 4)
> +#define GNA_STS_PCI_DMA_ERR (1 << 5)
> +#define GNA_STS_PCI_UNEXCOMPL_ERR (1 << 6)
> +#define GNA_STS_VA_OOR (1 << 7)
> +#define GNA_STS_PARAM_OOR (1 << 8)
> +#define GNA_STS_OUTBUF_FULL (1 << 16)
> +#define GNA_STS_SATURATE (1 << 17)
You can use _BITUL() from const.h, but it's up to you.
...
> +#define GNA_ERROR (GNA_STS_PCI_DMA_ERR | \
When definitions start on the next line it will be easier to read.
> + GNA_STS_PCI_MMU_ERR | \
> + GNA_STS_PCI_UNEXCOMPL_ERR | \
> + GNA_STS_PARAM_OOR | \
> + GNA_STS_VA_OOR)
--
With Best Regards,
Andy Shevchenko