Messages in this thread | | | Subject | Re: [PATCH 01/15] habanalabs: add skeleton driver | From | Joe Perches <> | Date | Tue, 22 Jan 2019 16:49:17 -0800 |
| |
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.
| |