| Message ID | cd020900567e5c26af29e0735a3945a855d26a30.1496718529.git.dvhart@infradead.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Tue, Jun 6, 2017 at 6:16 AM, Andy Lutomirski <luto@kernel.org> wrote: > Many laptops (and maybe servers?) have embedded WMI Binary MOF metadata. > We do not yet have open-source tools for processing the data, although > one is in the works thanks to Pali: > > https://github.com/pali/bmfdec > > There is currently no interface to get the data in the first place. By > exposing it, we facilitate the development of new tools. My comments below. Overall, FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > +config WMI_BMOF > + tristate "WMI embedded Binary MOF driver" > + depends on ACPI_WMI > + default y Since it can be module it would be better to have more sane default (distros usually prefers modules over built-in). Thus, I would go, for example, with default ACPI_WMI > + ---help--- > + Say Y here if you want to be able to read a firmware-embedded > + WMI Binary MOF data. Using this requires userspace tools and may be > + rather tedious. > + > + To compile this driver as a module, choose M here: the module will > + be called wmi-bmof. > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/types.h> > +#include <linux/input.h> > +#include <linux/input/sparse-keymap.h> > +#include <linux/acpi.h> > +#include <linux/string.h> > +#include <linux/dmi.h> > +#include <linux/wmi.h> > +#include <acpi/video.h> Alphabetical order? Up to you. > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910" > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID); I would gather all MODULE_* together, but it's also matter of taste. > +static ssize_t > +read_bmof(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, > + char *buf, loff_t off, size_t count) > +{ > + struct bmof_priv *priv = > + container_of(attr, struct bmof_priv, bmof_bin_attr); > + > + if (off >= priv->bmofdata->buffer.length) > + return 0; Shouldn't we return an error code here? -ERANGE or alike? > +static int wmi_bmof_probe(struct wmi_device *wdev) > +{ > + int ret; > + > + struct bmof_priv *priv = > + devm_kzalloc(&wdev->dev, sizeof(struct bmof_priv), GFP_KERNEL); I'm not a fan of memory allocation in definition block, so, I would rewrite this struct bmof_priv *priv; int ret; priv = devm_kzalloc(&wdev->dev, sizeof(struct bmof_priv), GFP_KERNEL); (sizeof(*priv) by your choice) > + > + if (!priv) > + return -ENOMEM;
On Monday 05 June 2017 20:16:44 Andy Lutomirski wrote: > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910" > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID); Cannot we generate MODULE_ALIAS from module_wmi_driver()? IIRC it is working for i2c drivers.
On Tue, Jun 06, 2017 at 12:30:38PM +0300, Andy Shevchenko wrote: > On Tue, Jun 6, 2017 at 6:16 AM, Andy Lutomirski <luto@kernel.org> wrote: > > Many laptops (and maybe servers?) have embedded WMI Binary MOF metadata. > > We do not yet have open-source tools for processing the data, although > > one is in the works thanks to Pali: > > > > https://github.com/pali/bmfdec > > > > There is currently no interface to get the data in the first place. By > > exposing it, we facilitate the development of new tools. > > My comments below. > Overall, FWIW, > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > +config WMI_BMOF > > + tristate "WMI embedded Binary MOF driver" > > + depends on ACPI_WMI > > > + default y > > Since it can be module it would be better to have more sane default > (distros usually prefers modules over built-in). > Thus, I would go, for example, with > > default ACPI_WMI Good point, done. > > > + ---help--- > > + Say Y here if you want to be able to read a firmware-embedded > > + WMI Binary MOF data. Using this requires userspace tools and may be > > + rather tedious. > > + > > + To compile this driver as a module, choose M here: the module will > > + be called wmi-bmof. > > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/init.h> > > +#include <linux/slab.h> > > +#include <linux/types.h> > > +#include <linux/input.h> > > +#include <linux/input/sparse-keymap.h> > > +#include <linux/acpi.h> > > +#include <linux/string.h> > > +#include <linux/dmi.h> > > +#include <linux/wmi.h> > > +#include <acpi/video.h> > > Alphabetical order? Up to you. Hrm. There seems to be plenty of similar suggestions on the mailing lists, but nothing documented in coding-style.rst. If this is a thing we are going to ask of our contributors, it should be documented. I'm happy to reorder, would you consider sending the coding-style patch? > > > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910" > > > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID); > > I would gather all MODULE_* together, but it's also matter of taste. > Sure, done. > > +static ssize_t > > +read_bmof(struct file *filp, struct kobject *kobj, > > + struct bin_attribute *attr, > > + char *buf, loff_t off, size_t count) > > +{ > > + struct bmof_priv *priv = > > + container_of(attr, struct bmof_priv, bmof_bin_attr); > > + > > + if (off >= priv->bmofdata->buffer.length) > > + return 0; > > Shouldn't we return an error code here? -ERANGE or alike? > I took some time and compared this with: read(2) lseek(2) fseek(3) memory_read_from_buffer() If offset is <0, we should return EINVAL If offset is >end_of_buffer.... it's not so cut and dry. It is simpler to just return 0, and as far as how it affects usage... returning 0 seems perfectly acceptable for typical read loop usage. As loff_t is a long long, it could conceivably be < 0, so I've added a check for that and return -EINVAL in that case. > > +static int wmi_bmof_probe(struct wmi_device *wdev) > > +{ > > > + int ret; > > + > > + struct bmof_priv *priv = > > + devm_kzalloc(&wdev->dev, sizeof(struct bmof_priv), GFP_KERNEL); > > I'm not a fan of memory allocation in definition block, so, I would rewrite this > > struct bmof_priv *priv; > int ret; > > priv = devm_kzalloc(&wdev->dev, sizeof(struct bmof_priv), GFP_KERNEL); > Agreed, changed. Thanks for the review Andy.
On Tue, Jun 06, 2017 at 12:30:38PM +0300, Andy Shevchenko wrote: > On Tue, Jun 6, 2017 at 6:16 AM, Andy Lutomirski <luto@kernel.org> wrote: > > Many laptops (and maybe servers?) have embedded WMI Binary MOF metadata. > > We do not yet have open-source tools for processing the data, although > > one is in the works thanks to Pali: > > > > https://github.com/pali/bmfdec > > > > There is currently no interface to get the data in the first place. By > > exposing it, we facilitate the development of new tools. > > My comments below. > Overall, FWIW, > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/init.h> > > +#include <linux/slab.h> > > +#include <linux/types.h> > > +#include <linux/input.h> > > +#include <linux/input/sparse-keymap.h> > > +#include <linux/acpi.h> > > +#include <linux/string.h> > > +#include <linux/dmi.h> > > +#include <linux/wmi.h> > > +#include <acpi/video.h> > > Alphabetical order? Up to you. OK, I failed to audit this... lots we don't need in here. The minimum to build is: #include <linux/wmi.h> So assuming this was copy/pasted from another file. Again, no guidance in coding-style.rst on includes. Seems to me we should include what we specifically require, regardless of whether or not another header also happens to include it. We need acpi for example, even though wmi also includes it. We should include modules, even though acpi includes it. We use several other things we aren't including for, like memcpy dev_kzalloc sysfs_create_bin_file So I suggest: #include <linux/acpi.h> #include <linux/device.h> #include <linux/fs.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/string.h> #include <linux/sysfs.h> #include <linux/types.h> #include <linux/wmi.h> Which removes: #include <acpi/video.h> #include <linux/dmi.h> #include <linux/init.h> #include <linux/input.h> #include <linux/input/sparse-keymap.h> #include <linux/slab.h> And adds: #include <linux/device.h> #include <linux/fs.h> #include <linux/sysfs.h>
On Tue, Jun 06, 2017 at 12:04:40PM +0200, Pali Rohár wrote: > On Monday 05 June 2017 20:16:44 Andy Lutomirski wrote: > > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910" > > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID); > > Cannot we generate MODULE_ALIAS from module_wmi_driver()? IIRC it is > working for i2c drivers. I could see this being automated since we always use wmi:GUID, but it isn't currently. Happy to consider it as a follow on. Do you have a specific i2c example you think we should consider following?
On Tue, Jun 6, 2017 at 7:54 PM, Darren Hart <dvhart@infradead.org> wrote: > On Tue, Jun 06, 2017 at 12:30:38PM +0300, Andy Shevchenko wrote: >> On Tue, Jun 6, 2017 at 6:16 AM, Andy Lutomirski <luto@kernel.org> wrote: >> > +#include <linux/kernel.h> >> > +#include <linux/module.h> >> > +#include <linux/init.h> >> > +#include <linux/slab.h> >> > +#include <linux/types.h> >> > +#include <linux/input.h> >> > +#include <linux/input/sparse-keymap.h> >> > +#include <linux/acpi.h> >> > +#include <linux/string.h> >> > +#include <linux/dmi.h> >> > +#include <linux/wmi.h> >> > +#include <acpi/video.h> >> >> Alphabetical order? Up to you. > > OK, I failed to audit this... lots we don't need in here. > > The minimum to build is: > > #include <linux/wmi.h> > > So assuming this was copy/pasted from another file. > Again, no guidance in coding-style.rst on includes. Seems to me we should > include what we specifically require, regardless of whether or not another > header also happens to include it. Usually it's a sane choice. Regarding to order the rationale I see there is easiest way to detect (on the glance) what headers are already there and there is no duplication. I saw in the past few patches to remove header duplication since the original list wasn't in order in the first place. Of course there might be exceptions. > We need acpi for example, even though wmi > also includes it. > > We should include modules, even though acpi includes it. > > We use several other things we aren't including for, like > > memcpy > dev_kzalloc > sysfs_create_bin_file > > So I suggest: > > #include <linux/acpi.h> > #include <linux/device.h> > #include <linux/fs.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/string.h> > #include <linux/sysfs.h> > #include <linux/types.h> > #include <linux/wmi.h> > > Which removes: > #include <acpi/video.h> > #include <linux/dmi.h> > #include <linux/init.h> > #include <linux/input.h> > #include <linux/input/sparse-keymap.h> > #include <linux/slab.h> > > And adds: > #include <linux/device.h> > #include <linux/fs.h> > #include <linux/sysfs.h> Works for me!
On Tuesday 06 June 2017 19:02:01 Darren Hart wrote: > On Tue, Jun 06, 2017 at 12:04:40PM +0200, Pali Rohár wrote: > > On Monday 05 June 2017 20:16:44 Andy Lutomirski wrote: > > > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910" > > > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID); > > > > Cannot we generate MODULE_ALIAS from module_wmi_driver()? IIRC it > > is working for i2c drivers. > > I could see this being automated since we always use wmi:GUID, but it > isn't currently. Happy to consider it as a follow on. > > Do you have a specific i2c example you think we should consider > following? For i2c you can specify in driver code: MODULE_DEVICE_TABLE(i2c, id_table); And it automatically provides (via file.mod.c) all needed MODULE_ALIAS. So when we have wmi_bmof_id_table in driver, cannot we use this? MODULE_DEVICE_TABLE(wmi, wmi_bmof_id_table);
On Mon, Jun 5, 2017 at 8:16 PM, Andy Lutomirski <luto@kernel.org> wrote: > +static ssize_t > +read_bmof(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, > + char *buf, loff_t off, size_t count) > +{ > + struct bmof_priv *priv = > + container_of(attr, struct bmof_priv, bmof_bin_attr); > + > + if (off >= priv->bmofdata->buffer.length) > + return 0; > + > + if (count > priv->bmofdata->buffer.length - off) > + count = priv->bmofdata->buffer.length - off; > + > + memcpy(buf, priv->bmofdata->buffer.pointer + off, count); > + return count; > +} I just discovered simple_read_from_buffer(). I think this whole function could be: struct bmof_priv *priv = ...; return simple_read_from_buffer(buf, count, &off, priv->bmofdata->buffer.pointer, priv->bmofdata->buffer.length); --Andy
Hi Andy,
[auto build test ERROR on platform-drivers-x86/for-next]
[also build test ERROR on v4.12-rc4 next-20170606]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Andy-Lutomirski/platform-x86-wmi-bmof-New-driver-to-expose-embedded-Binary-WMI-MOF-metadata/20170607-062854
base: git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git for-next
config: x86_64-acpi-redef (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> drivers/platform//x86/wmi-bmof.c:28:23: fatal error: linux/wmi.h: No such file or directory
#include <linux/wmi.h>
^
compilation terminated.
vim +28 drivers/platform//x86/wmi-bmof.c
12 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13 * GNU General Public License for more details.
14 */
15
16 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
17
18 #include <linux/kernel.h>
19 #include <linux/module.h>
20 #include <linux/init.h>
21 #include <linux/slab.h>
22 #include <linux/types.h>
23 #include <linux/input.h>
24 #include <linux/input/sparse-keymap.h>
25 #include <linux/acpi.h>
26 #include <linux/string.h>
27 #include <linux/dmi.h>
> 28 #include <linux/wmi.h>
29 #include <acpi/video.h>
30
31 #define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
32 MODULE_ALIAS("wmi:" WMI_BMOF_GUID);
33
34 struct bmof_priv {
35 union acpi_object *bmofdata;
36 struct bin_attribute bmof_bin_attr;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tuesday 06 June 2017 05:16:44 Andy Lutomirski wrote: > Many laptops (and maybe servers?) have embedded WMI Binary MOF > metadata. We do not yet have open-source tools for processing the > data, although one is in the works thanks to Pali: > > https://github.com/pali/bmfdec > > There is currently no interface to get the data in the first place. > By exposing it, we facilitate the development of new tools. > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > Cc: Andy Lutomirski <luto@amacapital.net> > Cc: Mario Limonciello <mario_limonciello@dell.com> > Cc: Pali Rohár <pali.rohar@gmail.com> > Cc: linux-kernel@vger.kernel.org > Cc: platform-driver-x86@vger.kernel.org > Cc: linux-acpi@vger.kernel.org > [dvhart: make sysfs mof binary read only, fixup comment block format] > [dvhart: use bmof terminology and dev_err instead of dev_warn] > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org> > --- > since-v1: > * address Pali's comments: > * update the cover letter for clarity and accuracy > * update mof->bmof and MOF to Binary MOF throughout the patch > * use dev_err instead of dev_warn in wmi_bmof_probe > > > drivers/platform/x86/Kconfig | 12 ++++ > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/wmi-bmof.c | 125 Another suggestion (unrelated to this patch): For working with ACPI-WMI, this binary MOF buffer is not enough. It is needed also content of _WDG buffer. What about exporting it too via sysfs? Probably not part of of wmi-bmof driver, but wmi driver itself.
> -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Monday, June 19, 2017 11:08 AM > To: Andy Lutomirski <luto@kernel.org> > Cc: platform-driver-x86@vger.kernel.org; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; Andy Lutomirski <luto@amacapital.net>; > Limonciello, Mario <Mario_Limonciello@Dell.com>; Rafael Wysocki > <rjw@rjwysocki.net>; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org > Subject: Re: [PATCH v2] platform/x86: wmi-bmof: New driver to expose embedded > Binary WMI MOF metadata > > On Tuesday 06 June 2017 05:16:44 Andy Lutomirski wrote: > > Many laptops (and maybe servers?) have embedded WMI Binary MOF > > metadata. We do not yet have open-source tools for processing the > > data, although one is in the works thanks to Pali: > > > > https://github.com/pali/bmfdec > > > > There is currently no interface to get the data in the first place. > > By exposing it, we facilitate the development of new tools. > > > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > > Cc: Andy Lutomirski <luto@amacapital.net> > > Cc: Mario Limonciello <mario_limonciello@dell.com> > > Cc: Pali Rohár <pali.rohar@gmail.com> > > Cc: linux-kernel@vger.kernel.org > > Cc: platform-driver-x86@vger.kernel.org > > Cc: linux-acpi@vger.kernel.org > > [dvhart: make sysfs mof binary read only, fixup comment block format] > > [dvhart: use bmof terminology and dev_err instead of dev_warn] > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org> > > --- > > since-v1: > > * address Pali's comments: > > * update the cover letter for clarity and accuracy > > * update mof->bmof and MOF to Binary MOF throughout the patch > > * use dev_err instead of dev_warn in wmi_bmof_probe > > > > > > drivers/platform/x86/Kconfig | 12 ++++ > > drivers/platform/x86/Makefile | 1 + > > drivers/platform/x86/wmi-bmof.c | 125 > > Another suggestion (unrelated to this patch): For working with ACPI-WMI, > this binary MOF buffer is not enough. It is needed also content of _WDG > buffer. What about exporting it too via sysfs? Probably not part of of > wmi-bmof driver, but wmi driver itself. > I think this depends upon how the userspace access to WMI methods gets implemented, no? If userpsace access to WMI methods show up as /dev/wmi-$GUID-$INSTANCE and those internally to the kernel map to the proper ASL methods for example, what you get from wmi-bmof should be enough shouldn't it?
On Monday 19 June 2017 18:13:13 Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Pali Rohár [mailto:pali.rohar@gmail.com] > > Sent: Monday, June 19, 2017 11:08 AM > > To: Andy Lutomirski <luto@kernel.org> > > Cc: platform-driver-x86@vger.kernel.org; Andy Shevchenko > > <andriy.shevchenko@linux.intel.com>; Andy Lutomirski > > <luto@amacapital.net>; Limonciello, Mario > > <Mario_Limonciello@Dell.com>; Rafael Wysocki <rjw@rjwysocki.net>; > > linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org Subject: > > Re: [PATCH v2] platform/x86: wmi-bmof: New driver to expose > > embedded Binary WMI MOF metadata > > > > On Tuesday 06 June 2017 05:16:44 Andy Lutomirski wrote: > > > Many laptops (and maybe servers?) have embedded WMI Binary MOF > > > metadata. We do not yet have open-source tools for processing the > > > > > > data, although one is in the works thanks to Pali: > > > https://github.com/pali/bmfdec > > > > > > There is currently no interface to get the data in the first > > > place. By exposing it, we facilitate the development of new > > > tools. > > > > > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > > > Cc: Andy Lutomirski <luto@amacapital.net> > > > Cc: Mario Limonciello <mario_limonciello@dell.com> > > > Cc: Pali Rohár <pali.rohar@gmail.com> > > > Cc: linux-kernel@vger.kernel.org > > > Cc: platform-driver-x86@vger.kernel.org > > > Cc: linux-acpi@vger.kernel.org > > > [dvhart: make sysfs mof binary read only, fixup comment block > > > format] [dvhart: use bmof terminology and dev_err instead of > > > dev_warn] Acked-by: Rafael J. Wysocki > > > <rafael.j.wysocki@intel.com> Signed-off-by: Darren Hart (VMware) > > > <dvhart@infradead.org> --- > > > > > > since-v1: > > > * address Pali's comments: > > > * update the cover letter for clarity and accuracy > > > * update mof->bmof and MOF to Binary MOF throughout the patch > > > * use dev_err instead of dev_warn in wmi_bmof_probe > > > > > > drivers/platform/x86/Kconfig | 12 ++++ > > > drivers/platform/x86/Makefile | 1 + > > > drivers/platform/x86/wmi-bmof.c | 125 > > > > Another suggestion (unrelated to this patch): For working with > > ACPI-WMI, this binary MOF buffer is not enough. It is needed also > > content of _WDG buffer. What about exporting it too via sysfs? > > Probably not part of of wmi-bmof driver, but wmi driver itself. > > I think this depends upon how the userspace access to WMI methods > gets implemented, no? If userpsace access to WMI methods show up as > /dev/wmi-$GUID-$INSTANCE and those internally to the kernel map to > the proper ASL methods for example, what you get from wmi-bmof > should be enough shouldn't it? Ok. Such interface for userspace application could be enough. But for debugging purposes or writing new WMI driver it is needed to have both _WDG + BMOF.
On Mon, Jun 19, 2017 at 9:19 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Monday 19 June 2017 18:13:13 Mario.Limonciello@dell.com wrote: >> > -----Original Message----- >> > From: Pali Rohár [mailto:pali.rohar@gmail.com] >> > Sent: Monday, June 19, 2017 11:08 AM >> > To: Andy Lutomirski <luto@kernel.org> >> > Cc: platform-driver-x86@vger.kernel.org; Andy Shevchenko >> > <andriy.shevchenko@linux.intel.com>; Andy Lutomirski >> > <luto@amacapital.net>; Limonciello, Mario >> > <Mario_Limonciello@Dell.com>; Rafael Wysocki <rjw@rjwysocki.net>; >> > linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org Subject: >> > Re: [PATCH v2] platform/x86: wmi-bmof: New driver to expose >> > embedded Binary WMI MOF metadata >> > >> > On Tuesday 06 June 2017 05:16:44 Andy Lutomirski wrote: >> > > Many laptops (and maybe servers?) have embedded WMI Binary MOF >> > > metadata. We do not yet have open-source tools for processing the >> > > >> > > data, although one is in the works thanks to Pali: >> > > https://github.com/pali/bmfdec >> > > >> > > There is currently no interface to get the data in the first >> > > place. By exposing it, we facilitate the development of new >> > > tools. >> > > >> > > Signed-off-by: Andy Lutomirski <luto@kernel.org> >> > > Cc: Andy Lutomirski <luto@amacapital.net> >> > > Cc: Mario Limonciello <mario_limonciello@dell.com> >> > > Cc: Pali Rohár <pali.rohar@gmail.com> >> > > Cc: linux-kernel@vger.kernel.org >> > > Cc: platform-driver-x86@vger.kernel.org >> > > Cc: linux-acpi@vger.kernel.org >> > > [dvhart: make sysfs mof binary read only, fixup comment block >> > > format] [dvhart: use bmof terminology and dev_err instead of >> > > dev_warn] Acked-by: Rafael J. Wysocki >> > > <rafael.j.wysocki@intel.com> Signed-off-by: Darren Hart (VMware) >> > > <dvhart@infradead.org> --- >> > > >> > > since-v1: >> > > * address Pali's comments: >> > > * update the cover letter for clarity and accuracy >> > > * update mof->bmof and MOF to Binary MOF throughout the patch >> > > * use dev_err instead of dev_warn in wmi_bmof_probe >> > > >> > > drivers/platform/x86/Kconfig | 12 ++++ >> > > drivers/platform/x86/Makefile | 1 + >> > > drivers/platform/x86/wmi-bmof.c | 125 >> > >> > Another suggestion (unrelated to this patch): For working with >> > ACPI-WMI, this binary MOF buffer is not enough. It is needed also >> > content of _WDG buffer. What about exporting it too via sysfs? >> > Probably not part of of wmi-bmof driver, but wmi driver itself. >> >> I think this depends upon how the userspace access to WMI methods >> gets implemented, no? If userpsace access to WMI methods show up as >> /dev/wmi-$GUID-$INSTANCE and those internally to the kernel map to >> the proper ASL methods for example, what you get from wmi-bmof >> should be enough shouldn't it? > > Ok. Such interface for userspace application could be enough. > > But for debugging purposes or writing new WMI driver it is needed to > have both _WDG + BMOF. With the busification patches applied, all the _WDG data should be available in sysfs in parsed form. I have no particular objection to adding a new sysfs for debugfs file to give the raw binary blob, but I'm not sure it's needed. --Andy
On Monday 19 June 2017 09:23:45 Andy Lutomirski wrote: > On Mon, Jun 19, 2017 at 9:19 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > > On Monday 19 June 2017 18:13:13 Mario.Limonciello@dell.com wrote: > >> > -----Original Message----- > >> > From: Pali Rohár [mailto:pali.rohar@gmail.com] > >> > Sent: Monday, June 19, 2017 11:08 AM > >> > To: Andy Lutomirski <luto@kernel.org> > >> > Cc: platform-driver-x86@vger.kernel.org; Andy Shevchenko > >> > <andriy.shevchenko@linux.intel.com>; Andy Lutomirski > >> > <luto@amacapital.net>; Limonciello, Mario > >> > <Mario_Limonciello@Dell.com>; Rafael Wysocki <rjw@rjwysocki.net>; > >> > linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org Subject: > >> > Re: [PATCH v2] platform/x86: wmi-bmof: New driver to expose > >> > embedded Binary WMI MOF metadata > >> > > >> > On Tuesday 06 June 2017 05:16:44 Andy Lutomirski wrote: > >> > > Many laptops (and maybe servers?) have embedded WMI Binary MOF > >> > > metadata. We do not yet have open-source tools for processing the > >> > > > >> > > data, although one is in the works thanks to Pali: > >> > > https://github.com/pali/bmfdec > >> > > > >> > > There is currently no interface to get the data in the first > >> > > place. By exposing it, we facilitate the development of new > >> > > tools. > >> > > > >> > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > >> > > Cc: Andy Lutomirski <luto@amacapital.net> > >> > > Cc: Mario Limonciello <mario_limonciello@dell.com> > >> > > Cc: Pali Rohár <pali.rohar@gmail.com> > >> > > Cc: linux-kernel@vger.kernel.org > >> > > Cc: platform-driver-x86@vger.kernel.org > >> > > Cc: linux-acpi@vger.kernel.org > >> > > [dvhart: make sysfs mof binary read only, fixup comment block > >> > > format] [dvhart: use bmof terminology and dev_err instead of > >> > > dev_warn] Acked-by: Rafael J. Wysocki > >> > > <rafael.j.wysocki@intel.com> Signed-off-by: Darren Hart (VMware) > >> > > <dvhart@infradead.org> --- > >> > > > >> > > since-v1: > >> > > * address Pali's comments: > >> > > * update the cover letter for clarity and accuracy > >> > > * update mof->bmof and MOF to Binary MOF throughout the patch > >> > > * use dev_err instead of dev_warn in wmi_bmof_probe > >> > > > >> > > drivers/platform/x86/Kconfig | 12 ++++ > >> > > drivers/platform/x86/Makefile | 1 + > >> > > drivers/platform/x86/wmi-bmof.c | 125 > >> > > >> > Another suggestion (unrelated to this patch): For working with > >> > ACPI-WMI, this binary MOF buffer is not enough. It is needed also > >> > content of _WDG buffer. What about exporting it too via sysfs? > >> > Probably not part of of wmi-bmof driver, but wmi driver itself. > >> > >> I think this depends upon how the userspace access to WMI methods > >> gets implemented, no? If userpsace access to WMI methods show up as > >> /dev/wmi-$GUID-$INSTANCE and those internally to the kernel map to > >> the proper ASL methods for example, what you get from wmi-bmof > >> should be enough shouldn't it? > > > > Ok. Such interface for userspace application could be enough. > > > > But for debugging purposes or writing new WMI driver it is needed to > > have both _WDG + BMOF. > > With the busification patches applied, all the _WDG data should be > available in sysfs in parsed form. I have no particular objection to > adding a new sysfs for debugfs file to give the raw binary blob, but > I'm not sure it's needed. I'm thinking about writing userspace tool which print information mapping from MOF namespace/class/method name to ACPI method. Ideally if it could parse all WMI data on its own and not depends on new parsed /sys/ tree structure. From _WDG it is needed to know ACPI ID or WMI event IDs.
On Tuesday 06 June 2017 22:50:52 Pali Rohár wrote: > On Tuesday 06 June 2017 19:02:01 Darren Hart wrote: > > On Tue, Jun 06, 2017 at 12:04:40PM +0200, Pali Rohár wrote: > > > On Monday 05 June 2017 20:16:44 Andy Lutomirski wrote: > > > > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910" > > > > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID); > > > > > > Cannot we generate MODULE_ALIAS from module_wmi_driver()? IIRC it > > > is working for i2c drivers. > > > > I could see this being automated since we always use wmi:GUID, but it > > isn't currently. Happy to consider it as a follow on. > > > > Do you have a specific i2c example you think we should consider > > following? > > For i2c you can specify in driver code: > > MODULE_DEVICE_TABLE(i2c, id_table); > > And it automatically provides (via file.mod.c) all needed MODULE_ALIAS. > > So when we have wmi_bmof_id_table in driver, cannot we use this? > > MODULE_DEVICE_TABLE(wmi, wmi_bmof_id_table); Just reminder for above idea ↑↑↑
On Tuesday 04 July 2017 15:28:19 Pali Rohár wrote: > On Tuesday 06 June 2017 22:50:52 Pali Rohár wrote: > > On Tuesday 06 June 2017 19:02:01 Darren Hart wrote: > > > On Tue, Jun 06, 2017 at 12:04:40PM +0200, Pali Rohár wrote: > > > > On Monday 05 June 2017 20:16:44 Andy Lutomirski wrote: > > > > > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910" > > > > > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID); > > > > > > > > Cannot we generate MODULE_ALIAS from module_wmi_driver()? IIRC it > > > > is working for i2c drivers. > > > > > > I could see this being automated since we always use wmi:GUID, but it > > > isn't currently. Happy to consider it as a follow on. > > > > > > Do you have a specific i2c example you think we should consider > > > following? > > > > For i2c you can specify in driver code: > > > > MODULE_DEVICE_TABLE(i2c, id_table); > > > > And it automatically provides (via file.mod.c) all needed MODULE_ALIAS. > > > > So when we have wmi_bmof_id_table in driver, cannot we use this? > > > > MODULE_DEVICE_TABLE(wmi, wmi_bmof_id_table); > > Just reminder for above idea ↑↑↑ Hi! This email is some months old, so do not know if something was implemented or not. Does somebody know?
On Thu, Nov 23, 2017 at 6:39 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Tuesday 04 July 2017 15:28:19 Pali Rohár wrote: >> On Tuesday 06 June 2017 22:50:52 Pali Rohár wrote: >> > On Tuesday 06 June 2017 19:02:01 Darren Hart wrote: >> > > On Tue, Jun 06, 2017 at 12:04:40PM +0200, Pali Rohár wrote: >> > > > On Monday 05 June 2017 20:16:44 Andy Lutomirski wrote: >> > > > > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910" >> > > > > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID); >> > > > >> > > > Cannot we generate MODULE_ALIAS from module_wmi_driver()? IIRC it >> > > > is working for i2c drivers. >> > > >> > > I could see this being automated since we always use wmi:GUID, but it >> > > isn't currently. Happy to consider it as a follow on. >> > > >> > > Do you have a specific i2c example you think we should consider >> > > following? >> > >> > For i2c you can specify in driver code: >> > >> > MODULE_DEVICE_TABLE(i2c, id_table); >> > >> > And it automatically provides (via file.mod.c) all needed MODULE_ALIAS. >> > >> > So when we have wmi_bmof_id_table in driver, cannot we use this? >> > >> > MODULE_DEVICE_TABLE(wmi, wmi_bmof_id_table); >> >> Just reminder for above idea ↑↑↑ > > Hi! This email is some months old, so do not know if something was > implemented or not. Does somebody know? > I don't think so. I was way too lazy.
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 49a1d01..6ebe393 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -656,6 +656,18 @@ config ACPI_WMI It is safe to enable this driver even if your DSDT doesn't define any ACPI-WMI devices. +config WMI_BMOF + tristate "WMI embedded Binary MOF driver" + depends on ACPI_WMI + default y + ---help--- + Say Y here if you want to be able to read a firmware-embedded + WMI Binary MOF data. Using this requires userspace tools and may be + rather tedious. + + To compile this driver as a module, choose M here: the module will + be called wmi-bmof. + config MSI_WMI tristate "MSI WMI extras" depends on ACPI_WMI diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 652d7c8..6a1063e 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -38,6 +38,7 @@ obj-$(CONFIG_MSI_WMI) += msi-wmi.o obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o obj-$(CONFIG_SURFACE3_WMI) += surface3-wmi.o obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o +obj-$(CONFIG_WMI_BMOF) += wmi-bmof.o # toshiba_acpi must link after wmi to ensure that wmi devices are found # before toshiba_acpi initializes diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c new file mode 100644 index 0000000..e1c0963 --- /dev/null +++ b/drivers/platform/x86/wmi-bmof.c @@ -0,0 +1,125 @@ +/* + * WMI embedded Binary MOF driver + * + * Copyright (c) 2015 Andrew Lutomirski + * + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/input.h> +#include <linux/input/sparse-keymap.h> +#include <linux/acpi.h> +#include <linux/string.h> +#include <linux/dmi.h> +#include <linux/wmi.h> +#include <acpi/video.h> + +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910" +MODULE_ALIAS("wmi:" WMI_BMOF_GUID); + +struct bmof_priv { + union acpi_object *bmofdata; + struct bin_attribute bmof_bin_attr; +}; + +static ssize_t +read_bmof(struct file *filp, struct kobject *kobj, + struct bin_attribute *attr, + char *buf, loff_t off, size_t count) +{ + struct bmof_priv *priv = + container_of(attr, struct bmof_priv, bmof_bin_attr); + + if (off >= priv->bmofdata->buffer.length) + return 0; + + if (count > priv->bmofdata->buffer.length - off) + count = priv->bmofdata->buffer.length - off; + + memcpy(buf, priv->bmofdata->buffer.pointer + off, count); + return count; +} + +static int wmi_bmof_probe(struct wmi_device *wdev) +{ + int ret; + + struct bmof_priv *priv = + devm_kzalloc(&wdev->dev, sizeof(struct bmof_priv), GFP_KERNEL); + + if (!priv) + return -ENOMEM; + + dev_set_drvdata(&wdev->dev, priv); + + priv->bmofdata = wmidev_block_query(wdev, 0); + if (!priv->bmofdata) { + dev_err(&wdev->dev, "failed to read Binary MOF\n"); + return -EIO; + } + + if (priv->bmofdata->type != ACPI_TYPE_BUFFER) { + dev_err(&wdev->dev, "Binary MOF is not a buffer\n"); + ret = -EIO; + goto err_free; + } + + sysfs_bin_attr_init(&priv->bmof_bin_attr); + priv->bmof_bin_attr.attr.name = "bmof"; + priv->bmof_bin_attr.attr.mode = 0400; + priv->bmof_bin_attr.read = read_bmof; + priv->bmof_bin_attr.size = priv->bmofdata->buffer.length; + + ret = sysfs_create_bin_file(&wdev->dev.kobj, &priv->bmof_bin_attr); + if (ret) + goto err_free; + + return 0; + + err_free: + kfree(priv->bmofdata); + return ret; +} + +static int wmi_bmof_remove(struct wmi_device *wdev) +{ + struct bmof_priv *priv = dev_get_drvdata(&wdev->dev); + + sysfs_remove_bin_file(&wdev->dev.kobj, &priv->bmof_bin_attr); + kfree(priv->bmofdata); + return 0; +} + +static const struct wmi_device_id wmi_bmof_id_table[] = { + { .guid_string = WMI_BMOF_GUID }, + { }, +}; + +static struct wmi_driver wmi_bmof_driver = { + .driver = { + .name = "wmi-bmof", + }, + .probe = wmi_bmof_probe, + .remove = wmi_bmof_remove, + .id_table = wmi_bmof_id_table, +}; + +module_wmi_driver(wmi_bmof_driver); + +MODULE_AUTHOR("Andrew Lutomirski <luto@kernel.org>"); +MODULE_DESCRIPTION("WMI embedded Binary MOF driver"); +MODULE_LICENSE("GPL");