Message-ID: <5c248a52-84e9-4900-aabe-43251c53f542@gmx.de>
Date: Sat, 11 Jan 2025 00:23:52 +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 2/4] platform/x86: Add Lenovo GameZone WMI Driver

Am 10.01.25 um 22:33 schrieb Derek John Clark:

> On Thu, Jan 9, 2025 at 2:12 PM Armin Wolf <W_Armin@....de> wrote:
>> Am 02.01.25 um 01:47 schrieb Derek J. Clark:
>>
>>> Adds lenovo-wmi-gamezone.c which provides a driver for the Lenovo
>>> GameZone WMI interface that comes on Lenovo "Gaming Series" hardware.
>>> Provides ACPI platform profiles over WMI.
>>>
>>> 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 gamezone_wmi to lenovo_wmi_gz_priv and gz_wmi to priv.
>>> - Remove GZ_WMI symbol exporting.
>>>
>>> Signed-off-by: Derek J. Clark <derekjohn.clark@...il.com>
>>> ---
>>>    MAINTAINERS                                |   7 +
>>>    drivers/platform/x86/Kconfig               |  11 ++
>>>    drivers/platform/x86/Makefile              |   1 +
>>>    drivers/platform/x86/lenovo-wmi-gamezone.c | 203 +++++++++++++++++++++
>>>    drivers/platform/x86/lenovo-wmi.h          | 105 +++++++++++
>>>    5 files changed, 327 insertions(+)
>>>    create mode 100644 drivers/platform/x86/lenovo-wmi-gamezone.c
>>>    create mode 100644 drivers/platform/x86/lenovo-wmi.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index baf0eeb9a355..8f8a6aec6b92 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -13034,6 +13034,13 @@ S:   Maintained
>>>    W:  http://legousb.sourceforge.net/
>>>    F:  drivers/usb/misc/legousbtower.c
>>>
>>> +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-gamezone.c
>>> +F:   drivers/platform/x86/lenovo-wmi.h
>>> +
>>>    LETSKETCH HID TABLET DRIVER
>>>    M:  Hans de Goede <hdegoede@...hat.com>
>>>    L:  linux-input@...r.kernel.org
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index 0258dd879d64..9a6ac7fdec9f 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -459,6 +459,17 @@ config IBM_RTL
>>>         state = 0 (BIOS SMIs on)
>>>         state = 1 (BIOS SMIs off)
>>>
>>> +config LENOVO_WMI_GAMEZONE
>>> +     tristate "Lenovo GameZone WMI Driver"
>>> +     depends on ACPI_WMI
>>> +     select ACPI_PLATFORM_PROFILE
>>> +     help
>>> +       Say Y here if you have a WMI aware Lenovo Legion device and would like to use the
>>> +       platform-profile firmware interface.
>>> +
>>> +       To compile this driver as a module, choose M here: the module will
>>> +       be called lenovo_wmi_gamezone.
>> Could it be that the resulting kernel module is actually named lenovo-wmi-gamezone?.
>> If yes then please adjust the config description.
>>
> the .o/.ko are named as you described with -, but lsmod lists them
> with _ which is how most would interact with the driver if manually
> loading or blocking it. I'll put whichever you think is most
> appropriate.

I would prefer if you use the .ko names.

>>> +
>>>    config IDEAPAD_LAPTOP
>>>        tristate "Lenovo IdeaPad Laptop Extras"
>>>        depends on ACPI
>>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>>> index e1b142947067..7cb29a480ed2 100644
>>> --- a/drivers/platform/x86/Makefile
>>> +++ b/drivers/platform/x86/Makefile
>>> @@ -68,6 +68,7 @@ obj-$(CONFIG_THINKPAD_LMI)  += think-lmi.o
>>>    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
>>>
>>>    # Intel
>>>    obj-y                               += intel/
>>> diff --git a/drivers/platform/x86/lenovo-wmi-gamezone.c b/drivers/platform/x86/lenovo-wmi-gamezone.c
>>> new file mode 100644
>>> index 000000000000..da5e2bc41f39
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/lenovo-wmi-gamezone.c
>>> @@ -0,0 +1,203 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Lenovo GameZone WMI interface driver. The GameZone WMI interface provides
>>> + * platform profile and fan curve settings for devices that fall under the
>>> + * "Gaming Series" of Lenovo Legion devices.
>>> + *
>>> + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@...il.com>
>>> + */
>>> +
>>> +#include <linux/platform_profile.h>
>>> +#include "lenovo-wmi.h"
>> Please add the necessary includes here and do not rely on the header file to pull them in.
>>
> Ack
>
>>> +
>>> +#define LENOVO_GAMEZONE_GUID "887B54E3-DDDC-4B2C-8B88-68A26A8835D0"
>>> +
>>> +/* Method IDs */
>>> +#define WMI_METHOD_ID_SMARTFAN_SUPP 43 /* IsSupportSmartFan */
>>> +#define WMI_METHOD_ID_SMARTFAN_SET 44 /* SetSmartFanMode */
>>> +#define WMI_METHOD_ID_SMARTFAN_GET 45 /* GetSmartFanMode */
>>> +
>>> +static DEFINE_MUTEX(call_mutex);
>>> +
>>> +static const struct wmi_device_id lenovo_wmi_gamezone_id_table[] = {
>>> +     { LENOVO_GAMEZONE_GUID, NULL }, /* LENOVO_GAMEZONE_DATA */
>>> +     {}
>>> +};
>> Please move those device IDs closer to the driver struct which uses them.
>>
> Ack
>
>>> +
>>> +struct lenovo_wmi_gz_priv {
>>> +     struct wmi_device *wdev;
>>> +     enum platform_profile_option current_profile;
>>> +     struct platform_profile_handler pprof;
>>> +     bool platform_profile_support;
>>> +};
>>> +
>>> +/* Platform Profile Methods */
>>> +static int lenovo_wmi_gamezone_platform_profile_supported(
>>> +     struct platform_profile_handler *pprof, int *supported)
>> Please use ./scripts/checkpatch --strict to catch any coding style violations like this one.
>>
> Ack. Sorry about that.
>
>>> +{
>>> +     struct lenovo_wmi_gz_priv *priv;
>>> +
>>> +     priv = container_of(pprof, struct lenovo_wmi_gz_priv, pprof);
>> Is there a reason why you are not passing priv as an argument? If no then please pass priv
>> as an argument so you can avoid having to use container_of().
>>
>>> +
>>> +     guard(mutex)(&call_mutex);
>> Is there a technical reason why you have to use a mutex for WMI method access? If no then please remove
>> this mutex.
>>
> We weren't sure and figured you would know best practice. I'll remove them.
>
>>> +     return lenovo_wmidev_evaluate_method_1(
>>> +             priv->wdev, 0x0, WMI_METHOD_ID_SMARTFAN_SUPP, 0, supported);
>>> +}
>>> +
>>> +static int
>>> +lenovo_wmi_gamezone_profile_get(struct platform_profile_handler *pprof,
>>> +                             enum platform_profile_option *profile)
>>> +{
>>> +     struct lenovo_wmi_gz_priv *priv;
>>> +     int sel_prof;
>>> +     int err;
>>> +
>>> +     priv = container_of(pprof, struct lenovo_wmi_gz_priv, pprof);
>>> +
>>> +     guard(mutex)(&call_mutex);
>>> +     err = lenovo_wmidev_evaluate_method_1(
>>> +             priv->wdev, 0x0, WMI_METHOD_ID_SMARTFAN_GET, 0, &sel_prof);
>>> +     if (err) {
>>> +             pr_err("Error getting fan profile from WMI interface: %d\n",
>>> +                    err);
>> Please just return here without printing anything, userspace does not benefit from such
>> an error message which only states the obvious.
>>
> Ack all debug return messages.
>
>>> +             return err;
>>> +     }
>>> +
>>> +     switch (sel_prof) {
>>> +     case SMARTFAN_MODE_QUIET:
>>> +             *profile = PLATFORM_PROFILE_QUIET;
>>> +             break;
>>> +     case SMARTFAN_MODE_BALANCED:
>>> +             *profile = PLATFORM_PROFILE_BALANCED;
>>> +             break;
>>> +     case SMARTFAN_MODE_PERFORMANCE:
>>> +             *profile = PLATFORM_PROFILE_PERFORMANCE;
>>> +             break;
>>> +     case SMARTFAN_MODE_CUSTOM:
>>> +             *profile = PLATFORM_PROFILE_CUSTOM;
>>> +             break;
>>> +     default:
>>> +             return -EINVAL;
>>> +     }
>>> +     priv->current_profile = *profile;
>> Please remove this unused variable from priv.
>>
> Ack
>
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int
>>> +lenovo_wmi_gamezone_profile_set(struct platform_profile_handler *pprof,
>>> +                             enum platform_profile_option profile)
>>> +{
>>> +     struct lenovo_wmi_gz_priv *priv;
>>> +     int sel_prof;
>>> +     int err;
>>> +
>>> +     switch (profile) {
>>> +     case PLATFORM_PROFILE_QUIET:
>>> +             sel_prof = SMARTFAN_MODE_QUIET;
>>> +             break;
>>> +     case PLATFORM_PROFILE_BALANCED:
>>> +             sel_prof = SMARTFAN_MODE_BALANCED;
>>> +             break;
>>> +     case PLATFORM_PROFILE_PERFORMANCE:
>>> +             sel_prof = SMARTFAN_MODE_PERFORMANCE;
>>> +             break;
>>> +     case PLATFORM_PROFILE_CUSTOM:
>>> +             sel_prof = SMARTFAN_MODE_CUSTOM;
>>> +             break;
>>> +     default:
>>> +             return -EOPNOTSUPP;
>>> +     }
>>> +
>>> +     priv = container_of(pprof, struct lenovo_wmi_gz_priv, pprof);
>> Please assign priv during declaration.
>>
> Ack
>
>>> +
>>> +     guard(mutex)(&call_mutex);
>>> +     err = lenovo_wmidev_evaluate_method_1(
>>> +             priv->wdev, 0x0, WMI_METHOD_ID_SMARTFAN_SET, sel_prof, NULL);
>>> +     if (err) {
>>> +             pr_err("Error setting fan profile on WMI interface: %u\n", err);
>> Again, this error message only states the obvious, please remove it.
>>
>>> +             return err;
>>> +     }
>>> +
>>> +     priv->current_profile = profile;
>>> +     return 0;
>>> +}
>>> +
>>> +/* Driver Setup */
>>> +static int platform_profile_setup(struct lenovo_wmi_gz_priv *priv)
>>> +{
>>> +     int supported;
>>> +     int err;
>>> +
>>> +     err = lenovo_wmi_gamezone_platform_profile_supported(&priv->pprof,
>>> +                                                          &supported);
>>> +     if (err) {
>>> +             pr_err("Error checking platform profile support: %d\n", err);
>>> +             return err;
>> Please use dev_err() here.
> Ack
>
>>> +     }
>>> +
>>> +     priv->platform_profile_support = supported;
>> The value of platform_profile_support is not used anywhere, please remove it.
>>
>>> +
>>> +     if (!supported)
>>> +             return -EOPNOTSUPP;
>> IMHO returning -ENODEV would make more sense here.
>>
> Ack
>
>>> +
>>> +     priv->pprof.name = "lenovo-wmi-gamezone";
>>> +     priv->pprof.profile_get = lenovo_wmi_gamezone_profile_get;
>>> +     priv->pprof.profile_set = lenovo_wmi_gamezone_profile_set;
>>> +
>>> +     set_bit(PLATFORM_PROFILE_QUIET, priv->pprof.choices);
>>> +     set_bit(PLATFORM_PROFILE_BALANCED, priv->pprof.choices);
>>> +     set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->pprof.choices);
>>> +     set_bit(PLATFORM_PROFILE_CUSTOM, priv->pprof.choices);
>>> +
>>> +     err = lenovo_wmi_gamezone_profile_get(&priv->pprof,
>>> +                                           &priv->current_profile);
>>> +     if (err) {
>>> +             pr_err("Error getting current platform profile: %d\n", err);
>>> +             return err;
>>> +     }
>> Is there a technical reason for reading the current platform profile during device
>> initialization(? If no then please remove this call.
>>
> There isn't, just a misconception on my part. WIll remove.
>
>>> +
>>> +     guard(mutex)(&call_mutex);
>>> +     err = platform_profile_register(&priv->pprof);
>> Using devm_platform_profile_register() would make sense here. This function was added very recently
>> so you have to base your patch series onto the for-next branch.
>>
> Ack
>
>>> +     if (err) {
>>> +             pr_err("Error registering platform profile: %d\n", err);
>>> +             return err;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int lenovo_wmi_gamezone_probe(struct wmi_device *wdev,
>>> +                                  const void *context)
>>> +{
>>> +     struct lenovo_wmi_gz_priv *priv;
>>> +
>>> +     priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>>> +     if (!priv)
>>> +             return -ENOMEM;
>>> +
>>> +     priv->wdev = wdev;
>> Since you are using dev_get_drvdata(), you also need to use dev_set_drvdata() here, otherwise
>> dev_get_drvdata() will return no valid value.
>>
> Ack
>
>>> +     return platform_profile_setup(priv);
>>> +}
>>> +
>>> +static void lenovo_wmi_gamezone_remove(struct wmi_device *wdev)
>>> +{
>>> +     struct lenovo_wmi_gz_priv *priv = dev_get_drvdata(&wdev->dev);
>>> +
>>> +     guard(mutex)(&call_mutex);
>>> +     platform_profile_remove(&priv->pprof);
>>> +}
>>> +
>>> +static struct wmi_driver lenovo_wmi_gamezone_driver = {
>>> +     .driver = { .name = "lenovo_wmi_gamezone" },
>> Please set ".probe_type = PROBE_PREFER_ASYNCHRONOUS" here.
>>
> Ack
>
>> Also does the selected fan profile remain the same after suspending or hibernating?
>> If no then please add the necessary PM callbacks to save/restore the fan profile
>> before suspend/after resume if necessary.
>>
> It remains the same after suspend, hibernate, reboot, and shutdown.

Nice, so no special pm handling is necessary here :).

Thanks,
Armin Wolf

>
>>> +     .id_table = lenovo_wmi_gamezone_id_table,
>>> +     .probe = lenovo_wmi_gamezone_probe,
>>> +     .remove = lenovo_wmi_gamezone_remove,
>> Please set ".no_singleton = true" here.
>>
> Ack
>
>>> +};
>>> +
>>> +module_wmi_driver(lenovo_wmi_gamezone_driver);
>>> +
>>> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_gamezone_id_table);
>>> +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@...il.com>");
>>> +MODULE_DESCRIPTION("Lenovo GameZone WMI Driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h
>>> new file mode 100644
>>> index 000000000000..8a302c6c47cb
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/lenovo-wmi.h
>>> @@ -0,0 +1,105 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later
>>> + *
>>> + * Lenovo Legion WMI interface driver. The Lenovo Legion WMI interface is
>>> + * broken up into multiple GUID interfaces that require cross-references
>>> + * between GUID's for some functionality. The "Custom Mode" interface is a
>>> + * legacy interface for managing and displaying CPU & GPU power and hwmon
>>> + * settings and readings. The "Other Mode" interface is a modern interface
>>> + * that replaces or extends the "Custom Mode" interface methods. The
>>> + * "GameZone" interface adds advanced features such as fan profiles and
>>> + * overclocking. The "Lighting" interface adds control of various status
>>> + * lights related to different hardware components. "Other Method" uses
>>> + * the data structs LENOVO_CAPABILITY_DATA_00, LENOVO_CAPABILITY_DATA_01
>>> + * and LENOVO_CAPABILITY_DATA_02 structs for capability information.
>>> + *
>>> + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@...il.com>
>>> + *
>>> + */
>>> +
>>> +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__
>>> +
>>> +#ifndef _LENOVO_WMI_H_
>>> +#define _LENOVO_WMI_H_
>>> +
>>> +#include <linux/mutex.h>
>>> +#include <linux/types.h>
>>> +#include <linux/wmi.h>
>>> +
>>> +/* Platform Profile Modes */
>>> +#define SMARTFAN_MODE_QUIET 0x01
>>> +#define SMARTFAN_MODE_BALANCED 0x02
>>> +#define SMARTFAN_MODE_PERFORMANCE 0x03
>>> +#define SMARTFAN_MODE_CUSTOM 0xFF
>>> +
>>> +struct wmi_method_args {
>>> +     u32 arg0;
>>> +     u32 arg1;
>>> +};
>>> +
>>> +/* General Use functions */
>>> +static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance,
>>> +                                      u32 method_id, struct acpi_buffer *in,
>>> +                                      struct acpi_buffer *out)
>>> +{
>>> +     acpi_status status;
>>> +
>>> +     status = wmidev_evaluate_method(wdev, instance, method_id, in, out);
>>> +
>>> +     if (ACPI_FAILURE(status))
>>> +             return -EIO;
>>> +
>>> +     return 0;
>>> +};
>>> +
>>> +int lenovo_wmidev_evaluate_method_2(struct wmi_device *wdev, u8 instance,
>>> +                                 u32 method_id, u32 arg0, u32 arg1,
>>> +                                 u32 *retval);
>>> +
>>> +int lenovo_wmidev_evaluate_method_2(struct wmi_device *wdev, u8 instance,
>>> +                                 u32 method_id, u32 arg0, u32 arg1,
>>> +                                 u32 *retval)
>>> +{
>>> +     struct wmi_method_args args = { arg0, arg1 };
>>> +     struct acpi_buffer input = { (acpi_size)sizeof(args), &args };
>>> +     struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>>> +     union acpi_object *ret_obj = NULL;
>>> +     int err;
>> Please order the variable declarations in reverse XMAS tree order.
>>
> Ack
>
>>> +
>>> +     err = lenovo_wmidev_evaluate_method(wdev, instance, method_id, &input,
>>> +                                         &output);
>>> +
>>> +     if (err) {
>>> +             pr_err("Attempt to get method value failed.\n");
>> Please remove any error messages in this part of the code, printing error messages should
>> ideally happen at the higher layers of the driver if necessary.
>>
>>> +             return err;
>>> +     }
>>> +
>>> +     if (retval) {
>>> +             ret_obj = (union acpi_object *)output.pointer;
>>> +             if (!ret_obj) {
>>> +                     pr_err("Failed to get valid ACPI object from WMI interface\n");
>>> +                     return -EIO;
>> -ENODATA.
>>
> Ack
>
>>> +             }
>>> +             if (ret_obj->type != ACPI_TYPE_INTEGER) {
>>> +                     pr_err("WMI query returnd ACPI object with wrong type.\n");
>>> +                     kfree(ret_obj);
>>> +                     return -EIO;
>> -ENXIO.
>>
> Ack
>
>>> +             }
>>> +             *retval = (u32)ret_obj->integer.value;
>>> +     }
>>> +
>>> +     kfree(ret_obj);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance,
>>> +                                 u32 method_id, u32 arg0, u32 *retval);
>>> +
>>> +int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance,
>>> +                                 u32 method_id, u32 arg0, u32 *retval)
>>> +{
>>> +     return lenovo_wmidev_evaluate_method_2(wdev, instance, method_id, arg0,
>>> +                                            0, retval);
>>> +}
>>> +
>>> +#endif /* !_LENOVO_WMI_H_ */
> Thanks Armin,
> Derek

Powered by blists - more mailing lists