Re: [PATCH v4 02/16] misc: bcm-vk: add Broadcom VK driver
From: Greg Kroah-Hartman
Date: Tue Sep 29 2020 - 06:28:18 EST
On Mon, Sep 28, 2020 at 05:11:55PM -0700, Scott Branden wrote:
> Add initial version of Broadcom VK driver to enumerate PCI device IDs
> of Valkyrie and Viper device IDs.
>
> VK based cards provide real-time high performance, high throughput,
> low latency offload compute engine operations.
> They are used for multiple parallel offload tasks as:
> audio, video and image processing and crypto operations.
As we have had this come up for other offload engine devices, we need a
pointer to the userspace code for this as well, so that we can properly
review the whole thing together. Otherwise this is just one half, the
other is a black-box that is pretty impossible to understand :)
>
> Further commits add additional features to driver beyond probe/remove.
>
> Signed-off-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
> ---
> drivers/misc/Kconfig | 1 +
> drivers/misc/Makefile | 1 +
> drivers/misc/bcm-vk/Kconfig | 15 ++++
> drivers/misc/bcm-vk/Makefile | 8 ++
> drivers/misc/bcm-vk/bcm_vk.h | 29 +++++++
> drivers/misc/bcm-vk/bcm_vk_dev.c | 137 +++++++++++++++++++++++++++++++
> 6 files changed, 191 insertions(+)
> create mode 100644 drivers/misc/bcm-vk/Kconfig
> create mode 100644 drivers/misc/bcm-vk/Makefile
> create mode 100644 drivers/misc/bcm-vk/bcm_vk.h
> create mode 100644 drivers/misc/bcm-vk/bcm_vk_dev.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index ce136d685d14..9d42b5def81b 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -469,6 +469,7 @@ source "drivers/misc/genwqe/Kconfig"
> source "drivers/misc/echo/Kconfig"
> source "drivers/misc/cxl/Kconfig"
> source "drivers/misc/ocxl/Kconfig"
> +source "drivers/misc/bcm-vk/Kconfig"
> source "drivers/misc/cardreader/Kconfig"
> source "drivers/misc/habanalabs/Kconfig"
> source "drivers/misc/uacce/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index c7bd01ac6291..766837e4b961 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_ECHO) += echo/
> obj-$(CONFIG_CXL_BASE) += cxl/
> obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o
> obj-$(CONFIG_OCXL) += ocxl/
> +obj-$(CONFIG_BCM_VK) += bcm-vk/
> obj-y += cardreader/
> obj-$(CONFIG_PVPANIC) += pvpanic.o
> obj-$(CONFIG_HABANA_AI) += habanalabs/
> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
> new file mode 100644
> index 000000000000..2272e47655ed
> --- /dev/null
> +++ b/drivers/misc/bcm-vk/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Broadcom VK device
> +#
> +config BCM_VK
> + tristate "Support for Broadcom VK Accelerators"
> + depends on PCI_MSI
> + help
> + Select this option to enable support for Broadcom
> + VK Accelerators. VK is used for performing
> + specific offload processing.
> + This driver enables userspace programs to access these
> + accelerators via /dev/bcm-vk.N devices.
> +
> + If unsure, say N.
> diff --git a/drivers/misc/bcm-vk/Makefile b/drivers/misc/bcm-vk/Makefile
> new file mode 100644
> index 000000000000..f8a7ac4c242f
> --- /dev/null
> +++ b/drivers/misc/bcm-vk/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for Broadcom VK driver
> +#
> +
> +obj-$(CONFIG_BCM_VK) += bcm_vk.o
> +bcm_vk-objs := \
> + bcm_vk_dev.o
> diff --git a/drivers/misc/bcm-vk/bcm_vk.h b/drivers/misc/bcm-vk/bcm_vk.h
> new file mode 100644
> index 000000000000..9152785199ab
> --- /dev/null
> +++ b/drivers/misc/bcm-vk/bcm_vk.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2018-2020 Broadcom.
> + */
> +
> +#ifndef BCM_VK_H
> +#define BCM_VK_H
> +
> +#include <linux/pci.h>
> +
> +#define DRV_MODULE_NAME "bcm-vk"
> +
> +/* VK device supports a maximum of 3 bars */
> +#define MAX_BAR 3
> +
> +enum pci_barno {
> + BAR_0 = 0,
> + BAR_1,
> + BAR_2
> +};
> +
> +#define BCM_VK_NUM_TTY 2
> +
> +struct bcm_vk {
> + struct pci_dev *pdev;
> + void __iomem *bar[MAX_BAR];
> +};
> +
> +#endif
> diff --git a/drivers/misc/bcm-vk/bcm_vk_dev.c b/drivers/misc/bcm-vk/bcm_vk_dev.c
> new file mode 100644
> index 000000000000..bb24efb1b9fb
> --- /dev/null
> +++ b/drivers/misc/bcm-vk/bcm_vk_dev.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018-2020 Broadcom.
> + */
> +
> +#include <linux/dma-mapping.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/pci_regs.h>
> +
> +#include "bcm_vk.h"
> +
> +#define PCI_DEVICE_ID_VALKYRIE 0x5e87
> +#define PCI_DEVICE_ID_VIPER 0x5e88
> +
> +/* MSIX usages */
> +#define VK_MSIX_MSGQ_MAX 3
> +#define VK_MSIX_NOTF_MAX 1
> +#define VK_MSIX_TTY_MAX BCM_VK_NUM_TTY
> +#define VK_MSIX_IRQ_MAX (VK_MSIX_MSGQ_MAX + VK_MSIX_NOTF_MAX + \
> + VK_MSIX_TTY_MAX)
> +#define VK_MSIX_IRQ_MIN_REQ (VK_MSIX_MSGQ_MAX + VK_MSIX_NOTF_MAX)
> +
> +/* Number of bits set in DMA mask*/
> +#define BCM_VK_DMA_BITS 64
> +
> +static int bcm_vk_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> + int err;
> + int i;
> + int irq;
> + struct bcm_vk *vk;
> + struct device *dev = &pdev->dev;
> +
> + vk = kzalloc(sizeof(*vk), GFP_KERNEL);
> + if (!vk)
> + return -ENOMEM;
> +
> + err = pci_enable_device(pdev);
> + if (err) {
> + dev_err(dev, "Cannot enable PCI device\n");
> + return err;
You just leaked memory :(
That was a short review, I didn't get all that far...
> + }
> + vk->pdev = pci_dev_get(pdev);
> +
> + err = pci_request_regions(pdev, DRV_MODULE_NAME);
> + if (err) {
> + dev_err(dev, "Cannot obtain PCI resources\n");
> + goto err_disable_pdev;
> + }
> +
> + /* make sure DMA is good */
> + err = dma_set_mask_and_coherent(&pdev->dev,
> + DMA_BIT_MASK(BCM_VK_DMA_BITS));
> + if (err) {
> + dev_err(dev, "failed to set DMA mask\n");
> + goto err_disable_pdev;
> + }
> +
> + pci_set_master(pdev);
> + pci_set_drvdata(pdev, vk);
> +
> + irq = pci_alloc_irq_vectors(pdev,
> + 1,
> + VK_MSIX_IRQ_MAX,
> + PCI_IRQ_MSI | PCI_IRQ_MSIX);
> +
> + if (irq < VK_MSIX_IRQ_MIN_REQ) {
> + dev_err(dev, "failed to get min %d MSIX interrupts, irq(%d)\n",
> + VK_MSIX_IRQ_MIN_REQ, irq);
> + err = (irq >= 0) ? -EINVAL : irq;
> + goto err_disable_pdev;
> + }
> +
> + dev_info(dev, "Number of IRQs %d allocated - requested(%d).\n",
> + irq, VK_MSIX_IRQ_MAX);
Drivers are quiet when all works properly.
> +
> + for (i = 0; i < MAX_BAR; i++) {
> + /* multiple by 2 for 64 bit BAR mapping */
> + vk->bar[i] = pci_ioremap_bar(pdev, i * 2);
> + if (!vk->bar[i]) {
> + dev_err(dev, "failed to remap BAR%d\n", i);
> + goto err_iounmap;
> + }
> + }
> +
> + return 0;
> +
> +err_iounmap:
> + for (i = 0; i < MAX_BAR; i++) {
> + if (vk->bar[i])
> + pci_iounmap(pdev, vk->bar[i]);
> + }
> + pci_release_regions(pdev);
> +
> +err_disable_pdev:
> + pci_free_irq_vectors(pdev);
> + pci_disable_device(pdev);
> + pci_dev_put(pdev);
> +
> + return err;
Again, memory is leaked :(
thanks,
greg k-h