acer-wmi auto-loaded on a Lenovo Thinkpad X1 Carbon 4th gen

acer-wmi auto-loaded on a Lenovo Thinkpad X1 Carbon 4th gen

[Bjørn Mork]

I recently got a new laptop.  And now I'm trying to figure out what all
the stuff they put into it actually is :)
.
One thing that doesn't seem quite right is that the acer-wmi driver is
loaded and claims that this laptop has an "Acer BMA150 accelerometer".

It is true that there is a device there matching

  #define AMW0_GUID1            "67C3371D-95A3-4C37-BB61-DD47B491DAAB"

as can be seen:

 bjorn@miraculix:~$ ls -l /sys/devices/virtual/wmi/67C3371D-95A3-4C37-BB61-DD47B491DAAB
 total 0
 -r--r--r-- 1 root root 4096 Jul 14 23:10 modalias
 drwxr-xr-x 2 root root    0 Jul 14 23:10 power
 lrwxrwxrwx 1 root root    0 Jul 14 23:10 subsystem -> ../../../../class/wmi
 -rw-r--r-- 1 root root 4096 Jul 14 23:10 uevent


But I sort of doubt there is an Acer specific device here.  Maybe this
is a generic accelerometer interface?  Or is there something else going
on?

The input device created by the driver just just returns -EPERM when I
try to open it, so I don't think this actually works as-is:

 root@miraculix:/tmp# cat /dev/input/event8
 cat: /dev/input/event8: Operation not permitted


And it does seem a little weird that a Thinkpad should need an Acer
specific platform driver anyway....  thinkpad_acpi is of course handling
all(?) the Thinkpad specifics as expected.


Bjørn

Re: acer-wmi auto-loaded on a Lenovo Thinkpad X1 Carbon 4th gen

[joeyli]

Hi Bjørn,

On Thu, Jul 14, 2016 at 11:39:31PM +0200, Bjørn Mork wrote:
> I recently got a new laptop.  And now I'm trying to figure out what all
> the stuff they put into it actually is :)
> .
> One thing that doesn't seem quite right is that the acer-wmi driver is
> loaded and claims that this laptop has an "Acer BMA150 accelerometer".
> 
> It is true that there is a device there matching
> 
>   #define AMW0_GUID1            "67C3371D-95A3-4C37-BB61-DD47B491DAAB"
> 
> as can be seen:
> 
>  bjorn@miraculix:~$ ls -l /sys/devices/virtual/wmi/67C3371D-95A3-4C37-BB61-DD47B491DAAB
>  total 0
>  -r--r--r-- 1 root root 4096 Jul 14 23:10 modalias
>  drwxr-xr-x 2 root root    0 Jul 14 23:10 power
>  lrwxrwxrwx 1 root root    0 Jul 14 23:10 subsystem -> ../../../../class/wmi
>  -rw-r--r-- 1 root root 4096 Jul 14 23:10 uevent
> 
> 
> But I sort of doubt there is an Acer specific device here.  Maybe this
> is a generic accelerometer interface?  Or is there something else going
> on?
> 
> The input device created by the driver just just returns -EPERM when I
> try to open it, so I don't think this actually works as-is:
> 
>  root@miraculix:/tmp# cat /dev/input/event8
>  cat: /dev/input/event8: Operation not permitted
> 
> 
> And it does seem a little weird that a Thinkpad should need an Acer
> specific platform driver anyway....  thinkpad_acpi is of course handling
> all(?) the Thinkpad specifics as expected.
> 
> 
> Bjørn

Could you please check that what does the _HID of HKEY device in your DSDT?

or please attached your acpidump data to me:
 # acpidump > acpidump.raw

Then I want to add HID to the norfkill_ids list in acer-wmi to avoid that
it loaded on your Lenovo machine like LEN0068.

Then, thinkpad_acpi needs to support this _HID.


Thanks a lot!
Joey Lee

Re: acer-wmi auto-loaded on a Lenovo Thinkpad X1 Carbon 4th gen

[Bjørn Mork]

joeyli <jlee@suse.com> writes:

> Hi Bjørn,
>
> On Thu, Jul 14, 2016 at 11:39:31PM +0200, Bjørn Mork wrote:
>> I recently got a new laptop.  And now I'm trying to figure out what all
>> the stuff they put into it actually is :)
>> .
>> One thing that doesn't seem quite right is that the acer-wmi driver is
>> loaded and claims that this laptop has an "Acer BMA150 accelerometer".
>> 
>> It is true that there is a device there matching
>> 
>>   #define AMW0_GUID1            "67C3371D-95A3-4C37-BB61-DD47B491DAAB"
>> 
>> as can be seen:
>> 
>>  bjorn@miraculix:~$ ls -l /sys/devices/virtual/wmi/67C3371D-95A3-4C37-BB61-DD47B491DAAB
>>  total 0
>>  -r--r--r-- 1 root root 4096 Jul 14 23:10 modalias
>>  drwxr-xr-x 2 root root    0 Jul 14 23:10 power
>>  lrwxrwxrwx 1 root root    0 Jul 14 23:10 subsystem -> ../../../../class/wmi
>>  -rw-r--r-- 1 root root 4096 Jul 14 23:10 uevent
>> 
>> 
>> But I sort of doubt there is an Acer specific device here.  Maybe this
>> is a generic accelerometer interface?  Or is there something else going
>> on?
>> 
>> The input device created by the driver just just returns -EPERM when I
>> try to open it, so I don't think this actually works as-is:
>> 
>>  root@miraculix:/tmp# cat /dev/input/event8
>>  cat: /dev/input/event8: Operation not permitted
>> 
>> 
>> And it does seem a little weird that a Thinkpad should need an Acer
>> specific platform driver anyway....  thinkpad_acpi is of course handling
>> all(?) the Thinkpad specifics as expected.
>> 
>> 
>> Bjørn
>
> Could you please check that what does the _HID of HKEY device in your DSDT?
>
> or please attached your acpidump data to me:
>  # acpidump > acpidump.raw

Sorry for the delay.  Just back from vacation.  I took a quick peek at
the driver and I don't think you need that dump. The DSDT is completely
irrelevant.  The driver will match and load on that generic WMI UUID,
and this code ensures that it successfully probes regardless of DSDT:


static acpi_status __init acer_wmi_get_handle_cb(acpi_handle ah, u32 level,
						void *ctx, void **retval)
{
	*(acpi_handle *)retval = ah;
	return AE_OK;
}

static int __init acer_wmi_get_handle(const char *name, const char *prop,
					acpi_handle *ah)
{
	acpi_status status;
	acpi_handle handle;

	BUG_ON(!name || !ah);

	handle = NULL;
	status = acpi_get_devices(prop, acer_wmi_get_handle_cb,
					(void *)name, &handle);

	if (ACPI_SUCCESS(status)) {
		*ah = handle;
		return 0;
	} else {
		return -ENODEV;
	}
}


Since acer_wmi_get_handle_cb() doesn't actually test anything, this will
just return an arbitrary handle regardless of the value of *prop.


> Then I want to add HID to the norfkill_ids list in acer-wmi to avoid that
> it loaded on your Lenovo machine like LEN0068.

I see that this has been done before when this bug has been reported.
Sorry, but I believe that solution is plain wrong.  You are papering
over a more fundamental problem.

Solve the *real* problems in the driver instead:
1) "67C3371D-95A3-4C37-BB61-DD47B491DAAB"
>> 
>> as can be seen:
>> 
>>  bjorn@miraculix:~$ ls -l /sys/devices/virtual/wmi/67C3371D-95A3-4C37-BB61-DD47B491DAAB
>>  total 0
>>  -r--r--r-- 1 root root 4096 Jul 14 23:10 modalias
>>  drwxr-xr-x 2 root root    0 Jul 14 23:10 power
>>  lrwxrwxrwx 1 root root    0 Jul 14 23:10 subsystem -> ../../../../class/wmi
>>  -rw-r--r-- 1 root root 4096 Jul 14 23:10 uevent
>> 
>> 
>> But I sort of doubt there is an Acer specific device here.  Maybe this
>> is a generic accelerometer interface?  Or is there something else going
>> on?
>> 
>> The input device created by the driver just just returns -EPERM when I
>> try to open it, so I don't think this actually works as-is:
>> 
>>  root@miraculix:/tmp# cat /dev/input/event8
>>  cat: /dev/input/event8: Operation not permitted
>> 
>> 
>> And it does seem a little weird that a Thinkpad should need an Acer
>> specific platform driver anyway....  thinkpad_acpi is of course handling
>> all(?) the Thinkpad specifics as expected.
>> 
>> 
>> Bjørn
>
> Could you please check that what does the _HID of HKEY device in your DSDT?
>
> or please attached your acpidump data to me:
>  # acpidump > acpidump.raw

Sorry for the delay.  Just back from vacation.  I took a quick peek at
the driver and I don't think you need that dump. The DSDT is completely
irrelevant.  The driver will match and load on that generic WMI UUID,
and this code ensures that it successfully probes regardless of DSDT:


static acpi_status __init acer_wmi_get_handle_cb(acpi_handle ah, u32 level,
						void *ctx, void **retval)
{
	*(acpi_handle *)retval = ah;
	return AE_OK;
}

static int __init acer_wmi_get_handle(const char *name, const char *prop,
					acpi_handle *ah)
{
	acpi_status status;
	acpi_handle handle;

	BUG_ON(!name || !ah);

	handle = NULL;
	status = acpi_get_devices(prop, acer_wmi_get_handle_cb,
					(void *)name, &handle);

	if (ACPI_SUCCESS(status)) {
		*ah = handle;
		return 0;
	} else {
		return -ENODEV;
	}
}


Since acer_wmi_get_handle_cb() doesn't actually test anything, this will
just return an arbitrary handle regardless of the value of *prop.


> Then I want to add HID to the norfkill_ids list in acer-wmi to avoid that
> it loaded on your Lenovo machine like LEN0068.

I see that this has been done before when this bug has been reported.
Sorry, but I believe that solution is plain wrong.  You are papering
over a more fundamental problem.

Solve the *real* problems in the driver instead:
1)  "67C3371D-95A3-4C37-BB61-DD47B491DAAB" is not an Acer specific GUID
 and should probably never have been used to match Acer specific
 devices.  The fact that it happens to work for some aribitrary laptop is
 not sufficient.

2) the "get_handle" code shown above is broken.  If it works for some
 specific DSDT, then that is just pure luck.

These things should be fixed ASAP, and backported to stable.  The driver
as it is will probably both load and probe successfully on a large
number of systems it has no business messing with.


> Then, thinkpad_acpi needs to support this _HID.

No.  Looking further at this, I don't think so. There is no indication
that there is any relation between that WMI ID and any of the hardware
functions supported by the acer-wmi driver.  So there is no reason to
believe that any of the matched Lenovo laptops have the same hardware
functions.



Bjørn

Re: acer-wmi auto-loaded on a Lenovo Thinkpad X1 Carbon 4th gen

[joeyli]

Hi Bjørn, 

Sorry for my delay because I stuck on other issues.

On Mon, Aug 08, 2016 at 12:09:55PM +0200, Bjørn Mork wrote:
> joeyli <jlee@suse.com> writes:
> 
[...snip]
> 
> Sorry for the delay.  Just back from vacation.  I took a quick peek at
> the driver and I don't think you need that dump. The DSDT is completely
> irrelevant.  The driver will match and load on that generic WMI UUID,
> and this code ensures that it successfully probes regardless of DSDT:
> 
> 
> static acpi_status __init acer_wmi_get_handle_cb(acpi_handle ah, u32 level,
> 						void *ctx, void **retval)
> {
> 	*(acpi_handle *)retval = ah;
> 	return AE_OK;
> }
> 
> static int __init acer_wmi_get_handle(const char *name, const char *prop,
> 					acpi_handle *ah)
> {
> 	acpi_status status;
> 	acpi_handle handle;
> 
> 	BUG_ON(!name || !ah);
> 
> 	handle = NULL;
> 	status = acpi_get_devices(prop, acer_wmi_get_handle_cb,
> 					(void *)name, &handle);
> 
> 	if (ACPI_SUCCESS(status)) {
> 		*ah = handle;
> 		return 0;
> 	} else {
> 		return -ENODEV;
> 	}
> }
> 
> 
> Since acer_wmi_get_handle_cb() doesn't actually test anything, this will
> just return an arbitrary handle regardless of the value of *prop.
> 
>

OK! Understood!
 
> > Then I want to add HID to the norfkill_ids list in acer-wmi to avoid that
> > it loaded on your Lenovo machine like LEN0068.
> 
> I see that this has been done before when this bug has been reported.
> Sorry, but I believe that solution is plain wrong.  You are papering
> over a more fundamental problem.
> 
> Solve the *real* problems in the driver instead:
> 1) "67C3371D-95A3-4C37-BB61-DD47B491DAAB"
> >> 
> >> as can be seen:
> >> 
> >>  bjorn@miraculix:~$ ls -l /sys/devices/virtual/wmi/67C3371D-95A3-4C37-BB61-DD47B491DAAB
> >>  total 0
> >>  -r--r--r-- 1 root root 4096 Jul 14 23:10 modalias
> >>  drwxr-xr-x 2 root root    0 Jul 14 23:10 power
> >>  lrwxrwxrwx 1 root root    0 Jul 14 23:10 subsystem -> ../../../../class/wmi
> >>  -rw-r--r-- 1 root root 4096 Jul 14 23:10 uevent
> >> 
> >> 
> >> But I sort of doubt there is an Acer specific device here.  Maybe this
> >> is a generic accelerometer interface?  Or is there something else going
> >> on?
> >> 
> >> The input device created by the driver just just returns -EPERM when I
> >> try to open it, so I don't think this actually works as-is:
> >> 
> >>  root@miraculix:/tmp# cat /dev/input/event8
> >>  cat: /dev/input/event8: Operation not permitted
> >> 
> >> 
> >> And it does seem a little weird that a Thinkpad should need an Acer
> >> specific platform driver anyway....  thinkpad_acpi is of course handling
> >> all(?) the Thinkpad specifics as expected.
> >> 
> >> 
> >> Bjørn
> >
> > Could you please check that what does the _HID of HKEY device in your DSDT?
> >
> > or please attached your acpidump data to me:
> >  # acpidump > acpidump.raw
> 
> Sorry for the delay.  Just back from vacation.  I took a quick peek at
> the driver and I don't think you need that dump. The DSDT is completely
> irrelevant.  The driver will match and load on that generic WMI UUID,
> and this code ensures that it successfully probes regardless of DSDT:
> 
> 
> static acpi_status __init acer_wmi_get_handle_cb(acpi_handle ah, u32 level,
> 						void *ctx, void **retval)
> {
> 	*(acpi_handle *)retval = ah;
> 	return AE_OK;
> }
> 
> static int __init acer_wmi_get_handle(const char *name, const char *prop,
> 					acpi_handle *ah)
> {
> 	acpi_status status;
> 	acpi_handle handle;
> 
> 	BUG_ON(!name || !ah);
> 
> 	handle = NULL;
> 	status = acpi_get_devices(prop, acer_wmi_get_handle_cb,
> 					(void *)name, &handle);
> 
> 	if (ACPI_SUCCESS(status)) {
> 		*ah = handle;
> 		return 0;
> 	} else {
> 		return -ENODEV;
> 	}
> }
> 
> 
> Since acer_wmi_get_handle_cb() doesn't actually test anything, this will
> just return an arbitrary handle regardless of the value of *prop.
> 
> 
> > Then I want to add HID to the norfkill_ids list in acer-wmi to avoid that
> > it loaded on your Lenovo machine like LEN0068.
> 
> I see that this has been done before when this bug has been reported.
> Sorry, but I believe that solution is plain wrong.  You are papering
> over a more fundamental problem.
> 
> Solve the *real* problems in the driver instead:
> 1)  "67C3371D-95A3-4C37-BB61-DD47B491DAAB" is not an Acer specific GUID
>  and should probably never have been used to match Acer specific
>  devices.  The fact that it happens to work for some aribitrary laptop is
>  not sufficient.
> 
> 2) the "get_handle" code shown above is broken.  If it works for some
>  specific DSDT, then that is just pure luck.
> 
> These things should be fixed ASAP, and backported to stable.  The driver
> as it is will probably both load and probe successfully on a large
> number of systems it has no business messing with.
> 
> 
> > Then, thinkpad_acpi needs to support this _HID.
> 
> No.  Looking further at this, I don't think so. There is no indication
> that there is any relation between that WMI ID and any of the hardware
> functions supported by the acer-wmi driver.  So there is no reason to
> believe that any of the matched Lenovo laptops have the same hardware
> functions.
> 
> 
> 
> Bjørn
>

I have a question about removed 67C3371D-95A3-4C37-BB61-DD47B491DAAB. It
already in acer-wmi a long time since Carlos contributed this driver in
2008. 

Does it possible cause regression on some laptops if we removed this GUID
support in acer-wmi? 


Thanks a lot!
Joey Lee

Re: acer-wmi auto-loaded on a Lenovo Thinkpad X1 Carbon 4th gen

[Bjørn Mork]

joeyli <jlee@suse.com> writes:

> Hi Bjørn, 
>
> Sorry for my delay because I stuck on other issues.

No, problem. The problem is an old one, so there is no hurry :)

And it's always best to take the time necessary to work out a good long
term solution.

> I have a question about removed 67C3371D-95A3-4C37-BB61-DD47B491DAAB. It
> already in acer-wmi a long time since Carlos contributed this driver in
> 2008. 
>
> Does it possible cause regression on some laptops if we removed this GUID
> support in acer-wmi? 

Yes, I guess the risk of that is high.  It might be unacceptably high.
That's for you to decide...

I just wanted to point out that this WMI match seems to be based on
false assumptions initially, and that the current blacklisting strategy
is a game you *will* lose. Or have already lost... Only a small fraction
of the false positives will ever be reported.

The negative match on the "norfkill_ids" list of ACPI IDs from a number
of different vendors for example, will of course make the driver work
with any Acer IDs.  But why not list those instead, doing a positive
match on the systems you want to support? So much easier, and actually
possible to complete.  The problem, of course, is that you don't have a
record of the IDs you want to match...  But collecting those should be
possible if you start making the driver report them. It is most likely
just a single one?

IMHO, for a driver like this, "working" on the target system is not
enough.  You need to ensure that it doesn't negatively affect other
systems. That's impossible when you do negative matching. I don't think
ACPI or WMI drivers should ever have to run a probe on any system they
don't support. Either you have a matching ID, which can be coded into
the module alias table, or you don't.  Or am I missing something?

Anyway, fixing the acer_wmi_get_handle() matching logic in
acer_wmi_accel_setup() should not cause any regressions, since that one
seems to be based on positive matching of the "SENR" method of the
"BST0001" device".  There's just a minor flaw in the ACPI lookup logic.


Bjørn