Re: [PATCH] platform/x86: wmi: change notification handler type

[Hans de Goede <hdegoede@redhat.com>]

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
>