| Message ID | 53c8c8aa295046d3a10b2bc8aeb6b610@ausx13mpc124.AMER.DELL.COM (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Darren Hart |
| Headers | show |
Just my 2c, I like this simplification Mario. Reviewed-by: Edward O'Callaghan <quasisec@google.com> On Thu, Oct 19, 2017 at 9:27 AM, <Mario.Limonciello@dell.com> wrote: >> -----Original Message----- >> From: Limonciello, Mario >> Sent: Wednesday, October 18, 2017 8:56 AM >> To: 'Pali Rohár' <pali.rohar@gmail.com>; Greg KH <greg@kroah.com>; Alan Cox >> <gnomes@lxorguk.ukuu.org.uk> >> Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; >> LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; Andy >> Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net; >> mjg59@google.com; hch@lst.de >> Subject: RE: [PATCH v9 17/17] tools/wmi: add a sample for dell smbios >> communication over WMI >> >> > -----Original Message----- >> > From: Pali Rohár [mailto:pali.rohar@gmail.com] >> > Sent: Wednesday, October 18, 2017 2:29 AM >> > To: Greg KH <greg@kroah.com>; Alan Cox <gnomes@lxorguk.ukuu.org.uk> >> > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; >> > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; >> Andy >> > Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net; >> > mjg59@google.com; hch@lst.de; Limonciello, Mario >> > <Mario_Limonciello@Dell.com> >> > Subject: Re: [PATCH v9 17/17] tools/wmi: add a sample for dell smbios >> > communication over WMI >> > >> > On Tuesday 17 October 2017 13:22:01 Mario Limonciello wrote: >> > > diff --git a/tools/wmi/dell-smbios-example.c b/tools/wmi/dell-smbios- >> example.c >> > > new file mode 100644 >> > > index 000000000000..69c4dd9c6056 >> > > --- /dev/null >> > > +++ b/tools/wmi/dell-smbios-example.c >> > > @@ -0,0 +1,214 @@ >> > > +/* >> > > + * Sample application for SMBIOS communication over WMI interface >> > > + * Performs the following: >> > > + * - Simple class/select lookup for TPM information >> > > + * - Simple query of known tokens and their values >> > > + * - Simple activation of a token >> > > + * >> > > + * Copyright (C) 2017 Dell, Inc. >> > > + * >> > > + * This program is free software; you can redistribute it and/or modify >> > > + * it under the terms of the GNU General Public License version 2 as >> > > + * published by the Free Software Foundation. >> > > + */ >> > > + >> > > +#include <errno.h> >> > > +#include <fcntl.h> >> > > +#include <stdio.h> >> > > +#include <stdlib.h> >> > > +#include <sys/ioctl.h> >> > > +#include <unistd.h> >> > > + >> > > +/* if uapi header isn't installed, this might not yet exist */ >> > > +#ifndef __packed >> > > +#define __packed __attribute__((packed)) >> > > +#endif >> > > +#include <linux/wmi.h> >> > > + >> > > +/* It would be better to discover these using udev, but for a simple >> > > + * application they're hardcoded >> > > + */ >> > > +static const char *ioctl_devfs = "/dev/wmi/dell-smbios"; >> > > +static const char *token_sysfs = >> > > + "/sys/bus/platform/devices/dell-smbios.0/tokens"; >> > > +static const char *buffer_sysfs = >> > > + "/sys/bus/wmi/devices/A80593CE-A997-11DA-B012- >> > B622A1EF5492/required_buffer_size"; >> > >> > Greg, Alan, could userspace expects those paths to be part of kernel >> > <--> userspace ABI? Looking e.g. at "dell-smbios.0" name and I'm not >> > sure if this is something which is going to be stable between kernel >> > versions and forever as part of ABI. >> >> In my sample application to be distributed with the kernel these are >> hardcoded paths, but if more dependencies were used, I would >> expect all 3 of these paths to be discovered using udev. >> I do include a comment for that point specifically. >> >> > >> > Also if everything is part of smbios API, would not it better to provide >> > everything via IOCTL over /dev/wmi/dell-smbios? I think this code is too >> > complicated, just because for correct IOCTL buffer size it needs to read >> > other properties via sysfs, etc... For me it looks like that it is not a >> > good API for userspace developers. >> > >> > -- >> >> This does give me an idea, how about a read on the character device >> will return required buffer size instead of needing to find a sysfs >> attribute? This seems more intuitive to me. > > So as to not send the whole series again only to get this idea shot down, > this is what it looks like (minus documentation updates). > Thoughts? > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index c7de80f96183..922a87d7cf34 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -799,23 +799,12 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver) > > return 0; > } > - > -static long match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg, > - int compat) > +static int wmi_char_open(struct inode *inode, struct file *filp) > { > - struct wmi_ioctl_buffer __user *input = > - (struct wmi_ioctl_buffer __user *) arg; > + const char *driver_name = filp->f_path.dentry->d_iname; > struct wmi_driver *wdriver = NULL; > struct wmi_block *wblock = NULL; > struct wmi_block *next = NULL; > - const char *driver_name; > - u64 size; > - int ret; > - > - if (_IOC_TYPE(cmd) != WMI_IOC) > - return -ENOTTY; > - > - driver_name = filp->f_path.dentry->d_iname; > > list_for_each_entry_safe(wblock, next, &wmi_block_list, list) { > wdriver = container_of(wblock->dev.dev.driver, > @@ -826,6 +815,52 @@ static long match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg, > break; > } > > + if (!wdriver) > + return -ENODEV; > + > + filp->private_data = wblock; > + nonseekable_open(inode, filp); > + return 0; > +} > + > +static int wmi_char_release(struct inode *inode, struct file *filp) > +{ > + return 0; > +} > + > +static ssize_t wmi_char_read(struct file *filp, char __user *buffer, > + size_t length, loff_t *offset) > +{ > + struct wmi_block *wblock = filp->private_data; > + size_t count; > + > + if (*offset != 0) > + return 0; > + > + count = sizeof(wblock->req_buf_size); > + if (copy_to_user(buffer, &wblock->req_buf_size, count)) > + return -EFAULT; > + > + *offset = count; > + return count; > +} > + > +static long match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg, > + int compat) > +{ > + struct wmi_ioctl_buffer __user *input = > + (struct wmi_ioctl_buffer __user *) arg; > + struct wmi_block *wblock = filp->private_data; > + struct wmi_driver *wdriver = NULL; > + u64 size; > + int ret; > + > + if (_IOC_TYPE(cmd) != WMI_IOC) > + return -ENOTTY; > + > + wdriver = container_of(wblock->dev.dev.driver, > + struct wmi_driver, driver); > + > if (!wdriver) > return -ENODEV; > > @@ -886,6 +921,9 @@ static long wmi_compat_ioctl(struct file *filp, unsigned int cmd, > > static const struct file_operations wmi_fops = { > .owner = THIS_MODULE, > + .read = wmi_char_read, > + .open = wmi_char_open, > + .release = wmi_char_release, > .unlocked_ioctl = wmi_unlocked_ioctl, > #ifdef CONFIG_COMPAT > .compat_ioctl = wmi_compat_ioctl, > diff --git a/tools/wmi/dell-smbios-example.c b/tools/wmi/dell-smbios-example.c > index 69c4dd9c6056..a5f97647c9c5 100644 > --- a/tools/wmi/dell-smbios-example.c > +++ b/tools/wmi/dell-smbios-example.c > @@ -31,8 +31,6 @@ > static const char *ioctl_devfs = "/dev/wmi/dell-smbios"; > static const char *token_sysfs = > "/sys/bus/platform/devices/dell-smbios.0/tokens"; > -static const char *buffer_sysfs = > - "/sys/bus/wmi/devices/A80593CE-A997-11DA-B012-B622A1EF5492/required_buffer_size"; > > static void show_buffer(struct dell_wmi_smbios_buffer *buffer) > { > @@ -147,15 +145,13 @@ static int activate_token(struct dell_wmi_smbios_buffer *buffer, > > static int query_buffer_size(__u64 *buffer_size) > { > - char buf[4096]; > FILE *f; > > - f = fopen(buffer_sysfs, "rb"); > + f = fopen(ioctl_devfs, "rb"); > if (!f) > return -EINVAL; > - fread(buf, 1, 4096, f); > + fread(buffer_size, sizeof(__u64), 1, f); > fclose(f); > - *buffer_size = strtol(buf, NULL, 10); > return EXIT_SUCCESS; > } > > >> >> Token information is provided over sysfs for multiple reasons. >> 1) It's applicable to all dispatchers. Even if the WMI dispatcher wasn't >> used it's useful for userspace to query through. For example the SMI call >> to get tokens in libsmbios can be simplified to just read sysfs files. >> >> 2) it's information not coming from ACPI-WMI. This series is setting >> precedent for how to interact with ACPI-WMI methods in userspace. >> putting in random data on the IOCTL that is not used in the ACPI-WMI >> method or provided by the WMI bus doesn't fit. >> >> 3) It is static information that won't change until you reboot.
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index c7de80f96183..922a87d7cf34 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -799,23 +799,12 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver) return 0; } - -static long match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg, - int compat) +static int wmi_char_open(struct inode *inode, struct file *filp) { - struct wmi_ioctl_buffer __user *input = - (struct wmi_ioctl_buffer __user *) arg; + const char *driver_name = filp->f_path.dentry->d_iname; struct wmi_driver *wdriver = NULL; struct wmi_block *wblock = NULL; struct wmi_block *next = NULL; - const char *driver_name; - u64 size; - int ret; - - if (_IOC_TYPE(cmd) != WMI_IOC) - return -ENOTTY; - - driver_name = filp->f_path.dentry->d_iname; list_for_each_entry_safe(wblock, next, &wmi_block_list, list) { wdriver = container_of(wblock->dev.dev.driver, @@ -826,6 +815,52 @@ static long match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg, break; } + if (!wdriver) + return -ENODEV; + + filp->private_data = wblock; + nonseekable_open(inode, filp); + return 0; +} + +static int wmi_char_release(struct inode *inode, struct file *filp) +{ + return 0; +} + +static ssize_t wmi_char_read(struct file *filp, char __user *buffer, + size_t length, loff_t *offset) +{ + struct wmi_block *wblock = filp->private_data; + size_t count; + + if (*offset != 0) + return 0; + + count = sizeof(wblock->req_buf_size); + if (copy_to_user(buffer, &wblock->req_buf_size, count)) + return -EFAULT; + + *offset = count; + return count; +} + +static long match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg, + int compat) +{ + struct wmi_ioctl_buffer __user *input = + (struct wmi_ioctl_buffer __user *) arg; + struct wmi_block *wblock = filp->private_data; + struct wmi_driver *wdriver = NULL; + u64 size; + int ret; + + if (_IOC_TYPE(cmd) != WMI_IOC) + return -ENOTTY; + + wdriver = container_of(wblock->dev.dev.driver, + struct wmi_driver, driver); + if (!wdriver) return -ENODEV; @@ -886,6 +921,9 @@ static long wmi_compat_ioctl(struct file *filp, unsigned int cmd, static const struct file_operations wmi_fops = { .owner = THIS_MODULE, + .read = wmi_char_read, + .open = wmi_char_open, + .release = wmi_char_release, .unlocked_ioctl = wmi_unlocked_ioctl, #ifdef CONFIG_COMPAT .compat_ioctl = wmi_compat_ioctl, diff --git a/tools/wmi/dell-smbios-example.c b/tools/wmi/dell-smbios-example.c index 69c4dd9c6056..a5f97647c9c5 100644 --- a/tools/wmi/dell-smbios-example.c +++ b/tools/wmi/dell-smbios-example.c @@ -31,8 +31,6 @@ static const char *ioctl_devfs = "/dev/wmi/dell-smbios"; static const char *token_sysfs = "/sys/bus/platform/devices/dell-smbios.0/tokens"; -static const char *buffer_sysfs = - "/sys/bus/wmi/devices/A80593CE-A997-11DA-B012-B622A1EF5492/required_buffer_size"; static void show_buffer(struct dell_wmi_smbios_buffer *buffer) { @@ -147,15 +145,13 @@ static int activate_token(struct dell_wmi_smbios_buffer *buffer, static int query_buffer_size(__u64 *buffer_size) { - char buf[4096]; FILE *f; - f = fopen(buffer_sysfs, "rb"); + f = fopen(ioctl_devfs, "rb"); if (!f) return -EINVAL; - fread(buf, 1, 4096, f); + fread(buffer_size, sizeof(__u64), 1, f); fclose(f); - *buffer_size = strtol(buf, NULL, 10); return EXIT_SUCCESS; }