[PATCH 0/2] Fixes for dell-wmi

[PATCH 0/2] Fixes for dell-wmi

[Pali Rohár]

This patch series adds check if Dell WMI descriptor structure is valid and
fixes processing WMI events on devices with WMI interface version 0.

After testing, second patch is good candidate for backporting into stable
kernels, but problem is that it cannot be used without first patch. So I
let decision to other people.

Gabriele and Michał, this patch series should fix processing events on
yours Dell laptops (you have in DSDT defined version 0). Can you test it?

Pali Rohár (2):
  dell-wmi: Check if Dell WMI descriptor structure is valid
  dell-wmi: Process only one event on devices with interface version 0

 drivers/platform/x86/dell-wmi.c |   94 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 2 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Pali Rohár]

According to Dell WMI document mentioned in ML dicussion archived at
http://www.spinics.net/lists/platform-driver-x86/msg07220.html OS should
check Dell WMI descriptor structure. Structure also provide Dell WMI
interface version which is used later.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   78 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 57402c4..09ee8ed 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -2,6 +2,7 @@
  * Dell WMI hotkeys
  *
  * Copyright (C) 2008 Red Hat <mjg@redhat.com>
+ * Copyright (C) 2014-2015 Pali Rohár <pali.rohar@gmail.com>
  *
  * Portions based on wistron_btns.c:
  * Copyright (C) 2005 Miloslav Trmac <mitr@volny.cz>
@@ -38,14 +39,18 @@
 #include <acpi/video.h>
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
 MODULE_DESCRIPTION("Dell laptop WMI hotkeys driver");
 MODULE_LICENSE("GPL");
 
 #define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
+#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
 static int acpi_video;
+static u32 dell_wmi_interface_version;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
+MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
 /*
  * Certain keys are flagged as KE_IGNORE. All of these are either
@@ -422,16 +427,85 @@ static void __init find_hk_type(const struct dmi_header *dm, void *dummy)
 	}
 }
 
+/**
+ * Descriptor buffer is 128 byte long and contains:
+ *
+ *       Name             Offset  Length  Value
+ * Vendor Signature          0       4    "DELL"
+ * Object Signature          4       4    " WMI"
+ * WMI Interface Version     8       4    <version>
+ * WMI buffer length        12       4    4096
+ */
+static int __init dell_wmi_check_descriptor_buffer(void)
+{
+	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	acpi_status status;
+	u32 *buffer;
+
+	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;
+	}
+
+	obj = (union acpi_object *)out.pointer;
+	if (!obj) {
+		pr_err("Dell descriptor buffer is empty\n");
+		return -EINVAL;
+	}
+
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		pr_err("Cannot read Dell descriptor buffer\n");
+		kfree(obj);
+		return -EINVAL;
+	}
+
+	if (obj->buffer.length != 128) {
+		pr_err("Dell descriptor buffer has invalid length (%d)\n",
+			obj->buffer.length);
+		kfree(obj);
+		return -EINVAL;
+	}
+
+	buffer = (u32 *)obj->buffer.pointer;
+
+	if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720)
+		pr_warn("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",
+			buffer[2]);
+
+	if (buffer[3] != 4096)
+		pr_warn("Dell descriptor buffer has invalid buffer length (%d)\n",
+			buffer[3]);
+
+	dell_wmi_interface_version = buffer[2];
+
+	pr_info("Detected Dell WMI interface version %u\n",
+		dell_wmi_interface_version);
+
+	kfree(obj);
+	return 0;
+}
+
 static int __init dell_wmi_init(void)
 {
 	int err;
 	acpi_status status;
 
-	if (!wmi_has_guid(DELL_EVENT_GUID)) {
-		pr_warn("No known WMI GUID found\n");
+	if (!wmi_has_guid(DELL_EVENT_GUID) ||
+	    !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
+		pr_warn("Dell WMI GUID were not found\n");
 		return -ENODEV;
 	}
 
+	err = dell_wmi_check_descriptor_buffer();
+	if (err)
+		return err;
+
 	dmi_walk(find_hk_type, NULL);
 	acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor;
 
-- 
1.7.9.5

[PATCH 2/2] dell-wmi: Process only one event on devices with interface version 0

[Pali Rohár]

BIOS/ACPI on devices with WMI interface version 0 does not clear buffer
before filling it. So next time when BIOS/ACPI send WMI event which is
smaller as previous then it contains garbage in buffer from previous event.

BIOS/ACPI on devices with WMI interface version 1 clears buffer and
sometimes send more events in buffer at one call.

Since commit 83fc44c32ad8 ("dell-wmi: Update code for processing WMI
events") dell-wmi process all events in buffer (and not just first).

So to prevent reading garbage from buffer we will process only first one
event on devices with WMI interface version 0.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 09ee8ed..62f2f7a 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -237,6 +237,22 @@ static void dell_wmi_notify(u32 value, void *context)
 
 	buffer_end = buffer_entry + buffer_size;
 
+	/*
+	 * BIOS/ACPI on devices with WMI interface version 0 does not clear
+	 * buffer before filling it. So next time when BIOS/ACPI send WMI event
+	 * which is smaller as previous then it contains garbage in buffer from
+	 * previous event.
+	 *
+	 * BIOS/ACPI on devices with WMI interface version 1 clears buffer and
+	 * sometimes send more events in buffer at one call.
+	 *
+	 * 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 (buffer_end > buffer_entry + buffer_entry[0] + 1)
+			buffer_end = buffer_entry + buffer_entry[0] + 1;
+
 	while (buffer_entry < buffer_end) {
 
 		len = buffer_entry[0];
-- 
1.7.9.5

Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Andy Lutomirski]

On Thu, Dec 24, 2015 at 1:18 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> According to Dell WMI document mentioned in ML dicussion archived at
> http://www.spinics.net/lists/platform-driver-x86/msg07220.html OS should
> check Dell WMI descriptor structure. Structure also provide Dell WMI
> interface version which is used later.

I will rebase my big series on top of this.  It'll give me a good
excuse to test that I got the probe ordering right.  (The code is
explicitly intended to support use cases like this, and now I'll have
a real-world test for it.)  I'll also test this in a bit.

> +MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);

I don't think this is necessary.  The driver will only work if both
wmi devices and, hence, modaliases are present, so there's no need to
cause just one or the other to trigger dell-wmi autoloading.

> +/**
> + * Descriptor buffer is 128 byte long and contains:

This isn't kerneldoc format, so I think this should just be "/*".

> +       if (obj->buffer.length != 128) {
> +               pr_err("Dell descriptor buffer has invalid length (%d)\n",
> +                       obj->buffer.length);
> +               kfree(obj);
> +               return -EINVAL;
> +       }

I would advocate for being more permissive: a buffer that is actually
too short for the fields we need would result in -EINVAL, but a buffer
that isn't 128 bytes would just be a warning and not cause module load
to fail.

--Andy

Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Pali Rohár]

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

On Friday 25 December 2015 02:23:04 Andy Lutomirski wrote:
> On Thu, Dec 24, 2015 at 1:18 PM, Pali Rohár <pali.rohar@gmail.com>
> wrote:
> > According to Dell WMI document mentioned in ML dicussion archived
> > at http://www.spinics.net/lists/platform-driver-x86/msg07220.html
> > OS should check Dell WMI descriptor structure. Structure also
> > provide Dell WMI interface version which is used later.
> 
> I will rebase my big series on top of this.  It'll give me a good
> excuse to test that I got the probe ordering right.  (The code is
> explicitly intended to support use cases like this, and now I'll have
> a real-world test for it.)  I'll also test this in a bit.

Ok!

> > +MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
> 
> I don't think this is necessary.  The driver will only work if both
> wmi devices and, hence, modaliases are present, so there's no need to
> cause just one or the other to trigger dell-wmi autoloading.

Maybe now when you are working on big WMI patch series is time to change 
modalias support in WMI to support AND-conjunction (&&) on WMI aliases, 
not just OR (one alias match).

Something like: load dell-wmi.ko driver if system provides both WMI 
GUIDs.

> > +/**
> 
> > + * Descriptor buffer is 128 byte long and contains:
> This isn't kerneldoc format, so I think this should just be "/*".
> 

Ok, I will fix this in next version.

> > +       if (obj->buffer.length != 128) {
> > +               pr_err("Dell descriptor buffer has invalid length
> > (%d)\n", +                       obj->buffer.length);
> > +               kfree(obj);
> > +               return -EINVAL;
> > +       }
> 
> I would advocate for being more permissive: a buffer that is actually
> too short for the fields we need would result in -EINVAL, but a
> buffer that isn't 128 bytes would just be a warning and not cause
> module load to fail.
> 
> --Andy

Sounds good, I will change this part.

-- 
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 0/2] Fixes for dell-wmi

[Gabriele Mazzotta]

On 24/12/2015 22:18, Pali Rohár wrote:
> This patch series adds check if Dell WMI descriptor structure is valid and
> fixes processing WMI events on devices with WMI interface version 0.
> 
> After testing, second patch is good candidate for backporting into stable
> kernels, but problem is that it cannot be used without first patch. So I
> let decision to other people.
> 
> Gabriele and Michał, this patch series should fix processing events on
> yours Dell laptops (you have in DSDT defined version 0). Can you test it?

Hi,

I tested the patches and all the function keys work, but I can see
that there are some differences in the dmesg.

Here a before and after comparison.

The radio button seems to generate longer messages when compared to
the others.


Before:
# Brightness down
dell_wmi: Received WMI event (03 00 00 00 05 e0 0e 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
dell_wmi: Key e005 pressed

# Volume down
dell_wmi: Received WMI event (02 00 00 00 2e e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
dell_wmi: Key e02e pressed

# Radio
dell_wmi: Received WMI event (06 00 00 00 08 e0 1d 03 0a 00 00 01 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
dell_wmi: Key e008 pressed

# Brightness down
dell_wmi: Received WMI event (03 00 00 00 05 e0 0e 00 0a 00 00 01 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
dell_wmi: Key e005 pressed

# Volume down
dell_wmi: Received WMI event (02 00 00 00 2e e0 0f 00 0a 00 00 01 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
dell_wmi: Key e02e pressed


After:
# Brightness down
dell_wmi: Received WMI event (03 00 00 00 05 e0 0e 00 00 00 00 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
dell_wmi: Key e005 pressed

# Volume down
dell_wmi: Received WMI event (02 00 00 00 2e e0 0e 00 00 00 00 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
dell_wmi: Key e02e pressed

# Radio
dell_wmi: Received WMI event (06 00 00 00 08 e0 1d 03 0a 00 00 01 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
dell_wmi: Key e008 pressed

# Brightness down
dell_wmi: Received WMI event (03 00 00 00 05 e0 0e 00 09 00 00 01 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
dell_wmi: Key e005 pressed

# Volume down
dell_wmi: Received WMI event (02 00 00 00 2e e0 0e 00 09 00 00 01 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
dell_wmi: Key e02e pressed


> Pali Rohár (2):
>   dell-wmi: Check if Dell WMI descriptor structure is valid
>   dell-wmi: Process only one event on devices with interface version 0
> 
>  drivers/platform/x86/dell-wmi.c |   94 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 92 insertions(+), 2 deletions(-)
> 

Re: [PATCH 0/2] Fixes for dell-wmi

[Pali Rohár]

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

On Sunday 27 December 2015 13:59:57 Gabriele Mazzotta wrote:
> On 24/12/2015 22:18, Pali Rohár wrote:
> > This patch series adds check if Dell WMI descriptor structure is
> > valid and fixes processing WMI events on devices with WMI
> > interface version 0.
> > 
> > After testing, second patch is good candidate for backporting into
> > stable kernels, but problem is that it cannot be used without
> > first patch. So I let decision to other people.
> > 
> > Gabriele and Michał, this patch series should fix processing events
> > on yours Dell laptops (you have in DSDT defined version 0). Can
> > you test it?
> 
> Hi,
> 
> I tested the patches and all the function keys work, but I can see
> that there are some differences in the dmesg.
> 
> Here a before and after comparison.
> 
> The radio button seems to generate longer messages when compared to
> the others.

Hi! Thanks for testing. Can you please recompile driver with line 
#define DEBUG at beginning of dell-wmi.c file and send dmesg output 
again? I want to see processing buffer debug lines in dmesg. Here we 
should see that after this patch series garbage will not be parsed.

-- 
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 0/2] Fixes for dell-wmi

[Gabriele Mazzotta]

On 27/12/2015 14:07, Pali Rohár wrote:
> On Sunday 27 December 2015 13:59:57 Gabriele Mazzotta wrote:
>> On 24/12/2015 22:18, Pali Rohár wrote:
>>> This patch series adds check if Dell WMI descriptor structure is
>>> valid and fixes processing WMI events on devices with WMI
>>> interface version 0.
>>>
>>> After testing, second patch is good candidate for backporting into
>>> stable kernels, but problem is that it cannot be used without
>>> first patch. So I let decision to other people.
>>>
>>> Gabriele and Michał, this patch series should fix processing events
>>> on yours Dell laptops (you have in DSDT defined version 0). Can
>>> you test it?
>>
>> Hi,
>>
>> I tested the patches and all the function keys work, but I can see
>> that there are some differences in the dmesg.
>>
>> Here a before and after comparison.
>>
>> The radio button seems to generate longer messages when compared to
>> the others.
> 
> Hi! Thanks for testing. Can you please recompile driver with line 
> #define DEBUG at beginning of dell-wmi.c file and send dmesg output 
> again? I want to see processing buffer debug lines in dmesg. Here we 
> should see that after this patch series garbage will not be parsed.

I had to define DEBUG to get those lines.
dell_new_hk_type is false, so that's all I can get.

Re: [PATCH 0/2] Fixes for dell-wmi

[Pali Rohár]

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

On Sunday 27 December 2015 14:10:55 Gabriele Mazzotta wrote:
> On 27/12/2015 14:07, Pali Rohár wrote:
> > On Sunday 27 December 2015 13:59:57 Gabriele Mazzotta wrote:
> >> On 24/12/2015 22:18, Pali Rohár wrote:
> >>> This patch series adds check if Dell WMI descriptor structure is
> >>> valid and fixes processing WMI events on devices with WMI
> >>> interface version 0.
> >>> 
> >>> After testing, second patch is good candidate for backporting
> >>> into stable kernels, but problem is that it cannot be used
> >>> without first patch. So I let decision to other people.
> >>> 
> >>> Gabriele and Michał, this patch series should fix processing
> >>> events on yours Dell laptops (you have in DSDT defined version
> >>> 0). Can you test it?
> >> 
> >> Hi,
> >> 
> >> I tested the patches and all the function keys work, but I can see
> >> that there are some differences in the dmesg.
> >> 
> >> Here a before and after comparison.
> >> 
> >> The radio button seems to generate longer messages when compared
> >> to the others.
> > 
> > Hi! Thanks for testing. Can you please recompile driver with line
> > #define DEBUG at beginning of dell-wmi.c file and send dmesg output
> > again? I want to see processing buffer debug lines in dmesg. Here
> > we should see that after this patch series garbage will not be
> > parsed.
> 
> I had to define DEBUG to get those lines.
> dell_new_hk_type is false, so that's all I can get.

Ah right, now I see... When dell_new_hk_type is false then code below 
which this series changing is not executed.

-- 
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 0/2] Fixes for dell-wmi

[Michał Kępień]

> This patch series adds check if Dell WMI descriptor structure is valid and
> fixes processing WMI events on devices with WMI interface version 0.
> 
> After testing, second patch is good candidate for backporting into stable
> kernels, but problem is that it cannot be used without first patch. So I
> let decision to other people.
> 
> Gabriele and Michał, this patch series should fix processing events on
> yours Dell laptops (you have in DSDT defined version 0). Can you test it?

My Vostro V131 (and any other as well, judging from the DSDT dumps I
found scattered around the web) has the WMI Interface Version field set
to 1, as the first one of your patches correctly asserts.

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Michał Kępień]

> According to Dell WMI document mentioned in ML dicussion archived at
> http://www.spinics.net/lists/platform-driver-x86/msg07220.html OS should
> check Dell WMI descriptor structure.

"Should" or "can"?  I skimmed through the ACPI-WMI PDF and Mario's
message again and I couldn't find any explicit statement urging the
reader to check the structure in question before doing anything else.

-- 
Best regards,
Michał Kępień

Re: [PATCH 2/2] dell-wmi: Process only one event on devices with interface version 0

[Michał Kępień]

> BIOS/ACPI on devices with WMI interface version 0 does not clear buffer
> before filling it. So next time when BIOS/ACPI send WMI event which is
> smaller as previous then it contains garbage in buffer from previous event.
> 
> BIOS/ACPI on devices with WMI interface version 1 clears buffer and
> sometimes send more events in buffer at one call.

Are the explanations above based on your observations or perhaps some
documentation?

-- 
Best regards,
Michał Kępień

Re: [PATCH 0/2] Fixes for dell-wmi

[Pali Rohár]

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

On Monday 28 December 2015 14:33:17 Michał Kępień wrote:
> > This patch series adds check if Dell WMI descriptor structure is
> > valid and fixes processing WMI events on devices with WMI
> > interface version 0.
> > 
> > After testing, second patch is good candidate for backporting into
> > stable kernels, but problem is that it cannot be used without
> > first patch. So I let decision to other people.
> > 
> > Gabriele and Michał, this patch series should fix processing events
> > on yours Dell laptops (you have in DSDT defined version 0). Can
> > you test it?
> 
> My Vostro V131 (and any other as well, judging from the DSDT dumps I
> found scattered around the web) has the WMI Interface Version field
> set to 1, as the first one of your patches correctly asserts.

Ok, great. Can you post DEBUG output when receiving events before and 
after patch (to check that processing is correct)? 

-- 
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 2/2] dell-wmi: Process only one event on devices with interface version 0

[Pali Rohár]

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

On Monday 28 December 2015 14:40:56 Michał Kępień wrote:
> > BIOS/ACPI on devices with WMI interface version 0 does not clear
> > buffer before filling it. So next time when BIOS/ACPI send WMI
> > event which is smaller as previous then it contains garbage in
> > buffer from previous event.
> > 
> > BIOS/ACPI on devices with WMI interface version 1 clears buffer and
> > sometimes send more events in buffer at one call.
> 
> Are the explanations above based on your observations or perhaps some
> documentation?

Just observation from all DSDT files which I have seen.

-- 
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 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Pali Rohár]

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

On Monday 28 December 2015 14:37:07 Michał Kępień wrote:
> > According to Dell WMI document mentioned in ML dicussion archived
> > at http://www.spinics.net/lists/platform-driver-x86/msg07220.html
> > OS should check Dell WMI descriptor structure.
> 
> "Should" or "can"?  I skimmed through the ACPI-WMI PDF and Mario's
> message again and I couldn't find any explicit statement urging the
> reader to check the structure in question before doing anything else.

That's questionable... In "Design flow" is first point that WMI 
descriptor check.

-- 
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 0/2] Fixes for dell-wmi

[Michał Kępień]

> > > This patch series adds check if Dell WMI descriptor structure is
> > > valid and fixes processing WMI events on devices with WMI
> > > interface version 0.
> > > 
> > > After testing, second patch is good candidate for backporting into
> > > stable kernels, but problem is that it cannot be used without
> > > first patch. So I let decision to other people.
> > > 
> > > Gabriele and Michał, this patch series should fix processing events
> > > on yours Dell laptops (you have in DSDT defined version 0). Can
> > > you test it?
> > 
> > My Vostro V131 (and any other as well, judging from the DSDT dumps I
> > found scattered around the web) has the WMI Interface Version field
> > set to 1, as the first one of your patches correctly asserts.
> 
> Ok, great. Can you post DEBUG output when receiving events before and 
> after patch (to check that processing is correct)? 

After applying your patches on my Vostro V131, the WMI events reported
are exactly the same as they were when I originally posted them [1].

[1] http://www.spinics.net/lists/platform-driver-x86/msg07191.html

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Michał Kępień]

> > > According to Dell WMI document mentioned in ML dicussion archived
> > > at http://www.spinics.net/lists/platform-driver-x86/msg07220.html
> > > OS should check Dell WMI descriptor structure.
> > 
> > "Should" or "can"?  I skimmed through the ACPI-WMI PDF and Mario's
> > message again and I couldn't find any explicit statement urging the
> > reader to check the structure in question before doing anything else.
> 
> That's questionable... In "Design flow" is first point that WMI 
> descriptor check.

Which "Design flow" are you referring to?  Because I found at least two:
chapter 2.3 and a subsection of chapter 2.3.3.  Funnily enough, in both
of these locations the WMI Descriptor Method is discussed first.

Personally, I wouldn't use the structure of that document to draw
cause-effect conclusions.  Just look at the last chapter (2.3.4), which
shows how to tell whether the BIOS supports the ACPI-WMI interface.
Shouldn't that be the first thing to check, before doing anything else
mentioned in that document?  Yet, it's the last thing discussed.

Anyway, while the document mentions in several places that the BIOS WMI
Descriptor object can be queried, it fails to convince me as to why this
is necessary at all as all values in the returned buffer are constant.
Perhaps parsing the buffer is useful as a sanity check of some kind, but
it certainly isn't a prerequisite for performing further actions.

Given the nature of your patchset, I'd personally rephrase the commit
message(s) to state that according to your observations, there are
behavioral differences between models with different versions of the WMI
Interface, so we parse the WMI Descriptor object to determine which WMI
Interface version is used on the machine we're running on.  Perhaps with
an additional word or two that it won't hurt to also check the WMI
Descriptor object's correctness while we're at it.

If you feel like I'm nit-picking and none of the above matters, please
feel free to disregard my input and just follow your gut.

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Pali Rohár]

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

On Tuesday 29 December 2015 13:44:13 Michał Kępień wrote:
> > > > According to Dell WMI document mentioned in ML dicussion
> > > > archived at
> > > > http://www.spinics.net/lists/platform-driver-x86/msg07220.html
> > > > OS should check Dell WMI descriptor structure.
> > > 
> > > "Should" or "can"?  I skimmed through the ACPI-WMI PDF and
> > > Mario's message again and I couldn't find any explicit statement
> > > urging the reader to check the structure in question before
> > > doing anything else.
> > 
> > That's questionable... In "Design flow" is first point that WMI
> > descriptor check.
> 
> Which "Design flow" are you referring to?  Because I found at least
> two: chapter 2.3 and a subsection of chapter 2.3.3.  Funnily enough,
> in both of these locations the WMI Descriptor Method is discussed
> first.
> 
> Personally, I wouldn't use the structure of that document to draw
> cause-effect conclusions.  Just look at the last chapter (2.3.4),
> which shows how to tell whether the BIOS supports the ACPI-WMI
> interface. Shouldn't that be the first thing to check, before doing
> anything else mentioned in that document?  Yet, it's the last thing
> discussed.
> 
> Anyway, while the document mentions in several places that the BIOS
> WMI Descriptor object can be queried, it fails to convince me as to
> why this is necessary at all as all values in the returned buffer
> are constant. Perhaps parsing the buffer is useful as a sanity check
> of some kind, but it certainly isn't a prerequisite for performing
> further actions.
> 
> Given the nature of your patchset, I'd personally rephrase the commit
> message(s) to state that according to your observations, there are
> behavioral differences between models with different versions of the
> WMI Interface, so we parse the WMI Descriptor object to determine
> which WMI Interface version is used on the machine we're running on.
>  Perhaps with an additional word or two that it won't hurt to also
> check the WMI Descriptor object's correctness while we're at it.
> 
> If you feel like I'm nit-picking and none of the above matters,
> please feel free to disregard my input and just follow your gut.

It's ok. We just understand it quite differently. And in this case what 
about changing commit message to something like this?

===
dell-wmi: Check if Dell WMI descriptor structure is valid

After examining existing DSDT ACPI tables of more laptops and looking 
into Dell WMI document mentioned in ML dicussion archived at 
http://www.spinics.net/lists/platform-driver-x86/msg07220.html we will 
parse and check WMI descriptor if contains expected data. It is because 
WMI descriptor contains interface version number and it is needed to 
know in next commit.
===

-- 
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 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Michał Kępień]

> > If you feel like I'm nit-picking and none of the above matters,
> > please feel free to disregard my input and just follow your gut.
> 
> It's ok. We just understand it quite differently. And in this case what 
> about changing commit message to something like this?
> 
> ===
> dell-wmi: Check if Dell WMI descriptor structure is valid
> 
> After examining existing DSDT ACPI tables of more laptops and looking 
> into Dell WMI document mentioned in ML dicussion archived at 
> http://www.spinics.net/lists/platform-driver-x86/msg07220.html we will 
> parse and check WMI descriptor if contains expected data. It is because 
> WMI descriptor contains interface version number and it is needed to 
> know in next commit.
> ===

I like it way more than the previous one.

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Andy Lutomirski]

On Wed, Dec 30, 2015 at 3:27 AM, Michał Kępień <kernel@kempniu.pl> wrote:
>> > If you feel like I'm nit-picking and none of the above matters,
>> > please feel free to disregard my input and just follow your gut.
>>
>> It's ok. We just understand it quite differently. And in this case what
>> about changing commit message to something like this?
>>
>> ===
>> dell-wmi: Check if Dell WMI descriptor structure is valid
>>
>> After examining existing DSDT ACPI tables of more laptops and looking
>> into Dell WMI document mentioned in ML dicussion archived at
>> http://www.spinics.net/lists/platform-driver-x86/msg07220.html we will
>> parse and check WMI descriptor if contains expected data. It is because
>> WMI descriptor contains interface version number and it is needed to
>> know in next commit.
>> ===
>
> I like it way more than the previous one.
>

FWIW, the original version of this series works fine on my laptop
(after fixing the separate dmi_walk misuse bug).

--Andy

Re: [PATCH 0/2] Fixes for dell-wmi

[Darren Hart]

On Thu, Dec 24, 2015 at 10:18:44PM +0100, Pali Rohár wrote:
> This patch series adds check if Dell WMI descriptor structure is valid and
> fixes processing WMI events on devices with WMI interface version 0.

Given the discussion here, I'm dropping this one and waiting for v2.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH v2 0/2] Fixes for dell-wmi

[Pali Rohár]

This patch series adds check if Dell WMI descriptor structure is valid and
fixes processing WMI events on devices with WMI interface version 0.

After testing, second patch is good candidate for backporting into stable
kernels, but problem is that it cannot be used without first patch. So I
let decision to other people.

Pali Rohár (2):
  dell-wmi: Check if Dell WMI descriptor structure is valid
  dell-wmi: Process only one event on devices with interface version 0

 drivers/platform/x86/dell-wmi.c |   96 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 2 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Pali Rohár]

After examining existing DSDT ACPI tables of more laptops and looking
into Dell WMI document mentioned in ML dicussion archived at
http://www.spinics.net/lists/platform-driver-x86/msg07220.html we will
parse and check WMI descriptor if contains expected data. It is because
WMI descriptor contains interface version number and it is needed to
know in next commit.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   80 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 57402c4..1ad7a7b 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -2,6 +2,7 @@
  * Dell WMI hotkeys
  *
  * Copyright (C) 2008 Red Hat <mjg@redhat.com>
+ * Copyright (C) 2014-2015 Pali Rohár <pali.rohar@gmail.com>
  *
  * Portions based on wistron_btns.c:
  * Copyright (C) 2005 Miloslav Trmac <mitr@volny.cz>
@@ -38,14 +39,18 @@
 #include <acpi/video.h>
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
 MODULE_DESCRIPTION("Dell laptop WMI hotkeys driver");
 MODULE_LICENSE("GPL");
 
 #define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
+#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
 static int acpi_video;
+static u32 dell_wmi_interface_version;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
+MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
 /*
  * Certain keys are flagged as KE_IGNORE. All of these are either
@@ -422,16 +427,87 @@ static void __init find_hk_type(const struct dmi_header *dm, void *dummy)
 	}
 }
 
+/*
+ * Descriptor buffer is 128 byte long and contains:
+ *
+ *       Name             Offset  Length  Value
+ * Vendor Signature          0       4    "DELL"
+ * Object Signature          4       4    " WMI"
+ * WMI Interface Version     8       4    <version>
+ * WMI buffer length        12       4    4096
+ */
+static int __init dell_wmi_check_descriptor_buffer(void)
+{
+	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	acpi_status status;
+	u32 *buffer;
+
+	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;
+	}
+
+	obj = (union acpi_object *)out.pointer;
+	if (!obj) {
+		pr_err("Dell descriptor buffer is empty\n");
+		return -EINVAL;
+	}
+
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		pr_err("Cannot read Dell descriptor buffer\n");
+		kfree(obj);
+		return -EINVAL;
+	}
+
+	if (obj->buffer.length != 128) {
+		pr_err("Dell descriptor buffer has invalid length (%d)\n",
+			obj->buffer.length);
+		if (obj->buffer.length < 16) {
+			kfree(obj);
+			return -EINVAL;
+		}
+	}
+
+	buffer = (u32 *)obj->buffer.pointer;
+
+	if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720)
+		pr_warn("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",
+			buffer[2]);
+
+	if (buffer[3] != 4096)
+		pr_warn("Dell descriptor buffer has invalid buffer length (%d)\n",
+			buffer[3]);
+
+	dell_wmi_interface_version = buffer[2];
+
+	pr_info("Detected Dell WMI interface version %u\n",
+		dell_wmi_interface_version);
+
+	kfree(obj);
+	return 0;
+}
+
 static int __init dell_wmi_init(void)
 {
 	int err;
 	acpi_status status;
 
-	if (!wmi_has_guid(DELL_EVENT_GUID)) {
-		pr_warn("No known WMI GUID found\n");
+	if (!wmi_has_guid(DELL_EVENT_GUID) ||
+	    !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
+		pr_warn("Dell WMI GUID were not found\n");
 		return -ENODEV;
 	}
 
+	err = dell_wmi_check_descriptor_buffer();
+	if (err)
+		return err;
+
 	dmi_walk(find_hk_type, NULL);
 	acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor;
 
-- 
1.7.9.5

[PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0

[Pali Rohár]

BIOS/ACPI on devices with WMI interface version 0 does not clear buffer
before filling it. So next time when BIOS/ACPI send WMI event which is
smaller as previous then it contains garbage in buffer from previous event.

BIOS/ACPI on devices with WMI interface version 1 clears buffer and
sometimes send more events in buffer at one call.

Since commit 83fc44c32ad8 ("dell-wmi: Update code for processing WMI
events") dell-wmi process all events in buffer (and not just first).

So to prevent reading garbage from buffer we will process only first one
event on devices with WMI interface version 0.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 1ad7a7b..5db9efb 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -237,6 +237,22 @@ static void dell_wmi_notify(u32 value, void *context)
 
 	buffer_end = buffer_entry + buffer_size;
 
+	/*
+	 * BIOS/ACPI on devices with WMI interface version 0 does not clear
+	 * buffer before filling it. So next time when BIOS/ACPI send WMI event
+	 * which is smaller as previous then it contains garbage in buffer from
+	 * previous event.
+	 *
+	 * BIOS/ACPI on devices with WMI interface version 1 clears buffer and
+	 * sometimes send more events in buffer at one call.
+	 *
+	 * 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 (buffer_end > buffer_entry + buffer_entry[0] + 1)
+			buffer_end = buffer_entry + buffer_entry[0] + 1;
+
 	while (buffer_entry < buffer_end) {
 
 		len = buffer_entry[0];
-- 
1.7.9.5

Re: [PATCH 0/2] Fixes for dell-wmi

[Pali Rohár]

On Monday 04 January 2016 12:48:14 Darren Hart wrote:
> On Thu, Dec 24, 2015 at 10:18:44PM +0100, Pali Rohár wrote:
> > This patch series adds check if Dell WMI descriptor structure is valid and
> > fixes processing WMI events on devices with WMI interface version 0.
> 
> Given the discussion here, I'm dropping this one and waiting for v2.
> 

I have already sent v2 patches.

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

Re: [PATCH v2 0/2] Fixes for dell-wmi

[Darren Hart]

On Mon, Jan 04, 2016 at 10:26:34PM +0100, Pali Rohár wrote:
> This patch series adds check if Dell WMI descriptor structure is valid and
> fixes processing WMI events on devices with WMI interface version 0.
> 
> After testing, second patch is good candidate for backporting into stable
> kernels, but problem is that it cannot be used without first patch. So I
> let decision to other people.
> 
> Pali Rohár (2):
>   dell-wmi: Check if Dell WMI descriptor structure is valid
>   dell-wmi: Process only one event on devices with interface version 0
> 
>  drivers/platform/x86/dell-wmi.c |   96 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 2 deletions(-)

Thank you everyone. I have queued these to testing.

Should I add a tested-by for Michal, Andy, and Gabriele?

Do I have any Reviewed-by's to add?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2 0/2] Fixes for dell-wmi

[Gabriele Mazzotta]

2016-01-11 20:22 GMT+01:00 Darren Hart <dvhart@infradead.org>:
> On Mon, Jan 04, 2016 at 10:26:34PM +0100, Pali Rohár wrote:
>> This patch series adds check if Dell WMI descriptor structure is valid and
>> fixes processing WMI events on devices with WMI interface version 0.
>>
>> After testing, second patch is good candidate for backporting into stable
>> kernels, but problem is that it cannot be used without first patch. So I
>> let decision to other people.
>>
>> Pali Rohár (2):
>>   dell-wmi: Check if Dell WMI descriptor structure is valid
>>   dell-wmi: Process only one event on devices with interface version 0
>>
>>  drivers/platform/x86/dell-wmi.c |   96 ++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 94 insertions(+), 2 deletions(-)
>
> Thank you everyone. I have queued these to testing.
>
> Should I add a tested-by for Michal, Andy, and Gabriele?

I tested both the patches, but I'm actually touched only by the
first one. So at least of that one

Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>

> Do I have any Reviewed-by's to add?
>
> --
> Darren Hart
> Intel Open Source Technology Center

Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0

[Michał Kępień]

> BIOS/ACPI on devices with WMI interface version 0 does not clear buffer
> before filling it. So next time when BIOS/ACPI send WMI event which is
> smaller as previous then it contains garbage in buffer from previous event.
> 
> BIOS/ACPI on devices with WMI interface version 1 clears buffer and
> sometimes send more events in buffer at one call.
> 
> Since commit 83fc44c32ad8 ("dell-wmi: Update code for processing WMI
> events") dell-wmi process all events in buffer (and not just first).
> 
> So to prevent reading garbage from buffer we will process only first one
> event on devices with WMI interface version 0.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/platform/x86/dell-wmi.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 1ad7a7b..5db9efb 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -237,6 +237,22 @@ static void dell_wmi_notify(u32 value, void *context)
>  
>  	buffer_end = buffer_entry + buffer_size;
>  
> +	/*
> +	 * BIOS/ACPI on devices with WMI interface version 0 does not clear
> +	 * buffer before filling it. So next time when BIOS/ACPI send WMI event
> +	 * which is smaller as previous then it contains garbage in buffer from
> +	 * previous event.
> +	 *
> +	 * BIOS/ACPI on devices with WMI interface version 1 clears buffer and
> +	 * sometimes send more events in buffer at one call.
> +	 *
> +	 * 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 (buffer_end > buffer_entry + buffer_entry[0] + 1)
> +			buffer_end = buffer_entry + buffer_entry[0] + 1;

Wouldn't it be a bit more clear if we clamped buffer_size before setting
buffer_end?  E.g. like this:

	if (buffer_size == 0)
		return;

	if (dell_wmi_interface_version == 0 &&
	    buffer_size > buffer_entry[0] + 1)
		buffer_size = buffer_entry[0] + 1;

	buffer_end = buffer_entry + buffer_size;

If I understand correctly, the second check on the first line added by
your patch prevents a bad dereference when accesing buffer_entry[0].
The only case when that may happen is when buffer_size is 0, which means
we got notified with rubbish anyway, so we can just return (perhaps with
a log message, which I omitted above).

One more minor nit: you should probably decide between "first" and "one"
as the phrase "only first one event" (found both in the commit message
and in the code comment) sounds incorrect to me.

-- 
Best regards,
Michał Kępień

Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0

[Pali Rohár]

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

On Tuesday 12 January 2016 12:14:39 Michał Kępień wrote:
> > BIOS/ACPI on devices with WMI interface version 0 does not clear
> > buffer before filling it. So next time when BIOS/ACPI send WMI
> > event which is smaller as previous then it contains garbage in
> > buffer from previous event.
> > 
> > BIOS/ACPI on devices with WMI interface version 1 clears buffer and
> > sometimes send more events in buffer at one call.
> > 
> > Since commit 83fc44c32ad8 ("dell-wmi: Update code for processing
> > WMI events") dell-wmi process all events in buffer (and not just
> > first).
> > 
> > So to prevent reading garbage from buffer we will process only
> > first one event on devices with WMI interface version 0.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > 
> >  drivers/platform/x86/dell-wmi.c |   16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/dell-wmi.c
> > b/drivers/platform/x86/dell-wmi.c index 1ad7a7b..5db9efb 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -237,6 +237,22 @@ static void dell_wmi_notify(u32 value, void
> > *context)
> > 
> >  	buffer_end = buffer_entry + buffer_size;
> > 
> > +	/*
> > +	 * BIOS/ACPI on devices with WMI interface version 0 does not
> > clear +	 * buffer before filling it. So next time when BIOS/ACPI
> > send WMI event +	 * which is smaller as previous then it contains
> > garbage in buffer from +	 * previous event.
> > +	 *
> > +	 * BIOS/ACPI on devices with WMI interface version 1 clears
> > buffer and +	 * sometimes send more events in buffer at one call.
> > +	 *
> > +	 * 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 (buffer_end > buffer_entry + buffer_entry[0] + 1)
> > +			buffer_end = buffer_entry + buffer_entry[0] + 1;
> 
> Wouldn't it be a bit more clear if we clamped buffer_size before
> setting buffer_end?  E.g. like this:
> 
> 	if (buffer_size == 0)
> 		return;
> 
> 	if (dell_wmi_interface_version == 0 &&
> 	    buffer_size > buffer_entry[0] + 1)
> 		buffer_size = buffer_entry[0] + 1;
> 
> 	buffer_end = buffer_entry + buffer_size;

Before return adds correct cleanup part and code will be same as my 
original patch.

So if more people think that your code is cleaner I'm OK with replacing 
it.

> If I understand correctly, the second check on the first line added
> by your patch prevents a bad dereference when accesing
> buffer_entry[0].

Yes. Same check (buffer_entry < buffer_end) is used in next whole loop, 
so I uses it in my patch too...

> The only case when that may happen is when
> buffer_size is 0,

In this one case, yes. But you can see that buffer_entry variable is 
changing (increasing pointer offset), it means that it points to some 
entry in buffer.

> which means we got notified with rubbish anyway,
> so we can just return (perhaps with a log message, which I omitted
> above).
> 
> One more minor nit: you should probably decide between "first" and
> "one" as the phrase "only first one event" (found both in the commit
> message and in the code comment) sounds incorrect to me.

Feel free to correct commit message, I'm not very good in english...

It should mean something like this... in buffer received by bios can be 
more events. That while loop iterate over events. And this my patch on 
machines with wmi version 0 will process only *one* event. And that 
event is *first* in buffer.

-- 
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 v2 2/2] dell-wmi: Process only one event on devices with interface version 0

[Michał Kępień]

> > Wouldn't it be a bit more clear if we clamped buffer_size before
> > setting buffer_end?  E.g. like this:
> > 
> > 	if (buffer_size == 0)
> > 		return;
> > 
> > 	if (dell_wmi_interface_version == 0 &&
> > 	    buffer_size > buffer_entry[0] + 1)
> > 		buffer_size = buffer_entry[0] + 1;
> > 
> > 	buffer_end = buffer_entry + buffer_size;
> 
> Before return adds correct cleanup part and code will be same as my 
> original patch.
> 
> So if more people think that your code is cleaner I'm OK with replacing 
> it.

Both solutions are fine and I realize I'm a bit late to the party as you
posted the original patch almost 3 weeks ago, so I don't want to delay
it any longer.  I think it's just a matter of deciding whether to
enforce the buffer size limit using buffer_size or buffer_end.  As the
first option involves a little bit less writing, I thought I'd suggest
it.

> > One more minor nit: you should probably decide between "first" and
> > "one" as the phrase "only first one event" (found both in the commit
> > message and in the code comment) sounds incorrect to me.
> 
> Feel free to correct commit message, I'm not very good in english...
> 
> It should mean something like this... in buffer received by bios can be 
> more events. That while loop iterate over events. And this my patch on 
> machines with wmi version 0 will process only *one* event. And that 
> event is *first* in buffer.

Don't worry, I understood your intentions from the commit message, so I
don't think it's worth posting a v3 only to correct minor stylistic
errors.

-- 
Best regards,
Michał Kępień

Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0

[Darren Hart]

On Tue, Jan 12, 2016 at 09:12:46PM +0100, Michał Kępień wrote:
> > > Wouldn't it be a bit more clear if we clamped buffer_size before
> > > setting buffer_end?  E.g. like this:
> > > 
> > > 	if (buffer_size == 0)
> > > 		return;
> > > 
> > > 	if (dell_wmi_interface_version == 0 &&
> > > 	    buffer_size > buffer_entry[0] + 1)
> > > 		buffer_size = buffer_entry[0] + 1;
> > > 
> > > 	buffer_end = buffer_entry + buffer_size;
> > 
> > Before return adds correct cleanup part and code will be same as my 
> > original patch.
> > 
> > So if more people think that your code is cleaner I'm OK with replacing 
> > it.
> 
> Both solutions are fine and I realize I'm a bit late to the party as you
> posted the original patch almost 3 weeks ago, so I don't want to delay
> it any longer.  I think it's just a matter of deciding whether to
> enforce the buffer size limit using buffer_size or buffer_end.  As the
> first option involves a little bit less writing, I thought I'd suggest
> it.
> 
> > > One more minor nit: you should probably decide between "first" and
> > > "one" as the phrase "only first one event" (found both in the commit
> > > message and in the code comment) sounds incorrect to me.
> > 
> > Feel free to correct commit message, I'm not very good in english...
> > 
> > It should mean something like this... in buffer received by bios can be 
> > more events. That while loop iterate over events. And this my patch on 
> > machines with wmi version 0 will process only *one* event. And that 
> > event is *first* in buffer.
> 
> Don't worry, I understood your intentions from the commit message, so I
> don't think it's worth posting a v3 only to correct minor stylistic
> errors.
> 
> -- 
> Best regards,
> Michał Kępień

I've cleaned up that bit.

-- 
Darren Hart
Intel Open Source Technology Center