Re: [PATCH 01/15] habanalabs: add skeleton driver

Mike Rapoport
2019-01-23T12:28:21Z

On Wed, Jan 23, 2019 at 02:00:43AM +0200, Oded Gabbay wrote:
> This patch adds the habanalabs skeleton driver. The driver does nothing at
> this stage except very basic operations. It contains the minimal code to
> insmod and rmmod the driver and to create a /dev/hlX file per PCI device.
>
> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
> ---
> drivers/misc/Kconfig | 1 +
> drivers/misc/Makefile | 1 +
> drivers/misc/habanalabs/Kconfig | 22 ++
> drivers/misc/habanalabs/Makefile | 7 +
> drivers/misc/habanalabs/device.c | 331 ++++++++++++++++
> drivers/misc/habanalabs/habanalabs.h | 149 +++++++
> drivers/misc/habanalabs/habanalabs_drv.c | 366 ++++++++++++++++++
> .../habanalabs/include/habanalabs_device_if.h | 125 ++++++
> 8 files changed, 1002 insertions(+)
> create mode 100644 drivers/misc/habanalabs/Kconfig
> create mode 100644 drivers/misc/habanalabs/Makefile
> create mode 100644 drivers/misc/habanalabs/device.c
> create mode 100644 drivers/misc/habanalabs/habanalabs.h
> create mode 100644 drivers/misc/habanalabs/habanalabs_drv.c
> create mode 100644 drivers/misc/habanalabs/include/habanalabs_device_if.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f417b06e11c5..fecab53c4f21 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -535,4 +535,5 @@ source "drivers/misc/echo/Kconfig"
> source "drivers/misc/cxl/Kconfig"
> source "drivers/misc/ocxl/Kconfig"
> source "drivers/misc/cardreader/Kconfig"
> +source "drivers/misc/habanalabs/Kconfig"
> endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index e39ccbbc1b3a..ae77dfd790a4 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o
> obj-$(CONFIG_OCXL) += ocxl/
> obj-y += cardreader/
> obj-$(CONFIG_PVPANIC) += pvpanic.o
> +obj-$(CONFIG_HABANA_AI) += habanalabs/
> diff --git a/drivers/misc/habanalabs/Kconfig b/drivers/misc/habanalabs/Kconfig
> new file mode 100644
> index 000000000000..b7f38a14caf5
> --- /dev/null
> +++ b/drivers/misc/habanalabs/Kconfig
> @@ -0,0 +1,22 @@
> +#
> +# HabanaLabs AI accelerators driver
> +#
> +
> +config HABANA_AI
> + tristate "HabanaAI accelerators (habanalabs)"
> + depends on PCI
> + select FRAME_VECTOR
> + help
> + Enables PCIe card driver for Habana's AI Processors (AIP) that are
> + designed to accelerate Deep Learning inference and training workloads.
> +
> + The driver manages the PCIe devices and provides IOCTL interface for
> + the user to submit workloads to the devices.
> +
> + The user-space interface is described in
> + include/uapi/misc/habanalabs.h
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called habanalabs.
> diff --git a/drivers/misc/habanalabs/Makefile b/drivers/misc/habanalabs/Makefile
> new file mode 100644
> index 000000000000..b41433a09e02
> --- /dev/null
> +++ b/drivers/misc/habanalabs/Makefile
> @@ -0,0 +1,7 @@
> +#
> +# Makefile for HabanaLabs AI accelerators driver
> +#
> +
> +obj-m := habanalabs.o
> +
> +habanalabs-y := habanalabs_drv.o device.o
> \ No newline at end of file
> diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
> new file mode 100644
> index 000000000000..376b55eb73d4
> --- /dev/null
> +++ b/drivers/misc/habanalabs/device.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright 2016-2018 HabanaLabs, Ltd.
> + * All Rights Reserved.
> + */
> +
> +#include "habanalabs.h"
> +
> +#include <linux/fs.h>
> +#include <linux/kthread.h>
> +#include <linux/sched/signal.h>
> +
> +static void hpriv_release(struct kref *ref)
> +{
> + struct hl_fpriv *hpriv;
> + struct hl_device *hdev;
> +
> + hpriv = container_of(ref, struct hl_fpriv, refcount);
> +
> + hdev = hpriv->hdev;
> +
> + put_pid(hpriv->taskpid);
> +
> + kfree(hpriv);
> +}
> +
> +void hl_hpriv_get(struct hl_fpriv *hpriv)
> +{
> + kref_get(&hpriv->refcount);
> +}
> +
> +void hl_hpriv_put(struct hl_fpriv *hpriv)
> +{
> + kref_put(&hpriv->refcount, hpriv_release);
> +}
> +
> +/**
> + * hl_device_release - release function for habanalabs device
> + *
> + * @inode: pointer to inode structure
> + * @filp: pointer to file structure
> + *
> + * Called when process closes an habanalabs device
> + */

It's nice to see docs coming along with the codei
I have some comments for the formatting.

kernel-doc won't be happy about missing return value descriptions, and
although they are sometimes redundant or too obvious their absence makes
'make V=1 htmldocs' really noisy.

In general, it would be nice if you could link hanabnalabs driver
kernel-doc somewhere in Documentation/ run 'make V=1 htmldocs'.

> +static int hl_device_release(struct inode *inode, struct file *filp)
> +{
> + struct hl_fpriv *hpriv = filp->private_data;
> +
> + filp->private_data = NULL;
> +
> + hl_hpriv_put(hpriv);
> +
> + return 0;
> +}
> +
> +static const struct file_operations hl_ops = {
> + .owner = THIS_MODULE,
> + .open = hl_device_open,
> + .release = hl_device_release
> +};
> +
> +/**
> + * device_setup_cdev - setup cdev and device for habanalabs device
> + *
> + * @hdev: pointer to habanalabs device structure
> + * @hclass: pointer to the class object of the device
> + * @minor: minor number of the specific device
> + * @fpos : file operations to install for this device
> + *
> + * Create a cdev and a Linux device for habanalabs's device. Need to be
> + * called at the end of the habanalabs device initialization process,
> + * because this function exposes the device to the user
> + */
> +static int device_setup_cdev(struct hl_device *hdev, struct class *hclass,
> + int minor, const struct file_operations *fops)
> +{
> + int err, devno = MKDEV(hdev->major, minor);
> + struct cdev *hdev_cdev = &hdev->cdev;
> + char name[8];
> +
> + sprintf(name, "hl%d", hdev->id);
> +
> + cdev_init(hdev_cdev, fops);
> + hdev_cdev->owner = THIS_MODULE;
> + err = cdev_add(hdev_cdev, devno, 1);
> + if (err) {
> + pr_err("habanalabs: Failed to add char device %s", name);
> + goto err_cdev_add;
> + }
> +
> + hdev->dev = device_create(hclass, NULL, devno, NULL, "%s", name);
> + if (IS_ERR(hdev->dev)) {
> + pr_err("habanalabs: Failed to create device %s\n", name);
> + err = PTR_ERR(hdev->dev);
> + goto err_device_create;
> + }
> +
> + dev_set_drvdata(hdev->dev, hdev);
> +
> + return 0;
> +
> +err_device_create:
> + cdev_del(hdev_cdev);
> +err_cdev_add:
> + return err;
> +}
> +
> +/**
> + * device_early_init - do some early initialization for the habanalabs device
> + *
> + * @hdev: pointer to habanalabs device structure
> + *
> + * Install the relevant function pointers and call the early_init function,
> + * if such a function exists
> + */
> +static int device_early_init(struct hl_device *hdev)
> +{
> + switch (hdev->asic_type) {
> + case ASIC_GOYA:
> + sprintf(hdev->asic_name, "GOYA");
> + break;
> + default:
> + dev_err(hdev->dev, "Unrecognized ASIC type %d\n",
> + hdev->asic_type);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * device_early_fini - finalize all that was done in device_early_fini

^init
> + *
> + * @hdev: pointer to habanalabs device structure
> + *
> + */
> +static void device_early_fini(struct hl_device *hdev)
> +{
> +}
> +
> +/**
> + * hl_device_suspend - initiate device suspend
> + *
> + * @hdev: pointer to habanalabs device structure
> + *
> + * Puts the hw in the suspend state (all asics).
> + * Returns 0 for success or an error on failure.

Should be Return: or Returns: for kernel-doc to understand it.

> + * Called at driver suspend.

This probably should be marked as Context:

> + */
> +int hl_device_suspend(struct hl_device *hdev)
> +{
> + pci_save_state(hdev->pdev);
> +
> + /* Shut down the device */
> + pci_disable_device(hdev->pdev);
> + pci_set_power_state(hdev->pdev, PCI_D3hot);
> +
> + return 0;
> +}
> +
> +/**
> + * hl_device_resume - initiate device resume
> + *
> + * @hdev: pointer to habanalabs device structure
> + *
> + * Bring the hw back to operating state (all asics).
> + * Returns 0 for success or an error on failure.
> + * Called at driver resume.

Same comments as for the previous functions.

> + */
> +int hl_device_resume(struct hl_device *hdev)
> +{
> + int rc;
> +
> + pci_set_power_state(hdev->pdev, PCI_D0);
> + pci_restore_state(hdev->pdev);
> + rc = pci_enable_device(hdev->pdev);
> + if (rc) {
> + dev_err(hdev->dev,
> + "Failed to enable PCI device in resume\n");
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * hl_device_init - main initialization function for habanalabs device
> + *
> + * @hdev: pointer to habanalabs device structure
> + *
> + * Allocate an id for the device, do early initialization and then call the
> + * ASIC specific initialization functions. Finally, create the cdev and the
> + * Linux device to expose it to the user
> + */
> +int hl_device_init(struct hl_device *hdev, struct class *hclass)
> +{
> + int rc;
> +
> + /* Create device */
> + rc = device_setup_cdev(hdev, hclass, hdev->id, &hl_ops);
> +
> + if (rc)
> + goto out_disabled;
> +
> + /* Initialize ASIC function pointers and perform early init */
> + rc = device_early_init(hdev);
> + if (rc)
> + goto release_device;
> +
> + dev_notice(hdev->dev,
> + "Successfully added device to habanalabs driver\n");
> +
> + return 0;
> +
> +release_device:
> + device_destroy(hclass, hdev->dev->devt);
> + cdev_del(&hdev->cdev);
> +out_disabled:
> + hdev->disabled = true;
> + if (hdev->pdev)
> + dev_err(&hdev->pdev->dev,
> + "Failed to initialize hl%d. Device is NOT usable !!!\n",
> + hdev->id);
> + else
> + pr_err("habanalabs: Failed to initialize hl%d. Device is NOT usable !!!\n",
> + hdev->id);

Maybe three exclamation marks would be too much?

> +
> + return rc;
> +}
> +
> +/**
> + * hl_device_fini - main tear-down function for habanalabs device
> + *
> + * @hdev: pointer to habanalabs device structure
> + *
> + * Destroy the device, call ASIC fini functions and release the id
> + */
> +void hl_device_fini(struct hl_device *hdev)
> +{
> + dev_info(hdev->dev, "Removing device\n");
> +
> + /* Mark device as disabled */
> + hdev->disabled = true;
> +
> + device_early_fini(hdev);
> +
> + /* Hide device from user */
> + device_destroy(hdev->dev->class, hdev->dev->devt);
> + cdev_del(&hdev->cdev);
> +
> + pr_info("habanalabs: removed device successfully\n");
> +}
> +
> +/**
> + * hl_poll_timeout_memory - Periodically poll a host memory address
> + * until it is not zero or a timeout occurs
> + * @hdev: pointer to habanalabs device structure
> + * @addr: Address to poll
> + * @timeout_us: timeout in us
> + * @val: Variable to read the value into
> + *
> + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
> + * case, the last read value at @addr is stored in @val. Must not
> + * be called from atomic context if sleep_us or timeout_us are used.
> + *
> + * The function sleeps for 100us with timeout value of
> + * timeout_us
> + */
> +int hl_poll_timeout_memory(struct hl_device *hdev, u64 addr,
> + u32 timeout_us, u32 *val)
> +{
> + /*
> + * pReturnVal is defined as volatile because it points to HOST memory,
> + * which is being written to by the device. Therefore, we can't use
> + * locks to synchronize it and it is not a memory-mapped register space
> + */
> + volatile u32 *pReturnVal = (volatile u32 *) addr;
> + ktime_t timeout = ktime_add_us(ktime_get(), timeout_us);
> +
> + might_sleep();
> +
> + for (;;) {
> + *val = *pReturnVal;
> + if (*val)
> + break;
> + if (ktime_compare(ktime_get(), timeout) > 0) {
> + *val = *pReturnVal;
> + break;
> + }
> + usleep_range((100 >> 2) + 1, 100);
> + }
> +
> + return (*val ? 0 : -ETIMEDOUT);
> +}
> +
> +/**
> + * hl_poll_timeout_devicememory - Periodically poll a device memory address
> + * until it is not zero or a timeout occurs
> + * @hdev: pointer to habanalabs device structure
> + * @addr: Device address to poll
> + * @timeout_us: timeout in us
> + * @val: Variable to read the value into
> + *
> + * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
> + * case, the last read value at @addr is stored in @val. Must not
> + * be called from atomic context if sleep_us or timeout_us are used.
> + *
> + * The function sleeps for 100us with timeout value of
> + * timeout_us
> + */
> +int hl_poll_timeout_device_memory(struct hl_device *hdev, void __iomem *addr,
> + u32 timeout_us, u32 *val)
> +{
> + ktime_t timeout = ktime_add_us(ktime_get(), timeout_us);
> +
> + might_sleep();
> +
> + for (;;) {
> + *val = readl(addr);
> + if (*val)
> + break;
> + if (ktime_compare(ktime_get(), timeout) > 0) {
> + *val = readl(addr);
> + break;
> + }
> + usleep_range((100 >> 2) + 1, 100);
> + }
> +
> + return (*val ? 0 : -ETIMEDOUT);
> +}
> diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
> new file mode 100644
> index 000000000000..7e1b088b677c
> --- /dev/null
> +++ b/drivers/misc/habanalabs/habanalabs.h
> @@ -0,0 +1,149 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright 2016-2018 HabanaLabs, Ltd.
> + * All Rights Reserved.
> + *
> + */
> +
> +#ifndef HABANALABSP_H_
> +#define HABANALABSP_H_
> +
> +#include "include/habanalabs_device_if.h"
> +
> +#include <linux/pci.h>
> +#include <linux/types.h>
> +#include <linux/cdev.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/dma-fence.h>
> +#include <linux/hashtable.h>
> +#include <linux/hwmon.h>
> +
> +#define HL_NAME "habanalabs"
> +
> +struct hl_device;
> +
> +
> +
> +
> +
> +

Too many blank lines, IMHO.

> +/*
> + * ASICs
> + */
> +
> +/**
> + * enum hl_asic_type - supported ASIC types.
> + * @ASIC_AUTO_DETECT: ASIC type will be automatically set.
> + * @ASIC_GOYA: Goya device.
> + * @ASIC_LAST: last ASIC type.
> + */
> +enum hl_asic_type {
> + ASIC_AUTO_DETECT,
> + ASIC_GOYA,
> + ASIC_LAST
> +};
> +
> +
> +
> +
> +
> +/*
> + * FILE PRIVATE STRUCTURE
> + */
> +
> +/**
> + * struct hl_fpriv - process information stored in FD private data.
> + * @hdev: habanalabs device structure.
> + * @filp: pointer to the given file structure.
> + * @taskpid: current process ID.
> + * @refcount: number of related contexts.
> + */
> +struct hl_fpriv {
> + struct hl_device *hdev;
> + struct file *filp;
> + struct pid *taskpid;
> + struct kref refcount;
> +};
> +
> +
> +
> +
> +/*
> + * DEVICES
> + */
> +
> +/* Theoretical limit only. A single host can only contain up to 4 or 8 PCIe
> + * x16 cards. In extereme cases, there are hosts that can accommodate 16 cards
> + */
> +#define HL_MAX_MINORS 256
> +
> +/**
> + * struct hl_device - habanalabs device structure.
> + * @pdev: pointer to PCI device, can be NULL in case of simulator device.
> + * @cdev: related char device.
> + * @dev: realted kernel basic device structure.
> + * @asic_name: ASIC specific nmae.
> + * @asic_type: ASIC specific type.
> + * @major: habanalabs KMD major.
> + * @id: device minor.
> + * @disabled: is device disabled.
> + */
> +struct hl_device {
> + struct pci_dev *pdev;
> + struct cdev cdev;
> + struct device *dev;
> + char asic_name[16];
> + enum hl_asic_type asic_type;
> + u32 major;
> + u16 id;
> + u8 disabled;
> +};
> +
> +/*
> + * IOCTLs
> + */
> +
> +/**
> + * typedef hl_ioctl_t - typedef for ioctl function in the driver
> + * @hpriv: pointer to the FD's private data, which contains state of
> + * user process
> + * @data: pointer to the input/output arguments structure of the IOCTL
> + *
> + * Return: 0 for success, negative value for error
> + */
> +typedef int hl_ioctl_t(struct hl_fpriv *hpriv, void *data);
> +
> +/**
> + * struct hl_ioctl_desc - describes an IOCTL entry of the driver.
> + * @cmd: the IOCTL code as created by the kernel macros.
> + * @func: pointer to the driver's function that should be called for this IOCTL.
> + */
> +struct hl_ioctl_desc {
> + unsigned int cmd;
> + hl_ioctl_t *func;
> +};
> +
> +
> +
> +
> +
> +/*
> + * Kernel module functions that can be accessed by entire module
> + */
> +
> +int hl_device_open(struct inode *inode, struct file *filp);
> +int create_hdev(struct hl_device **dev, struct pci_dev *pdev,
> + enum hl_asic_type asic_type, int minor);
> +void destroy_hdev(struct hl_device *hdev);
> +int hl_poll_timeout_memory(struct hl_device *hdev, u64 addr, u32 timeout_us,
> + u32 *val);
> +int hl_poll_timeout_device_memory(struct hl_device *hdev, void __iomem *addr,
> + u32 timeout_us, u32 *val);
> +
> +int hl_device_init(struct hl_device *hdev, struct class *hclass);
> +void hl_device_fini(struct hl_device *hdev);
> +int hl_device_suspend(struct hl_device *hdev);
> +int hl_device_resume(struct hl_device *hdev);
> +
> +#endif /* HABANALABSP_H_ */
> diff --git a/drivers/misc/habanalabs/habanalabs_drv.c b/drivers/misc/habanalabs/habanalabs_drv.c
> new file mode 100644
> index 000000000000..15217975327b
> --- /dev/null
> +++ b/drivers/misc/habanalabs/habanalabs_drv.c
> @@ -0,0 +1,366 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright 2016-2018 HabanaLabs, Ltd.
> + * All Rights Reserved.
> + *
> + * Author: Oded Gabbay <oded.gabbay@gmail.com>
> + *
> + */
> +
> +#include "habanalabs.h"
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kthread.h>
> +
> +#include <linux/fs.h>
> +
> +#define HL_DRIVER_AUTHOR "HabanaLabs Kernel Driver Team"
> +
> +#define HL_DRIVER_DESC "Driver for HabanaLabs's AI Accelerators"
> +
> +MODULE_AUTHOR(HL_DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(HL_DRIVER_DESC);
> +MODULE_LICENSE("GPL v2");
> +
> +static int hl_major;
> +static struct class *hl_class;
> +DEFINE_IDR(hl_devs_idr);
> +DEFINE_MUTEX(hl_devs_idr_lock);
> +
> +#define PCI_VENDOR_ID_HABANALABS 0x1da3
> +
> +#define PCI_IDS_GOYA 0x0001
> +
> +static struct pci_device_id ids[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_HABANALABS, PCI_IDS_GOYA), },
> + { 0, }
> +};
> +MODULE_DEVICE_TABLE(pci, ids);
> +
> +/**
> + * get_asic_type - translate device id to asic type
> + *
> + * @device: id of the PCI device
> + * @asic_type: pointer that will be filled by the asic type
> + *
> + * Translate device id to asic type.
> + * In case of unidentified device, return -1
> + */
> +static int get_asic_type(u16 device, enum hl_asic_type *asic_type)

This can simply return the hl_asic_type, see also a comment in
create_hdev(().

> +{
> + int rc = 0;
> +
> + switch (device) {
> + case PCI_IDS_GOYA:
> + *asic_type = ASIC_GOYA;
> + break;
> + default:
> + *asic_type = rc = -1;
> + break;
> + }
> +
> + return rc;
> +}
> +
> +/**
> + * hl_device_open - open function for habanalabs device
> + *
> + * @inode: pointer to inode structure
> + * @filp: pointer to file structure
> + *
> + * Called when process opens an habanalabs device.
> + */
> +int hl_device_open(struct inode *inode, struct file *filp)
> +{
> + struct hl_device *hdev;
> + struct hl_fpriv *hpriv;
> +
> + mutex_lock(&hl_devs_idr_lock);
> + hdev = idr_find(&hl_devs_idr, iminor(inode));
> + mutex_unlock(&hl_devs_idr_lock);
> +
> + if (!hdev) {
> + pr_err("habanalabs: Couldn't find device %d:%d\n",
> + imajor(inode), iminor(inode));
> + return -ENXIO;
> + }
> +
> + hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
> + if (!hpriv)
> + return -ENOMEM;
> +
> + hpriv->hdev = hdev;
> + filp->private_data = hpriv;
> + hpriv->filp = filp;
> + kref_init(&hpriv->refcount);
> + nonseekable_open(inode, filp);
> +
> + hpriv->taskpid = find_get_pid(current->pid);
> +
> + return 0;
> +}
> +
> +/**
> + * create_hdev - create habanalabs device instance
> + *
> + * @dev: will hold the pointer to the new habanalabs device structure
> + * @pdev: pointer to the pci device
> + * @asic_type: in case of simulator device, which device is it
> + * @minor: in case of simulator device, the minor of the device
> + *
> + * Allocate memory for habanalabs device and initialize basic fields
> + * Identify the ASIC type
> + * Allocate ID (minor) for the device (only for real devices)
> + */
> +int create_hdev(struct hl_device **dev, struct pci_dev *pdev,
> + enum hl_asic_type asic_type, int minor)
> +{
> + struct hl_device *hdev;
> + int rc;
> +
> + *dev = NULL;
> +
> + hdev = kzalloc(sizeof(*hdev), GFP_KERNEL);
> + if (!hdev) {
> + if (pdev)
> + dev_err(&pdev->dev,
> + "Not enough memory for habanalabs device\n");
> + else
> + pr_err("habanalabs: Not enough memory for device\n");
> +
> + return -ENOMEM;
> + }
> +
> + hdev->major = hl_major;
> +
> + hdev->disabled = true;
> + hdev->pdev = pdev; /* can be NULL in case of simulator device */
> +
> + if (asic_type == ASIC_AUTO_DETECT) {
> + rc = get_asic_type(pdev->device, &hdev->asic_type);

You can just make it

&hdev->asic_type = get_asic_type(pdev->device);

> + if (rc) {
> + dev_err(&pdev->dev, "Unsupported ASIC\n");
> + rc = -ENODEV;
> + goto free_hdev;
> + }
> + } else {
> + hdev->asic_type = asic_type;
> + }

In the current version create_hdev() is always called with
ASIC_AUTO_DETECT, what are the usecases for other types?

> +
> + mutex_lock(&hl_devs_idr_lock);
> +
> + if (minor == -1) {
> + rc = idr_alloc(&hl_devs_idr, hdev, 0, HL_MAX_MINORS,
> + GFP_KERNEL);
> + } else {
> + idr_replace(&hl_devs_idr, hdev, minor);

idr_replace can fail, can't it?

> + rc = minor;
> + }
> +
> + mutex_unlock(&hl_devs_idr_lock);
> +
> + if (rc < 0) {
> + if (rc == -ENOSPC) {
> + pr_err("habanalabs: too many devices in the system\n");
> + rc = -EBUSY;
> + }
> + goto free_hdev;
> + }
> +
> + hdev->id = rc;
> +
> + *dev = hdev;
> +
> + return 0;
> +
> +free_hdev:
> + kfree(hdev);
> + return rc;
> +}
> +
> +/**
> + * destroy_hdev - destroy habanalabs device instance
> + *
> + * @dev: pointer to the habanalabs device structure
> + *
> + */
> +void destroy_hdev(struct hl_device *hdev)
> +{
> + /* Remove device from the device list */
> + mutex_lock(&hl_devs_idr_lock);
> + idr_remove(&hl_devs_idr, hdev->id);
> + mutex_unlock(&hl_devs_idr_lock);
> +
> + kfree(hdev);
> +}
> +
> +static int hl_pmops_suspend(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct hl_device *hdev = pci_get_drvdata(pdev);
> +
> + pr_debug("habanalabs: Going to suspend PCI device\n");
> +
> + if (!hdev) {
> + pr_err("habanalabs: device pointer is NULL in suspend\n");
> + return 0;
> + }
> +
> + return hl_device_suspend(hdev);
> +}
> +
> +static int hl_pmops_resume(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct hl_device *hdev = pci_get_drvdata(pdev);
> +
> + pr_debug("habanalabs: Going to resume PCI device\n");
> +
> + if (!hdev) {
> + pr_err("habanalabs: device pointer is NULL in resume\n");
> + return 0;
> + }
> +
> + return hl_device_resume(hdev);
> +}
> +
> +/**
> + * hl_pci_probe - probe PCI habanalabs devices
> + *
> + * @pdev: pointer to pci device
> + * @id: pointer to pci device id structure
> + *
> + * Standard PCI probe function for habanalabs device.
> + * Create a new habanalabs device and initialize it according to the
> + * device's type
> + */
> +static int hl_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct hl_device *hdev;
> + int rc;
> +
> + dev_info(&pdev->dev, HL_NAME
> + " device found [%04x:%04x] (rev %x)\n",
> + (int)pdev->vendor, (int)pdev->device, (int)pdev->revision);
> +
> + rc = create_hdev(&hdev, pdev, ASIC_AUTO_DETECT, -1);
> + if (rc)
> + return rc;
> +
> + pci_set_drvdata(pdev, hdev);
> +
> + rc = hl_device_init(hdev, hl_class);
> + if (rc) {
> + dev_err(&pdev->dev, "Fatal error during habanalabs device init\n");
> + rc = -ENODEV;
> + goto disable_device;
> + }
> +
> + return 0;
> +
> +disable_device:
> + pci_set_drvdata(pdev, NULL);
> + destroy_hdev(hdev);
> +
> + return rc;
> +}
> +
> +/**
> + * hl_pci_remove - remove PCI habanalabs devices
> + *
> + * @pdev: pointer to pci device
> + *
> + * Standard PCI remove function for habanalabs device
> + */
> +static void hl_pci_remove(struct pci_dev *pdev)
> +{
> + struct hl_device *hdev;
> +
> + hdev = pci_get_drvdata(pdev);
> + if (!hdev)
> + return;
> +
> + hl_device_fini(hdev);
> + pci_set_drvdata(pdev, NULL);
> +
> + destroy_hdev(hdev);
> +}
> +
> +static const struct dev_pm_ops hl_pm_ops = {
> + .suspend = hl_pmops_suspend,
> + .resume = hl_pmops_resume,
> +};
> +
> +static struct pci_driver hl_pci_driver = {
> + .name = HL_NAME,
> + .id_table = ids,
> + .probe = hl_pci_probe,
> + .remove = hl_pci_remove,
> + .driver.pm = &hl_pm_ops,
> +};
> +
> +/**
> + * hl_init - Initialize the habanalabs kernel driver
> + *
> + */
> +static int __init hl_init(void)
> +{
> + int rc;
> + dev_t dev;
> +
> + pr_info("habanalabs: loading driver\n");
> +
> + rc = alloc_chrdev_region(&dev, 0, HL_MAX_MINORS, HL_NAME);
> + if (rc < 0) {
> + pr_err("habanalabs: unable to get major\n");
> + return rc;
> + }
> +
> + hl_major = MAJOR(dev);
> +
> + hl_class = class_create(THIS_MODULE, HL_NAME);
> + if (IS_ERR(hl_class)) {
> + pr_err("habanalabs: failed to allocate class\n");
> + rc = PTR_ERR(hl_class);
> + goto remove_major;
> + }
> +
> + rc = pci_register_driver(&hl_pci_driver);
> + if (rc) {
> + pr_err("habanalabs: failed to register pci device\n");
> + goto remove_class;
> + }
> +
> + pr_debug("habanalabs: driver loaded\n");
> +
> + return 0;
> +
> +remove_class:
> + class_destroy(hl_class);
> +remove_major:
> + unregister_chrdev_region(MKDEV(hl_major, 0), HL_MAX_MINORS);
> + return rc;
> +}
> +
> +/**
> + * hl_exit - Release all resources of the habanalabs kernel driver
> + *
> + */
> +static void __exit hl_exit(void)
> +{
> + pci_unregister_driver(&hl_pci_driver);
> +
> + class_destroy(hl_class);
> + unregister_chrdev_region(MKDEV(hl_major, 0), HL_MAX_MINORS);
> +
> + idr_destroy(&hl_devs_idr);
> +
> + pr_debug("habanalabs: driver removed\n");
> +}
> +
> +module_init(hl_init);
> +module_exit(hl_exit);
> diff --git a/drivers/misc/habanalabs/include/habanalabs_device_if.h b/drivers/misc/habanalabs/include/habanalabs_device_if.h
> new file mode 100644
> index 000000000000..9dbb7077eabd
> --- /dev/null
> +++ b/drivers/misc/habanalabs/include/habanalabs_device_if.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright 2016-2018 HabanaLabs, Ltd.
> + * All Rights Reserved.
> + *
> + */
> +
> +#ifndef HABANALABS_DEVICE_IF_H
> +#define HABANALABS_DEVICE_IF_H
> +
> +#include <linux/types.h>
> +
> +/*
> + * PRIMARY QUEUE
> + */
> +
> +struct hl_bd {
> + __u64 ptr;
> + __u32 len;
> + union {
> + struct {
> + __u32 repeat:16;
> + __u32 res1:8;
> + __u32 repeat_valid:1;
> + __u32 res2:7;
> + };
> + __u32 ctl;
> + };
> +};
> +
> +#define HL_BD_SIZE sizeof(struct hl_bd)
> +
> +/*
> + * BD_CTL_REPEAT_VALID tells the CP whether the repeat field in the BD CTL is
> + * valid. 1 means the repeat field is valid, 0 means not-valid,
> + * i.e. repeat == 1
> + */
> +#define BD_CTL_REPEAT_VALID_SHIFT 24
> +#define BD_CTL_REPEAT_VALID_MASK 0x01000000
> +
> +#define BD_CTL_SHADOW_INDEX_SHIFT 0
> +#define BD_CTL_SHADOW_INDEX_MASK 0x00000FFF
> +
> +/*
> + * COMPLETION QUEUE
> + */
> +
> +struct hl_cq_entry {
> + __u32 data;
> +};
> +
> +#define HL_CQ_ENTRY_SIZE sizeof(struct hl_cq_entry)
> +
> +#define CQ_ENTRY_READY_SHIFT 31
> +#define CQ_ENTRY_READY_MASK 0x80000000
> +
> +#define CQ_ENTRY_SHADOW_INDEX_VALID_SHIFT 30
> +#define CQ_ENTRY_SHADOW_INDEX_VALID_MASK 0x40000000
> +
> +#define CQ_ENTRY_SHADOW_INDEX_SHIFT BD_CTL_SHADOW_INDEX_SHIFT
> +#define CQ_ENTRY_SHADOW_INDEX_MASK BD_CTL_SHADOW_INDEX_MASK
> +
> +/*
> + * EVENT QUEUE
> + */
> +
> +struct hl_eq_header {
> + __u32 reserved;
> + union {
> + struct {
> + __u32 ctx_id :10;
> + __u32:6;
> + __u32 opcode :10;
> + __u32:5;
> + __u32 ready :1;
> + };
> + __u32 ctl;
> + };
> +};
> +
> +struct hl_eq_entry {
> + struct hl_eq_header hdr;
> + __u64 data[7];
> +};
> +
> +#define HL_EQ_ENTRY_SIZE sizeof(struct hl_eq_entry)
> +
> +#define EQ_CTL_READY_SHIFT 31
> +#define EQ_CTL_READY_MASK 0x80000000
> +
> +#define EQ_CTL_EVENT_TYPE_SHIFT 16
> +#define EQ_CTL_EVENT_TYPE_MASK 0x03FF0000
> +
> +enum pq_init_status {
> + PQ_INIT_STATUS_NA = 0,
> + PQ_INIT_STATUS_READY_FOR_CP,
> + PQ_INIT_STATUS_READY_FOR_HOST
> +};
> +
> +/*
> + * ArmCP info
> + */
> +
> +#define VERSION_MAX_LEN 128
> +#define ARMCP_MAX_SENSORS 128
> +
> +struct armcp_sensor {
> + __u32 type;
> + __u32 flags;
> +};
> +
> +/* must be aligned to 4 bytes */
> +struct armcp_info {
> + struct armcp_sensor sensors[ARMCP_MAX_SENSORS];
> + __u8 kernel_version[VERSION_MAX_LEN];
> + __u32 reserved[3];
> + __u32 cpld_version;
> + __u32 infineon_version;
> + __u8 fuse_version[VERSION_MAX_LEN];
> + __u8 thermal_version[VERSION_MAX_LEN];
> + __u8 armcp_version[VERSION_MAX_LEN];
> + __u64 dram_size;
> +};
> +
> +#endif /* HABANALABS_DEVICE_IF_H */
> --
> 2.17.1
>

--
Sincerely yours,
Mike.