Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <f6616b8d1983286a9df8e0b62849558487da3847.camel@perches.com>
Subject: Re: [PATCH 01/15] habanalabs: add skeleton driver
From:   Joe Perches <joe@perches.com>
To:     Oded Gabbay <oded.gabbay@gmail.com>, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org
Cc:     ogabbay@habana.ai
Date:   Tue, 22 Jan 2019 16:49:17 -0800
In-Reply-To: <20190123000057.31477-2-oded.gabbay@gmail.com>
References: <20190123000057.31477-1-oded.gabbay@gmail.com>
         <20190123000057.31477-2-oded.gabbay@gmail.com>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.30.1-1build1 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.