On Wed, Oct 04, 2017 at 05:48:38PM -0500, Mario Limonciello wrote:
> For WMI operations that are only Set or Query read or write sysfs
> attributes created by WMI vendor drivers make sense.
>
> For other WMI operations that are run on Method, there needs to be a
> way to guarantee to userspace that the results from the method call
> belong to the data request to the method call. Sysfs attributes don't
> work well in this scenario because two userspace processes may be
> competing at reading/writing an attribute and step on each other's
> data.
>
> When a WMI vendor driver declares an ioctl in a file_operations object
> the WMI bus driver will create a character device that maps to those
> file operations.
>
> That character device will correspond to this path:
> /dev/wmi/$driver
>
> The WMI bus driver will interpret the IOCTL calls, test them for
> a valid instance and pass them on to the vendor driver to run.
>
> This creates an implicit policy that only driver per character
> device. If a module matches multiple GUID's, the wmi_devices
> will need to be all handled by the same wmi_driver if the same
> character device is used.
>
> The WMI vendor drivers will be responsible for managing access to
> this character device and proper locking on it.
>
> When a WMI vendor driver is unloaded the WMI bus driver will clean
> up the character device.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxxx>
> ---
> MAINTAINERS | 1 +
> drivers/platform/x86/wmi.c | 67 +++++++++++++++++++++++++++++++++++++++++++++-
> include/linux/wmi.h | 2 ++
> include/uapi/linux/wmi.h | 10 +++++++
> 4 files changed, 79 insertions(+), 1 deletion(-)
> create mode 100644 include/uapi/linux/wmi.h
>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
...
> +static long wmi_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct wmi_driver *wdriver;
> + struct wmi_block *wblock;
> + const char *driver_name;
> + struct list_head *p;
> + bool found = false;
> +
> + if (_IOC_TYPE(cmd) != WMI_IOC)
> + return -ENOTTY;
> +
> + driver_name = filp->f_path.dentry->d_iname;
> +
> + list_for_each(p, &wmi_block_list) {
> + wblock = list_entry(p, struct wmi_block, list);
> + wdriver = container_of(wblock->dev.dev.driver,
> + struct wmi_driver, driver);
> + if (strcmp(driver_name, wdriver->driver.name) == 0) {
> + found = true;
> + break;
A bit of a nitpic, but the "found" variable isn't necessary. The wdriver
pointer is sufficient:
if (strcmp(driver_name, wdriver->driver.name) == 0)
break;
wdriver = NULL;
> + if (!found ||
if (wdriver || ...
And you save a local variable and a couple lines.
...
> static int wmi_dev_probe(struct device *dev)
> {
> struct wmi_block *wblock = dev_to_wblock(dev);
> struct wmi_driver *wdriver =
> container_of(dev->driver, struct wmi_driver, driver);
> int ret = 0;
> + char *buf;
>
> if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
> dev_warn(dev, "failed to enable device -- probing anyway\n");
>
> + /* driver wants a character device made */
> + if (wdriver->file_operations) {
> + buf = kmalloc(strlen(wdriver->driver.name) + 4, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> + strcpy(buf, "wmi/");
> + strcpy(buf + 4, wdriver->driver.name);
sprintf(buf, "wmi/%s", wdriver->driver.name)
--
Darren Hart
VMware Open Source Technology Center
