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

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

[Pali Rohár]

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 <pali.rohar@gmail.com>
---
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(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index cd7045f..df63037 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -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 */
-- 
1.7.9.5

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

[Pali Rohár]

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

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 <pali.rohar@gmail.com>
> ---
> 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.

Hi! Any comments?

> ---
>  drivers/platform/x86/wmi.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index cd7045f..df63037 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -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 */

-- 
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] RFC: platform/x86: wmi: Fix check for method instance number

[Darren Hart]

On Sat, Jun 10, 2017 at 09:15:57PM +0200, Pali Rohár wrote:
> 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 <pali.rohar@gmail.com>
> > ---
> > 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.
> 
> 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.

-- 
Darren Hart
VMware Open Source Technology Center

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

[Pali Rohár]

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

On Tuesday 13 June 2017 18:49:51 Darren Hart wrote:
> On Sat, Jun 10, 2017 at 09:15:57PM +0200, Pali Rohár wrote:
> > 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 <pali.rohar@gmail.com>
> > > ---
> > > 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.
> > 
> > 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 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.

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

-- 
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] RFC: platform/x86: wmi: Fix check for method instance number

[Darren Hart]

On Tue, Jun 13, 2017 at 08:04:57PM +0200, Pali Rohár wrote:
> On Tuesday 13 June 2017 18:49:51 Darren Hart wrote:
> > On Sat, Jun 10, 2017 at 09:15:57PM +0200, Pali Rohár wrote:
> > > 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 <pali.rohar@gmail.com>
> > > > ---
> > > > 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.
> > > 
> > > 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 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.

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

-- 
Darren Hart
VMware Open Source Technology Center

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

[Pali Rohár]

On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
> On Tue, Jun 13, 2017 at 08:04:57PM +0200, Pali Rohár wrote:
> > 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.
> 
> 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

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

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

[Darren Hart]

On Wed, Jun 14, 2017 at 05:46:54PM +0200, Pali Rohár wrote:
> On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
> > On Tue, Jun 13, 2017 at 08:04:57PM +0200, Pali Rohár wrote:
> > > 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.
> > 
> > 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.

...

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

-- 
Darren Hart
VMware Open Source Technology Center

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

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

On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
> Function wmi_evaluate_method:
...
>   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 */
...
>   dell-wmi-led.c:
>   instance=1 /* no comment why, guid=F6E4FE6E-909D-47cb-8BAB-C9F6F2F8D396 */

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

RE: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Thursday, June 15, 2017 8:59 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; Darren Hart
> <dvhart@infradead.org>
> Cc: Andy Shevchenko <andy@infradead.org>; Andy Lutomirski <luto@kernel.org>;
> platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> 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?
> 
> 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.

> On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
> > Function wmi_evaluate_method:
> ...
> >   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 */
> ...
> >   dell-wmi-led.c:
> >   instance=1 /* no comment why, guid=F6E4FE6E-909D-47cb-8BAB-
> C9F6F2F8D396 */	
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

RE: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number

[Mario.Limonciello]

> -----Original Message-----
> From: Limonciello, Mario
> Sent: Thursday, June 15, 2017 10:16 AM
> To: 'Pali Rohár' <pali.rohar@gmail.com>; Darren Hart <dvhart@infradead.org>
> Cc: Andy Shevchenko <andy@infradead.org>; Andy Lutomirski <luto@kernel.org>;
> platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] RFC: platform/x86: wmi: Fix check for method instance
> number
> 
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Thursday, June 15, 2017 8:59 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>; Darren Hart
> > <dvhart@infradead.org>
> > Cc: Andy Shevchenko <andy@infradead.org>; Andy Lutomirski
> <luto@kernel.org>;
> > platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > 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?
> >
> > 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.

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

> 
> > On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
> > > Function wmi_evaluate_method:
> > ...
> > >   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 */
> > ...
> > >   dell-wmi-led.c:
> > >   instance=1 /* no comment why, guid=F6E4FE6E-909D-47cb-8BAB-
> > C9F6F2F8D396 */
> >
> > --
> > Pali Rohár
> > pali.rohar@gmail.com

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

[Pali Rohár]

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

On Friday 16 June 2017 18:33:54 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Limonciello, Mario
> > Sent: Thursday, June 15, 2017 10:16 AM
> > To: 'Pali Rohár' <pali.rohar@gmail.com>; Darren Hart
> > <dvhart@infradead.org> Cc: Andy Shevchenko <andy@infradead.org>;
> > Andy Lutomirski <luto@kernel.org>;
> > platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] RFC: platform/x86: wmi: Fix check for method
> > instance number
> > 
> > > -----Original Message-----
> > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > Sent: Thursday, June 15, 2017 8:59 AM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>; Darren Hart
> > > <dvhart@infradead.org>
> > > Cc: Andy Shevchenko <andy@infradead.org>; Andy Lutomirski
> > 
> > <luto@kernel.org>;
> > 
> > > platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > > 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?
> > > 
> > > 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.
> 
> 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?

-- 
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] RFC: platform/x86: wmi: Fix check for method instance number

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Saturday, June 17, 2017 11:35 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; andy@infradead.org; luto@kernel.org; platform-driver-
> x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance
> number
> 
> On Friday 16 June 2017 18:33:54 Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Limonciello, Mario
> > > Sent: Thursday, June 15, 2017 10:16 AM
> > > To: 'Pali Rohár' <pali.rohar@gmail.com>; Darren Hart
> > > <dvhart@infradead.org> Cc: Andy Shevchenko <andy@infradead.org>;
> > > Andy Lutomirski <luto@kernel.org>;
> > > platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: RE: [PATCH] RFC: platform/x86: wmi: Fix check for method
> > > instance number
> > >
> > > > -----Original Message-----
> > > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > > Sent: Thursday, June 15, 2017 8:59 AM
> > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>; Darren Hart
> > > > <dvhart@infradead.org>
> > > > Cc: Andy Shevchenko <andy@infradead.org>; Andy Lutomirski
> > >
> > > <luto@kernel.org>;
> > >
> > > > platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > 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?
> > > >
> > > > 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.
> >
> > 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,

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

[Pali Rohár]

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

On Wednesday 21 June 2017 23:52:12 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Saturday, June 17, 2017 11:35 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: dvhart@infradead.org; andy@infradead.org; luto@kernel.org;
> > platform-driver- x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method
> > instance number
> > 
> > On Friday 16 June 2017 18:33:54 Mario.Limonciello@dell.com wrote:
> > > > -----Original Message-----
> > > > From: Limonciello, Mario
> > > > Sent: Thursday, June 15, 2017 10:16 AM
> > > > To: 'Pali Rohár' <pali.rohar@gmail.com>; Darren Hart
> > > > <dvhart@infradead.org> Cc: Andy Shevchenko
> > > > <andy@infradead.org>; Andy Lutomirski <luto@kernel.org>;
> > > > platform-driver-x86@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org Subject: RE: [PATCH] RFC:
> > > > platform/x86: wmi: Fix check for method instance number
> > > > 
> > > > > -----Original Message-----
> > > > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > > > Sent: Thursday, June 15, 2017 8:59 AM
> > > > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>; Darren
> > > > > Hart <dvhart@infradead.org>
> > > > > Cc: Andy Shevchenko <andy@infradead.org>; Andy Lutomirski
> > > > 
> > > > <luto@kernel.org>;
> > > > 
> > > > > platform-driver-x86@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org 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?
> > > > > 
> > > > > 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.
> > > 
> > > 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?

-- 
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] RFC: platform/x86: wmi: Fix check for method instance number

[Pali Rohár]

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

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

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

-- 
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] RFC: platform/x86: wmi: Fix check for method instance number

[joeyli]

Hi Pali,

On Sat, Jun 17, 2017 at 06:47:54PM +0200, Pali Rohár wrote:
> > 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.
> 
> 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

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

[Pali Rohár]

On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
> > 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?

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

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

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

[David Airlie]

----- Original Message -----
> From: "Pali Rohár" <pali.rohar@gmail.com>
> To: "Chun-Yi Lee" <jlee@suse.com>, "Corentin Chary" <corentin.chary@gmail.com>, acpi4asus-user@lists.sourceforge.net,
> "Dave Airlie" <airlied@redhat.com>, "Oleksij Rempel" <linux@rempel-privat.de>, "João Paulo Rechi Vita"
> <jprvita@gmail.com>
> Cc: "Darren Hart" <dvhart@infradead.org>, "Andy Shevchenko" <andy@infradead.org>, "Andy Lutomirski"
> <luto@kernel.org>, platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
> Sent: Wednesday, 5 July, 2017 7:51:13 PM
> Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number
> 
> On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
> > > 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?
> 

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

Dave.

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

[Pali Rohár]

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

On Wednesday 05 July 2017 21:30:35 David Airlie wrote:
> ----- Original Message -----
> 
> > From: "Pali Rohár" <pali.rohar@gmail.com>
> > To: "Chun-Yi Lee" <jlee@suse.com>, "Corentin Chary"
> > <corentin.chary@gmail.com>, acpi4asus-user@lists.sourceforge.net,
> > "Dave Airlie" <airlied@redhat.com>, "Oleksij Rempel"
> > <linux@rempel-privat.de>, "João Paulo Rechi Vita"
> > <jprvita@gmail.com>
> > Cc: "Darren Hart" <dvhart@infradead.org>, "Andy Shevchenko"
> > <andy@infradead.org>, "Andy Lutomirski" <luto@kernel.org>,
> > platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
> > Sent: Wednesday, 5 July, 2017 7:51:13 PM
> > Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method
> > instance number
> > 
> > On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
> > > > 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?
> 
> 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?

-- 
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] RFC: platform/x86: wmi: Fix check for method instance number

[Pali Rohár]

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

On Wednesday 05 July 2017 22:24:20 Pali Rohár wrote:
> On Wednesday 05 July 2017 21:30:35 David Airlie wrote:
> > ----- Original Message -----
> > 
> > > From: "Pali Rohár" <pali.rohar@gmail.com>
> > > To: "Chun-Yi Lee" <jlee@suse.com>, "Corentin Chary"
> > > <corentin.chary@gmail.com>, acpi4asus-user@lists.sourceforge.net,
> > > "Dave Airlie" <airlied@redhat.com>, "Oleksij Rempel"
> > > <linux@rempel-privat.de>, "João Paulo Rechi Vita"
> > > <jprvita@gmail.com>
> > > Cc: "Darren Hart" <dvhart@infradead.org>, "Andy Shevchenko"
> > > <andy@infradead.org>, "Andy Lutomirski" <luto@kernel.org>,
> > > platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
> > > Sent: Wednesday, 5 July, 2017 7:51:13 PM
> > > Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method
> > > instance number
> > > 
> > > On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
> > > > > 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?
> > 
> > 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?

diff --git a/drivers/platform/x86/mxm-wmi.c b/drivers/platform/x86/mxm-wmi.c
index f4bad83..35d8b9a 100644
--- a/drivers/platform/x86/mxm-wmi.c
+++ b/drivers/platform/x86/mxm-wmi.c
@@ -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))

-- 
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] RFC: platform/x86: wmi: Fix check for method instance number

[Andy Shevchenko]

On Sun, Aug 6, 2017 at 6:35 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Wednesday 05 July 2017 22:24:20 Pali Rohár wrote:
>> On Wednesday 05 July 2017 21:30:35 David Airlie wrote:

>> > > On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
>> > > > > 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)
>
> 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.

-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

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

On Sunday 06 August 2017 18:10:12 Andy Shevchenko wrote:
> On Sun, Aug 6, 2017 at 6:35 PM, Pali Rohár <pali.rohar@gmail.com>
> wrote:
> > On Wednesday 05 July 2017 22:24:20 Pali Rohár wrote:
> >> On Wednesday 05 July 2017 21:30:35 David Airlie wrote:
> >> > > On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
> >> > > > > 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)
> > 
> > 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.

-- 
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] RFC: platform/x86: wmi: Fix check for method instance number

[Pali Rohár]

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

On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
> On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
> > On Tue, Jun 13, 2017 at 08:04:57PM +0200, Pali Rohár wrote:
> > > 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.
> > 
> > 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

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

-- 
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] RFC: platform/x86: wmi: Fix check for method instance number

[Hans de Goede]

Hi,

On 06-08-17 17:42, Pali Rohár wrote:
> On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
>> On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
>>> On Tue, Jun 13, 2017 at 08:04:57PM +0200, Pali Rohár wrote:
>>>> 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.
>>>
>>> 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
> 
> 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

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

[Pali Rohár]

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

On Sunday 06 August 2017 18:18:06 Hans de Goede wrote:
> Hi,
> 
> On 06-08-17 17:42, Pali Rohár wrote:
> > On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
> >> On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
> >>> On Tue, Jun 13, 2017 at 08:04:57PM +0200, Pali Rohár wrote:
> >>>> 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.
> >>> 
> >>> 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
> > 
> > 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.

Ok, thanks for testing.

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

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

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