Three WMI improvements

When I converted dell-wmi to the new bus infrastructure, I left the call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This could cause two problems:

- An error message when loading the driver on a system without dell-wmi. We'd try to read the event descriptor even if the WMI GUID wasn't there.

- A possible race if dell-wmi was loaded manually before wmi was fully initialized.

Fix it by moving the call to the probe function where it belongs.

Fixes bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/platform/x86/dell-wmi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/platform/x86/dell-wmi.c +7 -1

index f8978464df31..dad8f4afa17c 100644
@@ -626,7 +626,7 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
  * WMI Interface Version     8       4    <version>
  * WMI buffer length        12       4    4096
  */
-static int __init dell_wmi_check_descriptor_buffer(void)
+static int dell_wmi_check_descriptor_buffer(void)
 {
 	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *obj;

@@ -717,9 +717,15 @@ static int dell_wmi_events_set_enabled(bool enable)
 
 static int dell_wmi_probe(struct wmi_device *wdev)
 {
+	int err;
+
 	struct dell_wmi_priv *priv = devm_kzalloc(
 		&wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
 
+	err = dell_wmi_check_descriptor_buffer();
+	if (err)
+		return err;
+
 	dev_set_drvdata(&wdev->dev, priv);
 
 	return dell_wmi_input_setup(wdev);

Quoted 58 lines · On Tuesday 01 August 2017 08:37:26 Andy Lutomirski wrote:

When I converted dell-wmi to the new bus infrastructure, I left the call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This could cause two problems:

- An error message when loading the driver on a system without dell-wmi. We'd try to read the event descriptor even if the WMI GUID wasn't there.

- A possible race if dell-wmi was loaded manually before wmi was fully initialized.

Fix it by moving the call to the probe function where it belongs.

Fixes bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/platform/x86/dell-wmi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/platform/x86/dell-wmi.c +7 -5

index f8978464df31..dad8f4afa17c 100644
@@ -626,7 +626,7 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
  * WMI Interface Version     8       4    <version>
  * WMI buffer length        12       4    4096
  */
-static int __init dell_wmi_check_descriptor_buffer(void)
+static int dell_wmi_check_descriptor_buffer(void)
 {
      struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
      union acpi_object *obj;

@@ -717,9 +717,15 @@ static int dell_wmi_events_set_enabled(bool enable)
 
 static int dell_wmi_probe(struct wmi_device *wdev)
 {
+     int err;
+
      struct dell_wmi_priv *priv = devm_kzalloc(
              &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
 
+     err = dell_wmi_check_descriptor_buffer();
+     if (err)
+             return err;
+
      dev_set_drvdata(&wdev->dev, priv);
 
      return dell_wmi_input_setup(wdev);

@@ -749,10 +755,6 @@ static int __init dell_wmi_init(void)
 {
      int err;
 
-     err = dell_wmi_check_descriptor_buffer();
-     if (err)
-             return err;
-
      dmi_check_system(dell_wmi_smbios_list);
 

drivers/platform/x86/dell-wmi.c static int __init dell_wmi_init(void) · line 760

Quoted 1 line

drivers/platform/x86/dell-wmi.c +0 -0

      if (wmi_requires_smbios_request) {

Hi! You should move also dell_wmi_events_set_enabled() into dell_wmi_probe() as there is no need to enable receiving events prior to creating input device.

Quoted 12 lines · Pali Rohár

Quoted 11 lines · On Tuesday 01 August 2017 08:37:26 Andy Lutomirski wrote:

When I converted dell-wmi to the new bus infrastructure, I left the call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This could cause two problems:

- An error message when loading the driver on a system without dell-wmi. We'd try to read the event descriptor even if the WMI GUID wasn't there.

- A possible race if dell-wmi was loaded manually before wmi was fully initialized.

Quoted 8 lines

Quoted 4 lines

Fix it by moving the call to the probe function where it belongs.

Fixes bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")

Signed-off-by Andy Lutomirski <[email protected]>

Hi! You should move also dell_wmi_events_set_enabled() into dell_wmi_probe() as there is no need to enable receiving events prior to creating input device.

I thought of that and intentionally didn't do it: I wanted to leave enable and the disable paired properly, and there's nothing that tracks the enabled state per device. Also, it's at least theoretically possible to have more than one instance of dell-wmi in a system (my laptop already has *two* wmi busses), and moving this code to ->probe would break this.

(The current wmi.c code can't handle the same GUID on two busses, but it could easily be added if anyone ever finds a laptop that does that.)

Quoted 59 lines · Pali Rohár

Quoted 58 lines · On Tuesday 01 August 2017 08:37:26 Andy Lutomirski wrote:

When I converted dell-wmi to the new bus infrastructure, I left the call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This could cause two problems:

- An error message when loading the driver on a system without dell-wmi. We'd try to read the event descriptor even if the WMI GUID wasn't there.

- A possible race if dell-wmi was loaded manually before wmi was fully initialized.

Fix it by moving the call to the probe function where it belongs.

Fixes bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/platform/x86/dell-wmi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/platform/x86/dell-wmi.c +7 -5

index f8978464df31..dad8f4afa17c 100644
@@ -626,7 +626,7 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
  * WMI Interface Version     8       4    <version>
  * WMI buffer length        12       4    4096
  */
-static int __init dell_wmi_check_descriptor_buffer(void)
+static int dell_wmi_check_descriptor_buffer(void)
 {
      struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
      union acpi_object *obj;

@@ -717,9 +717,15 @@ static int dell_wmi_events_set_enabled(bool enable)
 
 static int dell_wmi_probe(struct wmi_device *wdev)
 {
+     int err;
+
      struct dell_wmi_priv *priv = devm_kzalloc(
              &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
 
+     err = dell_wmi_check_descriptor_buffer();
+     if (err)
+             return err;
+
      dev_set_drvdata(&wdev->dev, priv);
 
      return dell_wmi_input_setup(wdev);

@@ -749,10 +755,6 @@ static int __init dell_wmi_init(void)
 {
      int err;
 
-     err = dell_wmi_check_descriptor_buffer();
-     if (err)
-             return err;
-
      dmi_check_system(dell_wmi_smbios_list);
 

Quoted 5 lines

drivers/platform/x86/dell-wmi.c static int __init dell_wmi_init(void) · line 760

Quoted 1 line

drivers/platform/x86/dell-wmi.c +0 -0

      if (wmi_requires_smbios_request) {

Hi! You should move also dell_wmi_events_set_enabled() into dell_wmi_probe() as there is no need to enable receiving events prior to creating input device.

I thought of that and intentionally didn't do it: I wanted to leave enable and the disable paired properly, and there's nothing that tracks the enabled state per device. Also, it's at least theoretically possible to have more than one instance of dell-wmi in a system (my laptop already has *two* wmi busses), and moving this code to ->probe would break this.

(The current wmi.c code can't handle the same GUID on two busses, but it could easily be added if anyone ever finds a laptop that does that.)

Quoted 73 lines · On Tuesday 01 August 2017 11:08:26 Andy Lutomirski wrote:

Quoted 59 lines · Pali Rohár

Quoted 58 lines · On Tuesday 01 August 2017 08:37:26 Andy Lutomirski wrote:

When I converted dell-wmi to the new bus infrastructure, I left the call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This could cause two problems:

- An error message when loading the driver on a system without dell-wmi. We'd try to read the event descriptor even if the WMI GUID wasn't there.

- A possible race if dell-wmi was loaded manually before wmi was fully initialized.

Fix it by moving the call to the probe function where it belongs.

Fixes bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/platform/x86/dell-wmi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/platform/x86/dell-wmi.c +7 -5

index f8978464df31..dad8f4afa17c 100644
@@ -626,7 +626,7 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
  * WMI Interface Version     8       4    <version>
  * WMI buffer length        12       4    4096
  */
-static int __init dell_wmi_check_descriptor_buffer(void)
+static int dell_wmi_check_descriptor_buffer(void)
 {
      struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
      union acpi_object *obj;

@@ -717,9 +717,15 @@ static int dell_wmi_events_set_enabled(bool enable)
 
 static int dell_wmi_probe(struct wmi_device *wdev)
 {
+     int err;
+
      struct dell_wmi_priv *priv = devm_kzalloc(
              &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
 
+     err = dell_wmi_check_descriptor_buffer();
+     if (err)
+             return err;
+
      dev_set_drvdata(&wdev->dev, priv);
 
      return dell_wmi_input_setup(wdev);

@@ -749,10 +755,6 @@ static int __init dell_wmi_init(void)
 {
      int err;
 
-     err = dell_wmi_check_descriptor_buffer();
-     if (err)
-             return err;
-
      dmi_check_system(dell_wmi_smbios_list);
 

Quoted 5 lines

drivers/platform/x86/dell-wmi.c static int __init dell_wmi_init(void) · line 760

Quoted 1 line

drivers/platform/x86/dell-wmi.c +0 -0

      if (wmi_requires_smbios_request) {

Hi! You should move also dell_wmi_events_set_enabled() into dell_wmi_probe() as there is no need to enable receiving events prior to creating input device.

I thought of that and intentionally didn't do it: I wanted to leave enable and the disable paired properly, and there's nothing that tracks the enabled state per device. Also, it's at least theoretically possible to have more than one instance of dell-wmi in a system (my laptop already has *two* wmi busses), and moving this code to ->probe would break this.

Quoted 3 lines

(The current wmi.c code can't handle the same GUID on two busses, but it could easily be added if anyone ever finds a laptop that does that.)

Yes, thanks for clarification, it makes sense.

Just one hypothetical situation, but as we know we should not trust what vendors put into ACPI DSDT...

Before whole wmi bus patches were introduced, function dell_wmi_events_set_enabled() was called only after every check passed:

1) WMI GUID exists

2) WMI descriptor buffer has correct type

3) machine is on DMI whitelist

Now after all those patches, only the last (3) check need to pass to call that dell_wmi_events_set_enabled() function which issue SMM call.

Do not know how big this issue is, I just want to point to this hypothetical problem that SMM call could be issued also in more cases.

Signature · 3 lines

-- 
Pali Rohár
[email protected]

Quoted 95 lines · Pali Rohár

Quoted 73 lines · On Tuesday 01 August 2017 11:08:26 Andy Lutomirski wrote:

Quoted 59 lines · Pali Rohár

Quoted 58 lines · On Tuesday 01 August 2017 08:37:26 Andy Lutomirski wrote:

When I converted dell-wmi to the new bus infrastructure, I left the call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This could cause two problems:

- An error message when loading the driver on a system without dell-wmi. We'd try to read the event descriptor even if the WMI GUID wasn't there.

- A possible race if dell-wmi was loaded manually before wmi was fully initialized.

Fix it by moving the call to the probe function where it belongs.

Fixes bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/platform/x86/dell-wmi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/platform/x86/dell-wmi.c +7 -5

index f8978464df31..dad8f4afa17c 100644
@@ -626,7 +626,7 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
  * WMI Interface Version     8       4    <version>
  * WMI buffer length        12       4    4096
  */
-static int __init dell_wmi_check_descriptor_buffer(void)
+static int dell_wmi_check_descriptor_buffer(void)
 {
      struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
      union acpi_object *obj;

@@ -717,9 +717,15 @@ static int dell_wmi_events_set_enabled(bool enable)
 
 static int dell_wmi_probe(struct wmi_device *wdev)
 {
+     int err;
+
      struct dell_wmi_priv *priv = devm_kzalloc(
              &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
 
+     err = dell_wmi_check_descriptor_buffer();
+     if (err)
+             return err;
+
      dev_set_drvdata(&wdev->dev, priv);
 
      return dell_wmi_input_setup(wdev);

@@ -749,10 +755,6 @@ static int __init dell_wmi_init(void)
 {
      int err;
 
-     err = dell_wmi_check_descriptor_buffer();
-     if (err)
-             return err;
-
      dmi_check_system(dell_wmi_smbios_list);
 

Quoted 5 lines

drivers/platform/x86/dell-wmi.c static int __init dell_wmi_init(void) · line 760

Quoted 1 line

drivers/platform/x86/dell-wmi.c +0 -0

      if (wmi_requires_smbios_request) {

Hi! You should move also dell_wmi_events_set_enabled() into dell_wmi_probe() as there is no need to enable receiving events prior to creating input device.

I thought of that and intentionally didn't do it: I wanted to leave enable and the disable paired properly, and there's nothing that tracks the enabled state per device. Also, it's at least theoretically possible to have more than one instance of dell-wmi in a system (my laptop already has *two* wmi busses), and moving this code to ->probe would break this.

Quoted 3 lines

(The current wmi.c code can't handle the same GUID on two busses, but it could easily be added if anyone ever finds a laptop that does that.)

Yes, thanks for clarification, it makes sense.

Just one hypothetical situation, but as we know we should not trust what vendors put into ACPI DSDT...

Before whole wmi bus patches were introduced, function dell_wmi_events_set_enabled() was called only after every check passed:

1) WMI GUID exists

2) WMI descriptor buffer has correct type

3) machine is on DMI whitelist

Now after all those patches, only the last (3) check need to pass to call that dell_wmi_events_set_enabled() function which issue SMM call.

Quoted 2 lines

Do not know how big this issue is, I just want to point to this hypothetical problem that SMM call could be issued also in more cases.

Thanks for raising the point. In my opinion, it seems reasonable to expect that #1 and #2 are implicit in #3 being coded up. We don't like to rely on hard coded assumptions of course. What is the failure mode if #1 or #2 aren't satisfied?

Quoted 5 lines

Signature · 3 lines

-- 
Pali Rohár
[email protected]

Quoted 96 lines · On Tuesday 01 August 2017 13:59:56 Darren Hart wrote:

Quoted 95 lines · Pali Rohár

Quoted 73 lines · On Tuesday 01 August 2017 11:08:26 Andy Lutomirski wrote:

Quoted 59 lines · Pali Rohár

Quoted 58 lines · On Tuesday 01 August 2017 08:37:26 Andy Lutomirski wrote:

When I converted dell-wmi to the new bus infrastructure, I left the call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This could cause two problems:

- An error message when loading the driver on a system without dell-wmi. We'd try to read the event descriptor even if the WMI GUID wasn't there.

- A possible race if dell-wmi was loaded manually before wmi was fully initialized.

Fix it by moving the call to the probe function where it belongs.

Fixes bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/platform/x86/dell-wmi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/platform/x86/dell-wmi.c +7 -5

index f8978464df31..dad8f4afa17c 100644
@@ -626,7 +626,7 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
  * WMI Interface Version     8       4    <version>
  * WMI buffer length        12       4    4096
  */
-static int __init dell_wmi_check_descriptor_buffer(void)
+static int dell_wmi_check_descriptor_buffer(void)
 {
      struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
      union acpi_object *obj;

@@ -717,9 +717,15 @@ static int dell_wmi_events_set_enabled(bool enable)
 
 static int dell_wmi_probe(struct wmi_device *wdev)
 {
+     int err;
+
      struct dell_wmi_priv *priv = devm_kzalloc(
              &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
 
+     err = dell_wmi_check_descriptor_buffer();
+     if (err)
+             return err;
+
      dev_set_drvdata(&wdev->dev, priv);
 
      return dell_wmi_input_setup(wdev);

@@ -749,10 +755,6 @@ static int __init dell_wmi_init(void)
 {
      int err;
 
-     err = dell_wmi_check_descriptor_buffer();
-     if (err)
-             return err;
-
      dmi_check_system(dell_wmi_smbios_list);
 

Quoted 5 lines

drivers/platform/x86/dell-wmi.c static int __init dell_wmi_init(void) · line 760

Quoted 1 line

drivers/platform/x86/dell-wmi.c +0 -0

      if (wmi_requires_smbios_request) {

Hi! You should move also dell_wmi_events_set_enabled() into dell_wmi_probe() as there is no need to enable receiving events prior to creating input device.

I thought of that and intentionally didn't do it: I wanted to leave enable and the disable paired properly, and there's nothing that tracks the enabled state per device. Also, it's at least theoretically possible to have more than one instance of dell-wmi in a system (my laptop already has *two* wmi busses), and moving this code to ->probe would break this.

Quoted 3 lines

(The current wmi.c code can't handle the same GUID on two busses, but it could easily be added if anyone ever finds a laptop that does that.)

Yes, thanks for clarification, it makes sense.

Just one hypothetical situation, but as we know we should not trust what vendors put into ACPI DSDT...

Before whole wmi bus patches were introduced, function dell_wmi_events_set_enabled() was called only after every check passed:

1) WMI GUID exists

2) WMI descriptor buffer has correct type

3) machine is on DMI whitelist

Now after all those patches, only the last (3) check need to pass to call that dell_wmi_events_set_enabled() function which issue SMM call.

Quoted 7 lines

Quoted 2 lines

Do not know how big this issue is, I just want to point to this hypothetical problem that SMM call could be issued also in more cases.

Thanks for raising the point. In my opinion, it seems reasonable to expect that #1 and #2 are implicit in #3 being coded up. We don't like to rely on hard coded assumptions of course. What is the failure mode if #1 or #2 aren't satisfied?

What would happen if we call dell_wmi_events_set_enabled() on unsupported platform? E.g. on some Dell Latitudes it cause resetting keyboard backlight settings to default values, e.g. turn keyboard backlight on for every key press and turn keyboard backlight off after 5s of inactivity.

I do not know more.

But yes, #3 should imply that #1 and #2 are truth too...

Quoted 5 lines · Pali Rohár

Quoted 4 lines · On Tuesday 01 August 2017 13:59:56 Darren Hart wrote:

Quoted 3 lines · Pali Rohár

Quoted 2 lines · On Tuesday 01 August 2017 11:08:26 Andy Lutomirski wrote:

Quoted 1 line · Pali Rohár

On Tuesday 01 August 2017 08:37:26 Andy Lutomirski wrote:

Quoted 46 lines

Quoted 32 lines

Quoted 32 lines

Quoted 11 lines

Quoted 3 lines

Hi! You should move also dell_wmi_events_set_enabled() into dell_wmi_probe() as there is no need to enable receiving events prior to creating input device.

I thought of that and intentionally didn't do it: I wanted to leave enable and the disable paired properly, and there's nothing that tracks the enabled state per device. Also, it's at least theoretically possible to have more than one instance of dell-wmi in a system (my laptop already has *two* wmi busses), and moving this code to ->probe would break this.

Quoted 3 lines

(The current wmi.c code can't handle the same GUID on two busses, but it could easily be added if anyone ever finds a laptop that does that.)

Yes, thanks for clarification, it makes sense.

Just one hypothetical situation, but as we know we should not trust what vendors put into ACPI DSDT...

Before whole wmi bus patches were introduced, function dell_wmi_events_set_enabled() was called only after every check passed:

1) WMI GUID exists

2) WMI descriptor buffer has correct type

3) machine is on DMI whitelist

Now after all those patches, only the last (3) check need to pass to call that dell_wmi_events_set_enabled() function which issue SMM call.

Quoted 7 lines

Quoted 2 lines

Do not know how big this issue is, I just want to point to this hypothetical problem that SMM call could be issued also in more cases.

Thanks for raising the point. In my opinion, it seems reasonable to expect that #1 and #2 are implicit in #3 being coded up. We don't like to rely on hard coded assumptions of course. What is the failure mode if #1 or #2 aren't satisfied?

What would happen if we call dell_wmi_events_set_enabled() on unsupported platform? E.g. on some Dell Latitudes it cause resetting keyboard backlight settings to default values, e.g. turn keyboard backlight on for every key press and turn keyboard backlight off after 5s of inactivity.

Quoted 3 lines

I do not know more.

But yes, #3 should imply that #1 and #2 are truth too...

Hmm. I could do a followup patch to move it into ->probe and refcount it.

Quoted 11 lines · On Tuesday 01 August 2017 08:37:26 Andy Lutomirski wrote:

When I converted dell-wmi to the new bus infrastructure, I left the call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This could cause two problems:

- An error message when loading the driver on a system without dell-wmi. We'd try to read the event descriptor even if the WMI GUID wasn't there.

- A possible race if dell-wmi was loaded manually before wmi was fully initialized.

Quoted 4 lines

Fix it by moving the call to the probe function where it belongs.

Fixes bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")

Signed-off-by Andy Lutomirski <[email protected]>

Hi! You should move also dell_wmi_events_set_enabled() into dell_wmi_probe() as there is no need to enable receiving events prior to creating input device.

When I converted dell-wmi to the new bus infrastructure, I left the call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This could cause two problems:

- An error message when loading the driver on a system without dell-wmi. We'd try to read the event descriptor even if the WMI GUID wasn't there.

- A possible race if dell-wmi was loaded manually before wmi was fully initialized.

Fix it by moving the call to the probe function where it belongs.

Fixes bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")

Signed-off-by Andy Lutomirski <[email protected]>

Quoted 13 lines · Andy Lutomirski

When I converted dell-wmi to the new bus infrastructure, I left the call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This could cause two problems:

- An error message when loading the driver on a system without dell-wmi. We'd try to read the event descriptor even if the WMI GUID wasn't there.

- A possible race if dell-wmi was loaded manually before wmi was fully initialized.

Fix it by moving the call to the probe function where it belongs.

Quoted 2 lines

Fixes bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")

Signed-off-by Andy Lutomirski <[email protected]>

Queueing this one (1/3) to testing.

Pali, aside from the separate input event receiving discussion, do you have any concerns with this patch? I'd like to get this to Linus this week for our 4.13-2 PR for this RC cycle.

This converts dell_wmi_check_descriptor_buffer() to the new driver model interface and puts the interface version in dell_wmi_priv where it belongs.

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/platform/x86/dell-wmi.c | 69 +++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 30 deletions(-)

drivers/platform/x86/dell-wmi.c +39 -30

index dad8f4afa17c..28d9f8696081 100644
@@ -48,7 +48,6 @@ MODULE_LICENSE("GPL");
 #define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
 #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
-static u32 dell_wmi_interface_version;
 static bool wmi_requires_smbios_request;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);

@@ -56,6 +55,7 @@ MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
 struct dell_wmi_priv {
 	struct input_dev *input_dev;
+	u32 interface_version;
 };
 
 static int __init dmi_matched(const struct dmi_system_id *dmi)

@@ -348,6 +348,7 @@ static void dell_wmi_process_key(struct wmi_device *wdev, int type, int code)
 static void dell_wmi_notify(struct wmi_device *wdev,
 			    union acpi_object *obj)
 {
+	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
 	u16 *buffer_entry, *buffer_end;
 	acpi_size buffer_size;
 	int len, i;

@@ -376,7 +377,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,
 	 * So to prevent reading garbage from buffer we will process only first
 	 * one event on devices with WMI interface version 0.
 	 */
-	if (dell_wmi_interface_version == 0 && buffer_entry < buffer_end)
+	if (priv->interface_version == 0 && buffer_entry < buffer_end)
 		if (buffer_end > buffer_entry + buffer_entry[0] + 1)
 			buffer_end = buffer_entry + buffer_entry[0] + 1;
 

@@ -626,61 +627,67 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
  * WMI Interface Version     8       4    <version>
  * WMI buffer length        12       4    4096
  */
-static int dell_wmi_check_descriptor_buffer(void)
+static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
 {
-	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
-	acpi_status status;
+	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
+	union acpi_object *obj = NULL;
+	struct wmi_device *desc_dev;
 	u32 *buffer;
+	int ret;
 
-	status = wmi_query_block(DELL_DESCRIPTOR_GUID, 0, &out);
-	if (ACPI_FAILURE(status)) {
-		pr_err("Cannot read Dell descriptor buffer - %d\n", status);
-		return status;
+	desc_dev = wmidev_get_other_guid(wdev, DELL_DESCRIPTOR_GUID);
+	if (!desc_dev) {
+		dev_err(&wdev->dev, "Dell WMI descriptor does not exist\n");
+		return -ENODEV;
 	}
 
-	obj = (union acpi_object *)out.pointer;
+	obj = wmidev_block_query(desc_dev, 0);
 	if (!obj) {
-		pr_err("Dell descriptor buffer is empty\n");
-		return -EINVAL;
+		dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
+		ret = -EIO;
+		goto out;
 	}
 
 	if (obj->type != ACPI_TYPE_BUFFER) {
-		pr_err("Cannot read Dell descriptor buffer\n");
-		kfree(obj);
-		return -EINVAL;
+		dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
+		ret = -EINVAL;
+		goto out;
 	}
 
 	if (obj->buffer.length != 128) {
-		pr_err("Dell descriptor buffer has invalid length (%d)\n",
+		dev_err(&wdev->dev,
+			"Dell descriptor buffer has invalid length (%d)\n",
 			obj->buffer.length);
 		if (obj->buffer.length < 16) {
-			kfree(obj);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 	}
 
 	buffer = (u32 *)obj->buffer.pointer;
 
 	if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720)
-		pr_warn("Dell descriptor buffer has invalid signature (%*ph)\n",
+		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
 			8, buffer);
 
 	if (buffer[2] != 0 && buffer[2] != 1)
-		pr_warn("Dell descriptor buffer has unknown version (%d)\n",
+		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
 			buffer[2]);
 
 	if (buffer[3] != 4096)
-		pr_warn("Dell descriptor buffer has invalid buffer length (%d)\n",
+		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
 			buffer[3]);
 
-	dell_wmi_interface_version = buffer[2];
+	priv->interface_version = buffer[2];
+	ret = 0;
 
-	pr_info("Detected Dell WMI interface version %u\n",
-		dell_wmi_interface_version);
+	dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n",
+		priv->interface_version);
 
+out:
 	kfree(obj);
-	return 0;
+	put_device(&desc_dev->dev);
+	return ret;
 }
 
 /*

@@ -717,17 +724,19 @@ static int dell_wmi_events_set_enabled(bool enable)
 
 static int dell_wmi_probe(struct wmi_device *wdev)
 {
+	struct dell_wmi_priv *priv;
 	int err;
 
-	struct dell_wmi_priv *priv = devm_kzalloc(
+	priv = devm_kzalloc(
 		&wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	dev_set_drvdata(&wdev->dev, priv);
 
-	err = dell_wmi_check_descriptor_buffer();
+	err = dell_wmi_check_descriptor_buffer(wdev);
 	if (err)
 		return err;
 
-	dev_set_drvdata(&wdev->dev, priv);
-
 	return dell_wmi_input_setup(wdev);
 }
 

Quoted 5 lines · On Tuesday 01 August 2017 08:37:27 Andy Lutomirski wrote:

This converts dell_wmi_check_descriptor_buffer() to the new driver model interface and puts the interface version in dell_wmi_priv where it belongs.

Signed-off-by Andy Lutomirski <[email protected]>

Looks good,

Reviewed-by Pali Rohár <[email protected]>

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 <[email protected]>

---
 drivers/platform/x86/wmi.c | 63 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

drivers/platform/x86/wmi.c +48 -2

index 1a764e311e11..37ca56aa6025 100644
@@ -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;
 	}

drivers/platform/x86/wmi.c +4 -0

+
 	return 0;
 
+err_free_priv:
+	kfree(priv);
+
 err_remove_busdev:
 	device_unregister(wmi_bus_dev);
 

Quoted 123 lines · 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 <[email protected]>

---
 drivers/platform/x86/wmi.c | 63 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

drivers/platform/x86/wmi.c +48 -2

index 1a764e311e11..37ca56aa6025 100644
@@ -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;
      }
 

drivers/platform/x86/wmi.c static int acpi_wmi_probe(struct platform_device *device) · line 1253

Quoted 9 lines

drivers/platform/x86/wmi.c +9 -0

+     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...

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.

Quoted 4 lines · Andy Lutomirski

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?

Quoted 10 lines · Darren Hart

Quoted 4 lines · Andy Lutomirski

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?

Quoted 1 line

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.

Quoted 13 lines · On Tuesday 01 August 2017 13:19:16 Andy Lutomirski wrote:

Quoted 11 lines · Darren Hart

Quoted 4 lines · Andy Lutomirski

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?

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 14 lines · Pali Rohár

Quoted 13 lines · On Tuesday 01 August 2017 13:19:16 Andy Lutomirski wrote:

Quoted 11 lines · Darren Hart

Quoted 4 lines · Andy Lutomirski

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?

Quoted 8 lines

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

Quoted 6 lines

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

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.

Quoted 8 lines · 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.

Quoted 2 lines

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.

Quoted 12 lines · Pali Rohár

Quoted 10 lines · 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.

Quoted 4 lines

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.

Quoted 14 lines · Pali Rohár

Quoted 13 lines · On Tuesday 01 August 2017 13:19:16 Andy Lutomirski wrote:

Quoted 11 lines · Darren Hart

Quoted 4 lines · Andy Lutomirski

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?

Quoted 8 lines

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

Quoted 6 lines

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 01 August 2017 14:17:02 Darren Hart wrote:

Quoted 5 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 1 line · Pali Rohár

On Tuesday 01 August 2017 14:17:02 Darren Hart wrote:

...

Quoted 6 lines

Quoted 5 lines

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.

Quoted 13 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

--
Darren Hart
VMware Open Source Technology Center

Quoted 30 lines · Darren Hart

Quoted 1 line · Pali Rohár

On Tuesday 01 August 2017 14:17:02 Darren Hart wrote:

...

Quoted 6 lines

Quoted 5 lines

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.

Quoted 13 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 1 line

/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

Quoted 18 lines · Andy Lutomirski

Quoted 17 lines · Darren Hart

Quoted 1 line · Pali Rohár

On Tuesday 01 August 2017 14:17:02 Darren Hart wrote:

...

Quoted 14 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

Quoted 10 lines

Quoted 10 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 1 line

/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?

Quoted 2 lines · 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.

Quoted 14 lines · Pali Rohár

Quoted 3 lines · 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

Quoted 5 lines

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.

Quoted 22 lines · On Friday 04 August 2017 17:03:07 Andy Lutomirski wrote:

Quoted 14 lines · Pali Rohár

Quoted 3 lines · 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

Quoted 5 lines

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.

Quoted 3 lines

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

--
Pali Rohár
[email protected]