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

[Oded Gabbay <oded.gabbay@gmail.com>]

On Wed, Jan 23, 2019 at 2:28 PM Mike Rapoport <rppt@linux.ibm.com> wrote:
>
> 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?
>
So I don't think I mentioned this, but we have a software simulator
that we wrote for our ASICs.
To support that, you can load the driver in simulation mode.
Most of the simulation code is in an asic-specific file
(goya_simulator.c) which I don't intend to upstream because:
1. It does really nasty things to make the simulator work and those
nasty things are totally un-upstreamable :)
2. We don't intend to open source the simulator, nor give it to
customers, so there is no need to upstream that code.

Having said that, there are very few places in the common code, which
I think all of them are in habanalabs_drv.c, that contain code which
is for simulation mode.
One of those places is:
hdev->asic_type = asic_type;

Another place is the idr_replace below.

I hope that because we are talking about a couple of lines in the
entire driver, and because by themselves they are totally valid, I
could upstream them even if that path will never be taken.
If even those few lines are problematic, I will remove them, but it
will just make my life a bit harder.

Oded

> > +
> > +     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.
>