Re: [PATCH v4 02/16] misc: bcm-vk: add Broadcom VK driver

[Scott Branden]

Hi Greg,

On 2020-09-29 3:28 a.m., Greg Kroah-Hartman wrote:
> 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 :)
We've been meaning to publish the userspace code.
Here is the vk interface library, vkil, and associated unittest and
card utilities.

https://github.com/Broadcom/vkil/
>
>
>> 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 :(
Yes, missing kfree(vk).
>
> 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.
Will change to dev_warn when number allocated != number requested
and dev_dbg otherwise.
>
>> +
>> +	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 :(
Yes, will kfree.
>
> thanks,
>
> greg k-h

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature