> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Sunday, June 4, 2023 12:40 PM
> To: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
> Cc: KY Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang
> <haiyangz@xxxxxxxxxxxxx>; wei.liu@xxxxxxxxxx; Dexuan Cui
> <decui@xxxxxxxxxxxxx>; Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx; linux-hyperv@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH 1/5] uio: Add hv_vmbus_client driver
>
> On Fri, Jun 02, 2023 at 12:57:05AM -0700, Saurabh Sengar wrote:
> > + * Since the driver does not declare any device ids, you must
> > + allocate
> > + * id and bind the device to the driver yourself. For example:
> > + * driverctl -b vmbus set-override <dev uuid> uio_hv_vmbus_client
>
> Shouldn't this be documented somewhere?
I will update it in Documentation/driver-api/uio-howto.rst.
>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/uio_driver.h>
> > +#include <linux/hyperv.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/sysfs.h>
> > +
> > +#define DRIVER_VERSION "0.0.1"
>
> Why this number? Why not "1"?
>
> The whole "driver version" thing doesn't really make sense here, we should
> just drop it from the uio later, right?
will remove in V2.
>
> > +#define DRIVER_AUTHOR "Saurabh Sengar <ssengar@xxxxxxxxxxxxx>"
> > +#define DRIVER_DESC "Generic UIO driver for low speed VMBus
> devices"
> > +
> > +#define DEFAULT_HV_RING_SIZE VMBUS_RING_SIZE(3 *
> HV_HYP_PAGE_SIZE)
> > +static int ring_size = DEFAULT_HV_RING_SIZE;
> > +
> > +struct uio_hv_vmbus_dev {
> > + struct uio_info info;
> > + struct hv_device *device;
> > + atomic_t refcnt;
>
> Why is this refcnt needed?
>
> Have you seen the other threads about how attempting to block multiple
> opens in UIO drivers really does not work at all? Please drop all of that logic,
> it is not correct.
>
Will remove.
>
> > +static ssize_t ring_size_show(struct device *dev, struct device_attribute
> *attr,
> > + char *buf)
> > +{
> > + return scnprintf(buf, PAGE_SIZE, "%d\n", ring_size);
>
> Did you use checkpatch?
>
> It should have said to use sysfs_emit(), right?
Yes, I am using "--strict" switch for checkpatch.pl, still didn't get this warning.
However, I will use sysfs_emit() in V2.
>
> > + ret = sysfs_create_file(&dev->device.kobj,
> > +&dev_attr_ring_size.attr);
>
> If you ever have to use a sysfs_* call in a driver, that's a huge hint something
> is wrong. Please fix this to not race with userspace and loose.
Thanks for pointing this out, will look into this.
>
> > + if (ret)
> > + dev_notice(&dev->device, "sysfs create ring size file failed;
> > +%d\n", ret);
>
> Why not just use dev_err() for this and other errors? Why "notice"?
Will fix.
>
> > +MODULE_VERSION(DRIVER_VERSION);
>
> This means nothing, please drop.
OK.
Thanks for review.
- Saurabh
>
> thanks,
>
> greg k-h
