| Message ID | 1495886134-8276-1-git-send-email-pali.rohar@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Darren Hart |
| Headers | show |
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 */
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.
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.
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.
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
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.
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 */
> -----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
> -----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
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?
> 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 */
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
> -----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,
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?
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 */ >
----- 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.
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?
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"?
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
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.
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 */
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(-)