Just my 2c, I like this simplification Mario. Reviewed-by: Edward O'Callaghan <quasisec@xxxxxxxxxx> On Thu, Oct 19, 2017 at 9:27 AM, <Mario.Limonciello@xxxxxxxx> wrote: >> -----Original Message----- >> From: Limonciello, Mario >> Sent: Wednesday, October 18, 2017 8:56 AM >> To: 'Pali Rohár' <pali.rohar@xxxxxxxxx>; Greg KH <greg@xxxxxxxxx>; Alan Cox >> <gnomes@xxxxxxxxxxxxxxxxxxx> >> Cc: dvhart@xxxxxxxxxxxxx; Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; >> LKML <linux-kernel@xxxxxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx; Andy >> Lutomirski <luto@xxxxxxxxxx>; quasisec@xxxxxxxxxx; rjw@xxxxxxxxxxxxx; >> mjg59@xxxxxxxxxx; hch@xxxxxx >> 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@xxxxxxxxx] >> > Sent: Wednesday, October 18, 2017 2:29 AM >> > To: Greg KH <greg@xxxxxxxxx>; Alan Cox <gnomes@xxxxxxxxxxxxxxxxxxx> >> > Cc: dvhart@xxxxxxxxxxxxx; Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; >> > LKML <linux-kernel@xxxxxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx; >> Andy >> > Lutomirski <luto@xxxxxxxxxx>; quasisec@xxxxxxxxxx; rjw@xxxxxxxxxxxxx; >> > mjg59@xxxxxxxxxx; hch@xxxxxx; Limonciello, Mario >> > <Mario_Limonciello@xxxxxxxx> >> > 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.
