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;
MIME-Version: 1.0
References: <20190123000057.31477-1-oded.gabbay@gmail.com> <20190123000057.31477-2-oded.gabbay@gmail.com>
 <f6616b8d1983286a9df8e0b62849558487da3847.camel@perches.com>
In-Reply-To: <f6616b8d1983286a9df8e0b62849558487da3847.camel@perches.com>
From:   Oded Gabbay <oded.gabbay@gmail.com>
Date:   Fri, 25 Jan 2019 21:18:00 +0200
Message-ID: <CAFCwf13in=4BUFwfmkQSc+77jNuCrWXTRv-Dx=aY+7oEHd-aMQ@mail.gmail.com>
Subject: Re: [PATCH 01/15] habanalabs: add skeleton driver
To:     Joe Perches <joe@perches.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jan 23, 2019 at 2:49 AM Joe Perches <joe@perches.com> wrote:
>
> 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.
>
>

Hi Joe,
Thanks for the review.
I fixed everything except for two things:
1. Alignment to open parenthesis. I never code like that in the kernel
and I don't really believe in anything that requires to combine spaces
and tabs.
2. The bit-endian format. We don't support big-endian architecture
(what's left after POWER moved to support little endian ?). And in any
case, our software stack is so big that this minor change in the
driver won't have any impact.

Thanks,
Oded