Message-ID: <dbaed77b-7fc4-4133-9f03-3d50c93e8b13@gmx.de>
Date: Sat, 11 Jan 2025 01:01:28 +0100
From: Armin Wolf <W_Armin@....de>
To: Derek John Clark <derekjohn.clark@...il.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Jonathan Corbet <corbet@....net>, Mario Limonciello <superm1@...nel.org>,
 Luke Jones <luke@...nes.dev>, Xino Ni <nijs1@...ovo.com>,
 Zhixin Zhang <zhangzx36@...ovo.com>, Mia Shao <shaohz1@...ovo.com>,
 Mark Pearson <mpearson-lenovo@...ebb.ca>,
 "Pierre-Loup A . Griffais" <pgriffais@...vesoftware.com>,
 "Cody T . -H . Chiu" <codyit@...il.com>, John Martens <johnfanv2@...il.com>,
 platform-driver-x86@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/4] platform/x86: Add Lenovo Capability Data 01 WMI
 Driver

Am 10.01.25 um 23:11 schrieb Derek John Clark:

> On Thu, Jan 9, 2025 at 2:35 PM Armin Wolf <W_Armin@....de> wrote:
>> Am 02.01.25 um 01:47 schrieb Derek J. Clark:
>>
>>> Adds lenovo-wmi-capdata01.c which provides a driver for the
>>> LENOVO_CAPABILITY_DATA_01 WMI data block that comes on "Other Method"
>>> enabled hardware. Provides an interface for querying if a given
>>> attribute is supported by the hardware, as well as its default_value,
>>> max_value, min_value, and step increment.
>>>
>>> v2:
>>> - Use devm_kzalloc to ensure driver can be instanced, remove global
>>>     reference.
>>> - Ensure reverse Christmas tree for all variable declarations.
>>> - Remove extra whitespace.
>>> - Use guard(mutex) in all mutex instances, global mutex.
>>> - Use pr_fmt instead of adding the driver name to each pr_err.
>>> - Remove noisy pr_info usage.
>>> - Rename capdata_wmi to lenovo_wmi_cd01_priv and cd01_wmi to priv.
>>> - Use list to get the lenovo_wmi_cd01_priv instance in
>>>     lenovo_wmi_capdata01_get as none of the data provided by the macros
>>>     that will use it can pass a member of the struct for use in
>>>     container_of.
>>>
>>> Signed-off-by: Derek J. Clark <derekjohn.clark@...il.com>
>>> ---
>>>    MAINTAINERS                                 |   1 +
>>>    drivers/platform/x86/Kconfig                |  11 ++
>>>    drivers/platform/x86/Makefile               |   1 +
>>>    drivers/platform/x86/lenovo-wmi-capdata01.c | 131 ++++++++++++++++++++
>>>    drivers/platform/x86/lenovo-wmi.h           |  20 +++
>>>    5 files changed, 164 insertions(+)
>>>    create mode 100644 drivers/platform/x86/lenovo-wmi-capdata01.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 8f8a6aec6b92..c9374c395905 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -13038,6 +13038,7 @@ LENOVO WMI drivers
>>>    M:  Derek J. Clark <derekjohn.clark@...il.com>
>>>    L:  platform-driver-x86@...r.kernel.org
>>>    S:  Maintained
>>> +F:   drivers/platform/x86/lenovo-wmi-capdata01.c
>>>    F:  drivers/platform/x86/lenovo-wmi-gamezone.c
>>>    F:  drivers/platform/x86/lenovo-wmi.h
>>>
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index 9a6ac7fdec9f..a2c1ab47ad9e 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -470,6 +470,17 @@ config LENOVO_WMI_GAMEZONE
>>>          To compile this driver as a module, choose M here: the module will
>>>          be called lenovo_wmi_gamezone.
>>>
>>> +config LENOVO_WMI_DATA01
>>> +     tristate "Lenovo Legion WMI capability Data 01 Driver"
>>> +     depends on ACPI_WMI
>>> +     help
>>> +       Say Y here if you have a WMI aware Lenovo Legion device in the "Gaming Series"
>>> +       line of hardware. This interface is a dependency for exposing tunable power
>>> +       settings.
>>> +
>>> +       To compile this driver as a module, choose M here: the module will
>>> +       be called lenovo_wmi_capdata01.
>> Could it be that the resulting module will be called lenovo-wmi-capdata01? Also if its a mere
>> dependency without any value when being used alone then it would make sense to hide it from
>> users by removing the help texts:
>>
>>          config LENOVO_WMI_DATA01
>>                  tristate
>>                  depends on ACPI_WMI
>>
> Makes sense, Will do
>
>>> +
>>>    config IDEAPAD_LAPTOP
>>>        tristate "Lenovo IdeaPad Laptop Extras"
>>>        depends on ACPI
>>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>>> index 7cb29a480ed2..6c96cc3f3855 100644
>>> --- a/drivers/platform/x86/Makefile
>>> +++ b/drivers/platform/x86/Makefile
>>> @@ -69,6 +69,7 @@ obj-$(CONFIG_YOGABOOK)              += lenovo-yogabook.o
>>>    obj-$(CONFIG_YT2_1380)              += lenovo-yoga-tab2-pro-1380-fastcharger.o
>>>    obj-$(CONFIG_LENOVO_WMI_CAMERA)     += lenovo-wmi-camera.o
>>>    obj-$(CONFIG_LENOVO_WMI_GAMEZONE)   += lenovo-wmi-gamezone.o
>>> +obj-$(CONFIG_LENOVO_WMI_DATA01)      += lenovo-wmi-capdata01.o
>>>
>>>    # Intel
>>>    obj-y                               += intel/
>>> diff --git a/drivers/platform/x86/lenovo-wmi-capdata01.c b/drivers/platform/x86/lenovo-wmi-capdata01.c
>>> new file mode 100644
>>> index 000000000000..b10a6e4b320f
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/lenovo-wmi-capdata01.c
>>> @@ -0,0 +1,131 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * LENOVO_CAPABILITY_DATA_01 WMI data block driver. This interface provides
>>> + * information on tunable attributes used by the "Other Method" WMI interface,
>>> + * including if it is supported by the hardware, the default_value, max_value,
>>> + * min_value, and step increment.
>>> + *
>>> + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@...il.com>
>>> + */
>>> +
>>> +#include <linux/list.h>
>>> +#include "lenovo-wmi.h"
>>> +
>>> +#define LENOVO_CAPABILITY_DATA_01_GUID "7A8F5407-CB67-4D6E-B547-39B3BE018154"
>>> +
>>> +static DEFINE_MUTEX(cd01_call_mutex);
>> Does the WMI interface really rely on such mutual exclusion of callers? If no then
>> please remove this mutex.
>>
> As with the other drivers, will remove.
>
>>> +static DEFINE_MUTEX(cd01_list_mutex);
>>> +static LIST_HEAD(cd01_wmi_list);
>>> +
>>> +static const struct wmi_device_id lenovo_wmi_capdata01_id_table[] = {
>>> +     { LENOVO_CAPABILITY_DATA_01_GUID, NULL },
>>> +     {}
>>> +};
>>> +
>>> +struct lenovo_wmi_cd01_priv {
>>> +     struct wmi_device *wdev;
>>> +     struct list_head list;
>>> +};
>>> +
>>> +static inline struct lenovo_wmi_cd01_priv *get_first_wmi_priv(void)
>>> +{
>>> +     guard(mutex)(&cd01_list_mutex);
>>> +     return list_first_entry_or_null(&cd01_wmi_list,
>>> +                                     struct lenovo_wmi_cd01_priv, list);
>> Two things:
>>
>> 1. This will cause issues should a WMI device in this list be removed while a
>> consumer is using it. In this case you will need extend the scope of the list mutex.
>>
>> 2. Do multiple capdata01 WMI devices exist in any systems? If no then please consider
>> using the component framework (https://docs.kernel.org/driver-api/component.html) which
>> will simplify the interop between the consumer driver of capdata01 and this driver.
>>
> I looked into this and struggled with it for a while, do you have any
> good examples I can reference?
> Will this allow me to pass struct lenovo_wmi_cd01_priv *priv to this
> function from the other mode driver? If so, should I avoid calling it
> priv since it will be accessible outside the driver?

You can use the i915 GSC proxy code as a reference.

For the component supplier (capdata01):

- use drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c as an example

1. Register a component with component_add().
2. Use the .bind callback inside struct component_ops to pass a pointer to an array of capdata01 items to the component master.

For the component master (lenovo-wmi-other):

- use drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c as an example

1. Add a match function using component_match_add(). I suggest that you implement a public function inside the capdata01 driver
which does the following (pseudo code):

	int lenovo_wmi_capdata01_match(struct device *dev, void *data)
	{
		return dev->driver == &lenovo_wmi_capdata01_driver.driver;
	}
	EXPORT_SYMBOL_GPL(lenovo_wmi_capdata01_match);

This function can then be used to match against the component provided by lenovo-wmi-capdata01.

2. Add the component master using component_master_add_with_match(). Use the .bind callback inside
struct component_master_ops to register the firmware attribute class device. You can use component_bind_all()
to pass a pointer to the bound components. In this case this pointer can be used by the capdata01 driver
to pass an array of capdata01 items to you.

Keep in mind that you cannot use devres inside the .bind callbacks.

>>> +}
>>> +
>>> +int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id,
>>> +                          struct capability_data_01 *cap_data)
>>> +{
>>> +     u32 attribute_id = *(int *)&attr_id;
>> This will cause alignment issues, please use FIELD_GET()/FIELD_PREP() to construct a u32 to
>> pass to this function.
>>
> Can do.
>
>>> +     struct lenovo_wmi_cd01_priv *priv;
>>> +     union acpi_object *ret_obj;
>>> +     int instance_idx;
>>> +     int count;
>>> +
>>> +     priv = get_first_wmi_priv();
>>> +     if (!priv)
>>> +             return -ENODEV;
>>> +
>>> +     guard(mutex)(&cd01_call_mutex);
>>> +     count = wmidev_instance_count(priv->wdev);
>>> +     pr_info("Got instance count: %u\n", count);
>>> +
>>> +     for (instance_idx = 0; instance_idx < count; instance_idx++) {
>>> +             ret_obj = wmidev_block_query(priv->wdev, instance_idx);
>>> +             if (!ret_obj) {
>>> +                     pr_err("WMI Data block query failed.\n");
>>> +                     continue;
>>> +             }
>>> +
>>> +             if (ret_obj->type != ACPI_TYPE_BUFFER) {
>>> +                     pr_err("WMI Data block query returned wrong type.\n");
>>> +                     kfree(ret_obj);
>>> +                     continue;
>>> +             }
>>> +
>>> +             if (ret_obj->buffer.length != sizeof(*cap_data)) {
>>> +                     pr_err("WMI Data block query returned wrong buffer length: %u vice expected %lu.\n",
>>> +                            ret_obj->buffer.length, sizeof(*cap_data));
>>> +                     kfree(ret_obj);
>>> +                     continue;
>>> +             }
>>> +
>>> +             memcpy(cap_data, ret_obj->buffer.pointer,
>>> +                    ret_obj->buffer.length);
>>> +             kfree(ret_obj);
>>> +
>>> +             if (cap_data->id != attribute_id)
>>> +                     continue;
>>> +             break;
>>> +     }
>> Maybe it would make sense to read this data during device initialization and store it
>> inside an array? This way looking up capability data would be _much_ faster especially
>> since WMI calls are usually quite slow.
>>
> I was looking into this as I agree that would be preferable but wasn't
> able to get a working version. Since I don't know the array length at
> compile time I tried using krealloc_array after getting
> wmidev_instance_count to resize a capdata array stored in priv, but
> that would result in the driver crashing for some reason. I'll take
> another shot at it.

I suggest you use devm_kmalloc_array(), with the first argument being the return value of wmidev_instance_count().
The second parameter will be the size of a capdata01 structure.

Then you just need to fill-in the capdata01 structures using a for-loop.

>> Also this function is way to noisy when it comes to error messages. Please leave this
>> to the caller of this function.
> Can do. If I don't get a ret_obj should I quit the loop here?

You can continue the loop in such a case, so that the other attributes are still available to the lenovo-wmi-other driver.
However please print a warning message if you skip an instance so that users can find out why some attributes are missing.

Thanks,
Armin Wolf

>>> +
>>> +     if (cap_data->id != attribute_id) {
>>> +             pr_err("Unable to find capability data for attribute_id %x\n",
>>> +                    attribute_id);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(lenovo_wmi_capdata01_get, "CAPDATA_WMI");
>>> +
>>> +static int lenovo_wmi_capdata01_probe(struct wmi_device *wdev,
>>> +                                   const void *context)
>>> +
>>> +{
>>> +     struct lenovo_wmi_cd01_priv *priv;
>>> +
>>> +     priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>>> +     if (!priv)
>>> +             return -ENOMEM;
>>> +
>>> +     priv->wdev = wdev;
>>> +
>>> +     guard(mutex)(&cd01_list_mutex);
>>> +     list_add_tail(&priv->list, &cd01_wmi_list);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static void lenovo_wmi_capdata01_remove(struct wmi_device *wdev)
>>> +{
>>> +     struct lenovo_wmi_cd01_priv *priv = dev_get_drvdata(&wdev->dev);
>>> +
>>> +     guard(mutex)(&cd01_list_mutex);
>>> +     list_del(&priv->list);
>>> +}
>>> +
>>> +static struct wmi_driver lenovo_wmi_capdata01_driver = {
>>> +     .driver = { .name = "lenovo_wmi_capdata01" },
>> Please set ".probe_type = PROBE_PREFER_ASYNCHRONOUS".
>>
> Ack
>
>>> +     .id_table = lenovo_wmi_capdata01_id_table,
>>> +     .probe = lenovo_wmi_capdata01_probe,
>>> +     .remove = lenovo_wmi_capdata01_remove,
>> Please set ".no_singleton = true".
>>
> Ack
>
> Thanks,
> Derek
>
>> Thanks,
>> Armin Wolf
>>
>>> +};
>>> +
>>> +module_wmi_driver(lenovo_wmi_capdata01_driver);
>>> +
>>> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_capdata01_id_table);
>>> +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@...il.com>");
>>> +MODULE_DESCRIPTION("Lenovo Capability Data 01 WMI Driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h
>>> index 8a302c6c47cb..53cea84a956b 100644
>>> --- a/drivers/platform/x86/lenovo-wmi.h
>>> +++ b/drivers/platform/x86/lenovo-wmi.h
>>> @@ -36,6 +36,22 @@ struct wmi_method_args {
>>>        u32 arg1;
>>>    };
>>>
>>> +struct lenovo_wmi_attr_id {
>>> +     u32 mode_id : 16; /* Fan profile */
>>> +     u32 feature_id : 8; /* Attribute (SPL/SPPT/...) */
>>> +     u32 device_id : 8; /* CPU/GPU/... */
>>> +} __packed;
>>> +
>>> +/* Data struct for LENOVO_CAPABILITY_DATA_01 */
>>> +struct capability_data_01 {
>>> +     u32 id;
>>> +     u32 supported;
>>> +     u32 default_value;
>>> +     u32 step;
>>> +     u32 min_value;
>>> +     u32 max_value;
>>> +};
>>> +
>>>    /* General Use functions */
>>>    static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance,
>>>                                         u32 method_id, struct acpi_buffer *in,
>>> @@ -102,4 +118,8 @@ int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance,
>>>                                               0, retval);
>>>    }
>>>
>>> +/* LENOVO_CAPABILITY_DATA_01 exported functions */
>>> +int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id,
>>> +                          struct capability_data_01 *cap_data);
>>> +
>>>    #endif /* !_LENOVO_WMI_H_ */

Powered by blists - more mailing lists