Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1662996 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number Pali Rohár <pali.rohar@gmail.com> - 2017-06-10 21:20 +0200
    Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance  number Darren Hart <dvhart@infradead.org> - 2017-06-13 19:00 +0200
      Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number Pali Rohár <pali.rohar@gmail.com> - 2017-06-13 20:10 +0200
        Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance  number Darren Hart <dvhart@infradead.org> - 2017-06-13 20:50 +0200
          Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance  number Pali Rohár <pali.rohar@gmail.com> - 2017-06-14 17:50 +0200
            Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance  number Darren Hart <dvhart@infradead.org> - 2017-06-14 22:40 +0200
            Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance  number Pali Rohár <pali.rohar@gmail.com> - 2017-06-15 16:00 +0200
              RE: [PATCH] RFC: platform/x86: wmi: Fix check for method instance  number <Mario.Limonciello@dell.com> - 2017-06-15 17:20 +0200
              RE: [PATCH] RFC: platform/x86: wmi: Fix check for method instance  number <Mario.Limonciello@dell.com> - 2017-06-16 18:40 +0200
                Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number Pali Rohár <pali.rohar@gmail.com> - 2017-06-17 18:40 +0200
            Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number Pali Rohár <pali.rohar@gmail.com> - 2017-06-17 18:50 +0200
              Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance  number joeyli <jlee@suse.com> - 2017-06-19 17:10 +0200

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1665091

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1666748 — 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.

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

[toc] | [prev] | [next] | [standalone]

#1666794 — 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.

> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1668364

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1668367

[Multipart message — attachments visible in raw view] — view raw

> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web