RE: [EXTERNAL] Re: [PATCH 1/5] uio: Add hv_vmbus_client driver

[Saurabh Singh Sengar]

> -----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