On Saturday 27 May 2017 13:55:34 Pali Rohár wrote:
> instance_count defines number of instances of data block and instance
> itself is indexed from zero, which means first instance has number 0.
> Therefore check for invalid instance should be non-strict inequality.
>
> Signed-off-by: Pali Rohár <[email protected]>
> ---
> I'm marking this patch as RFC because it is not tested at all and
> probably could break existing WMI drivers. Some WMI drivers pass
> instance number 1 and I'm not sure if ACPI-WMI bytecode for those
> machines has really two instances. In more cases ACPI-WMI bytecode
> does not check instance number if supports only one instance. So
> then any instance id can be used for correct execution of ACPI-WMI
> method.
>
> 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
[email protected]
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 <[email protected]>
> > ---
> > I'm marking this patch as RFC because it is not tested at all and
> > probably could break existing WMI drivers. Some WMI drivers pass
> > instance number 1 and I'm not sure if ACPI-WMI bytecode for those
> > machines has really two instances. In more cases ACPI-WMI bytecode
> > does not check instance number if supports only one instance. So
> > then any instance id can be used for correct execution of ACPI-WMI
> > method.
> >
> > So this patch is open for discussion.
>
> 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
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 <[email protected]>
> > > ---
> > > I'm marking this patch as RFC because it is not tested at all and
> > > probably could break existing WMI drivers. Some WMI drivers pass
> > > instance number 1 and I'm not sure if ACPI-WMI bytecode for those
> > > machines has really two instances. In more cases ACPI-WMI
> > > bytecode does not check instance number if supports only one
> > > instance. So then any instance id can be used for correct
> > > execution of ACPI-WMI method.
> > >
> > > So this patch is open for discussion.
> >
> > 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
[email protected]
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 <[email protected]>
> > > > ---
> > > > I'm marking this patch as RFC because it is not tested at all and
> > > > probably could break existing WMI drivers. Some WMI drivers pass
> > > > instance number 1 and I'm not sure if ACPI-WMI bytecode for those
> > > > machines has really two instances. In more cases ACPI-WMI
> > > > bytecode does not check instance number if supports only one
> > > > instance. So then any instance id can be used for correct
> > > > execution of ACPI-WMI method.
> > > >
> > > > So this patch is open for discussion.
> > >
> > > 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
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
[email protected]
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
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
[email protected]
> -----Original Message-----
> From: Pali Rohár [mailto:[email protected]]
> Sent: Thursday, June 15, 2017 8:59 AM
> To: Limonciello, Mario <[email protected]>; Darren Hart
> <[email protected]>
> Cc: Andy Shevchenko <[email protected]>; Andy Lutomirski <[email protected]>;
> [email protected]; [email protected]
> Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance
> number
>
> Mario, are you able to check if instance number passed to
> wmi_evaluate_method in following dell WMI drivers is correct and should
> be really 1?
>
> 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
> [email protected]
> 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
[email protected]
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
On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
> > So problematic drivers which use instance=1 without any comments are:
> >
> > acer-wmi
> > asus-wmi
> > mxm-wmi
>
> Adding authors & maintainers of those drivers in loop.
Hi!
Dell drivers and acer-wmi are fixed now. So only asus-wmi and mxm-wmi
needs to be investigated.
Adding more people who developed those drivers recently in loop. Can you
check if instance number is used correctly or not?
> WMI instance number is indexed from zero and therefore first instance
> number is 0, not 1. Can you check if for drivers and wmi functions
> (specified below) is really correct to use WMI instance number one?
>
> In case in _WDG is specified for particular GUID that instance_count is
> 1, it means the only allowed instance number is 0 (first and the only
> one).
>
> In some cases, when there is only one instance for WMI method, ACPI WMI
> bytecode does not check instance number, so any passed value is
> accepted by ACPI. But in current patch I'm trying to fix check for
> valid instance number based on instance_count information from _WDG.
>
> So I need to know if nothing would be broken. And in case those driver
> issue invalid/incorrect instance number, they needs to be fixed.
>
> Can you look at it? Simple look into _WDG dump should be enough... just
> check if instance number called from wmi driver is less then
> instance_count from _WDG.
>
> On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
> > Function wmi_query_block:
> >
> > acer-wmi.c:
> > instance=1 /* no comment why, guid=95764E09-FB56-4E83-B31A-37761F60994A */
> >
>
> > Function wmi_evaluate_method:
> >
> > acer-wmi.c:
> > instance=1 /* no comment why, guid=67C3371D-95A3-4C37-BB61-DD47B491DAAB */
> > instance=1 /* no comment why, guid=6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3 */
> >
> > asus-wmi.c:
> > instance=1 /* no comment why, guid=97845ED0-4E6D-11DE-8A39-0800200C9A66 */
> >
> > mxm-wmi.c:
> > instance=1 /* no comment why, guid=F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0 */
>
--
Pali Rohár
[email protected]
----- Original Message -----
> From: "Pali Rohár" <[email protected]>
> To: "Chun-Yi Lee" <[email protected]>, "Corentin Chary" <[email protected]>, [email protected],
> "Dave Airlie" <[email protected]>, "Oleksij Rempel" <[email protected]>, "João Paulo Rechi Vita"
> <[email protected]>
> Cc: "Darren Hart" <[email protected]>, "Andy Shevchenko" <[email protected]>, "Andy Lutomirski"
> <[email protected]>, [email protected], [email protected]
> Sent: Wednesday, 5 July, 2017 7:51:13 PM
> Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method instance number
>
> 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" <[email protected]>
> > To: "Chun-Yi Lee" <[email protected]>, "Corentin Chary"
> > <[email protected]>, [email protected],
> > "Dave Airlie" <[email protected]>, "Oleksij Rempel"
> > <[email protected]>, "João Paulo Rechi Vita"
> > <[email protected]>
> > Cc: "Darren Hart" <[email protected]>, "Andy Shevchenko"
> > <[email protected]>, "Andy Lutomirski" <[email protected]>,
> > [email protected], [email protected]
> > Sent: Wednesday, 5 July, 2017 7:51:13 PM
> > Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method
> > instance number
> >
> > On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
> > > > So problematic drivers which use instance=1 without any comments
> > > > are:
> > > > acer-wmi
> > > > asus-wmi
> > > > mxm-wmi
> > >
> > > Adding authors & maintainers of those drivers in loop.
> >
> > Hi!
> >
> > Dell drivers and acer-wmi are fixed now. So only asus-wmi and
> > mxm-wmi needs to be investigated.
> >
> > Adding more people who developed those drivers recently in loop.
> > Can you check if instance number is used correctly or not?
>
> I've no memory of why I picked 1 or 0, I probably cut-n-paste it from
> somewhere else.
>
> Dave.
And do you have at least ACPI DSDT dumps from that machine? Or are you
able to find some?
--
Pali Rohár
[email protected]
On Wednesday 05 July 2017 22:24:20 Pali Rohár wrote:
> On Wednesday 05 July 2017 21:30:35 David Airlie wrote:
> > ----- Original Message -----
> >
> > > From: "Pali Rohár" <[email protected]>
> > > To: "Chun-Yi Lee" <[email protected]>, "Corentin Chary"
> > > <[email protected]>, [email protected],
> > > "Dave Airlie" <[email protected]>, "Oleksij Rempel"
> > > <[email protected]>, "João Paulo Rechi Vita"
> > > <[email protected]>
> > > Cc: "Darren Hart" <[email protected]>, "Andy Shevchenko"
> > > <[email protected]>, "Andy Lutomirski" <[email protected]>,
> > > [email protected], [email protected]
> > > Sent: Wednesday, 5 July, 2017 7:51:13 PM
> > > Subject: Re: [PATCH] RFC: platform/x86: wmi: Fix check for method
> > > instance number
> > >
> > > On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
> > > > > So problematic drivers which use instance=1 without any
> > > > > comments
> > > > >
> > > > > are:
> > > > > acer-wmi
> > > > > asus-wmi
> > > > > mxm-wmi
> > > >
> > > > Adding authors & maintainers of those drivers in loop.
> > >
> > > Hi!
> > >
> > > Dell drivers and acer-wmi are fixed now. So only asus-wmi and
> > > mxm-wmi needs to be investigated.
> > >
> > > Adding more people who developed those drivers recently in loop.
> > > Can you check if instance number is used correctly or not?
> >
> > I've no memory of why I picked 1 or 0, I probably cut-n-paste it
> > from somewhere else.
> >
> > Dave.
>
> And do you have at least ACPI DSDT dumps from that machine? Or are
> you able to find some?
Hi! For mxm-wmi I found this document:
https://lekensteyn.nl/files/docs/mxm-2.1-software-spec.pdf
On page numbered 26 (resp. in PDF page 31) is information about WMI
GUID {F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0} interface and there is
written that instance count = 1.
// Methods GUID {F6CB5C3C-9CAE-4ebd-B577-931EA32A2CC0}
0x3C, 0x5C, 0xCB, 0xF6, 0xAE, 0x9C, 0xbd, 0x4e, 0xB5, 0x77, 0x93,
0x1E, 0xA3, 0x2A, 0x2C, 0xC0,
0x4D, 0x58, // Object ID “MX” = method “WMMX”
1, // Instance Count
0x02, // Flags (WMIACPI_REGFLAG_METHOD)
And ACPI method for handling this WMI call does not check Arg0 and Arg1
at all.
So... Andy, Darren, any objections for following patch which changes
instance number from one to zero?
diff --git a/drivers/platform/x86/mxm-wmi.c b/drivers/platform/x86/mxm-wmi.c
index f4bad83..35d8b9a 100644
--- a/drivers/platform/x86/mxm-wmi.c
+++ b/drivers/platform/x86/mxm-wmi.c
@@ -53,7 +53,7 @@ int mxm_wmi_call_mxds(int adapter)
printk("calling mux switch %d\n", adapter);
- status = wmi_evaluate_method(MXM_WMMX_GUID, 0x1, adapter, &input,
+ status = wmi_evaluate_method(MXM_WMMX_GUID, 0x0, adapter, &input,
&output);
if (ACPI_FAILURE(status))
@@ -78,7 +78,7 @@ int mxm_wmi_call_mxmx(int adapter)
printk("calling mux switch %d\n", adapter);
- status = wmi_evaluate_method(MXM_WMMX_GUID, 0x1, adapter, &input,
+ status = wmi_evaluate_method(MXM_WMMX_GUID, 0x0, adapter, &input,
&output);
if (ACPI_FAILURE(status))
--
Pali Rohár
[email protected]
On Wednesday 14 June 2017 17:46:54 Pali Rohár wrote:
> On Tuesday 13 June 2017 11:42:28 Darren Hart wrote:
> > On Tue, Jun 13, 2017 at 08:04:57PM +0200, Pali Rohár wrote:
> > > On Tuesday 13 June 2017 18:49:51 Darren Hart wrote:
> > > > I'd suggest reaching out to the maintainers and contributors to
> > > > the drivers you mention to request some help in testing.
> > >
> > > Seems sane. Grep for all methods with instance number different
> > > as zero (or just number one -- which can be suspicious as
> > > somebody could thought that indexing is from one, not zer) and
> > > try to receive ACPI/BMOF data and verify it.
> >
> > This would still be the ideal solution, verify we can do the right
> > thing without breaking existing drivers. Agreed.
>
> Here is all usage:
>
> Function wmi_set_block:
>
> msi-wmi.c:
> instance=0 /* Instance 0 is "set backlight" */
>
> tc1100-wmi.c:
> instance=TC1100_INSTANCE_WIRELESS /* defined as 1 */
> instance=TC1100_INSTANCE_JOGDIAL /* defined as 2 */
>
> Function wmi_query_block:
>
> acer-wmi.c:
> instance=1 /* no comment why,
> guid=95764E09-FB56-4E83-B31A-37761F60994A */
>
> dell-wmi.c:
> instance=0
>
> msi-wmi.c:
> instance=1 /* Instance 1 is "get backlight", cmp with DSDT */
>
> surface3-wmi.c:
> instance=0
>
> tc1100-wmi.c:
> (same as in wmi_set_block)
>
> Function wmi_evaluate_method:
>
> acer-wmi.c:
> instance=1 /* no comment why,
> guid=67C3371D-95A3-4C37-BB61-DD47B491DAAB */ instance=1 /* no
> comment why, guid=6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3 */ instance=0
>
> alienware-wmi.c:
> instance=1 /* no comment why,
> guid=A70591CE-A997-11DA-B012-B622A1EF5492 */ instance=1 /* no
> comment why, guid=A80593CE-A997-11DA-B012-B622A1EF5492 */ instance=1
> /* no comment why, guid=A70591CE-A997-11DA-B012-B622A1EF5492 */
>
> asus-wmi.c:
> instance=1 /* no comment why,
> guid=97845ED0-4E6D-11DE-8A39-0800200C9A66 */
>
> dell-wmi-led.c:
> instance=1 /* no comment why,
> guid=F6E4FE6E-909D-47cb-8BAB-C9F6F2F8D396 */
>
> hp-wmi.c:
> instance=0
>
> mxm-wmi.c:
> instance=1 /* no comment why,
> guid=F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0 */
>
> So problematic drivers which use instance=1 without any comments are:
>
> acer-wmi
> alienware-wmi
> asus-wmi
> dell-wmi-led
> mxm-wmi
Also there is a new problematic driver named peaq-wmi.c added by Hans.
Adding into loop. Hans, can you recheck if arguments for
wmi_evaluate_method() are correct, specially instance number "1"?
--
Pali Rohár
[email protected]
On Sun, Aug 6, 2017 at 6:35 PM, Pali Rohár <[email protected]> wrote:
> On Wednesday 05 July 2017 22:24:20 Pali Rohár wrote:
>> On Wednesday 05 July 2017 21:30:35 David Airlie wrote:
>> > > On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
>> > > > > So problematic drivers which use instance=1 without any
>> > > > > comments
>> > > > > are:
>> > > > > acer-wmi
>> > > > > asus-wmi
>> > > > > mxm-wmi
> Hi! For mxm-wmi I found this document:
> https://lekensteyn.nl/files/docs/mxm-2.1-software-spec.pdf
>
> On page numbered 26 (resp. in PDF page 31) is information about WMI
> GUID {F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0} interface and there is
> written that instance count = 1.
>
> // Methods GUID {F6CB5C3C-9CAE-4ebd-B577-931EA32A2CC0}
> 0x3C, 0x5C, 0xCB, 0xF6, 0xAE, 0x9C, 0xbd, 0x4e, 0xB5, 0x77, 0x93,
> 0x1E, 0xA3, 0x2A, 0x2C, 0xC0,
> 0x4D, 0x58, // Object ID “MX” = method “WMMX”
> 1, // Instance Count
> 0x02, // Flags (WMIACPI_REGFLAG_METHOD)
>
> And ACPI method for handling this WMI call does not check Arg0 and Arg1
> at all.
>
> So... Andy, Darren, any objections for following patch which changes
> instance number from one to zero?
No objections from me! Just put enough explanation into commit message.
--
With Best Regards,
Andy Shevchenko
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.
--
Pali Rohár
[email protected]
On Sunday 06 August 2017 18:10:12 Andy Shevchenko wrote:
> On Sun, Aug 6, 2017 at 6:35 PM, Pali Rohár <[email protected]>
> wrote:
> > On Wednesday 05 July 2017 22:24:20 Pali Rohár wrote:
> >> On Wednesday 05 July 2017 21:30:35 David Airlie wrote:
> >> > > On Saturday 17 June 2017 18:47:54 Pali Rohár wrote:
> >> > > > > So problematic drivers which use instance=1 without any
> >> > > > > comments
> >> > > > >
> >> > > > > are:
> >> > > > > acer-wmi
> >> > > > > asus-wmi
> >> > > > > mxm-wmi
> >
> > Hi! For mxm-wmi I found this document:
> > https://lekensteyn.nl/files/docs/mxm-2.1-software-spec.pdf
> >
> > On page numbered 26 (resp. in PDF page 31) is information about WMI
> > GUID {F6CB5C3C-9CAE-4EBD-B577-931EA32A2CC0} interface and there is
> > written that instance count = 1.
> >
> > // Methods GUID {F6CB5C3C-9CAE-4ebd-B577-931EA32A2CC0}
> > 0x3C, 0x5C, 0xCB, 0xF6, 0xAE, 0x9C, 0xbd, 0x4e, 0xB5, 0x77, 0x93,
> > 0x1E, 0xA3, 0x2A, 0x2C, 0xC0,
> > 0x4D, 0x58, // Object ID “MX” = method “WMMX”
> > 1, // Instance Count
> > 0x02, // Flags (WMIACPI_REGFLAG_METHOD)
> >
> > And ACPI method for handling this WMI call does not check Arg0 and
> > Arg1 at all.
> >
> > So... Andy, Darren, any objections for following patch which
> > changes instance number from one to zero?
>
> No objections from me! Just put enough explanation into commit
> message.
Ok.
Now I found ACPI code for asus-wmi.c GUID
97845ED0-4E6D-11DE-8A39-0800200C9A66 on https://lwn.net/Articles/391249/
and there is also in _WDG buffer instance count just 1 and WMBC method
do not check Arg0.
So asus-wmi.c needs to be fixed too.
--
Pali Rohár
[email protected]