| Message ID | e6df9d038af0a0ce4d4119395aaea58366ade26d.1501601667.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Tuesday 01 August 2017 08:37:28 Andy Lutomirski wrote: > This adds a sysfs binary attribute 'wdg' on the bus device > (i.e. /sys/class/wmi_bus/wmi_bus-*/wdg) that contains the raw _WDG > data from ACPI. This can be used along with the wmi-bmof driver to > decode the raw interface data from ACPI. > > There is very little new information here, as most of the contents > were already exposed in sysfs attributes on the wmi devices. > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > drivers/platform/x86/wmi.c | 63 ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 61 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index 1a764e311e11..37ca56aa6025 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -76,6 +76,11 @@ struct wmi_block { > bool read_takes_no_args; > }; > > +struct wmi_bus_priv { > + union acpi_object *wdg_obj; > + struct bin_attribute wdg_bin_attr; > + bool wdg_bin_attr_created; > +}; > > /* > * If the GUID data block is marked as expensive, we must enable and > @@ -938,6 +943,7 @@ static bool guid_already_parsed(struct acpi_device *device, > */ > static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device) > { > + struct wmi_bus_priv *priv = dev_get_drvdata(wmi_bus_dev); > struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL}; > const struct guid_block *gblock; > struct wmi_block *wblock, *next; > @@ -1017,11 +1023,35 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device) > } > } > > + priv->wdg_obj = obj; > + return 0; > + > out_free_pointer: > kfree(out.pointer); > return retval; > } > > +static ssize_t > +read_wdg(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, > + char *buf, loff_t off, size_t count) > +{ > + struct wmi_bus_priv *priv = > + container_of(attr, struct wmi_bus_priv, wdg_bin_attr); > + > + if (off < 0) > + return -EINVAL; > + > + if (off >= priv->wdg_obj->buffer.length) > + return 0; > + > + if (count > priv->wdg_obj->buffer.length - off) > + count = priv->wdg_obj->buffer.length - off; > + > + memcpy(buf, priv->wdg_obj->buffer.pointer + off, count); > + return count; > +} > + > /* > * WMI can have EmbeddedControl access regions. In which case, we just want to > * hand these off to the EC driver. > @@ -1138,6 +1168,8 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event, > > static int acpi_wmi_remove(struct platform_device *device) > { > + struct device *wmi_bus_dev = dev_get_drvdata(&device->dev); > + struct wmi_bus_priv *priv = dev_get_drvdata(wmi_bus_dev); > struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev); > > acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY, > @@ -1145,7 +1177,13 @@ static int acpi_wmi_remove(struct platform_device *device) > acpi_remove_address_space_handler(acpi_device->handle, > ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler); > wmi_free_devices(acpi_device); > - device_unregister((struct device *)dev_get_drvdata(&device->dev)); > + > + if (priv->wdg_bin_attr_created) > + sysfs_remove_bin_file(&wmi_bus_dev->kobj, > + &priv->wdg_bin_attr); > + kfree(priv->wdg_obj); > + kfree(priv); > + device_unregister(wmi_bus_dev); > > return 0; > } > @@ -1154,6 +1192,7 @@ static int acpi_wmi_probe(struct platform_device *device) > { > struct acpi_device *acpi_device; > struct device *wmi_bus_dev; > + struct wmi_bus_priv *priv; > acpi_status status; > int error; > > @@ -1190,14 +1229,34 @@ static int acpi_wmi_probe(struct platform_device *device) > } > dev_set_drvdata(&device->dev, wmi_bus_dev); > > + priv = kzalloc(sizeof(struct wmi_bus_priv), GFP_KERNEL); > + if (!priv) { > + error = -ENOMEM; > + goto err_remove_busdev; > + } > + dev_set_drvdata(wmi_bus_dev, priv); > + > error = parse_wdg(wmi_bus_dev, acpi_device); > if (error) { > pr_err("Failed to parse WDG method\n"); > - goto err_remove_busdev; > + goto err_free_priv; > } > > + sysfs_bin_attr_init(&priv->wdg_bin_attr); > + priv->wdg_bin_attr.attr.name = "wdg"; > + priv->wdg_bin_attr.attr.mode = 0400; > + priv->wdg_bin_attr.read = read_wdg; > + priv->wdg_bin_attr.size = priv->wdg_obj->buffer.length; > + > + /* A failure here isn't fatal. */ > + if (sysfs_create_bin_file(&wmi_bus_dev->kobj, &priv->wdg_bin_attr) == 0) > + priv->wdg_bin_attr_created = true; There should be at least some warning... > + > return 0; > > +err_free_priv: > + kfree(priv); > + > err_remove_busdev: > device_unregister(wmi_bus_dev); >
On Tue, Aug 1, 2017 at 8:46 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Tuesday 01 August 2017 08:37:28 Andy Lutomirski wrote: >> This adds a sysfs binary attribute 'wdg' on the bus device >> (i.e. /sys/class/wmi_bus/wmi_bus-*/wdg) that contains the raw _WDG >> data from ACPI. This can be used along with the wmi-bmof driver to >> decode the raw interface data from ACPI. >> >> There is very little new information here, as most of the contents >> were already exposed in sysfs attributes on the wmi devices. >> >> Signed-off-by: Andy Lutomirski <luto@kernel.org> >> --- >> drivers/platform/x86/wmi.c | 63 ++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 61 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c >> index 1a764e311e11..37ca56aa6025 100644 >> --- a/drivers/platform/x86/wmi.c >> +++ b/drivers/platform/x86/wmi.c >> @@ -76,6 +76,11 @@ struct wmi_block { >> bool read_takes_no_args; >> }; >> >> +struct wmi_bus_priv { >> + union acpi_object *wdg_obj; >> + struct bin_attribute wdg_bin_attr; >> + bool wdg_bin_attr_created; >> +}; >> >> /* >> * If the GUID data block is marked as expensive, we must enable and >> @@ -938,6 +943,7 @@ static bool guid_already_parsed(struct acpi_device *device, >> */ >> static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device) >> { >> + struct wmi_bus_priv *priv = dev_get_drvdata(wmi_bus_dev); >> struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL}; >> const struct guid_block *gblock; >> struct wmi_block *wblock, *next; >> @@ -1017,11 +1023,35 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device) >> } >> } >> >> + priv->wdg_obj = obj; >> + return 0; >> + >> out_free_pointer: >> kfree(out.pointer); >> return retval; >> } >> >> +static ssize_t >> +read_wdg(struct file *filp, struct kobject *kobj, >> + struct bin_attribute *attr, >> + char *buf, loff_t off, size_t count) >> +{ >> + struct wmi_bus_priv *priv = >> + container_of(attr, struct wmi_bus_priv, wdg_bin_attr); >> + >> + if (off < 0) >> + return -EINVAL; >> + >> + if (off >= priv->wdg_obj->buffer.length) >> + return 0; >> + >> + if (count > priv->wdg_obj->buffer.length - off) >> + count = priv->wdg_obj->buffer.length - off; >> + >> + memcpy(buf, priv->wdg_obj->buffer.pointer + off, count); >> + return count; >> +} >> + >> /* >> * WMI can have EmbeddedControl access regions. In which case, we just want to >> * hand these off to the EC driver. >> @@ -1138,6 +1168,8 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event, >> >> static int acpi_wmi_remove(struct platform_device *device) >> { >> + struct device *wmi_bus_dev = dev_get_drvdata(&device->dev); >> + struct wmi_bus_priv *priv = dev_get_drvdata(wmi_bus_dev); >> struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev); >> >> acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY, >> @@ -1145,7 +1177,13 @@ static int acpi_wmi_remove(struct platform_device *device) >> acpi_remove_address_space_handler(acpi_device->handle, >> ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler); >> wmi_free_devices(acpi_device); >> - device_unregister((struct device *)dev_get_drvdata(&device->dev)); >> + >> + if (priv->wdg_bin_attr_created) >> + sysfs_remove_bin_file(&wmi_bus_dev->kobj, >> + &priv->wdg_bin_attr); >> + kfree(priv->wdg_obj); >> + kfree(priv); >> + device_unregister(wmi_bus_dev); >> >> return 0; >> } >> @@ -1154,6 +1192,7 @@ static int acpi_wmi_probe(struct platform_device *device) >> { >> struct acpi_device *acpi_device; >> struct device *wmi_bus_dev; >> + struct wmi_bus_priv *priv; >> acpi_status status; >> int error; >> >> @@ -1190,14 +1229,34 @@ static int acpi_wmi_probe(struct platform_device *device) >> } >> dev_set_drvdata(&device->dev, wmi_bus_dev); >> >> + priv = kzalloc(sizeof(struct wmi_bus_priv), GFP_KERNEL); >> + if (!priv) { >> + error = -ENOMEM; >> + goto err_remove_busdev; >> + } >> + dev_set_drvdata(wmi_bus_dev, priv); >> + >> error = parse_wdg(wmi_bus_dev, acpi_device); >> if (error) { >> pr_err("Failed to parse WDG method\n"); >> - goto err_remove_busdev; >> + goto err_free_priv; >> } >> >> + sysfs_bin_attr_init(&priv->wdg_bin_attr); >> + priv->wdg_bin_attr.attr.name = "wdg"; >> + priv->wdg_bin_attr.attr.mode = 0400; >> + priv->wdg_bin_attr.read = read_wdg; >> + priv->wdg_bin_attr.size = priv->wdg_obj->buffer.length; >> + >> + /* A failure here isn't fatal. */ >> + if (sysfs_create_bin_file(&wmi_bus_dev->kobj, &priv->wdg_bin_attr) == 0) >> + priv->wdg_bin_attr_created = true; > > There should be at least some warning... Fixed for v2. --Andy
On Tue, Aug 01, 2017 at 08:37:28AM -0700, Andy Lutomirski wrote: > This adds a sysfs binary attribute 'wdg' on the bus device > (i.e. /sys/class/wmi_bus/wmi_bus-*/wdg) that contains the raw _WDG > data from ACPI. This can be used along with the wmi-bmof driver to > decode the raw interface data from ACPI. I don't recall seeing mention of this is the documentation I read as a requirement to decoding the interface with the bmof. Do Windows systems export _WDG to userspace? Why do we need to export _WDG?
On Tue, Aug 1, 2017 at 1:03 PM, Darren Hart <dvhart@infradead.org> wrote: > On Tue, Aug 01, 2017 at 08:37:28AM -0700, Andy Lutomirski wrote: >> This adds a sysfs binary attribute 'wdg' on the bus device >> (i.e. /sys/class/wmi_bus/wmi_bus-*/wdg) that contains the raw _WDG >> data from ACPI. This can be used along with the wmi-bmof driver to >> decode the raw interface data from ACPI. > > I don't recall seeing mention of this is the documentation I read as a > requirement to decoding the interface with the bmof. Do Windows systems export > _WDG to userspace? > > Why do we need to export _WDG? This was a feature that Pali asked for. Pali, since you seem to be working on userspace tooling, can you explain exactly what you'd use it for? On my laptop, at the very least, it would allow user code to determine that there's a WMI GUID that doesn't show up in sysfs. It doesn't show up in sysfs because the ACPI methods are completely missing, but it's still there in WDG. This prints a warning when wmi.ko is loaded, but maybe the userspace tooling would care, for debugging if nothing else.
On Tuesday 01 August 2017 13:19:16 Andy Lutomirski wrote: > On Tue, Aug 1, 2017 at 1:03 PM, Darren Hart <dvhart@infradead.org> wrote: > > On Tue, Aug 01, 2017 at 08:37:28AM -0700, Andy Lutomirski wrote: > >> This adds a sysfs binary attribute 'wdg' on the bus device > >> (i.e. /sys/class/wmi_bus/wmi_bus-*/wdg) that contains the raw _WDG > >> data from ACPI. This can be used along with the wmi-bmof driver to > >> decode the raw interface data from ACPI. > > > > I don't recall seeing mention of this is the documentation I read as a > > requirement to decoding the interface with the bmof. Do Windows systems export > > _WDG to userspace? > > > > Why do we need to export _WDG? > > This was a feature that Pali asked for. Pali, since you seem to be > working on userspace tooling, can you explain exactly what you'd use > it for? Take raw BMOF and WDG buffers from ACPI and parse them in userspace. Then provide needed information like mapping from MOF event name to ACPI event name based on info from WDG buffer. Ideally ability to create dump of BMOF and WDG on one computer and then parse those data on another. Having original BMOF and WDG structures is a good for debugging and development purpose. > On my laptop, at the very least, it would allow user code to determine > that there's a WMI GUID that doesn't show up in sysfs. It doesn't > show up in sysfs because the ACPI methods are completely missing, but > it's still there in WDG. This prints a warning when wmi.ko is loaded, > but maybe the userspace tooling would care, for debugging if nothing > else.
On Tue, Aug 01, 2017 at 10:40:40PM +0200, Pali Rohár wrote: > On Tuesday 01 August 2017 13:19:16 Andy Lutomirski wrote: > > On Tue, Aug 1, 2017 at 1:03 PM, Darren Hart <dvhart@infradead.org> wrote: > > > On Tue, Aug 01, 2017 at 08:37:28AM -0700, Andy Lutomirski wrote: > > >> This adds a sysfs binary attribute 'wdg' on the bus device > > >> (i.e. /sys/class/wmi_bus/wmi_bus-*/wdg) that contains the raw _WDG > > >> data from ACPI. This can be used along with the wmi-bmof driver to > > >> decode the raw interface data from ACPI. > > > > > > I don't recall seeing mention of this is the documentation I read as a > > > requirement to decoding the interface with the bmof. Do Windows systems export > > > _WDG to userspace? > > > > > > Why do we need to export _WDG? > > > > This was a feature that Pali asked for. Pali, since you seem to be > > working on userspace tooling, can you explain exactly what you'd use > > it for? > > Take raw BMOF and WDG buffers from ACPI and parse them in userspace. > Then provide needed information like mapping from MOF event name to ACPI > event name based on info from WDG buffer. > So first, I'm not opposed to adding it if that's what needed. However, I don't want to start pushing ACPI objects to sysfs without: 1) Confirming it's necessary 2) Getting Rafael's take on this practice, as if this is to become something we do, some kind of ACPI_OBJECT_SYSFS_EXPORT macro provided by ACPI would probably be the right way to do it. My understanding of the BMOF data is that it provided us "with a description of all data blocks, WMI methods, and events for the device" [1]. If that's accurate, why do we need the _WDG? This just seems contrary to what the BMOF data is supposed to be for. > Ideally ability to create dump of BMOF and WDG on one computer and then > parse those data on another. > > Having original BMOF and WDG structures is a good for debugging and > development purpose. For debugging purposes, the fwts and acpidump tools will provide this. > > On my laptop, at the very least, it would allow user code to determine > > that there's a WMI GUID that doesn't show up in sysfs. It doesn't > > show up in sysfs because the ACPI methods are completely missing, but > > it's still there in WDG. This prints a warning when wmi.ko is loaded, > > but maybe the userspace tooling would care, for debugging if nothing > > else. In this case, I'd argue userspace should only be aware of what the kernel decides to expose, based on, at the very least, compliance with the WMI documentation. 1. https://wiki.ubuntu.com/Kernel/Reference/WMI
On Tuesday, August 01, 2017 02:17:02 PM Darren Hart wrote: > On Tue, Aug 01, 2017 at 10:40:40PM +0200, Pali Rohár wrote: > > On Tuesday 01 August 2017 13:19:16 Andy Lutomirski wrote: > > > On Tue, Aug 1, 2017 at 1:03 PM, Darren Hart <dvhart@infradead.org> wrote: > > > > On Tue, Aug 01, 2017 at 08:37:28AM -0700, Andy Lutomirski wrote: > > > >> This adds a sysfs binary attribute 'wdg' on the bus device > > > >> (i.e. /sys/class/wmi_bus/wmi_bus-*/wdg) that contains the raw _WDG > > > >> data from ACPI. This can be used along with the wmi-bmof driver to > > > >> decode the raw interface data from ACPI. > > > > > > > > I don't recall seeing mention of this is the documentation I read as a > > > > requirement to decoding the interface with the bmof. Do Windows systems export > > > > _WDG to userspace? > > > > > > > > Why do we need to export _WDG? > > > > > > This was a feature that Pali asked for. Pali, since you seem to be > > > working on userspace tooling, can you explain exactly what you'd use > > > it for? > > > > Take raw BMOF and WDG buffers from ACPI and parse them in userspace. > > Then provide needed information like mapping from MOF event name to ACPI > > event name based on info from WDG buffer. > > > > So first, I'm not opposed to adding it if that's what needed. However, I don't > want to start pushing ACPI objects to sysfs without: > > 1) Confirming it's necessary > 2) Getting Rafael's take on this practice, as if this is to become something we > do, some kind of ACPI_OBJECT_SYSFS_EXPORT macro provided by ACPI would probably > be the right way to do it. No evaluation of AML methods from user space, please. This is plain dangerous, because you never know what those things do and doing that on production systems is just a plain "no". You can, however, expose the output of AML methods this way or another, for example if they are expected to generate packages of data or similar. The interface for that cannot be "evaluate this random method right now and give me the result", though. > > My understanding of the BMOF data is that it provided us "with a description of > all data blocks, WMI methods, and events for the device" [1]. > > If that's accurate, why do we need the _WDG? This just seems contrary to what > the BMOF data is supposed to be for. > > > Ideally ability to create dump of BMOF and WDG on one computer and then > > parse those data on another. > > > > Having original BMOF and WDG structures is a good for debugging and > > development purpose. > > For debugging purposes, the fwts and acpidump tools will provide this. > > > > On my laptop, at the very least, it would allow user code to determine > > > that there's a WMI GUID that doesn't show up in sysfs. It doesn't > > > show up in sysfs because the ACPI methods are completely missing, but > > > it's still there in WDG. This prints a warning when wmi.ko is loaded, > > > but maybe the userspace tooling would care, for debugging if nothing > > > else. > > In this case, I'd argue userspace should only be aware of what the kernel > decides to expose, based on, at the very least, compliance with the WMI > documentation. > > 1. https://wiki.ubuntu.com/Kernel/Reference/WMI Right.
On Tuesday 01 August 2017 14:17:02 Darren Hart wrote: > On Tue, Aug 01, 2017 at 10:40:40PM +0200, Pali Rohár wrote: > > On Tuesday 01 August 2017 13:19:16 Andy Lutomirski wrote: > > > On Tue, Aug 1, 2017 at 1:03 PM, Darren Hart <dvhart@infradead.org> wrote: > > > > On Tue, Aug 01, 2017 at 08:37:28AM -0700, Andy Lutomirski wrote: > > > >> This adds a sysfs binary attribute 'wdg' on the bus device > > > >> (i.e. /sys/class/wmi_bus/wmi_bus-*/wdg) that contains the raw _WDG > > > >> data from ACPI. This can be used along with the wmi-bmof driver to > > > >> decode the raw interface data from ACPI. > > > > > > > > I don't recall seeing mention of this is the documentation I read as a > > > > requirement to decoding the interface with the bmof. Do Windows systems export > > > > _WDG to userspace? > > > > > > > > Why do we need to export _WDG? > > > > > > This was a feature that Pali asked for. Pali, since you seem to be > > > working on userspace tooling, can you explain exactly what you'd use > > > it for? > > > > Take raw BMOF and WDG buffers from ACPI and parse them in userspace. > > Then provide needed information like mapping from MOF event name to ACPI > > event name based on info from WDG buffer. > > > > So first, I'm not opposed to adding it if that's what needed. However, I don't > want to start pushing ACPI objects to sysfs without: > > 1) Confirming it's necessary > 2) Getting Rafael's take on this practice, as if this is to become something we > do, some kind of ACPI_OBJECT_SYSFS_EXPORT macro provided by ACPI would probably > be the right way to do it. > > My understanding of the BMOF data is that it provided us "with a description of > all data blocks, WMI methods, and events for the device" [1]. > > If that's accurate, why do we need the _WDG? This just seems contrary to what > the BMOF data is supposed to be for. BMOF provides mapping from human class and method names to WMI GUID. WDG then provides mapping from WMI GUID to ACPI function names. Therefore if you upper layer in MOF world say that it want to call method M of class C and you want to know which ACPI function is called, you need to parse both BMOF and WDG to get mapping from MOF to ACPI. Same for WMI events. > > Ideally ability to create dump of BMOF and WDG on one computer and then > > parse those data on another. > > > > Having original BMOF and WDG structures is a good for debugging and > > development purpose. > > For debugging purposes, the fwts and acpidump tools will provide this. > > > > On my laptop, at the very least, it would allow user code to determine > > > that there's a WMI GUID that doesn't show up in sysfs. It doesn't > > > show up in sysfs because the ACPI methods are completely missing, but > > > it's still there in WDG. This prints a warning when wmi.ko is loaded, > > > but maybe the userspace tooling would care, for debugging if nothing > > > else. > > In this case, I'd argue userspace should only be aware of what the kernel > decides to expose, based on, at the very least, compliance with the WMI > documentation. > > 1. https://wiki.ubuntu.com/Kernel/Reference/WMI When you want to write a new WMI kernel driver you definitely need to see parsed WDG buffer. When you take ACPI dump and then decompile it (via iasl -d) as written in #1 you can then find particular WDG and BMOF buffers in that decompiled ASL file. But parsing such source file correctly is not easy and I do not know automated tool for it. Also #1 say to look at file and find some pattern...
On Tuesday, August 01, 2017 11:36:18 PM Pali Rohár wrote: > On Tuesday 01 August 2017 23:20:10 Rafael J. Wysocki wrote: > > No evaluation of AML methods from user space, please. > > > > This is plain dangerous, because you never know what those things do and doing > > that on production systems is just a plain "no". > > > > You can, however, expose the output of AML methods this way or another, > > for example if they are expected to generate packages of data or similar. > > > > The interface for that cannot be "evaluate this random method right now and > > give me the result", though. > > It is not a random AML method. It is _WDG buffer which wmi.ko already > reads + parse (IIRC only once when doing initialization) and purpose is > to provide copy of this buffer also to userspace. Similarly like > wmi-bmof.ko provides MOF buffer. OK, then.
On Tuesday 01 August 2017 23:20:10 Rafael J. Wysocki wrote: > No evaluation of AML methods from user space, please. > > This is plain dangerous, because you never know what those things do and doing > that on production systems is just a plain "no". > > You can, however, expose the output of AML methods this way or another, > for example if they are expected to generate packages of data or similar. > > The interface for that cannot be "evaluate this random method right now and > give me the result", though. It is not a random AML method. It is _WDG buffer which wmi.ko already reads + parse (IIRC only once when doing initialization) and purpose is to provide copy of this buffer also to userspace. Similarly like wmi-bmof.ko provides MOF buffer.
On Tue, Aug 01, 2017 at 11:32:41PM +0200, Rafael Wysocki wrote: > On Tuesday, August 01, 2017 11:36:18 PM Pali Rohár wrote: > > On Tuesday 01 August 2017 23:20:10 Rafael J. Wysocki wrote: > > > No evaluation of AML methods from user space, please. > > > > > > This is plain dangerous, because you never know what those things do and doing > > > that on production systems is just a plain "no". > > > > > > You can, however, expose the output of AML methods this way or another, > > > for example if they are expected to generate packages of data or similar. > > > > > > The interface for that cannot be "evaluate this random method right now and > > > give me the result", though. > > > > It is not a random AML method. It is _WDG buffer which wmi.ko already > > reads + parse (IIRC only once when doing initialization) and purpose is > > to provide copy of this buffer also to userspace. Similarly like > > wmi-bmof.ko provides MOF buffer. > > OK, then. > Specifically, _WDG is a method which evaluates to a buffer [1], and it is this buffer which Pali would like to see exported via sysfs. This data block provides a description (array of structs) for each data block, event, and control method provided by the WMI GUID. Each includes a 2 char ID, and this ID and data type provide the mapping needed to identify the enable/disable and control ACPI method names. 1. https://msdn.microsoft.com/en-us/library/windows/hardware/Dn614028(v=vs.85).aspx
On Tue, Aug 01, 2017 at 11:31:12PM +0200, Pali Rohár wrote: > On Tuesday 01 August 2017 14:17:02 Darren Hart wrote: ... > > My understanding of the BMOF data is that it provided us "with a description of > > all data blocks, WMI methods, and events for the device" [1]. > > > > If that's accurate, why do we need the _WDG? This just seems contrary to what > > the BMOF data is supposed to be for. > > BMOF provides mapping from human class and method names to WMI GUID. WDG > then provides mapping from WMI GUID to ACPI function names. > > Therefore if you upper layer in MOF world say that it want to call > method M of class C and you want to know which ACPI function is called, > you need to parse both BMOF and WDG to get mapping from MOF to ACPI. > Same for WMI events. > > > > Ideally ability to create dump of BMOF and WDG on one computer and then > > > parse those data on another. > > > > > > Having original BMOF and WDG structures is a good for debugging and > > > development purpose. OK, I went and reviewed the MOF docs, our previous discussions, and your bmf2moc code (awesome by the way). So, agreed - we need easy access to _WDG and BMOF for development purposes. Having the kernel expose a guid/class/method interface should provide the abstraction Rafael called for. The question then, is should these two things be in sysfs, where they become more or less permanent, or are they better off in debugfs, where they can be used by developers as needed, but are not present on production systems, and we are not bound to the representation we use from now until forever. Seems to me: /debug/wmi/<GUID>/bmof /debug/wmi/<GUID>/_wdg would be the most appropriate location for these.
On Tue, Aug 1, 2017 at 3:39 PM, Darren Hart <dvhart@infradead.org> wrote: > On Tue, Aug 01, 2017 at 11:31:12PM +0200, Pali Rohár wrote: >> On Tuesday 01 August 2017 14:17:02 Darren Hart wrote: > ... >> > My understanding of the BMOF data is that it provided us "with a description of >> > all data blocks, WMI methods, and events for the device" [1]. >> > >> > If that's accurate, why do we need the _WDG? This just seems contrary to what >> > the BMOF data is supposed to be for. >> >> BMOF provides mapping from human class and method names to WMI GUID. WDG >> then provides mapping from WMI GUID to ACPI function names. >> >> Therefore if you upper layer in MOF world say that it want to call >> method M of class C and you want to know which ACPI function is called, >> you need to parse both BMOF and WDG to get mapping from MOF to ACPI. >> Same for WMI events. >> >> > > Ideally ability to create dump of BMOF and WDG on one computer and then >> > > parse those data on another. >> > > >> > > Having original BMOF and WDG structures is a good for debugging and >> > > development purpose. > > OK, I went and reviewed the MOF docs, our previous discussions, and your bmf2moc > code (awesome by the way). So, agreed - we need easy access to _WDG and BMOF for > development purposes. > > Having the kernel expose a guid/class/method interface should provide the > abstraction Rafael called for. > > The question then, is should these two things be in sysfs, where they become > more or less permanent, or are they better off in debugfs, where they can be > used by developers as needed, but are not present on production systems, and > we are not bound to the representation we use from now until forever. > > Seems to me: > > /debug/wmi/<GUID>/bmof I can imagine a real production userspace app that parses BMOF data. Imagine you write a thingy that calls some laptop method foo(1) where foo is defined in the MOF. You could hardcode the mapping to the GUID, but you could also parse the BMOF. > /debug/wmi/<GUID>/_wdg More like /debug/wmi/<bus name>/_wdg, but yes. I wish there was a standard way to make a debugfs directory corresponding to a sysfs path. Maybe this exists -- I'll look. FWIW, almost all the _WDG data is already there in sysfs in Linus' tree. See, for example, cat /sys/devices/platform/PNP0C14:01/wmi_bus/wmi_bus-PNP0C14:01/05901221-D566-11D1-B2F0-00A0C9062910/object_id --Andy > > would be the most appropriate location for these. > > -- > Darren Hart > VMware Open Source Technology Center
On Tue, Aug 01, 2017 at 04:51:40PM -0700, Andy Lutomirski wrote: > On Tue, Aug 1, 2017 at 3:39 PM, Darren Hart <dvhart@infradead.org> wrote: > > On Tue, Aug 01, 2017 at 11:31:12PM +0200, Pali Rohár wrote: > >> On Tuesday 01 August 2017 14:17:02 Darren Hart wrote: > > ... > >> > My understanding of the BMOF data is that it provided us "with a description of > >> > all data blocks, WMI methods, and events for the device" [1]. > >> > > >> > If that's accurate, why do we need the _WDG? This just seems contrary to what > >> > the BMOF data is supposed to be for. > >> > >> BMOF provides mapping from human class and method names to WMI GUID. WDG > >> then provides mapping from WMI GUID to ACPI function names. > >> > >> Therefore if you upper layer in MOF world say that it want to call > >> method M of class C and you want to know which ACPI function is called, > >> you need to parse both BMOF and WDG to get mapping from MOF to ACPI. > >> Same for WMI events. > >> > >> > > Ideally ability to create dump of BMOF and WDG on one computer and then > >> > > parse those data on another. > >> > > > >> > > Having original BMOF and WDG structures is a good for debugging and > >> > > development purpose. > > > > OK, I went and reviewed the MOF docs, our previous discussions, and your bmf2moc > > code (awesome by the way). So, agreed - we need easy access to _WDG and BMOF for > > development purposes. > > > > Having the kernel expose a guid/class/method interface should provide the > > abstraction Rafael called for. > > > > The question then, is should these two things be in sysfs, where they become > > more or less permanent, or are they better off in debugfs, where they can be > > used by developers as needed, but are not present on production systems, and > > we are not bound to the representation we use from now until forever. > > > > Seems to me: > > > > /debug/wmi/<GUID>/bmof > > I can imagine a real production userspace app that parses BMOF data. > Imagine you write a thingy that calls some laptop method foo(1) where > foo is defined in the MOF. You could hardcode the mapping to the > GUID, but you could also parse the BMOF. > > > /debug/wmi/<GUID>/_wdg > > More like /debug/wmi/<bus name>/_wdg, but yes. I wish there was a > standard way to make a debugfs directory corresponding to a sysfs > path. Maybe this exists -- I'll look. > > FWIW, almost all the _WDG data is already there in sysfs in Linus' > tree. See, for example, cat > /sys/devices/platform/PNP0C14:01/wmi_bus/wmi_bus-PNP0C14:01/05901221-D566-11D1-B2F0-00A0C9062910/object_id Yeah, true - I probably should have prodded on that more at the time. In general, I'm much more comfortable presenting parsed and formatted data through sysfs than I am dumping ACPI buffer objects out there wholesale. So we have: guid instance_count expensive notify_id object_id setable Hrm. Is there something more we need that isn't already present here? Need to look more closely if all the values for the Flags are covered, but otherwise we seem to have already parsed and presented all the relevant content in the wdg. Pali, how does the "magic number identifier" in the BMOF map to the object_id? Have we retained that information in what we export today?
On Tuesday 01 August 2017 19:22:32 Darren Hart wrote: > On Tue, Aug 01, 2017 at 04:51:40PM -0700, Andy Lutomirski wrote: > > On Tue, Aug 1, 2017 at 3:39 PM, Darren Hart <dvhart@infradead.org> wrote: > > > On Tue, Aug 01, 2017 at 11:31:12PM +0200, Pali Rohár wrote: > > >> On Tuesday 01 August 2017 14:17:02 Darren Hart wrote: > > > ... > > >> > My understanding of the BMOF data is that it provided us "with a description of > > >> > all data blocks, WMI methods, and events for the device" [1]. > > >> > > > >> > If that's accurate, why do we need the _WDG? This just seems contrary to what > > >> > the BMOF data is supposed to be for. > > >> > > >> BMOF provides mapping from human class and method names to WMI GUID. WDG > > >> then provides mapping from WMI GUID to ACPI function names. > > >> > > >> Therefore if you upper layer in MOF world say that it want to call > > >> method M of class C and you want to know which ACPI function is called, > > >> you need to parse both BMOF and WDG to get mapping from MOF to ACPI. > > >> Same for WMI events. > > >> > > >> > > Ideally ability to create dump of BMOF and WDG on one computer and then > > >> > > parse those data on another. > > >> > > > > >> > > Having original BMOF and WDG structures is a good for debugging and > > >> > > development purpose. > > > > > > OK, I went and reviewed the MOF docs, our previous discussions, and your bmf2moc > > > code (awesome by the way). So, agreed - we need easy access to _WDG and BMOF for There was compilation bug in bmf2mof, which caused that bmf2mof show same output as bmfparse. Now it is fixed in git and bmf2mof really outputs real MOF text document (UTF-8 encoded, unlike windows which produce UTF-16). So if you are going to play with it, pull changes, run make clean and then make again. > > > development purposes. > > > > > > Having the kernel expose a guid/class/method interface should provide the > > > abstraction Rafael called for. > > > > > > The question then, is should these two things be in sysfs, where they become > > > more or less permanent, or are they better off in debugfs, where they can be > > > used by developers as needed, but are not present on production systems, and > > > we are not bound to the representation we use from now until forever. > > > > > > Seems to me: > > > > > > /debug/wmi/<GUID>/bmof > > > > I can imagine a real production userspace app that parses BMOF data. > > Imagine you write a thingy that calls some laptop method foo(1) where > > foo is defined in the MOF. You could hardcode the mapping to the > > GUID, but you could also parse the BMOF. If we are going to move bmof into debugfs, then it means bmof is only for debugging purpose. But we already had discussion that MOF supports should be in userspace, therefore reading bmof should be in /sys (or /dev or whatever is normal or stable for userspace). On the other hand if we want to parse bmof in kernel and do all validation (which IIRC is doing also Windows kernel) before calling "random" AML function from userspace, then it is probably good idea to put bmof only in debugfs. > > > /debug/wmi/<GUID>/_wdg > > > > More like /debug/wmi/<bus name>/_wdg, but yes. I wish there was a > > standard way to make a debugfs directory corresponding to a sysfs > > path. Maybe this exists -- I'll look. > > > > FWIW, almost all the _WDG data is already there in sysfs in Linus' > > tree. See, for example, cat > > /sys/devices/platform/PNP0C14:01/wmi_bus/wmi_bus-PNP0C14:01/05901221-D566-11D1-B2F0-00A0C9062910/object_id > > Yeah, true - I probably should have prodded on that more at the time. This has one big problem. Lot of times you want to show/parse WMI data from other computer (E.g. user send dumps via email and then somebody else parse them). Having parsed data in /sys is quite hard to pack again into same _WDG buffer. Plus most existing tools want to parse _WDG buffer on its own and provide parsed information on output. Here debugs for _WDG could make sense as by default it should not be needed for userspace, but it is really useful for debugging or writing new (or fixing existing) wmi kernel driver. > In general, I'm much more comfortable presenting parsed and formatted data > through sysfs than I am dumping ACPI buffer objects out there wholesale. > > So we have: > guid > instance_count > expensive > notify_id > object_id > setable > > Hrm. Is there something more we need that isn't already present here? > > Need to look more closely if all the values for the Flags are covered, but > otherwise we seem to have already parsed and presented all the relevant content > in the wdg. > > Pali, how does the "magic number identifier" in the BMOF map to the object_id? > Have we retained that information in what we export today? MOF describe C++ like object system and for particular structures or methods there is GUID and WMI id. When you want to call WMI function implemented in ACPI you need to know: * name of ACPI function * instance number * WMI id * structure of input buffer Name of ACPI function is taken from _WDG where is mapping from GUID to object_id and ACPI function consist of well-known prefix and object_id as a suffix. GUID is present in MOF. Where to get correct instance number is still question for me. WMI id and structure of input buffer is described in MOF.
On Wed, Aug 2, 2017 at 12:49 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Tuesday 01 August 2017 19:22:32 Darren Hart wrote: >> Pali, how does the "magic number identifier" in the BMOF map to the object_id? >> Have we retained that information in what we export today? > > MOF describe C++ like object system and for particular structures or > methods there is GUID and WMI id. > > When you want to call WMI function implemented in ACPI you need to know: > * name of ACPI function > * instance number > * WMI id > * structure of input buffer > > Name of ACPI function is taken from _WDG where is mapping from GUID to > object_id and ACPI function consist of well-known prefix and object_id > as a suffix. GUID is present in MOF. Where to get correct instance > number is still question for me. WMI id and structure of input buffer is > described in MOF. I assume that any user API for making WMI calls will have userspace pass in either the GUID or maybe the MOF method name, not the object id. --Andy
On Friday 04 August 2017 17:03:07 Andy Lutomirski wrote: > On Wed, Aug 2, 2017 at 12:49 AM, Pali Rohár <pali.rohar@gmail.com> > wrote: > > On Tuesday 01 August 2017 19:22:32 Darren Hart wrote: > >> Pali, how does the "magic number identifier" in the BMOF map to > >> the object_id? Have we retained that information in what we > >> export today? > > > > MOF describe C++ like object system and for particular structures > > or methods there is GUID and WMI id. > > > > When you want to call WMI function implemented in ACPI you need to > > know: * name of ACPI function > > * instance number > > * WMI id > > * structure of input buffer > > > > Name of ACPI function is taken from _WDG where is mapping from GUID > > to object_id and ACPI function consist of well-known prefix and > > object_id as a suffix. GUID is present in MOF. Where to get > > correct instance number is still question for me. WMI id and > > structure of input buffer is described in MOF. > > I assume that any user API for making WMI calls will have userspace > pass in either the GUID or maybe the MOF method name, not the object > id. So... When calling WMI method of some class it is really needed to supply also instance id. On Windows caller needs to specify Instance Name, e.g. ACPI\PNP0C14\0_0 which looks like correspondent to instance id 0. ACPI\PNP0C14\0_2 is then to instance id 2. Toshiba has triple (class, method, instance) in their BIOS WMI documentation [1]. Therefore userspace would need to know instance count which is stored in _WDG for doing WMI call. Is there already exported instance count via sysfs? [1] - https://aps2.toshiba-tro.de/kb0/TSB3803HR0000R01_TOSHIBA_BIOS_WMI_Interface_Guide_-_13_Rev_1.1.pdf
On Sun, Aug 6, 2017 at 2:36 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Friday 04 August 2017 17:03:07 Andy Lutomirski wrote: >> On Wed, Aug 2, 2017 at 12:49 AM, Pali Rohár <pali.rohar@gmail.com> >> wrote: >> > On Tuesday 01 August 2017 19:22:32 Darren Hart wrote: >> >> Pali, how does the "magic number identifier" in the BMOF map to >> >> the object_id? Have we retained that information in what we >> >> export today? >> > >> > MOF describe C++ like object system and for particular structures >> > or methods there is GUID and WMI id. >> > >> > When you want to call WMI function implemented in ACPI you need to >> > know: * name of ACPI function >> > * instance number >> > * WMI id >> > * structure of input buffer >> > >> > Name of ACPI function is taken from _WDG where is mapping from GUID >> > to object_id and ACPI function consist of well-known prefix and >> > object_id as a suffix. GUID is present in MOF. Where to get >> > correct instance number is still question for me. WMI id and >> > structure of input buffer is described in MOF. >> >> I assume that any user API for making WMI calls will have userspace >> pass in either the GUID or maybe the MOF method name, not the object >> id. > > So... When calling WMI method of some class it is really needed to > supply also instance id. On Windows caller needs to specify Instance > Name, e.g. ACPI\PNP0C14\0_0 which looks like correspondent to instance > id 0. ACPI\PNP0C14\0_2 is then to instance id 2. > > Toshiba has triple (class, method, instance) in their BIOS WMI > documentation [1]. > > Therefore userspace would need to know instance count which is stored in > _WDG for doing WMI call. > > Is there already exported instance count via sysfs? Yes. > > [1] - https://aps2.toshiba-tro.de/kb0/TSB3803HR0000R01_TOSHIBA_BIOS_WMI_Interface_Guide_-_13_Rev_1.1.pdf > > -- > Pali Rohár > pali.rohar@gmail.com
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index 1a764e311e11..37ca56aa6025 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -76,6 +76,11 @@ struct wmi_block { bool read_takes_no_args; }; +struct wmi_bus_priv { + union acpi_object *wdg_obj; + struct bin_attribute wdg_bin_attr; + bool wdg_bin_attr_created; +}; /* * If the GUID data block is marked as expensive, we must enable and @@ -938,6 +943,7 @@ static bool guid_already_parsed(struct acpi_device *device, */ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device) { + struct wmi_bus_priv *priv = dev_get_drvdata(wmi_bus_dev); struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL}; const struct guid_block *gblock; struct wmi_block *wblock, *next; @@ -1017,11 +1023,35 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device) } } + priv->wdg_obj = obj; + return 0; + out_free_pointer: kfree(out.pointer); return retval; } +static ssize_t +read_wdg(struct file *filp, struct kobject *kobj, + struct bin_attribute *attr, + char *buf, loff_t off, size_t count) +{ + struct wmi_bus_priv *priv = + container_of(attr, struct wmi_bus_priv, wdg_bin_attr); + + if (off < 0) + return -EINVAL; + + if (off >= priv->wdg_obj->buffer.length) + return 0; + + if (count > priv->wdg_obj->buffer.length - off) + count = priv->wdg_obj->buffer.length - off; + + memcpy(buf, priv->wdg_obj->buffer.pointer + off, count); + return count; +} + /* * WMI can have EmbeddedControl access regions. In which case, we just want to * hand these off to the EC driver. @@ -1138,6 +1168,8 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event, static int acpi_wmi_remove(struct platform_device *device) { + struct device *wmi_bus_dev = dev_get_drvdata(&device->dev); + struct wmi_bus_priv *priv = dev_get_drvdata(wmi_bus_dev); struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev); acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY, @@ -1145,7 +1177,13 @@ static int acpi_wmi_remove(struct platform_device *device) acpi_remove_address_space_handler(acpi_device->handle, ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler); wmi_free_devices(acpi_device); - device_unregister((struct device *)dev_get_drvdata(&device->dev)); + + if (priv->wdg_bin_attr_created) + sysfs_remove_bin_file(&wmi_bus_dev->kobj, + &priv->wdg_bin_attr); + kfree(priv->wdg_obj); + kfree(priv); + device_unregister(wmi_bus_dev); return 0; } @@ -1154,6 +1192,7 @@ static int acpi_wmi_probe(struct platform_device *device) { struct acpi_device *acpi_device; struct device *wmi_bus_dev; + struct wmi_bus_priv *priv; acpi_status status; int error; @@ -1190,14 +1229,34 @@ static int acpi_wmi_probe(struct platform_device *device) } dev_set_drvdata(&device->dev, wmi_bus_dev); + priv = kzalloc(sizeof(struct wmi_bus_priv), GFP_KERNEL); + if (!priv) { + error = -ENOMEM; + goto err_remove_busdev; + } + dev_set_drvdata(wmi_bus_dev, priv); + error = parse_wdg(wmi_bus_dev, acpi_device); if (error) { pr_err("Failed to parse WDG method\n"); - goto err_remove_busdev; + goto err_free_priv; } + sysfs_bin_attr_init(&priv->wdg_bin_attr); + priv->wdg_bin_attr.attr.name = "wdg"; + priv->wdg_bin_attr.attr.mode = 0400; + priv->wdg_bin_attr.read = read_wdg; + priv->wdg_bin_attr.size = priv->wdg_obj->buffer.length; + + /* A failure here isn't fatal. */ + if (sysfs_create_bin_file(&wmi_bus_dev->kobj, &priv->wdg_bin_attr) == 0) + priv->wdg_bin_attr_created = true; + return 0; +err_free_priv: + kfree(priv); + err_remove_busdev: device_unregister(wmi_bus_dev);
This adds a sysfs binary attribute 'wdg' on the bus device (i.e. /sys/class/wmi_bus/wmi_bus-*/wdg) that contains the raw _WDG data from ACPI. This can be used along with the wmi-bmof driver to decode the raw interface data from ACPI. There is very little new information here, as most of the contents were already exposed in sysfs attributes on the wmi devices. Signed-off-by: Andy Lutomirski <luto@kernel.org> --- drivers/platform/x86/wmi.c | 63 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 2 deletions(-)