On Wed, 2019-01-23 at 02:00 +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.
trivial notes:
>
> diff --git a/drivers/misc/habanalabs/Makefile
> b/drivers/misc/habanalabs/Makefile
[]
> \ No newline at end of file
You should fixes these. There are a least a couple of them.
> diff --git a/drivers/misc/habanalabs/device.c
> b/drivers/misc/habanalabs/device.c
[]
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright 2016-2018 HabanaLabs, Ltd.
> + * All Rights Reserved.
> + */
Add #define pr_fmt(fmt) "habanalabs: " fmt
> +
> +#include "habanalabs.h"
or add it in this file
> +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);
Might overflow name one day
> +
> + 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);
So #define pr_fmt can auto prefix these and this would be
pr_err("Failed to add char device %s\n", name);
missing terminating '\n' btw
> + 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);
And this would be:
pr_err("Failed to create device %s\n", name);
etc...
> +static int device_early_init(struct hl_device *hdev)
> +{
> + switch (hdev->asic_type) {
> + case ASIC_GOYA:
> + sprintf(hdev->asic_name, "GOYA");
strcpy or perhaps better still as strlcpy
> +int hl_device_init(struct hl_device *hdev, struct class *hclass)
> +{
[]
> + dev_notice(hdev->dev,
> + "Successfully added device to habanalabs driver\n");
This is mostly aligned to open parenthesis, but perhaps
it could check with scripts/checkpatch.pl --strict and
see if you agree with anything it bleats.
> +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;
It'd be nice to avoid hungarian and camelcase
> + 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);
Unnecessary parentheses
> diff --git a/drivers/misc/habanalabs/habanalabs_drv.c
> b/drivers/misc/habanalabs/habanalabs_drv.c
[]
> +static struct pci_device_id ids[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_HABANALABS, PCI_IDS_GOYA), },
> + { 0, }
> +};
static const?
> diff --git a/drivers/misc/habanalabs/include/habanalabs_device_if.h
> b/drivers/misc/habanalabs/include/habanalabs_device_if.h
[]
> +struct hl_bd {
> + __u64 ptr;
> + __u32 len;
> + union {
> + struct {
> + __u32 repeat:16;
> + __u32 res1:8;
> + __u32 repeat_valid:1;
> + __u32 res2:7;
> + };
> + __u32 ctl;
> + };
> +};
Maybe use the appropriate bit-endian __le<size> instead of __u<size>
with whatever cpu_to_le<size> / le<size>_to_cpu bits are necessary.
