[PATCH 0/4] platform/x86: Fix check for method instance number

[PATCH 0/4] platform/x86: Fix check for method instance number

[Pali Rohár]

This patch series fixes remaining mxm-wmi and asus-wmi drivers to evaluate
WMI methods with correct and available instance number. Now all WMI drivers
should call WMI functions with correct instance number and last patch fixes
WMI check for invalid instances. Patch for paeq-wmi is optional as machine
has 10 instances but instance number is ignored at all. But for consistency
with other drivers first instance would be used (instead of second).

Pali Rohár (4):
  platform/x86: mxm-wmi: Evaluate wmi method with instance number 0x0
  platform/x86: asus-wmi: Evaluate wmi method with instance number 0x0
  platform/x86: peaq-wmi: Evaluate wmi method with instance number 0x0
  platform/x86: wmi: Fix check for method instance number

 drivers/platform/x86/asus-wmi.c |    4 ++--
 drivers/platform/x86/mxm-wmi.c  |    4 ++--
 drivers/platform/x86/peaq-wmi.c |    2 +-
 drivers/platform/x86/wmi.c      |    6 +++---
 4 files changed, 8 insertions(+), 8 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] platform/x86: mxm-wmi: Evaluate wmi method with instance number 0x0

[Pali Rohár]

According to MXM 2.1 specification, there is the only one instance of the
WMI GUID F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0 and so it is instance 0x0.

MXM 2.1 specification:
https://lekensteyn.nl/files/docs/mxm-2.1-software-spec.pdf

_WDG dump:
// 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)

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

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

[PATCH 2/4] platform/x86: asus-wmi: Evaluate wmi method with instance number 0x0

[Pali Rohár]

According to available DSDT dump from Asus machine, there is the only one
instance of the WMI GUID 97845ED0-4E6D-11DE-8A39-0800200C9A66 and so it is
0x0. Moreover corresponding method WMBC does not check Arg0 (instance
number) at all.

DSDT dump is available at:
https://lwn.net/Articles/391249/

_WDG dump:
0xD0, 0x5E, 0x84, 0x97, 0x6D, 0x4E, 0xDE, 0x11, 0x8A, 0x39, 0x08,
0x00, 0x20, 0x0C, 0x9A, 0x66,
0x42, 0x43,    // Object ID "BC" = method "WMBC"
0x01,          // Instance count
0x02,          // Flags

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 709e3a6..48e1541 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -299,7 +299,7 @@ static int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
 	union acpi_object *obj;
 	u32 tmp = 0;
 
-	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID, 1, method_id,
+	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID, 0, method_id,
 				     &input, &output);
 
 	if (ACPI_FAILURE(status))
@@ -1946,7 +1946,7 @@ static int show_call(struct seq_file *m, void *data)
 	acpi_status status;
 
 	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
-				     1, asus->debug.method_id,
+				     0, asus->debug.method_id,
 				     &input, &output);
 
 	if (ACPI_FAILURE(status))
-- 
1.7.9.5

[PATCH 3/4] platform/x86: peaq-wmi: Evaluate wmi method with instance number 0x0

[Pali Rohár]

According to Hans de Goede, WMI interface of thh peaq-wmi module has 10
instances but corresponding ACPI WMBC method does not check Arg0 (instance
number) at all. Therefore evaluate WMI method with first instance number
(0x0) instead of second (0x1).

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/peaq-wmi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/peaq-wmi.c b/drivers/platform/x86/peaq-wmi.c
index 77d1f90..f59a5a2 100644
--- a/drivers/platform/x86/peaq-wmi.c
+++ b/drivers/platform/x86/peaq-wmi.c
@@ -39,7 +39,7 @@ static void peaq_wmi_poll(struct input_polled_dev *dev)
 	struct acpi_buffer input = { sizeof(dummy), &dummy };
 	struct acpi_buffer output = { sizeof(obj), &obj };
 
-	status = wmi_evaluate_method(PEAQ_DOLBY_BUTTON_GUID, 1,
+	status = wmi_evaluate_method(PEAQ_DOLBY_BUTTON_GUID, 0,
 				     PEAQ_DOLBY_BUTTON_METHOD_ID,
 				     &input, &output);
 	if (ACPI_FAILURE(status))
-- 
1.7.9.5

[PATCH 4/4] 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>
---
 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 e32ba57..0765b17 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -218,7 +218,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;
@@ -265,7 +265,7 @@ static acpi_status __query_block(struct wmi_block *wblock, u8 instance,
 	block = &wblock->gblock;
 	handle = wblock->acpi_device->handle;
 
-	if (block->instance_count < instance)
+	if (block->instance_count <= instance)
 		return AE_BAD_PARAMETER;
 
 	/* Check GUID is a data block */
@@ -392,7 +392,7 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,
 	block = &wblock->gblock;
 	handle = wblock->acpi_device->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 3/4] platform/x86: peaq-wmi: Evaluate wmi method with instance number 0x0

[Hans de Goede]

Hi,

On 12-08-17 09:44, Pali Rohár wrote:
> According to Hans de Goede, WMI interface of thh peaq-wmi module has 10
> instances but corresponding ACPI WMBC method does not check Arg0 (instance
> number) at all. Therefore evaluate WMI method with first instance number
> (0x0) instead of second (0x1).
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Cc: Hans de Goede <hdegoede@redhat.com>

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>   drivers/platform/x86/peaq-wmi.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/peaq-wmi.c b/drivers/platform/x86/peaq-wmi.c
> index 77d1f90..f59a5a2 100644
> --- a/drivers/platform/x86/peaq-wmi.c
> +++ b/drivers/platform/x86/peaq-wmi.c
> @@ -39,7 +39,7 @@ static void peaq_wmi_poll(struct input_polled_dev *dev)
>   	struct acpi_buffer input = { sizeof(dummy), &dummy };
>   	struct acpi_buffer output = { sizeof(obj), &obj };
>   
> -	status = wmi_evaluate_method(PEAQ_DOLBY_BUTTON_GUID, 1,
> +	status = wmi_evaluate_method(PEAQ_DOLBY_BUTTON_GUID, 0,
>   				     PEAQ_DOLBY_BUTTON_METHOD_ID,
>   				     &input, &output);
>   	if (ACPI_FAILURE(status))
> 

Re: [PATCH 0/4] platform/x86: Fix check for method instance number

[Andy Shevchenko]

On Sat, Aug 12, 2017 at 10:44 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> This patch series fixes remaining mxm-wmi and asus-wmi drivers to evaluate
> WMI methods with correct and available instance number. Now all WMI drivers
> should call WMI functions with correct instance number and last patch fixes
> WMI check for invalid instances. Patch for paeq-wmi is optional as machine
> has 10 instances but instance number is ignored at all. But for consistency
> with other drivers first instance would be used (instead of second).
>

I pushed first 3 to testing, which seems to me right thing to do, and
left the last one to be ACK by Darren and/or others (I'm not familiar
so far with that code).
Thanks!

> Pali Rohár (4):
>   platform/x86: mxm-wmi: Evaluate wmi method with instance number 0x0
>   platform/x86: asus-wmi: Evaluate wmi method with instance number 0x0
>   platform/x86: peaq-wmi: Evaluate wmi method with instance number 0x0
>   platform/x86: wmi: Fix check for method instance number
>
>  drivers/platform/x86/asus-wmi.c |    4 ++--
>  drivers/platform/x86/mxm-wmi.c  |    4 ++--
>  drivers/platform/x86/peaq-wmi.c |    2 +-
>  drivers/platform/x86/wmi.c      |    6 +++---
>  4 files changed, 8 insertions(+), 8 deletions(-)
>
> --
> 1.7.9.5
>



-- 
With Best Regards,
Andy Shevchenko

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

[Darren Hart]

On Sat, Aug 12, 2017 at 09:44:18AM +0200, 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>

This is correct per the WMI specification. Pali, now that Andy has
applied 1-3 of this series, is that all the required driver updates?

Reviewed-by: Darren Hart (VMware) <dvhart@infradead.org>


> ---
>  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 e32ba57..0765b17 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -218,7 +218,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;
> @@ -265,7 +265,7 @@ static acpi_status __query_block(struct wmi_block *wblock, u8 instance,
>  	block = &wblock->gblock;
>  	handle = wblock->acpi_device->handle;
>  
> -	if (block->instance_count < instance)
> +	if (block->instance_count <= instance)
>  		return AE_BAD_PARAMETER;
>  
>  	/* Check GUID is a data block */
> @@ -392,7 +392,7 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,
>  	block = &wblock->gblock;
>  	handle = wblock->acpi_device->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
> 
> 

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 0/4] platform/x86: Fix check for method instance number

[Darren Hart]

On Sun, Aug 13, 2017 at 03:54:26PM +0300, Andy Shevchenko wrote:
> On Sat, Aug 12, 2017 at 10:44 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > This patch series fixes remaining mxm-wmi and asus-wmi drivers to evaluate
> > WMI methods with correct and available instance number. Now all WMI drivers
> > should call WMI functions with correct instance number and last patch fixes
> > WMI check for invalid instances. Patch for paeq-wmi is optional as machine
> > has 10 instances but instance number is ignored at all. But for consistency
> > with other drivers first instance would be used (instead of second).
> >
> 
> I pushed first 3 to testing, which seems to me right thing to do, and
> left the last one to be ACK by Darren and/or others (I'm not familiar
> so far with that code).

Reviewed with one question to Pali. Since you merged 1-3, I'll leave 4 to you
unless you prefer I merge it.

> Thanks!
> 
> > Pali Rohár (4):
> >   platform/x86: mxm-wmi: Evaluate wmi method with instance number 0x0
> >   platform/x86: asus-wmi: Evaluate wmi method with instance number 0x0
> >   platform/x86: peaq-wmi: Evaluate wmi method with instance number 0x0
> >   platform/x86: wmi: Fix check for method instance number
> >
> >  drivers/platform/x86/asus-wmi.c |    4 ++--
> >  drivers/platform/x86/mxm-wmi.c  |    4 ++--
> >  drivers/platform/x86/peaq-wmi.c |    2 +-
> >  drivers/platform/x86/wmi.c      |    6 +++---
> >  4 files changed, 8 insertions(+), 8 deletions(-)
> >
> > --
> > 1.7.9.5
> >
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 0/4] platform/x86: Fix check for method instance number

[Andy Shevchenko]

On Thu, Aug 17, 2017 at 6:25 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Sun, Aug 13, 2017 at 03:54:26PM +0300, Andy Shevchenko wrote:
>> On Sat, Aug 12, 2017 at 10:44 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
>> > This patch series fixes remaining mxm-wmi and asus-wmi drivers to evaluate
>> > WMI methods with correct and available instance number. Now all WMI drivers
>> > should call WMI functions with correct instance number and last patch fixes
>> > WMI check for invalid instances. Patch for paeq-wmi is optional as machine
>> > has 10 instances but instance number is ignored at all. But for consistency
>> > with other drivers first instance would be used (instead of second).
>> >
>>
>> I pushed first 3 to testing, which seems to me right thing to do, and
>> left the last one to be ACK by Darren and/or others (I'm not familiar
>> so far with that code).
>
> Reviewed with one question to Pali. Since you merged 1-3, I'll leave 4 to you
> unless you prefer I merge it.

I will do. Thanks!

-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

On Thursday 17 August 2017 08:24:31 Darren Hart wrote:
> On Sat, Aug 12, 2017 at 09:44:18AM +0200, 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>
> 
> This is correct per the WMI specification. Pali, now that Andy has
> applied 1-3 of this series, is that all the required driver updates?

Hmmm... I have not understood your question.

I checked that all drivers which uses instance id 1 have comment
explaining why is id 1 used.

Problematic drivers without comment have either patch to use instance id
0 in this series or somebody else already supplied patch which is part
of linus tree.

> Reviewed-by: Darren Hart (VMware) <dvhart@infradead.org>
> 
> 
> > ---
> >  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 e32ba57..0765b17 100644
> > --- a/drivers/platform/x86/wmi.c
> > +++ b/drivers/platform/x86/wmi.c
> > @@ -218,7 +218,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;
> > @@ -265,7 +265,7 @@ static acpi_status __query_block(struct wmi_block *wblock, u8 instance,
> >  	block = &wblock->gblock;
> >  	handle = wblock->acpi_device->handle;
> >  
> > -	if (block->instance_count < instance)
> > +	if (block->instance_count <= instance)
> >  		return AE_BAD_PARAMETER;
> >  
> >  	/* Check GUID is a data block */
> > @@ -392,7 +392,7 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,
> >  	block = &wblock->gblock;
> >  	handle = wblock->acpi_device->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
> > 
> > 
> 

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

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

[Darren Hart]

On Thu, Aug 17, 2017 at 11:13:19PM +0200, Pali Rohár wrote:
> On Thursday 17 August 2017 08:24:31 Darren Hart wrote:
> > On Sat, Aug 12, 2017 at 09:44:18AM +0200, 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>
> > 
> > This is correct per the WMI specification. Pali, now that Andy has
> > applied 1-3 of this series, is that all the required driver updates?
> 
> Hmmm... I have not understood your question.
> 
> I checked that all drivers which uses instance id 1 have comment
> explaining why is id 1 used.
> 
> Problematic drivers without comment have either patch to use instance id
> 0 in this series or somebody else already supplied patch which is part
> of linus tree.

Perfect, that is what I was looking for. Thanks Pali!

> 
> > Reviewed-by: Darren Hart (VMware) <dvhart@infradead.org>
-- 
Darren Hart
VMware Open Source Technology Center

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

[Andy Shevchenko]

On Thu, Aug 17, 2017 at 6:24 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Sat, Aug 12, 2017 at 09:44:18AM +0200, 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>
>
> This is correct per the WMI specification. Pali, now that Andy has
> applied 1-3 of this series, is that all the required driver updates?
>
> Reviewed-by: Darren Hart (VMware) <dvhart@infradead.org>

Pushed to testing, thanks!

>
>
>> ---
>>  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 e32ba57..0765b17 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -218,7 +218,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;
>> @@ -265,7 +265,7 @@ static acpi_status __query_block(struct wmi_block *wblock, u8 instance,
>>       block = &wblock->gblock;
>>       handle = wblock->acpi_device->handle;
>>
>> -     if (block->instance_count < instance)
>> +     if (block->instance_count <= instance)
>>               return AE_BAD_PARAMETER;
>>
>>       /* Check GUID is a data block */
>> @@ -392,7 +392,7 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,
>>       block = &wblock->gblock;
>>       handle = wblock->acpi_device->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
>>
>>
>
> --
> Darren Hart
> VMware Open Source Technology Center



-- 
With Best Regards,
Andy Shevchenko