[PATCH 0/3] Three WMI improvements

[PATCH 0/3] Three WMI improvements

[Andy Lutomirski]

These are in decreasing order of appropriateness for 4.13.

Patch 1 is a straight-up bugfix.

Patch 2 is a further busification conversion inspired by patch 1.

Patch 3 implements a feature request from Pali.

Andy Lutomirski (3):
  platform/dell-wmi: Fix driver interface version query
  platform/dell-wmi: Update dell_wmi_check_descriptor_buffer() to new
    model
  platform/wmi: Expose the raw WDG data in sysfs

 drivers/platform/x86/dell-wmi.c | 75 +++++++++++++++++++++++------------------
 drivers/platform/x86/wmi.c      | 63 ++++++++++++++++++++++++++++++++--
 2 files changed, 104 insertions(+), 34 deletions(-)

-- 
2.13.3

[PATCH 1/3] platform/dell-wmi: Fix driver interface version query

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

Fixes: bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/platform/x86/dell-wmi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index f8978464df31..dad8f4afa17c 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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);
 
 	if (wmi_requires_smbios_request) {
-- 
2.13.3

[PATCH 2/3] platform/dell-wmi: Update dell_wmi_check_descriptor_buffer() to new model

[Andy Lutomirski]

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 <luto@kernel.org>
---
 drivers/platform/x86/dell-wmi.c | 69 +++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 30 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index dad8f4afa17c..28d9f8696081 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -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);
 }
 
-- 
2.13.3

[PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

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

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;
+
 	return 0;
 
+err_free_priv:
+	kfree(priv);
+
 err_remove_busdev:
 	device_unregister(wmi_bus_dev);
 
-- 
2.13.3

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Pali Rohár]

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 <luto@kernel.org>
> ---
>  drivers/platform/x86/dell-wmi.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index f8978464df31..dad8f4afa17c 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -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);
>  
>  	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.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/3] platform/dell-wmi: Update dell_wmi_check_descriptor_buffer() to new model

[Pali Rohár]

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 <luto@kernel.org>

Looks good,

Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

> ---
>  drivers/platform/x86/dell-wmi.c | 69 +++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index dad8f4afa17c..28d9f8696081 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -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);
>  }
>  

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Pali Rohár]

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);
>  

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Andy Lutomirski]

On Tue, Aug 1, 2017 at 8:43 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> 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 <luto@kernel.org>
>> ---
>>  drivers/platform/x86/dell-wmi.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> index f8978464df31..dad8f4afa17c 100644
>> --- a/drivers/platform/x86/dell-wmi.c
>> +++ b/drivers/platform/x86/dell-wmi.c
>> @@ -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);
>>
>>       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.)

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Andy Lutomirski]

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

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Darren Hart]

On Tue, Aug 01, 2017 at 05:43:12PM +0200, Pali Rohár wrote:
> 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 <luto@kernel.org>

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

That is a good idea, but it is a separate functional change. Especially as this
is addressing a specific bug, let's keep this as is, and add a second patch to
move the enable receiving events.


-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Darren Hart]

On Tue, Aug 01, 2017 at 11:08:26AM -0700, Andy Lutomirski wrote:
> On Tue, Aug 1, 2017 at 8:43 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > 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 <luto@kernel.org>

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

Better still, thanks Andy.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Darren Hart]

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?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Darren Hart]

On Tue, Aug 01, 2017 at 08:37:26AM -0700, 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 <luto@kernel.org>

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.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Andy Lutomirski]

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.

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Pali Rohár]

On Tuesday 01 August 2017 13:06:01 Darren Hart wrote:
> On Tue, Aug 01, 2017 at 08:37:26AM -0700, 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 <luto@kernel.org>
> 
> 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.

It is ok, add my

Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Pali Rohár]

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.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Pali Rohár]

On Tuesday 01 August 2017 11:08:26 Andy Lutomirski wrote:
> On Tue, Aug 1, 2017 at 8:43 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > 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 <luto@kernel.org>
> >> ---
> >>  drivers/platform/x86/dell-wmi.c | 12 +++++++-----
> >>  1 file changed, 7 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> >> index f8978464df31..dad8f4afa17c 100644
> >> --- a/drivers/platform/x86/dell-wmi.c
> >> +++ b/drivers/platform/x86/dell-wmi.c
> >> @@ -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);
> >>
> >>       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.)

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.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Darren Hart]

On Tue, Aug 01, 2017 at 10:45:46PM +0200, Pali Rohár wrote:
> On Tuesday 01 August 2017 11:08:26 Andy Lutomirski wrote:
> > On Tue, Aug 1, 2017 at 8:43 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > 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 <luto@kernel.org>
> > >> ---
> > >>  drivers/platform/x86/dell-wmi.c | 12 +++++++-----
> > >>  1 file changed, 7 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > >> index f8978464df31..dad8f4afa17c 100644
> > >> --- a/drivers/platform/x86/dell-wmi.c
> > >> +++ b/drivers/platform/x86/dell-wmi.c
> > >> @@ -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);
> > >>
> > >>       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.)
> 
> 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.


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?

> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com
> 

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Pali Rohár]

On Tuesday 01 August 2017 13:59:56 Darren Hart wrote:
> On Tue, Aug 01, 2017 at 10:45:46PM +0200, Pali Rohár wrote:
> > On Tuesday 01 August 2017 11:08:26 Andy Lutomirski wrote:
> > > On Tue, Aug 1, 2017 at 8:43 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > 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 <luto@kernel.org>
> > > >> ---
> > > >>  drivers/platform/x86/dell-wmi.c | 12 +++++++-----
> > > >>  1 file changed, 7 insertions(+), 5 deletions(-)
> > > >>
> > > >> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > > >> index f8978464df31..dad8f4afa17c 100644
> > > >> --- a/drivers/platform/x86/dell-wmi.c
> > > >> +++ b/drivers/platform/x86/dell-wmi.c
> > > >> @@ -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);
> > > >>
> > > >>       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.)
> > 
> > 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.
> 
> 
> 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...

> > 
> > -- 
> > Pali Rohár
> > pali.rohar@gmail.com
> > 
> 

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Darren Hart]

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
-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Rafael J. Wysocki]

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.

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Pali Rohár]

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

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Rafael J. Wysocki]

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.

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Pali Rohár]

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.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Darren Hart]

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
-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Darren Hart]

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.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Andy Lutomirski]

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

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Darren Hart]

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?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Andy Lutomirski]

On Tue, Aug 1, 2017 at 2:16 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Tuesday 01 August 2017 13:59:56 Darren Hart wrote:
>> On Tue, Aug 01, 2017 at 10:45:46PM +0200, Pali Rohár wrote:
>> > On Tuesday 01 August 2017 11:08:26 Andy Lutomirski wrote:
>> > > On Tue, Aug 1, 2017 at 8:43 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
>> > > > 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 <luto@kernel.org>
>> > > >> ---
>> > > >>  drivers/platform/x86/dell-wmi.c | 12 +++++++-----
>> > > >>  1 file changed, 7 insertions(+), 5 deletions(-)
>> > > >>
>> > > >> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> > > >> index f8978464df31..dad8f4afa17c 100644
>> > > >> --- a/drivers/platform/x86/dell-wmi.c
>> > > >> +++ b/drivers/platform/x86/dell-wmi.c
>> > > >> @@ -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);
>> > > >>
>> > > >>       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.)
>> >
>> > 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.
>>
>>
>> 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...

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

--Andy

Re: [PATCH 1/3] platform/dell-wmi: Fix driver interface version query

[Darren Hart]

On Tue, Aug 01, 2017 at 07:25:53PM -0700, Andy Lutomirski wrote:
> On Tue, Aug 1, 2017 at 2:16 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Tuesday 01 August 2017 13:59:56 Darren Hart wrote:
> >> On Tue, Aug 01, 2017 at 10:45:46PM +0200, Pali Rohár wrote:
> >> > On Tuesday 01 August 2017 11:08:26 Andy Lutomirski wrote:
> >> > > On Tue, Aug 1, 2017 at 8:43 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> >> > > > On Tuesday 01 August 2017 08:37:26 Andy Lutomirski wrote:
...
> >> > > > 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.)
> >> >
> >> > 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.
> >>
> >>
> >> 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...
> 
> Hmm.  I could do a followup patch to move it into ->probe and refcount it.
> 

Probe does seem like the logical place for it. Could we track the enabled state
in the private data structure?

From your comment above, what about moving enable to probe breaks the
theoretical possibility of two dell-wmi devices?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Pali Rohár]

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.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Andy Lutomirski]

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

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1796 bytes --]

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

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] platform/wmi: Expose the raw WDG data in sysfs

[Andy Lutomirski]

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

Re: [PATCH 2/3] platform/dell-wmi: Update dell_wmi_check_descriptor_buffer() to new model

[Darren Hart]

On Tue, Aug 01, 2017 at 05:44:57PM +0200, Pali Rohár wrote:
> 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 <luto@kernel.org>
> 
> Looks good,
> 
> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

Queued to testing.

-- 
Darren Hart
VMware Open Source Technology Center