Re: [PATCH] Add IdeaPad WMI Fn Keys driver

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

Hi Philipp,

On 11/10/22 21:02, Philipp Jungkamp wrote:
> On Mon, 2022-09-19 at 08:54 +0100, Hans de Goede wrote:
>> Hi again,
>>
>> On 9/11/22 17:18, Hans de Goede wrote:
>>> Hi Philipp,
>>>
>>> On 9/11/22 18:04, Philipp Jungkamp wrote:
>>>> Create an input device for WMI events corresponding to some
>>>> special
>>>> keys on the 'Lenovo Yoga' line.
>>>>
>>>> This include the 3 keys to the right on the 'Lenovo Yoga9 14IAP7'
>>>> and
>>>> additionally the 'Lenovo Support' and 'Lenovo Favorites' (star
>>>> with 'S'
>>>> inside) in the fn key row as well as the event emitted on 'Fn+R'
>>>> which
>>>> toggles between 60Hz and 90Hz display refresh rate on windows.
>>>>
>>>> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
>>>> ---
>>>> I found this patch by poking in the DSDT. I have not submitted
>>>> any
>>>> notable patches yet and hope you can help me improve in case I
>>>> make
>>>> unfortunate choices during submission.
>>>
>>> No worries at a first glance (I have not looked at this in any
>>> detail yet) this looks pretty good for a first submission.
>>>
>>> And thank you for contributing to the Linux kernel!
>>>
>>>
>>>> Philipp Jungkamp
>>>>
>>>>  drivers/platform/x86/Kconfig       |  13 +++
>>>>  drivers/platform/x86/Makefile      |   1 +
>>>>  drivers/platform/x86/ideapad-wmi.c | 153
>>>> +++++++++++++++++++++++++++++
>>>>  3 files changed, 167 insertions(+)
>>>>  create mode 100644 drivers/platform/x86/ideapad-wmi.c
>>>>
>>>> diff --git a/drivers/platform/x86/Kconfig
>>>> b/drivers/platform/x86/Kconfig
>>>> index f2f98e942cf2..e7c5148e5cb4 100644
>>>> --- a/drivers/platform/x86/Kconfig
>>>> +++ b/drivers/platform/x86/Kconfig
>>>> @@ -140,6 +140,19 @@ config YOGABOOK_WMI
>>>>           To compile this driver as a module, choose M here: the
>>>> module will
>>>>           be called lenovo-yogabook-wmi.
>>>>
>>>> +config IDEAPAD_WMI
>>>> +       tristate "Lenovo IdeaPad WMI Fn Keys"
>>>> +       depends on ACPI_WMI
>>>> +       depends on INPUT
>>>> +       select INPUT_SPARSEKMAP
>>>> +       help
>>>> +         Say Y here if you want to receive key presses from some
>>>> lenovo
>>>> +         specific keys. (Star Key, Support Key, Virtual
>>>> Background,
>>>> +         Dark Mode Toggle, ...)
>>>> +
>>>> +         To compile this driver as a module, choose M here: the
>>>> module will
>>>> +         be called ideapad-wmi.
>>>> +
>>>>  config ACERHDF
>>>>         tristate "Acer Aspire One temperature and fan driver"
>>>>         depends on ACPI && THERMAL
>>>> diff --git a/drivers/platform/x86/Makefile
>>>> b/drivers/platform/x86/Makefile
>>>> index 5a428caa654a..d8bec884d6bc 100644
>>>> --- a/drivers/platform/x86/Makefile
>>>> +++ b/drivers/platform/x86/Makefile
>>>> @@ -16,6 +16,7 @@ obj-
>>>> $(CONFIG_PEAQ_WMI)                        += peaq-wmi.o
>>>>  obj-$(CONFIG_XIAOMI_WMI)               += xiaomi-wmi.o
>>>>  obj-$(CONFIG_GIGABYTE_WMI)             += gigabyte-wmi.o
>>>>  obj-$(CONFIG_YOGABOOK_WMI)             += lenovo-yogabook-wmi.o
>>>> +obj-$(CONFIG_IDEAPAD_WMI)              += ideapad-wmi.o
>>>>
>>>>  # Acer
>>>>  obj-$(CONFIG_ACERHDF)          += acerhdf.o
>>>> diff --git a/drivers/platform/x86/ideapad-wmi.c
>>>> b/drivers/platform/x86/ideapad-wmi.c
>>>> new file mode 100644
>>>> index 000000000000..38f7b3d0c171
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/ideapad-wmi.c
>>>> @@ -0,0 +1,153 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>> +/*
>>>> + * ideapad-wmi.c - Ideapad WMI fn keys driver
>>>> + *
>>>> + * Copyright (C) 2022 Philipp Jungkamp <p.jungkamp@gmx.net>
>>>> + */
>>>> +
>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> +
>>>> +#include <linux/acpi.h>
>>>> +#include <linux/input.h>
>>>> +#include <linux/input/sparse-keymap.h>
>>>> +#include <linux/list.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/wmi.h>
>>>> +
>>>> +#define IDEAPAD_FN_KEY_EVENT_GUID      "8FC0DE0C-B4E4-43FD-B0F3-
>>>> 8871711C1294"
>>>
>>> At a first hunch (basically huh, don't we have a driver for that
>>> already?) I grepped through the kernel sources and found:
>>>
>>> drivers/platform/x86/ideapad-laptop.c
>>>
>>> can you see if you can make things work with that driver?
>>
>> So I have taken a quick look at this and it seems to me that this
>> really should be able to work with the existing ideapad-laptop.c code
>> ?
>>
>> For starters you could add a debug printk / dev_info to this block,
>>
>> #if IS_ENABLED(CONFIG_ACPI_WMI)
>>         for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
>>                 status =
>> wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
>>                                                    
>> ideapad_wmi_notify, priv);
>>                 if (ACPI_SUCCESS(status)) {
>>                         priv->fnesc_guid =
>> ideapad_wmi_fnesc_events[i];
>>                         break;
>>                 }
>>         }
>>
>>         if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
>>                 err = -EIO;
>>                 goto notification_failed_wmi;
>>         }
>> #endif
>>
>> checking which event GUID ideapad-laptop binds to on your laptop.
>> Assuming that
>> it does bind to the GUID this driver is binding to too, then it would
>> be
>> a matter of extending the existing ideapad_wmi_notify() to do the
>> same
>> as your notify function in this stand-alone driver. Note you can get
>> the
>> the equivalend of the union acpi_object *data argument in your wmi
>> handler
>> by calling wmi_get_event_data().
>>
>> Regards,
>>
>> Hans
>>
> 
> Hello Hans,
> 
> I did actually start by doing that.
> The problem lies with the wmi_get_event_data() function not working
> for my ACPI.
> 
> wmi_get_event_data() takes the event notify_id as input and is supposed
> to give the data associated with the event back. This occures by
> calling _WED on the first WMI block that contains said notify_id.
> 
> drivers/platform/x86/wmi.c:657:
> 	list_for_each_entry(wblock, &wmi_block_list, list) {
> 		struct guid_block *gblock = &wblock->gblock;
> 
> 		if ((gblock->flags & ACPI_WMI_EVENT) && gblock-
>> notify_id == event)
> 			return get_event_data(wblock, out);
> 	}
> 
> The ACPI of the Lenovo Yoga 9 14IAP7 contains multiple WMI event
> blocks hat contain the same notify_id 0xD0.
> 
> Here are two of the four WMI objects found in the DSDT:
> 
> in _WDG of block WMIY:
> 	06129D99-6083-4164-81AD-F092F9D773A6:
> 		notify_id: 0xD0
> 		instance_count: 1
> 		flags: 0x8 ACPI_WMI_EVENT
> 
> in _WDG of block WMIU:
> 	8FC0DE0C-B4E4-43FD-B0F3-8871711C1294:
> 		notify_id: 0xD0
> 		instance_count: 1
> 		flags: 0x8 ACPI_WMI_EVENT
> 
> These event block belong to different WMI devices and report
> unrelated values from different _WED handlers. WMIY for example
> triggers its event on "mode changes", e.g. laptop/tablet/tent based
> on the accelometers/hinge.
> WMIU is the WMI block with the event which reports the special keys
> I'm interested in.
> 
> WMIY comes before WMIU in the wmi_block_list.
> Calling wmi_get_event_data() in ideapad_wmi_notify() calls the wrong
> _WED (the one of WMIY) and thus returns the wrong event data.
> 
> I noticed that the wmi_driver interface does not incur the problem
> with the event because it binds to a wmi_block and calls the _WED
> directly without searching through other WMI devices.

Right, I see in that case I guess that using the wmi_driver
interface does make sense.

> I thought of changing the signature of wmi_get_event_data() to include
> the GUID of the correct WMI block, but chose wmi_driver instead.
> Would you consider adding a wmi_get_event_data_for_guid() function to
> the wmi module and using that in the ideapad_wmi_notify function to be
> a better solution than the one in the patch presented here?

So the problem is that ideapad-laptop is using the old interface
and it does:

                status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
                                                    ideapad_wmi_notify, priv);

And then the code to call that notifier in wmi.c goes like this:

        if (test_bit(WMI_PROBED, &wblock->flags) && wblock->dev.dev.driver) {
                struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
                struct acpi_buffer evdata = { ACPI_ALLOCATE_BUFFER, NULL };
                acpi_status status;

                if (!driver->no_notify_data) {
                        status = get_event_data(wblock, &evdata);
                        if (ACPI_FAILURE(status)) {
                                dev_warn(&wblock->dev.dev, "failed to get event data\n");
                                return;
                        }
                }

                if (driver->notify)
                        driver->notify(&wblock->dev, evdata.pointer);

                kfree(evdata.pointer);
        } else if (wblock->handler) {
                /* Legacy handler */
                wblock->handler(event, wblock->handler_data);
        }

So if we move ahead with your new style wmi-driver for GUID 
8FC0DE0C-B4E4-43FD-B0F3-8871711C1294.

Then we hit the "true" path of the if and the legacy handler
(registered by ideapad-laptop) will never trigger.

I guess you could argue that this is a feature since your
new-style driver "replaces" ideapad-laptop, but ending up with
2 drivers for ideapad laptops / for GUID
8FC0DE0C-B4E4-43FD-B0F3-8871711C1294 and them having them have
to coordinate to decide which one to load sounds less then ideal.

So I think that your idea to add a wmi_get_event_data_for_guid()
function to wmi.c is actually a good idea.

Can you give that a try please and see how that goes ?

Regards,

Hans