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

[Greg KH <gregkh@linuxfoundation.org>]

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?

> + */
> +
> +#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?

> +#define DRIVER_AUTHOR	"Saurabh Sengar <ssengar@microsoft.com>"
> +#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.


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

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

> +	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"?

> +MODULE_VERSION(DRIVER_VERSION);

This means nothing, please drop.

thanks,

greg k-h