Skip to contentSkip to conversation

RFC: platform/x86: wmi: Fix check for method instance number

PATCH In review Pali Rohár 7 participants 24 messages May 27, 2017 – Aug 6, 2017 x86 platform drivers

lore
Share & tools
lore

Pali Rohár

author
#0 lore raw

PATCH 1/1 RFC: platform/x86: wmi: Fix check for method instance number

instance_count defines number of instances of data block and instance itself is indexed from zero, which means first instance has number 0. Therefore check for invalid instance should be non-strict inequality.

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

---
I'm marking this patch as RFC because it is not tested at all and probably could break existing WMI drivers. Some WMI drivers pass instance number 1 and I'm not sure if ACPI-WMI bytecode for those machines has really two instances. In more cases ACPI-WMI bytecode does not check instance number if supports only one instance. So then any instance id can be used for correct execution of ACPI-WMI method.

So this patch is open for discussion.

---
 drivers/platform/x86/wmi.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/platform/x86/wmi.c +3 -3
index cd7045f..df63037 100644
@@ -191,7 +191,7 @@ acpi_status wmi_evaluate_method(const char *guid_string, u8 instance,
 	if (!(block->flags & ACPI_WMI_METHOD))
 		return AE_BAD_DATA;
 
-	if (block->instance_count < instance)
+	if (block->instance_count <= instance)
 		return AE_BAD_PARAMETER;
 
 	input.count = 2;

@@ -250,7 +250,7 @@ struct acpi_buffer *out)
 	block = &wblock->gblock;
 	handle = wblock->handle;
 
-	if (block->instance_count < instance)
+	if (block->instance_count <= instance)
 		return AE_BAD_PARAMETER;
 
 	/* Check GUID is a data block */

@@ -323,7 +323,7 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,
 	block = &wblock->gblock;
 	handle = wblock->handle;
 
-	if (block->instance_count < instance)
+	if (block->instance_count <= instance)
 		return AE_BAD_PARAMETER;
 
 	/* Check GUID is a data block */
Signature · 1 line
1.7.9.5
author
#1 lore raw
Quoted 14 lines · On Saturday 27 May 2017 13:55:34 Pali Rohár wrote:

instance_count defines number of instances of data block and instance itself is indexed from zero, which means first instance has number 0. Therefore check for invalid instance should be non-strict inequality.

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

---
I'm marking this patch as RFC because it is not tested at all and probably could break existing WMI drivers. Some WMI drivers pass instance number 1 and I'm not sure if ACPI-WMI bytecode for those machines has really two instances. In more cases ACPI-WMI bytecode does not check instance number if supports only one instance. So then any instance id can be used for correct execution of ACPI-WMI method.

Quoted 1 line

So this patch is open for discussion.

Hi! Any comments?

Quoted 35 lines
---
 drivers/platform/x86/wmi.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/platform/x86/wmi.c +3 -3
index cd7045f..df63037 100644
@@ -191,7 +191,7 @@ acpi_status wmi_evaluate_method(const char
 *guid_string, u8 instance, if (!(block->flags & ACPI_WMI_METHOD))
 		return AE_BAD_DATA;
 
-	if (block->instance_count < instance)
+	if (block->instance_count <= instance)
 		return AE_BAD_PARAMETER;
 
 	input.count = 2;

@@ -250,7 +250,7 @@ struct acpi_buffer *out)
 	block = &wblock->gblock;
 	handle = wblock->handle;
 
-	if (block->instance_count < instance)
+	if (block->instance_count <= instance)
 		return AE_BAD_PARAMETER;
 
 	/* Check GUID is a data block */

@@ -323,7 +323,7 @@ acpi_status wmi_set_block(const char
 *guid_string, u8 instance, block = &wblock->gblock;
 	handle = wblock->handle;
 
-	if (block->instance_count < instance)
+	if (block->instance_count <= instance)
 		return AE_BAD_PARAMETER;
 
 	/* Check GUID is a data block */
Signature · 2 lines
Pali Rohár
[email protected]
#2 lore raw
Quoted 15 lines · Pali Rohár
Quoted 14 lines · On Saturday 27 May 2017 13:55:34 Pali Rohár wrote:

instance_count defines number of instances of data block and instance itself is indexed from zero, which means first instance has number 0. Therefore check for invalid instance should be non-strict inequality.

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

---
I'm marking this patch as RFC because it is not tested at all and probably could break existing WMI drivers. Some WMI drivers pass instance number 1 and I'm not sure if ACPI-WMI bytecode for those machines has really two instances. In more cases ACPI-WMI bytecode does not check instance number if supports only one instance. So then any instance id can be used for correct execution of ACPI-WMI method.

Quoted 3 lines
Quoted 1 line

So this patch is open for discussion.

Hi! Any comments?

Hi Pali,

This change appears correct to me, but your comment about this parameter being ignored by ACPI-WMI is definitely concerning. Since this doesn't address a specific failure report, and it has the potential to break functional drivers, I wouldn't want to merge it without some evidence that those drivers still work.

I'd suggest reaching out to the maintainers and contributors to the drivers you mention to request some help in testing.

Signature · 2 lines
Darren Hart
VMware Open Source Technology Center
author
#3 lore raw
Quoted 21 lines · On Tuesday 13 June 2017 18:49:51 Darren Hart wrote:
Quoted 16 lines · Pali Rohár
Quoted 15 lines · On Saturday 27 May 2017 13:55:34 Pali Rohár wrote:

instance_count defines number of instances of data block and instance itself is indexed from zero, which means first instance has number 0. Therefore check for invalid instance should be non-strict inequality.

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

---
I'm marking this patch as RFC because it is not tested at all and probably could break existing WMI drivers. Some WMI drivers pass instance number 1 and I'm not sure if ACPI-WMI bytecode for those machines has really two instances. In more cases ACPI-WMI bytecode does not check instance number if supports only one instance. So then any instance id can be used for correct execution of ACPI-WMI method.

Quoted 3 lines
Quoted 1 line

So this patch is open for discussion.

Hi! Any comments?

Quoted 7 lines

Hi Pali,

This change appears correct to me, but your comment about this parameter being ignored by ACPI-WMI is definitely concerning. Since this doesn't address a specific failure report, and it has the potential to break functional drivers, I wouldn't want to merge it without some evidence that those drivers still work.

I agree that it should not be merged without any other testing or discussion. Reason why I marked it as RFC.

ACPI bytecode (which implements WMI functions) is often ignoring instance method if there is only one instance. So it does not have to decide which instance to call based on parameter.

IIRC it is also stated in that MSDN documentation.

Quoted 2 lines

I'd suggest reaching out to the maintainers and contributors to the drivers you mention to request some help in testing.

Seems sane. Grep for all methods with instance number different as zero (or just number one -- which can be suspicious as somebody could thought that indexing is from one, not zer) and try to receive ACPI/BMOF data and verify it.

Signature · 2 lines
Pali Rohár
[email protected]
#4 lore raw
Quoted 30 lines · Pali Rohár
Quoted 21 lines · On Tuesday 13 June 2017 18:49:51 Darren Hart wrote:
Quoted 16 lines · Pali Rohár
Quoted 15 lines · On Saturday 27 May 2017 13:55:34 Pali Rohár wrote:

instance_count defines number of instances of data block and instance itself is indexed from zero, which means first instance has number 0. Therefore check for invalid instance should be non-strict inequality.

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

---
I'm marking this patch as RFC because it is not tested at all and probably could break existing WMI drivers. Some WMI drivers pass instance number 1 and I'm not sure if ACPI-WMI bytecode for those machines has really two instances. In more cases ACPI-WMI bytecode does not check instance number if supports only one instance. So then any instance id can be used for correct execution of ACPI-WMI method.

Quoted 3 lines
Quoted 1 line

So this patch is open for discussion.

Hi! Any comments?

Quoted 7 lines

Hi Pali,

This change appears correct to me, but your comment about this parameter being ignored by ACPI-WMI is definitely concerning. Since this doesn't address a specific failure report, and it has the potential to break functional drivers, I wouldn't want to merge it without some evidence that those drivers still work.

Quoted 8 lines

I agree that it should not be merged without any other testing or discussion. Reason why I marked it as RFC.

ACPI bytecode (which implements WMI functions) is often ignoring instance method if there is only one instance. So it does not have to decide which instance to call based on parameter.

IIRC it is also stated in that MSDN documentation.

That is my reading of it as well:

"One parameter is passed to the method--the index of the instance, which is of type ULONG. Data blocks registered with only a single instance can ignore the parameter."

https://msdn.microsoft.com/en-us/library/windows/hardware/dn614028(v=vs.85).aspx

The "can" instead of "shall" makes our job harder. We could special case the instance_count == 1 case and either skip the test (relying on the WMI code to ignore or return an appropriate error - risky) or we could force it to 0, which should always work per the specification, but it's possible some firmware out there is just broken and misuses this input... oh man... I bet that exists somewhere... "we can ignore instance_count, so let's use it as a simple integer input for FOO.... ugh.

Quoted 8 lines
Quoted 3 lines

I'd suggest reaching out to the maintainers and contributors to the drivers you mention to request some help in testing.

Seems sane. Grep for all methods with instance number different as zero (or just number one -- which can be suspicious as somebody could thought that indexing is from one, not zer) and try to receive ACPI/BMOF data and verify it.

This would still be the ideal solution, verify we can do the right thing without breaking existing drivers. Agreed.

Signature · 2 lines
Darren Hart
VMware Open Source Technology Center
author
#5 lore raw
Quoted 5 lines · On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
Quoted 4 lines · Pali Rohár
Quoted 2 lines · On Tuesday 13 June 2017 18:49:51 Darren Hart wrote:

I'd suggest reaching out to the maintainers and contributors to the drivers you mention to request some help in testing.

Quoted 7 lines
Quoted 4 lines

Seems sane. Grep for all methods with instance number different as zero (or just number one -- which can be suspicious as somebody could thought that indexing is from one, not zer) and try to receive ACPI/BMOF data and verify it.

This would still be the ideal solution, verify we can do the right thing without breaking existing drivers. Agreed.

Here is all usage:

Function wmi_set_block:

msi-wmi.c:
instance=0 /* Instance 0 is "set backlight" */

tc1100-wmi.c:
instance=TC1100_INSTANCE_WIRELESS /* defined as 1 */ instance=TC1100_INSTANCE_JOGDIAL /* defined as 2 */

Function wmi_query_block:

acer-wmi.c:
instance=1 /* no comment why, guid=95764E09-FB56-4E83-B31A-37761F60994A */

dell-wmi.c:
instance=0

msi-wmi.c:
instance=1 /* Instance 1 is "get backlight", cmp with DSDT */

surface3-wmi.c:
instance=0

tc1100-wmi.c:
(same as in wmi_set_block)

Function wmi_evaluate_method:

acer-wmi.c:
instance=1 /* no comment why, guid=67C3371D-95A3-4C37-BB61-DD47B491DAAB */ instance=1 /* no comment why, guid=6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3 */ instance=0

alienware-wmi.c:
instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no comment why, guid=A80593CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-B622A1EF5492 */

asus-wmi.c:
instance=1 /* no comment why, guid=97845ED0-4E6D-11DE-8A39-0800200C9A66 */

dell-wmi-led.c:
instance=1 /* no comment why, guid=F6E4FE6E-909D-47cb-8BAB-C9F6F2F8D396 */

hp-wmi.c:
instance=0

mxm-wmi.c:
instance=1 /* no comment why, guid=F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0 */

So problematic drivers which use instance=1 without any comments are:

acer-wmi
alienware-wmi
asus-wmi
dell-wmi-led
mxm-wmi

Signature · 2 lines
Pali Rohár
[email protected]
#6 lore raw
Quoted 11 lines · Pali Rohár
Quoted 10 lines · On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
Quoted 8 lines · Pali Rohár
Quoted 2 lines · On Tuesday 13 June 2017 18:49:51 Darren Hart wrote:

I'd suggest reaching out to the maintainers and contributors to the drivers you mention to request some help in testing.

Seems sane. Grep for all methods with instance number different as zero (or just number one -- which can be suspicious as somebody could thought that indexing is from one, not zer) and try to receive ACPI/BMOF data and verify it.

Quoted 5 lines
Quoted 2 lines

This would still be the ideal solution, verify we can do the right thing without breaking existing drivers. Agreed.

Here is all usage:

Thanks for pulling this together Pali.

...

Quoted 8 lines

So problematic drivers which use instance=1 without any comments are:

acer-wmi
alienware-wmi
asus-wmi
dell-wmi-led
mxm-wmi

I'd suggest adding a WARN_ONCE() when instance >= instance_count (I guess only == concerns us) as a way to draw out any problematic usage. We can let that go while we try to collect data from the other driver maintainers and see if it generates any more hits.

Signature · 2 lines
Darren Hart
VMware Open Source Technology Center
author
#7 lore raw
In reply to Pali Rohár

Mario, are you able to check if instance number passed to wmi_evaluate_method in following dell WMI drivers is correct and should be really 1?

I suspect that it should be zero, as instance number is indexed from zero.

There is no comment in those dell WMI drivers why it is 1, nor what 1 means.

Ideally it needs to be checked in ACPI byte code, MOF file and WDG dump.

Quoted 1 line · On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:

Function wmi_evaluate_method:

...

Quoted 4 lines

alienware-wmi.c:
instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no comment why, guid=A80593CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-B622A1EF5492 */

...

Quoted 2 lines

dell-wmi-led.c:
instance=1 /* no comment why, guid=F6E4FE6E-909D-47cb-8BAB-C9F6F2F8D396 */

Signature · 2 lines
Pali Rohár
[email protected]
#8 lore raw
Quoted 14 lines

-----Original Message-----
From: Pali Rohár [mailto:[email protected]]
Sent: Thursday, June 15, 2017 8:59 AM
To: Limonciello, Mario <[email protected]>; Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>; Andy Lutomirski <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Mario, are you able to check if instance number passed to wmi_evaluate_method in following dell WMI drivers is correct and should be really 1?

Quoted 8 lines

I suspect that it should be zero, as instance number is indexed from zero.

There is no comment in those dell WMI drivers why it is 1, nor what 1 means.

Ideally it needs to be checked in ACPI byte code, MOF file and WDG dump.

I think you're likely correct. I don't have a box that supports alienware-wmi or dell-wmi-led.c handy at the current moment to confirm this hypothesis though. I'll confirm this later.

I didn't realize it was zero indexed when I wrote alienware-wmi, and I'm guessing the author of dell-wmi-led didn't either.

The reason it's probably working is the ACPI byte code isn't actually checking the instance since most times _WDG will only call out one instance.

Quoted 18 lines · On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
Quoted 1 line

Function wmi_evaluate_method:

...

Quoted 2 lines

alienware-wmi.c:
instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-

B622A1EF5492 */

Quoted 1 line

instance=1 /* no comment why, guid=A80593CE-A997-11DA-B012-

B622A1EF5492 */

Quoted 1 line

instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-

B622A1EF5492 */
...

Quoted 2 lines

dell-wmi-led.c:
instance=1 /* no comment why, guid=F6E4FE6E-909D-47cb-8BAB-

C9F6F2F8D396 */

--
Pali Rohár
[email protected]

#9 lore raw
In reply to Pali Rohár
Quoted 36 lines

-----Original Message-----
From: Limonciello, Mario
Sent: Thursday, June 15, 2017 10:16 AM
To: 'Pali Rohár' <[email protected]>; Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>; Andy Lutomirski <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Quoted 6 lines

-----Original Message-----
From: Pali Rohár [mailto:[email protected]]
Sent: Thursday, June 15, 2017 8:59 AM
To: Limonciello, Mario <[email protected]>; Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>; Andy Lutomirski

<[email protected]>;

Quoted 8 lines

[email protected]; [email protected]
Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Mario, are you able to check if instance number passed to wmi_evaluate_method in following dell WMI drivers is correct and should be really 1?

Quoted 8 lines

I suspect that it should be zero, as instance number is indexed from zero.

There is no comment in those dell WMI drivers why it is 1, nor what 1 means.

Ideally it needs to be checked in ACPI byte code, MOF file and WDG dump.

I think you're likely correct. I don't have a box that supports alienware-wmi or dell-wmi-led.c handy at the current moment to confirm this hypothesis though. I'll confirm this later.

Quoted 5 lines

I didn't realize it was zero indexed when I wrote alienware-wmi, and I'm guessing the author of dell-wmi-led didn't either.

The reason it's probably working is the ACPI byte code isn't actually checking the instance since most times _WDG will only call out one instance.

I confirmed you're correct. Switching instance over to 0 works properly on an ASM200 (supported by alienware-wmi).

Since only one instance is supported the ASL doesn't check Arg0 at all. snippet:

            Method (WMAX, 3, NotSerialized)
            {
                If ((Arg1 == One))
                {
                    CreateByteField (Arg2, Zero, SOUR)
                    If ((SOUR == One))
                    {
                        GU01 = (GU01 | 0x80)
                        GIO1 = (GIO1 & 0x7F)
                        GL01 = (GL01 & 0x7F)
                        Return (Zero)
                    }

                    If ((SOUR == 0x02))
                    {
                        GU01 = (GU01 | 0x80)
                        GIO1 = (GIO1 & 0x7F)
                        GL01 = (GL01 | 0x80)
                        Return (Zero)
                    }

                    If ((SOUR == 0x03))
                    {
                        If ((CN00 == Zero))
                        {
                            CN00 = One
                            If (((GL01 & 0x80) == 0x80))
                            {
                                GL01 &= 0x7F
                                Return (Zero)
                            }

                            If (((GL01 & 0x80) == Zero))
                            {
                                GL01 |= 0x80
                                Return (Zero)
                            }
                        }

                        If ((CN00 == One))
                        {
                            CN00 = Zero
                            Return (Zero)
                        }

                        Return (One)
                    }

                    Return (One)
                }
Quoted 19 lines
Quoted 19 lines · On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
Quoted 1 line

Function wmi_evaluate_method:

...

Quoted 2 lines

alienware-wmi.c:
instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-

B622A1EF5492 */

Quoted 1 line

instance=1 /* no comment why, guid=A80593CE-A997-11DA-B012-

B622A1EF5492 */

Quoted 1 line

instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-

B622A1EF5492 */
...

Quoted 2 lines

dell-wmi-led.c:
instance=1 /* no comment why, guid=F6E4FE6E-909D-47cb-8BAB-

C9F6F2F8D396 */

--
Pali Rohár
[email protected]

author
#10 lore raw
Quoted 43 lines · On Friday 16 June 2017 18:33:54 [email protected] wrote:
Quoted 43 lines

-----Original Message-----
From: Limonciello, Mario
Sent: Thursday, June 15, 2017 10:16 AM
To: 'Pali Rohár' <[email protected]>; Darren Hart <[email protected]> Cc: Andy Shevchenko <[email protected]>; Andy Lutomirski <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Quoted 6 lines

-----Original Message-----
From: Pali Rohár [mailto:[email protected]]
Sent: Thursday, June 15, 2017 8:59 AM
To: Limonciello, Mario <[email protected]>; Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>; Andy Lutomirski

<[email protected]>;

Quoted 8 lines

[email protected]; [email protected]
Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Mario, are you able to check if instance number passed to wmi_evaluate_method in following dell WMI drivers is correct and should be really 1?

Quoted 8 lines

I suspect that it should be zero, as instance number is indexed from zero.

There is no comment in those dell WMI drivers why it is 1, nor what 1 means.

Ideally it needs to be checked in ACPI byte code, MOF file and WDG dump.

I think you're likely correct. I don't have a box that supports alienware-wmi or dell-wmi-led.c handy at the current moment to confirm this hypothesis though. I'll confirm this later.

I didn't realize it was zero indexed when I wrote alienware-wmi, and I'm guessing the author of dell-wmi-led didn't either.

Quoted 6 lines
Quoted 3 lines

The reason it's probably working is the ACPI byte code isn't actually checking the instance since most times _WDG will only call out one instance.

I confirmed you're correct. Switching instance over to 0 works properly on an ASM200 (supported by alienware-wmi).

Can you check what is the value in the instance_count in _WDG?

Signature · 2 lines
Pali Rohár
[email protected]
#11 lore raw
Quoted 53 lines

-----Original Message-----
From: Pali Rohár [mailto:[email protected]]
Sent: Saturday, June 17, 2017 11:35 AM
To: Limonciello, Mario <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; platform-driver- [email protected]; [email protected]
Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Quoted 43 lines · On Friday 16 June 2017 18:33:54 [email protected] wrote:
Quoted 43 lines

-----Original Message-----
From: Limonciello, Mario
Sent: Thursday, June 15, 2017 10:16 AM
To: 'Pali Rohár' <[email protected]>; Darren Hart <[email protected]> Cc: Andy Shevchenko <[email protected]>; Andy Lutomirski <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Quoted 6 lines

-----Original Message-----
From: Pali Rohár [mailto:[email protected]]
Sent: Thursday, June 15, 2017 8:59 AM
To: Limonciello, Mario <[email protected]>; Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>; Andy Lutomirski

<[email protected]>;

Quoted 8 lines

[email protected]; [email protected]
Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Mario, are you able to check if instance number passed to wmi_evaluate_method in following dell WMI drivers is correct and should be really 1?

Quoted 8 lines

I suspect that it should be zero, as instance number is indexed from zero.

There is no comment in those dell WMI drivers why it is 1, nor what 1 means.

Ideally it needs to be checked in ACPI byte code, MOF file and WDG dump.

I think you're likely correct. I don't have a box that supports alienware-wmi or dell-wmi-led.c handy at the current moment to confirm this hypothesis though. I'll confirm this later.

I didn't realize it was zero indexed when I wrote alienware-wmi, and I'm guessing the author of dell-wmi-led didn't either.

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

The reason it's probably working is the ACPI byte code isn't actually checking the instance since most times _WDG will only call out one instance.

I confirmed you're correct. Switching instance over to 0 works properly on an ASM200 (supported by alienware-wmi).

Can you check what is the value in the instance_count in _WDG?

Instance count is 1.

Thanks,

author
#12 lore raw
Quoted 59 lines · On Wednesday 21 June 2017 23:52:12 [email protected] wrote:
Quoted 59 lines

-----Original Message-----
From: Pali Rohár [mailto:[email protected]]
Sent: Saturday, June 17, 2017 11:35 AM
To: Limonciello, Mario <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; platform-driver- [email protected]; [email protected]
Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Quoted 49 lines · On Friday 16 June 2017 18:33:54 [email protected] wrote:
Quoted 48 lines

-----Original Message-----
From: Limonciello, Mario
Sent: Thursday, June 15, 2017 10:16 AM
To: 'Pali Rohár' <[email protected]>; Darren Hart <[email protected]> Cc: Andy Shevchenko <[email protected]>; Andy Lutomirski <[email protected]>; [email protected]; [email protected] Subject: RE: [PATCH] RFC:
platform/x86: wmi: Fix check for method instance number

Quoted 6 lines

-----Original Message-----
From: Pali Rohár [mailto:[email protected]]
Sent: Thursday, June 15, 2017 8:59 AM
To: Limonciello, Mario <[email protected]>; Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>; Andy Lutomirski

<[email protected]>;

Quoted 8 lines

[email protected]; [email protected] Subject: Re: [PATCH] RFC:
platform/x86: wmi: Fix check for method instance number

Mario, are you able to check if instance number passed to wmi_evaluate_method in following dell WMI drivers is correct and should be really 1?

Quoted 8 lines

I suspect that it should be zero, as instance number is indexed from zero.

There is no comment in those dell WMI drivers why it is 1, nor what 1 means.

Ideally it needs to be checked in ACPI byte code, MOF file and WDG dump.

I think you're likely correct. I don't have a box that supports alienware-wmi or dell-wmi-led.c handy at the current moment to confirm this hypothesis though. I'll confirm this later.

I didn't realize it was zero indexed when I wrote alienware-wmi, and I'm guessing the author of dell-wmi-led didn't either.

The reason it's probably working is the ACPI byte code isn't actually checking the instance since most times _WDG will only call out one instance.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

I confirmed you're correct. Switching instance over to 0 works properly on an ASM200 (supported by alienware-wmi).

Can you check what is the value in the instance_count in _WDG?

Instance count is 1.

Thanks,

Ok, in this case instance number should be zero in driver code. Will you send patch for fixing it?

Signature · 2 lines
Pali Rohár
[email protected]
author
#13 lore raw
In reply to Pali Rohár
Quoted 5 lines

So problematic drivers which use instance=1 without any comments are:

acer-wmi
asus-wmi
mxm-wmi

Adding authors & maintainers of those drivers in loop.

WMI instance number is indexed from zero and therefore first instance number is 0, not 1. Can you check if for drivers and wmi functions (specified below) is really correct to use WMI instance number one?

In case in _WDG is specified for particular GUID that instance_count is 1, it means the only allowed instance number is 0 (first and the only one).

In some cases, when there is only one instance for WMI method, ACPI WMI bytecode does not check instance number, so any passed value is accepted by ACPI. But in current patch I'm trying to fix check for valid instance number based on instance_count information from _WDG.

So I need to know if nothing would be broken. And in case those driver issue invalid/incorrect instance number, they needs to be fixed.

Can you look at it? Simple look into _WDG dump should be enough... just check if instance number called from wmi driver is less then instance_count from _WDG.

Quoted 17 lines · On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:

Function wmi_query_block:

acer-wmi.c:
instance=1 /* no comment why, guid=95764E09-FB56-4E83-B31A-37761F60994A */

Function wmi_evaluate_method:

acer-wmi.c:
instance=1 /* no comment why, guid=67C3371D-95A3-4C37-BB61-DD47B491DAAB */ instance=1 /* no comment why, guid=6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3 */

asus-wmi.c:
instance=1 /* no comment why, guid=97845ED0-4E6D-11DE-8A39-0800200C9A66 */

mxm-wmi.c:
instance=1 /* no comment why, guid=F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0 */

Signature · 2 lines
Pali Rohár
[email protected]
#14 lore raw

Hi Pali,

Quoted 21 lines · Pali Rohár
Quoted 5 lines

So problematic drivers which use instance=1 without any comments are:

acer-wmi
asus-wmi
mxm-wmi

Adding authors & maintainers of those drivers in loop.

WMI instance number is indexed from zero and therefore first instance number is 0, not 1. Can you check if for drivers and wmi functions (specified below) is really correct to use WMI instance number one?

In case in _WDG is specified for particular GUID that instance_count is 1, it means the only allowed instance number is 0 (first and the only one).

In some cases, when there is only one instance for WMI method, ACPI WMI bytecode does not check instance number, so any passed value is accepted by ACPI. But in current patch I'm trying to fix check for valid instance number based on instance_count information from _WDG.

Quoted 20 lines

So I need to know if nothing would be broken. And in case those driver issue invalid/incorrect instance number, they needs to be fixed.

Can you look at it? Simple look into _WDG dump should be enough... just check if instance number called from wmi driver is less then instance_count from _WDG.

Quoted 12 lines · On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:

Function wmi_query_block:

acer-wmi.c:
instance=1 /* no comment why, guid=95764E09-FB56-4E83-B31A-37761F60994A */

Function wmi_evaluate_method:

acer-wmi.c:
instance=1 /* no comment why, guid=67C3371D-95A3-4C37-BB61-DD47B491DAAB */ instance=1 /* no comment why, guid=6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3 */

I have checked DSDT dump on my hand, I think that your fixing is resonable. I will send patch to fix acer-wmi driver.

Thanks
Joey Lee

author
#15 lore raw
In reply to Pali Rohár
Quoted 7 lines · On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
Quoted 5 lines

So problematic drivers which use instance=1 without any comments are:

acer-wmi
asus-wmi
mxm-wmi

Adding authors & maintainers of those drivers in loop.

Hi!

Dell drivers and acer-wmi are fixed now. So only asus-wmi and mxm-wmi needs to be investigated.

Adding more people who developed those drivers recently in loop. Can you check if instance number is used correctly or not?

Quoted 39 lines

WMI instance number is indexed from zero and therefore first instance number is 0, not 1. Can you check if for drivers and wmi functions (specified below) is really correct to use WMI instance number one?

In case in _WDG is specified for particular GUID that instance_count is 1, it means the only allowed instance number is 0 (first and the only one).

In some cases, when there is only one instance for WMI method, ACPI WMI bytecode does not check instance number, so any passed value is accepted by ACPI. But in current patch I'm trying to fix check for valid instance number based on instance_count information from _WDG.

So I need to know if nothing would be broken. And in case those driver issue invalid/incorrect instance number, they needs to be fixed.

Can you look at it? Simple look into _WDG dump should be enough... just check if instance number called from wmi driver is less then instance_count from _WDG.

Quoted 17 lines · On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:

Function wmi_query_block:

acer-wmi.c:
instance=1 /* no comment why, guid=95764E09-FB56-4E83-B31A-37761F60994A */

Function wmi_evaluate_method:

acer-wmi.c:
instance=1 /* no comment why, guid=67C3371D-95A3-4C37-BB61-DD47B491DAAB */ instance=1 /* no comment why, guid=6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3 */

asus-wmi.c:
instance=1 /* no comment why, guid=97845ED0-4E6D-11DE-8A39-0800200C9A66 */

mxm-wmi.c:
instance=1 /* no comment why, guid=F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0 */

Signature · 2 lines
Pali Rohár
[email protected]
#16 lore raw

----- Original Message -----

Quoted 18 lines

From: "Pali Rohár" <[email protected]>
To: "Chun-Yi Lee" <[email protected]>, "Corentin Chary" <[email protected]>, [email protected], "Dave Airlie" <[email protected]>, "Oleksij Rempel" <[email protected]>, "João Paulo Rechi Vita" <[email protected]>
Cc: "Darren Hart" <[email protected]>, "Andy Shevchenko" <[email protected]>, "Andy Lutomirski" <[email protected]>, [email protected], [email protected]
Sent: Wednesday, 5 July, 2017 7:51:13 PM
Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Quoted 7 lines · On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
Quoted 5 lines

So problematic drivers which use instance=1 without any comments are:

acer-wmi
asus-wmi
mxm-wmi

Adding authors & maintainers of those drivers in loop.

Quoted 8 lines

Hi!

Dell drivers and acer-wmi are fixed now. So only asus-wmi and mxm-wmi needs to be investigated.

Adding more people who developed those drivers recently in loop. Can you check if instance number is used correctly or not?

I've no memory of why I picked 1 or 0, I probably cut-n-paste it from somewhere else.

Dave.

author
#17 lore raw
Quoted 29 lines · On Wednesday 05 July 2017 21:30:35 David Airlie wrote:

----- Original Message -----

Quoted 27 lines

From: "Pali Rohár" <[email protected]>
To: "Chun-Yi Lee" <[email protected]>, "Corentin Chary" <[email protected]>, [email protected], "Dave Airlie" <[email protected]>, "Oleksij Rempel" <[email protected]>, "João Paulo Rechi Vita" <[email protected]>
Cc: "Darren Hart" <[email protected]>, "Andy Shevchenko" <[email protected]>, "Andy Lutomirski" <[email protected]>, [email protected], [email protected]
Sent: Wednesday, 5 July, 2017 7:51:13 PM
Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Quoted 7 lines · On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
Quoted 5 lines

So problematic drivers which use instance=1 without any comments are:
acer-wmi
asus-wmi
mxm-wmi

Adding authors & maintainers of those drivers in loop.

Hi!

Dell drivers and acer-wmi are fixed now. So only asus-wmi and mxm-wmi needs to be investigated.

Quoted 7 lines
Quoted 2 lines

Adding more people who developed those drivers recently in loop. Can you check if instance number is used correctly or not?

I've no memory of why I picked 1 or 0, I probably cut-n-paste it from somewhere else.

Dave.

And do you have at least ACPI DSDT dumps from that machine? Or are you able to find some?

Signature · 2 lines
Pali Rohár
[email protected]
author
#18 lore raw
Quoted 35 lines · On Wednesday 05 July 2017 22:24:20 Pali Rohár wrote:
Quoted 34 lines · On Wednesday 05 July 2017 21:30:35 David Airlie wrote:

----- Original Message -----

Quoted 31 lines

From: "Pali Rohár" <[email protected]>
To: "Chun-Yi Lee" <[email protected]>, "Corentin Chary" <[email protected]>, [email protected], "Dave Airlie" <[email protected]>, "Oleksij Rempel" <[email protected]>, "João Paulo Rechi Vita" <[email protected]>
Cc: "Darren Hart" <[email protected]>, "Andy Shevchenko" <[email protected]>, "Andy Lutomirski" <[email protected]>, [email protected], [email protected]
Sent: Wednesday, 5 July, 2017 7:51:13 PM
Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

Quoted 9 lines · On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
Quoted 7 lines

So problematic drivers which use instance=1 without any comments

are:
acer-wmi
asus-wmi
mxm-wmi

Adding authors & maintainers of those drivers in loop.

Hi!

Dell drivers and acer-wmi are fixed now. So only asus-wmi and mxm-wmi needs to be investigated.

Adding more people who developed those drivers recently in loop. Can you check if instance number is used correctly or not?

Quoted 7 lines
Quoted 4 lines

I've no memory of why I picked 1 or 0, I probably cut-n-paste it from somewhere else.

Dave.

And do you have at least ACPI DSDT dumps from that machine? Or are you able to find some?

Hi! For mxm-wmi I found this document:
https://lekensteyn.nl/files/docs/mxm-2.1-software-spec.pdf

On page numbered 26 (resp. in PDF page 31) is information about WMI GUID {F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0} interface and there is written that instance count = 1.

// Methods GUID {F6CB5C3C-9CAE-4ebd-B577-931EA32A2CC0} 0x3C, 0x5C, 0xCB, 0xF6, 0xAE, 0x9C, 0xbd, 0x4e, 0xB5, 0x77, 0x93, 0x1E, 0xA3, 0x2A, 0x2C, 0xC0,

0x4D, 0x58,    // Object ID “MX” = method “WMMX”
1,             // Instance Count
0x02,          // Flags (WMIACPI_REGFLAG_METHOD)

And ACPI method for handling this WMI call does not check Arg0 and Arg1 at all.

So... Andy, Darren, any objections for following patch which changes instance number from one to zero?

drivers/platform/x86/mxm-wmi.c +2 -2
index f4bad83..35d8b9a 100644
@@ -53,7 +53,7 @@ int mxm_wmi_call_mxds(int adapter)
 
 	printk("calling mux switch %d\n", adapter);
 
-	status = wmi_evaluate_method(MXM_WMMX_GUID, 0x1, adapter, &input,
+	status = wmi_evaluate_method(MXM_WMMX_GUID, 0x0, adapter, &input,
 				     &output);
 
 	if (ACPI_FAILURE(status))

@@ -78,7 +78,7 @@ int mxm_wmi_call_mxmx(int adapter)
 
 	printk("calling mux switch %d\n", adapter);
 
-	status = wmi_evaluate_method(MXM_WMMX_GUID, 0x1, adapter, &input,
+	status = wmi_evaluate_method(MXM_WMMX_GUID, 0x0, adapter, &input,
 				     &output);
 
 	if (ACPI_FAILURE(status))
Signature · 2 lines
Pali Rohár
[email protected]
#19 lore raw
Quoted 26 lines · Pali Rohár
Quoted 9 lines · On Wednesday 05 July 2017 22:24:20 Pali Rohár wrote:
Quoted 7 lines · On Wednesday 05 July 2017 21:30:35 David Airlie wrote:
Quoted 7 lines · On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
Quoted 6 lines
Quoted 6 lines

So problematic drivers which use instance=1 without any comments
are:
acer-wmi
asus-wmi
mxm-wmi

Hi! For mxm-wmi I found this document:
https://lekensteyn.nl/files/docs/mxm-2.1-software-spec.pdf

On page numbered 26 (resp. in PDF page 31) is information about WMI GUID {F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0} interface and there is written that instance count = 1.

// Methods GUID {F6CB5C3C-9CAE-4ebd-B577-931EA32A2CC0} 0x3C, 0x5C, 0xCB, 0xF6, 0xAE, 0x9C, 0xbd, 0x4e, 0xB5, 0x77, 0x93, 0x1E, 0xA3, 0x2A, 0x2C, 0xC0,

0x4D, 0x58,    // Object ID “MX” = method “WMMX”
1,             // Instance Count
0x02,          // Flags (WMIACPI_REGFLAG_METHOD)
Quoted 5 lines

And ACPI method for handling this WMI call does not check Arg0 and Arg1 at all.

So... Andy, Darren, any objections for following patch which changes instance number from one to zero?

No objections from me! Just put enough explanation into commit message.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#20 lore raw
Quoted 27 lines · On Sunday 06 August 2017 18:10:12 Andy Shevchenko wrote:
Quoted 25 lines · Pali Rohár
Quoted 9 lines · On Wednesday 05 July 2017 22:24:20 Pali Rohár wrote:
Quoted 8 lines · On Wednesday 05 July 2017 21:30:35 David Airlie wrote:
Quoted 8 lines · On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
Quoted 7 lines
Quoted 7 lines

So problematic drivers which use instance=1 without any comments

are:
acer-wmi
asus-wmi
mxm-wmi

Hi! For mxm-wmi I found this document:
https://lekensteyn.nl/files/docs/mxm-2.1-software-spec.pdf

On page numbered 26 (resp. in PDF page 31) is information about WMI GUID {F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0} interface and there is written that instance count = 1.

// Methods GUID {F6CB5C3C-9CAE-4ebd-B577-931EA32A2CC0} 0x3C, 0x5C, 0xCB, 0xF6, 0xAE, 0x9C, 0xbd, 0x4e, 0xB5, 0x77, 0x93, 0x1E, 0xA3, 0x2A, 0x2C, 0xC0,

0x4D, 0x58,    // Object ID “MX” = method “WMMX”
1,             // Instance Count
0x02,          // Flags (WMIACPI_REGFLAG_METHOD)
Quoted 8 lines
Quoted 5 lines

And ACPI method for handling this WMI call does not check Arg0 and Arg1 at all.

So... Andy, Darren, any objections for following patch which changes instance number from one to zero?

No objections from me! Just put enough explanation into commit message.

Ok.

Now I found ACPI code for asus-wmi.c GUID 97845ED0-4E6D-11DE-8A39-0800200C9A66 on https://lwn.net/Articles/391249/
and there is also in _WDG buffer instance count just 1 and WMBC method do not check Arg0.

So asus-wmi.c needs to be fixed too.

Signature · 2 lines
Pali Rohár
[email protected]
author
#21 lore raw
In reply to Pali Rohár
Quoted 71 lines · On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
Quoted 5 lines · On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
Quoted 4 lines · Pali Rohár
Quoted 2 lines · On Tuesday 13 June 2017 18:49:51 Darren Hart wrote:

I'd suggest reaching out to the maintainers and contributors to the drivers you mention to request some help in testing.

Quoted 7 lines
Quoted 4 lines

Seems sane. Grep for all methods with instance number different as zero (or just number one -- which can be suspicious as somebody could thought that indexing is from one, not zer) and try to receive ACPI/BMOF data and verify it.

This would still be the ideal solution, verify we can do the right thing without breaking existing drivers. Agreed.

Here is all usage:

Function wmi_set_block:

msi-wmi.c:
instance=0 /* Instance 0 is "set backlight" */

tc1100-wmi.c:
instance=TC1100_INSTANCE_WIRELESS /* defined as 1 */ instance=TC1100_INSTANCE_JOGDIAL /* defined as 2 */

Function wmi_query_block:

acer-wmi.c:
instance=1 /* no comment why,
guid=95764E09-FB56-4E83-B31A-37761F60994A */

dell-wmi.c:
instance=0

msi-wmi.c:
instance=1 /* Instance 1 is "get backlight", cmp with DSDT */

surface3-wmi.c:
instance=0

tc1100-wmi.c:
(same as in wmi_set_block)

Function wmi_evaluate_method:

acer-wmi.c:
instance=1 /* no comment why,
guid=67C3371D-95A3-4C37-BB61-DD47B491DAAB */ instance=1 /* no comment why, guid=6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3 */ instance=0

alienware-wmi.c:
instance=1 /* no comment why,
guid=A70591CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no comment why, guid=A80593CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-B622A1EF5492 */

asus-wmi.c:
instance=1 /* no comment why,
guid=97845ED0-4E6D-11DE-8A39-0800200C9A66 */

dell-wmi-led.c:
instance=1 /* no comment why,
guid=F6E4FE6E-909D-47cb-8BAB-C9F6F2F8D396 */

hp-wmi.c:
instance=0

mxm-wmi.c:
instance=1 /* no comment why,
guid=F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0 */

Quoted 7 lines

So problematic drivers which use instance=1 without any comments are:

acer-wmi
alienware-wmi
asus-wmi
dell-wmi-led
mxm-wmi

Also there is a new problematic driver named peaq-wmi.c added by Hans. Adding into loop. Hans, can you recheck if arguments for wmi_evaluate_method() are correct, specially instance number "1"?

Signature · 2 lines
Pali Rohár
[email protected]
#22 lore raw

Hi,

Quoted 80 lines · Pali Rohár
Quoted 71 lines · On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
Quoted 5 lines · On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
Quoted 4 lines · Pali Rohár
Quoted 2 lines · On Tuesday 13 June 2017 18:49:51 Darren Hart wrote:

I'd suggest reaching out to the maintainers and contributors to the drivers you mention to request some help in testing.

Quoted 7 lines
Quoted 4 lines

Seems sane. Grep for all methods with instance number different as zero (or just number one -- which can be suspicious as somebody could thought that indexing is from one, not zer) and try to receive ACPI/BMOF data and verify it.

This would still be the ideal solution, verify we can do the right thing without breaking existing drivers. Agreed.

Here is all usage:

Function wmi_set_block:

msi-wmi.c:
instance=0 /* Instance 0 is "set backlight" */

tc1100-wmi.c:
instance=TC1100_INSTANCE_WIRELESS /* defined as 1 */ instance=TC1100_INSTANCE_JOGDIAL /* defined as 2 */

Function wmi_query_block:

acer-wmi.c:
instance=1 /* no comment why,
guid=95764E09-FB56-4E83-B31A-37761F60994A */

   dell-wmi.c:
   instance=0

   msi-wmi.c:
   instance=1 /* Instance 1 is "get backlight", cmp with DSDT */

   surface3-wmi.c:
   instance=0

   tc1100-wmi.c:
   (same as in wmi_set_block)

Function wmi_evaluate_method:

acer-wmi.c:
instance=1 /* no comment why,
guid=67C3371D-95A3-4C37-BB61-DD47B491DAAB */ instance=1 /* no comment why, guid=6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3 */ instance=0

alienware-wmi.c:
instance=1 /* no comment why,
guid=A70591CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no comment why, guid=A80593CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-B622A1EF5492 */

asus-wmi.c:
instance=1 /* no comment why,
guid=97845ED0-4E6D-11DE-8A39-0800200C9A66 */

dell-wmi-led.c:
instance=1 /* no comment why,
guid=F6E4FE6E-909D-47cb-8BAB-C9F6F2F8D396 */

   hp-wmi.c:
   instance=0

   mxm-wmi.c:
   instance=1 /* no comment why,

guid=F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0 */

Quoted 7 lines

So problematic drivers which use instance=1 without any comments are:

acer-wmi
alienware-wmi
asus-wmi
dell-wmi-led
mxm-wmi

Quoted 3 lines

Also there is a new problematic driver named peaq-wmi.c added by Hans. Adding into loop. Hans, can you recheck if arguments for wmi_evaluate_method() are correct, specially instance number "1"?

Ok, so looking at wmi_evaluate_method() the instance number becomes arg0 and the DSDT implementation of the WMBC method which is the one we care about is:

            Method (WMBC, 3, NotSerialized)
            {
                If (Arg1 == 0x05)
                {
                    Local0 = ^^GPO0.DBLY /* \_SB_.GPO0.DBLY */
                    ^^GPO0.DBLY = Zero
                    Return (Local0)
                }

                Return (0xFFFFFFFF)
            }

So the instance_index / Arg0 does not matter. I just tested passing 0 and that works fine. Feel free to change this if that helps with the wmi refactoring.

Interestingly enough passing wmi.debug_dump_wdg=1 shows that the BC object claims to have 10 instances, but the whole peaq-wmi interface appears to be a messy quick hack from the manufacturer, so that is not surprising.

Regards,

Hans

author
#23 lore raw
Quoted 108 lines · On Sunday 06 August 2017 18:18:06 Hans de Goede wrote:

Hi,

Quoted 85 lines · Pali Rohár
Quoted 76 lines · On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
Quoted 5 lines · On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
Quoted 4 lines · Pali Rohár
Quoted 2 lines · On Tuesday 13 June 2017 18:49:51 Darren Hart wrote:

I'd suggest reaching out to the maintainers and contributors to the drivers you mention to request some help in testing.

Quoted 7 lines
Quoted 4 lines

Seems sane. Grep for all methods with instance number different as zero (or just number one -- which can be suspicious as somebody could thought that indexing is from one, not zer) and try to receive ACPI/BMOF data and verify it.

This would still be the ideal solution, verify we can do the right thing without breaking existing drivers. Agreed.

Here is all usage:

Function wmi_set_block:
msi-wmi.c:
instance=0 /* Instance 0 is "set backlight" */

tc1100-wmi.c:
instance=TC1100_INSTANCE_WIRELESS /* defined as 1 */ instance=TC1100_INSTANCE_JOGDIAL /* defined as 2 */

Function wmi_query_block:
acer-wmi.c:
instance=1 /* no comment why,

guid=95764E09-FB56-4E83-B31A-37761F60994A */

   dell-wmi.c:
   instance=0
   
   msi-wmi.c:
   instance=1 /* Instance 1 is "get backlight", cmp with DSDT */
   
   surface3-wmi.c:
   instance=0
   
   tc1100-wmi.c:
   (same as in wmi_set_block)

Function wmi_evaluate_method:
acer-wmi.c:
instance=1 /* no comment why,

guid=67C3371D-95A3-4C37-BB61-DD47B491DAAB */ instance=1 /* no comment why, guid=6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3 */ instance=0

alienware-wmi.c:
instance=1 /* no comment why,

guid=A70591CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no comment why, guid=A80593CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no comment why, guid=A70591CE-A997-11DA-B012-B622A1EF5492 */

asus-wmi.c:
instance=1 /* no comment why,

guid=97845ED0-4E6D-11DE-8A39-0800200C9A66 */

dell-wmi-led.c:
instance=1 /* no comment why,

guid=F6E4FE6E-909D-47cb-8BAB-C9F6F2F8D396 */

   hp-wmi.c:
   instance=0
   
   mxm-wmi.c:
   instance=1 /* no comment why,

guid=F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0 */

Quoted 7 lines

So problematic drivers which use instance=1 without any comments are:
acer-wmi
alienware-wmi
asus-wmi
dell-wmi-led
mxm-wmi

Quoted 3 lines

Also there is a new problematic driver named peaq-wmi.c added by Hans. Adding into loop. Hans, can you recheck if arguments for wmi_evaluate_method() are correct, specially instance number "1"?

Ok, so looking at wmi_evaluate_method() the instance number becomes arg0 and the DSDT implementation of the WMBC method which is the one we care about is:

             Method (WMBC, 3, NotSerialized)
             {
                 If (Arg1 == 0x05)
                 {
                     Local0 = ^^GPO0.DBLY /* \_SB_.GPO0.DBLY */
                     ^^GPO0.DBLY = Zero
                     Return (Local0)
                 }

                 Return (0xFFFFFFFF)
             }
Quoted 3 lines

So the instance_index / Arg0 does not matter. I just tested passing 0 and that works fine. Feel free to change this if that helps with the wmi refactoring.

Ok, thanks for testing.

Quoted 4 lines

Interestingly enough passing wmi.debug_dump_wdg=1 shows that the BC object claims to have 10 instances, but the whole peaq-wmi interface appears to be a messy quick hack from the manufacturer, so that is not surprising.

Apparently, this is fully correct and should not cause any problems. Just all instances would do same thing.

Signature · 2 lines
Pali Rohár
[email protected]