> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, June 15, 2023 2:43 AM
> 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>;
> corbet@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-hyperv@xxxxxxxxxxxxxxx;
> linux-doc@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH v2 1/5] uio: Add hv_vmbus_client driver
>
> On Wed, Jun 14, 2023 at 11:15:08AM -0700, Saurabh Sengar wrote:
> > --- a/Documentation/ABI/stable/sysfs-bus-vmbus
> > +++ b/Documentation/ABI/stable/sysfs-bus-vmbus
> > @@ -153,6 +153,13 @@ Contact: Stephen Hemminger
> <sthemmin@xxxxxxxxxxxxx>
> > Description: Binary file created by uio_hv_generic for ring buffer
> > Users: Userspace drivers
> >
> > +What: /sys/bus/vmbus/devices/<UUID>/ring_size
> > +Date: June. 2023
>
> No need for the "."
OK
>
> > +KernelVersion: 6.4
>
> 6.4 will be released without this, sorry.
Ok will change it to 6.5.
>
> > +Contact: Saurabh Sengar <ssengar@xxxxxxxxxxxxx>
> > +Description: File created by uio_hv_vmbus_client for setting device ring
> buffer size
> > +Users: Userspace drivers
> > +
> > What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/intr_in_full
> > Date: February 2019
> > KernelVersion: 5.0
> > diff --git a/Documentation/driver-api/uio-howto.rst
> > b/Documentation/driver-api/uio-howto.rst
> > index 907ffa3b38f5..33b67f876b96 100644
> > --- a/Documentation/driver-api/uio-howto.rst
> > +++ b/Documentation/driver-api/uio-howto.rst
> > @@ -722,6 +722,52 @@ For example::
> >
> >
> > /sys/bus/vmbus/devices/3811fe4d-0fa0-4b62-981a-
> 74fc1084c757/channels/2
> > 1/ring
> >
> > +Generic Hyper-V driver for low speed devices
> > +============================================
> > +
> > +The generic driver is a kernel module named uio_hv_vmbus_client. It
> > +supports slow devices on the Hyper-V VMBus similar to uio_hv_generic
> > +for faster devices. This driver also gives flexibility of customized
> > +ring buffer sizes.
> > +
> > +Making the driver recognize the device
> > +--------------------------------------
> > +
> > +Since the driver does not declare any device GUID's, it will not get
> > +loaded automatically and will not automatically bind to any devices,
> > +you must load it and allocate id to the driver yourself. For example,
> > +to use the fcopy device class GUID::
> > +
> > + DEV_UUID=eb765408-105f-49b6-b4aa-c123b64d17d4
> > + driverctl -b vmbus set-override $DEV_UUID uio_hv_vmbus_client
>
> Why are you adding a dependancy on a 300 line bash script that is not used
> by most distros?
>
> Why not just show the "real" commands that you can use here that don't
> require an external tool not controlled by the kernel at all.
Ok will mention the regular "echo" commands as you suggested.
>
> > --- /dev/null
> > +++ b/drivers/uio/uio_hv_vmbus_client.c
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * uio_hv_vmbus_client - UIO driver for low speed VMBus devices
> > + *
> > + * Copyright (c) 2023, Microsoft Corporation.
> > + *
> > + * Authors:
> > + * Saurabh Sengar <ssengar@xxxxxxxxxxxxx>
> > + *
> > + * 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
>
> Again, no need to discuss driverctl.
Noted.
>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/uio_driver.h>
> > +#include <linux/hyperv.h>
> > +
> > +#define DRIVER_AUTHOR "Saurabh Sengar <ssengar@xxxxxxxxxxxxx>"
> > +#define DRIVER_DESC "Generic UIO driver for low speed VMBus
> devices"
>
> You only use these defines in one place, so why not just spell them out there,
> no need for 2 extra lines, right?
Sure, will fix
>
> > +
> > +#define DEFAULT_HV_RING_SIZE VMBUS_RING_SIZE(3 *
> HV_HYP_PAGE_SIZE)
> > +static int ring_size = DEFAULT_HV_RING_SIZE;
>
> You only use that #define in one place, why have it at all?
Ok, will fix
>
> And you are defining a "global" variable that can be modified by an individual
> sysfs file for ANY device bound to this driver, messing with the other device's
> ring buffer size, right? This needs to be per-device, or explain in huge detail
> here why not.
The global variable is expected to be set by userspace per device before opening, the
particular uio device. For a particular Hyper-v device this value be same, and once
device is open the ring buffer is allocated and there won't be any impact afterwards
changing it. I can elaborate more of this in sysfs documentation.
>
> > +
> > +struct uio_hv_vmbus_dev {
> > + struct uio_info info;
> > + struct hv_device *device;
> > +};
> > +
> > +/* Sysfs API to allow mmap of the ring buffers */ static int
> > +uio_hv_vmbus_mmap(struct file *filp, struct kobject *kobj,
> > + struct bin_attribute *attr, struct vm_area_struct
> *vma) {
> > + struct device *dev = container_of(kobj, struct device, kobj);
> > + struct hv_device *hv_dev = container_of(dev, struct hv_device,
> device);
> > + struct vmbus_channel *channel = hv_dev->channel;
> > + void *ring_buffer = page_address(channel->ringbuffer_page);
> > +
> > + return vm_iomap_memory(vma, virt_to_phys(ring_buffer),
> > + channel->ringbuffer_pagecount << PAGE_SHIFT); }
> > +
> > +static const struct bin_attribute ring_buffer_bin_attr = {
> > + .attr = {
> > + .name = "ringbuffer",
> > + .mode = 0600,
> > + },
> > + .mmap = uio_hv_vmbus_mmap,
> > +};
> > +
> > +/*
> > + * This is the irqcontrol callback to be registered to uio_info.
> > + * It can be used to disable/enable interrupt from user space processes.
> > + *
> > + * @param info
> > + * pointer to uio_info.
> > + * @param irq_state
> > + * state value. 1 to enable interrupt, 0 to disable interrupt.
> > + */
> > +static int uio_hv_vmbus_irqcontrol(struct uio_info *info, s32
> > +irq_state) {
> > + struct uio_hv_vmbus_dev *pdata = info->priv;
> > + struct hv_device *hv_dev = pdata->device;
> > +
> > + /* Issue a full memory barrier before triggering the notification */
> > + virt_mb();
> > +
> > + vmbus_setevent(hv_dev->channel);
> > + return 0;
> > +}
> > +
> > +/*
> > + * Callback from vmbus_event when something is in inbound ring.
> > + */
> > +static void uio_hv_vmbus_channel_cb(void *context) {
> > + struct uio_hv_vmbus_dev *pdata = context;
> > +
> > + /* Issue a full memory barrier before sending the event to userspace
> */
> > + virt_mb();
> > +
> > + uio_event_notify(&pdata->info);
> > +}
> > +
> > +static int uio_hv_vmbus_open(struct uio_info *info, struct inode
> > +*inode) {
> > + struct uio_hv_vmbus_dev *pdata = container_of(info, struct
> uio_hv_vmbus_dev, info);
> > + struct hv_device *hv_dev = pdata->device;
> > + struct vmbus_channel *channel = hv_dev->channel;
> > + int ret;
> > +
> > + ret = vmbus_open(channel, ring_size, ring_size, NULL, 0,
> > + uio_hv_vmbus_channel_cb, pdata);
> > + if (ret) {
> > + dev_err(&hv_dev->device, "error %d when opening the
> channel\n", ret);
> > + return ret;
> > + }
> > + channel->inbound.ring_buffer->interrupt_mask = 0;
> > + set_channel_read_mode(channel, HV_CALL_ISR);
> > +
> > + ret = device_create_bin_file(&hv_dev->device,
> &ring_buffer_bin_attr);
> > + if (ret)
> > + dev_err(&hv_dev->device, "sysfs create ring bin file failed;
> %d\n",
> > +ret);
> > +
> > + return ret;
> > +}
> > +
> > +/* VMbus primary channel is closed on last close */ static int
> > +uio_hv_vmbus_release(struct uio_info *info, struct inode *inode) {
> > + struct uio_hv_vmbus_dev *pdata = container_of(info, struct
> uio_hv_vmbus_dev, info);
> > + struct hv_device *hv_dev = pdata->device;
> > + struct vmbus_channel *channel = hv_dev->channel;
> > +
> > + device_remove_bin_file(&hv_dev->device, &ring_buffer_bin_attr);
> > + vmbus_close(channel);
> > +
> > + return 0;
> > +}
> > +
> > +static ssize_t ring_size_show(struct device *dev, struct device_attribute
> *attr,
> > + char *buf)
> > +{
> > + return sysfs_emit(buf, "%d\n", ring_size); }
> > +
> > +static ssize_t ring_size_store(struct device *dev, struct device_attribute
> *attr,
> > + const char *buf, size_t count) {
> > + unsigned int val;
> > +
> > + if (kstrtouint(buf, 0, &val) < 0)
> > + return -EINVAL;
> > +
> > + if (val < HV_HYP_PAGE_SIZE)
> > + return -EINVAL;
> > +
> > + ring_size = val;
> > +
> > + return count;
> > +}
> > +
> > +static DEVICE_ATTR_RW(ring_size);
> > +
> > +static struct attribute *uio_hv_vmbus_client_attrs[] = {
> > + &dev_attr_ring_size.attr,
> > + NULL,
> > +};
> > +ATTRIBUTE_GROUPS(uio_hv_vmbus_client);
> > +
> > +static int uio_hv_vmbus_probe(struct hv_device *dev, const struct
> > +hv_vmbus_device_id *dev_id) {
> > + struct uio_hv_vmbus_dev *pdata;
> > + int ret;
> > + char *name = NULL;
> > +
> > + pdata = devm_kzalloc(&dev->device, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
> > +
> > + name = kasprintf(GFP_KERNEL, "%pUl", &dev->dev_instance);
> > +
> > + /* Fill general uio info */
> > + pdata->info.name = name; /* /sys/class/uio/uioX/name */
> > + pdata->info.version = "1";
> > + pdata->info.irqcontrol = uio_hv_vmbus_irqcontrol;
> > + pdata->info.open = uio_hv_vmbus_open;
> > + pdata->info.release = uio_hv_vmbus_release;
> > + pdata->info.irq = UIO_IRQ_CUSTOM;
> > + pdata->info.priv = pdata;
> > + pdata->device = dev;
> > +
> > + ret = uio_register_device(&dev->device, &pdata->info);
> > + if (ret) {
> > + dev_err(&dev->device, "uio_hv_vmbus register failed\n");
> > + return ret;
> > + }
> > +
> > + hv_set_drvdata(dev, pdata);
> > +
> > + return 0;
> > +}
> > +
> > +static void uio_hv_vmbus_remove(struct hv_device *dev) {
> > + struct uio_hv_vmbus_dev *pdata = hv_get_drvdata(dev);
> > +
> > + if (pdata)
> > + uio_unregister_device(&pdata->info);
> > +}
> > +
> > +static struct hv_driver uio_hv_vmbus_drv = {
> > + .driver.dev_groups = uio_hv_vmbus_client_groups,
> > + .name = "uio_hv_vmbus_client",
> > + .id_table = NULL, /* only dynamic id's */
>
> No need to set this if it's NULL.
Ok.
Thanks for your review.
- Saurabh
>
> thanks,
>
> greg k-h
