diff mbox series

platform/x86: wmi: change notification handler type

Message ID 20211015191322.73388-1-nikolay.romanovich.00@gmail.com (mailing list archive)
State Changes Requested, archived
Headers show
Series platform/x86: wmi: change notification handler type | expand

Commit Message

Mikalai Ramanovich Oct. 15, 2021, 7:13 p.m. UTC 
Since AML code on some Xiaomi laptops notifies the WMI hotkey with
0x20 event, we need ACPI_ALL_NOTIFY here to be able to handle it.

Signed-off-by: Mikalai Ramanovich <nikolay.romanovich.00@gmail.com>
---
 drivers/platform/x86/wmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Hans de Goede Oct. 19, 2021, 3:11 p.m. UTC | #1 
Hi,

On 10/15/21 21:13, Mikalai Ramanovich wrote:
> Since AML code on some Xiaomi laptops notifies the WMI hotkey with
> 0x20 event, we need ACPI_ALL_NOTIFY here to be able to handle it.
> 
> Signed-off-by: Mikalai Ramanovich <nikolay.romanovich.00@gmail.com>

Hmm, this is a rather unusual change and I'm worried that it may have
some bad side-effects.

Can you provide the model-number and an acpidump for the laptop where
you need this ? And maybe also point out which bit (which lines after
disassembling) of the DSDT needs this ?

ATM I'm thinking that it might be best to do something like this:

static u32 acpi_wmi_get_handler_type(void)
{
	if (dmi_name_in_vendors("XIAOMI"))
		return ACPI_ALL_NOTIFY;
	else
		return ACPI_DEVICE_NOTIFY;
}

	status = acpi_install_notify_handler(acpi_device->handle,
					     acpi_wmi_get_handler_type(),
					     acpi_wmi_notify_handler,
					     NULL);

(and the same for the remove)

So that we limit this behavior to the Xiaomi case.

Note you may need to change the capitalization of XIAOMI to match
the value used in /sys/class/dmi/id/sys_vendor

Regards,

Hans



> ---
>  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 e6997be206f1..c34341f4da76 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1352,7 +1352,7 @@ static int acpi_wmi_remove(struct platform_device *device)
>  {
>  	struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev);
>  
> -	acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
> +	acpi_remove_notify_handler(acpi_device->handle, ACPI_ALL_NOTIFY,
>  				   acpi_wmi_notify_handler);
>  	acpi_remove_address_space_handler(acpi_device->handle,
>  				ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler);
> @@ -1385,7 +1385,7 @@ static int acpi_wmi_probe(struct platform_device *device)
>  	}
>  
>  	status = acpi_install_notify_handler(acpi_device->handle,
> -					     ACPI_DEVICE_NOTIFY,
> +					     ACPI_ALL_NOTIFY,
>  					     acpi_wmi_notify_handler,
>  					     NULL);
>  	if (ACPI_FAILURE(status)) {
> @@ -1414,7 +1414,7 @@ static int acpi_wmi_probe(struct platform_device *device)
>  	device_unregister(wmi_bus_dev);
>  
>  err_remove_notify_handler:
> -	acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
> +	acpi_remove_notify_handler(acpi_device->handle, ACPI_ALL_NOTIFY,
>  				   acpi_wmi_notify_handler);
>  
>  err_remove_ec_handler:
>
Mikalai Ramanovich Oct. 21, 2021, 5:25 p.m. UTC | #2 
Hi, 
thank you for your reply.

On Tue, Oct 19, 2021 at 05:11:51PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 10/15/21 21:13, Mikalai Ramanovich wrote:
> > Since AML code on some Xiaomi laptops notifies the WMI hotkey with
> > 0x20 event, we need ACPI_ALL_NOTIFY here to be able to handle it.
> > 
> > Signed-off-by: Mikalai Ramanovich <nikolay.romanovich.00@gmail.com>
> 
> Hmm, this is a rather unusual change and I'm worried that it may have
> some bad side-effects.

I think it can't lead to bad side effects: this driver ignores events 
which are not described in the _WDG section (doesn't have GUID assiciated).

But if it's described it should be handled by this driver even if it
is less than 0x80. But this driver handles only 0x80-0xFF events.

> Can you provide the model-number and an acpidump for the laptop where
> you need this ? And maybe also point out which bit (which lines after
> disassembling) of the DSDT needs this ?

It's Xiaomi Mi Notebook Pro 14 2021. (TIMI A34 by DMI).

Here is a dump of interesting files: 
https://gist.github.com/MikalaiR/eee783cc0b1efdbe2aab158653e84935
(sorry for the link, i don't know it's good to attach files here or not).

The most interesting part is ssdt8.dsl file which contains only one
WMI device. Method \_SB.PC00.LPCB.EC0.XWEV (in ssdt10.dsl, line 2495) 
generates events for this device.

And this is a part of decompiled BMOF from this device:

[WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x40A"), 
 Description("Root/WMI/HID_EVENT20"), 
 guid("{46c93e13-ee9b-4262-8488-563bca757fef}")]
class HID_EVENT20 : WmiEvent {
  [key, read] string InstanceName;
  [read] boolean Active;
  [WmiDataId(1), read, write, Description("Package Data")] uint8 EventDetail[8];
};

ACPI event 0x20 associated with GUID 46c93e13-ee9b-4262-8488-563bca757fef.

> ATM I'm thinking that it might be best to do something like this:
> 
> static u32 acpi_wmi_get_handler_type(void)
> {
> 	if (dmi_name_in_vendors("XIAOMI"))
> 		return ACPI_ALL_NOTIFY;
> 	else
> 		return ACPI_DEVICE_NOTIFY;
> }
> 
> 	status = acpi_install_notify_handler(acpi_device->handle,
> 					     acpi_wmi_get_handler_type(),
> 					     acpi_wmi_notify_handler,
> 					     NULL);
> 
> (and the same for the remove)
> 
> So that we limit this behavior to the Xiaomi case.

In general i don't think it's a good idea, but if it's the only 
acceptable solution, why not.


Regards, 

Mikalai
Hans de Goede Oct. 21, 2021, 6:35 p.m. UTC | #3 
Hi,

On 10/21/21 19:25, Mikalai Ramanovich wrote:
> Hi, 
> thank you for your reply.
> 
> On Tue, Oct 19, 2021 at 05:11:51PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 10/15/21 21:13, Mikalai Ramanovich wrote:
>>> Since AML code on some Xiaomi laptops notifies the WMI hotkey with
>>> 0x20 event, we need ACPI_ALL_NOTIFY here to be able to handle it.
>>>
>>> Signed-off-by: Mikalai Ramanovich <nikolay.romanovich.00@gmail.com>
>>
>> Hmm, this is a rather unusual change and I'm worried that it may have
>> some bad side-effects.
> 
> I think it can't lead to bad side effects: this driver ignores events 
> which are not described in the _WDG section (doesn't have GUID assiciated).
> 
> But if it's described it should be handled by this driver even if it
> is less than 0x80. But this driver handles only 0x80-0xFF events.

Ah right, I forgot about the notify_id check in acpi_wmi_notify_handler(),
so since we have that check this should indeed not lead to wmi drivers
getting new unexpected events.

So I've now applied this patch as is:

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




> 
>> Can you provide the model-number and an acpidump for the laptop where
>> you need this ? And maybe also point out which bit (which lines after
>> disassembling) of the DSDT needs this ?
> 
> It's Xiaomi Mi Notebook Pro 14 2021. (TIMI A34 by DMI).
> 
> Here is a dump of interesting files: 
> https://gist.github.com/MikalaiR/eee783cc0b1efdbe2aab158653e84935
> (sorry for the link, i don't know it's good to attach files here or not).
> 
> The most interesting part is ssdt8.dsl file which contains only one
> WMI device. Method \_SB.PC00.LPCB.EC0.XWEV (in ssdt10.dsl, line 2495) 
> generates events for this device.
> 
> And this is a part of decompiled BMOF from this device:
> 
> [WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x40A"), 
>  Description("Root/WMI/HID_EVENT20"), 
>  guid("{46c93e13-ee9b-4262-8488-563bca757fef}")]
> class HID_EVENT20 : WmiEvent {
>   [key, read] string InstanceName;
>   [read] boolean Active;
>   [WmiDataId(1), read, write, Description("Package Data")] uint8 EventDetail[8];
> };
> 
> ACPI event 0x20 associated with GUID 46c93e13-ee9b-4262-8488-563bca757fef.
> 
>> ATM I'm thinking that it might be best to do something like this:
>>
>> static u32 acpi_wmi_get_handler_type(void)
>> {
>> 	if (dmi_name_in_vendors("XIAOMI"))
>> 		return ACPI_ALL_NOTIFY;
>> 	else
>> 		return ACPI_DEVICE_NOTIFY;
>> }
>>
>> 	status = acpi_install_notify_handler(acpi_device->handle,
>> 					     acpi_wmi_get_handler_type(),
>> 					     acpi_wmi_notify_handler,
>> 					     NULL);
>>
>> (and the same for the remove)
>>
>> So that we limit this behavior to the Xiaomi case.
> 
> In general i don't think it's a good idea, but if it's the only 
> acceptable solution, why not.
> 
> 
> Regards, 
> 
> Mikalai
>
diff mbox series

Patch

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index e6997be206f1..c34341f4da76 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1352,7 +1352,7 @@  static int acpi_wmi_remove(struct platform_device *device)
 {
 	struct acpi_device *acpi_device = ACPI_COMPANION(&device->dev);
 
-	acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
+	acpi_remove_notify_handler(acpi_device->handle, ACPI_ALL_NOTIFY,
 				   acpi_wmi_notify_handler);
 	acpi_remove_address_space_handler(acpi_device->handle,
 				ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler);
@@ -1385,7 +1385,7 @@  static int acpi_wmi_probe(struct platform_device *device)
 	}
 
 	status = acpi_install_notify_handler(acpi_device->handle,
-					     ACPI_DEVICE_NOTIFY,
+					     ACPI_ALL_NOTIFY,
 					     acpi_wmi_notify_handler,
 					     NULL);
 	if (ACPI_FAILURE(status)) {
@@ -1414,7 +1414,7 @@  static int acpi_wmi_probe(struct platform_device *device)
 	device_unregister(wmi_bus_dev);
 
 err_remove_notify_handler:
-	acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
+	acpi_remove_notify_handler(acpi_device->handle, ACPI_ALL_NOTIFY,
 				   acpi_wmi_notify_handler);
 
 err_remove_ec_handler: