lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <19207b85-5842-4ff9-81b3-da80aedf7995@gmx.de>
Date: Sun, 16 Mar 2025 01:01:14 +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 v3 2/4] platform/x86: Add Lenovo Gamezone WMI Driver

Am 14.03.25 um 22:29 schrieb Derek John Clark:

> On Tue, Mar 11, 2025 at 1:30 PM Armin Wolf <W_Armin@....de> wrote:
>> Am 10.03.25 um 23:11 schrieb Derek John Clark:
>>
>>> On Fri, Mar 7, 2025 at 2:48 PM Armin Wolf <W_Armin@....de> wrote:
>>>> Am 25.02.25 um 22:59 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.
>>>>>
>>>>> Adds lenovo-wmi.h and lenovo-wmi.c which provide helper functions for
>>>>> wmidev_evaluate_method as well as prototypes for exported functions.
>>>>> v3:
>>>>> - Use notifier chain to report platform profile changes to any
>>>>>      subscribed drivers.
>>>>> - Adds THERMAL_MODE_EVENT GUID and .notify function to trigger notifier
>>>>>      chain.
>>>>> - Adds support for Extreme Mode profile on supported hardware, as well
>>>>>      as a DMI quirk table for some devices that report extreme mode version
>>>>>      support but so not have it fully implemented.
>>>>> - Update to include recent changes to platform-profile.
>>>>> v2:
>>>>> - Use devm_kmalloc 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                                |   3 +
>>>>>     drivers/platform/x86/Kconfig               |  16 +
>>>>>     drivers/platform/x86/Makefile              |   2 +
>>>>>     drivers/platform/x86/lenovo-wmi-gamezone.c | 374 +++++++++++++++++++++
>>>>>     drivers/platform/x86/lenovo-wmi.c          |  77 +++++
>>>>>     drivers/platform/x86/lenovo-wmi.h          |  62 ++++
>>>>>     6 files changed, 534 insertions(+)
>>>>>     create mode 100644 drivers/platform/x86/lenovo-wmi-gamezone.c
>>>>>     create mode 100644 drivers/platform/x86/lenovo-wmi.c
>>>>>     create mode 100644 drivers/platform/x86/lenovo-wmi.h
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index e20c32b3c480..cf7f4fce1a25 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -13157,6 +13157,9 @@ L:    platform-driver-x86@...r.kernel.org
>>>>>     S:  Maintained
>>>>>     F:  Documentation/wmi/devices/lenovo-wmi-gamezone.rst
>>>>>     F:  Documentation/wmi/devices/lenovo-wmi-other.rst
>>>>> +F:   drivers/platform/x86/lenovo-wmi-gamezone.c
>>>>> +F:   drivers/platform/x86/lenovo-wmi.c
>>>>> +F:   drivers/platform/x86/lenovo-wmi.h
>>>>>
>>>>>     LETSKETCH HID TABLET DRIVER
>>>>>     M:  Hans de Goede <hdegoede@...hat.com>
>>>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>>>> index 7e20a58861eb..875822e6bd65 100644
>>>>> --- a/drivers/platform/x86/Kconfig
>>>>> +++ b/drivers/platform/x86/Kconfig
>>>>> @@ -459,6 +459,22 @@ config IBM_RTL
>>>>>          state = 0 (BIOS SMIs on)
>>>>>          state = 1 (BIOS SMIs off)
>>>>>
>>>>> +config LENOVO_WMI
>>>>> +     tristate
>>>>> +     depends on ACPI_WMI
>>>> Please rename this module to LENOVO_WMI_HELPERS.
>>> Acked
>>>
>>>>> +
>>>>> +config LENOVO_WMI_GAMEZONE
>>>>> +     tristate "Lenovo GameZone WMI Driver"
>>>>> +     depends on ACPI_WMI
>>>>> +     select ACPI_PLATFORM_PROFILE
>>>>> +     select LENOVO_WMI
>>>>> +     help
>>>>> +       Say Y here if you have a WMI aware Lenovo Legion device and would like to use the
>>>>> +       platform-profile firmware interface to manage power usage.
>>>>> +
>>>>> +       To compile this driver as a module, choose M here: the module will
>>>>> +       be called lenovo-wmi-gamezone.
>>>>> +
>>>>>     config IDEAPAD_LAPTOP
>>>>>         tristate "Lenovo IdeaPad Laptop Extras"
>>>>>         depends on ACPI
>>>>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>>>>> index 5f6307246e69..4a7b2d14eb82 100644
>>>>> --- a/drivers/platform/x86/Makefile
>>>>> +++ b/drivers/platform/x86/Makefile
>>>>> @@ -67,7 +67,9 @@ obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o
>>>>>     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)     += lenovo-wmi.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..d5329fecba47
>>>>> --- /dev/null
>>>>> +++ b/drivers/platform/x86/lenovo-wmi-gamezone.c
>>>>> @@ -0,0 +1,374 @@
>>>>> +// 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/container_of.h"
>>>>> +#include "linux/printk.h"
>>>>> +#include <linux/cleanup.h>
>>>>> +#include <linux/dev_printk.h>
>>>>> +#include <linux/dmi.h>
>>>>> +#include <linux/list.h>
>>>>> +#include <linux/notifier.h>
>>>>> +#include <linux/platform_profile.h>
>>>>> +#include <linux/types.h>
>>>>> +#include <linux/wmi.h>
>>>>> +#include "lenovo-wmi.h"
>>>>> +
>>>>> +/* Interface GUIDs */
>>>>> +#define LENOVO_GAMEZONE_GUID "887B54E3-DDDC-4B2C-8B88-68A26A8835D0"
>>>>> +#define THERMAL_MODE_EVENT_GUID "D320289E-8FEA-41E0-86F9-911D83151B5F"
>>>>> +
>>>>> +/* 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 */
>>>>> +
>>>>> +enum lenovo_wmi_gz_type {
>>>>> +     GAMEZONE_FULL = 1,
>>>>> +     THERMAL_MODE,
>>>>> +};
>>>>> +
>>>>> +#define GAMEZONE_WMI_DEVICE(guid, type)                              \
>>>>> +     .guid_string = (guid), .context = &(enum lenovo_wmi_gz_type) \
>>>>> +     {                                                            \
>>>>> +             type                                                 \
>>>>> +     }
>>>>> +
>>>>> +static BLOCKING_NOTIFIER_HEAD(gz_chain_head);
>>>>> +static DEFINE_MUTEX(gz_chain_mutex);
>>>>> +
>>>>> +struct lenovo_wmi_gz_priv {
>>>>> +     enum platform_profile_option current_profile;
>>>> This variable is only assigned and never read, please remove it.
>>> You're correct for this version. I re-added it when working on the
>>> notifier chain but didn't end up using it. I'll make sure no unused
>>> variables are in the next version.
>>>
>>>>> +     struct wmi_device *wdev;
>>>>> +     bool extreme_supported;
>>>>> +     struct device *ppdev; /*platform profile device */
>>>>> +     enum lenovo_wmi_gz_type type;
>>>>> +     struct blocking_notifier_head nhead;
>>>>> +};
>>>>> +
>>>>> +struct quirk_entry {
>>>>> +     bool extreme_supported;
>>>>> +};
>>>>> +
>>>>> +static struct quirk_entry quirk_no_extreme_bug = {
>>>>> +     .extreme_supported = false,
>>>>> +};
>>>> Can you make this const?
> I get warnings when setting the drvdata with this as a const
>
> drivers/platform/x86/lenovo-wmi-gamezone.c:144:32: warning:
> initialization discards ‘const’ qualifier from pointer target type
> [-Wdiscarded-qualifiers]
>   144 |                 .driver_data = &quirk_no_extreme_bug,
>       |                                ^
>
Alright, then you can keep it non-const.

>>>>> +
>>>>> +/* Platform Profile Methods & Setup */
>>>>> +static int
>>>>> +lenovo_wmi_gz_platform_profile_supported(struct lenovo_wmi_gz_priv *priv,
>>>>> +                                      int *supported)
>>>>> +{
>>>>> +     return lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0,
>>>>> +                                            WMI_METHOD_ID_SMARTFAN_SUPP, 0, supported);
>>>>> +}
>>>>> +
>>>>> +static int lenovo_wmi_gz_profile_get(struct device *dev,
>>>>> +                                  enum platform_profile_option *profile)
>>>>> +{
>>>>> +     struct lenovo_wmi_gz_priv *priv = dev_get_drvdata(dev);
>>>>> +     int sel_prof;
>>>>> +     int ret;
>>>>> +
>>>>> +     ret = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0,
>>>>> +                                           WMI_METHOD_ID_SMARTFAN_GET, 0, &sel_prof);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     switch (sel_prof) {
>>>>> +     case SMARTFAN_MODE_QUIET:
>>>>> +             *profile = PLATFORM_PROFILE_LOW_POWER;
>>>>> +             break;
>>>>> +     case SMARTFAN_MODE_BALANCED:
>>>>> +             *profile = PLATFORM_PROFILE_BALANCED;
>>>>> +             break;
>>>>> +     case SMARTFAN_MODE_PERFORMANCE:
>>>>> +             if (priv->extreme_supported) {
>>>>> +                     *profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
>>>>> +                     break;
>>>>> +             }
>>>>> +             *profile = PLATFORM_PROFILE_PERFORMANCE;
>>>>> +             break;
>>>>> +     case SMARTFAN_MODE_EXTREME:
>>>>> +             *profile = PLATFORM_PROFILE_PERFORMANCE;
>>>>> +             break;
>>>>> +     case SMARTFAN_MODE_CUSTOM:
>>>>> +             *profile = PLATFORM_PROFILE_CUSTOM;
>>>>> +             break;
>>>>> +     default:
>>>>> +             return -EINVAL;
>>>>> +     }
>>>>> +
>>>>> +     priv->current_profile = *profile;
>>>>> +
>>>>> +     ret = blocking_notifier_call_chain(&gz_chain_head, THERMAL_MODE_EVENT,
>>>>> +                                        &sel_prof);
>>>> Is it really necessary to call the notifier here? AFAIK the notifier needs to be called
>>>> only:
>>>>
>>>>     - when the platform profile was changed either by the user or the firmware.
>>>>     - when a new notifier handler was registered so that the handler does not have to wait for the next user input
>>>>
>>>> Please only call the notifier in those two situations.
>>>>
>>> I was originally calling it here to solve a problem with synchronizing
>>> the drivers. Lenovo-wmi-other registers a notifier block before
>>> gamezone inits the platform profile. That means I can't use _notify
>>> during the _register to get the initial profile to the block members.
>>> Since the platform profile makes an initial request after it registers
>>> this is a simple way to propagate when the platform profile is ready.
>>> I'm not sure of another way to trigger a notification chain once the
>>> information is available.
>>>
>>>>> +     if (ret == NOTIFY_BAD)
>>>>> +             pr_err("Failed to send notification to call chain for WMI event %u\n",
>>>>> +                    priv->type);
>>>> Use dev_err() here please.
>>>>
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int lenovo_wmi_gz_profile_set(struct device *dev,
>>>>> +                                  enum platform_profile_option profile)
>>>>> +{
>>>>> +     struct lenovo_wmi_gz_priv *priv = dev_get_drvdata(dev);
>>>>> +     int sel_prof;
>>>>> +     int ret;
>>>>> +
>>>>> +     switch (profile) {
>>>>> +     case PLATFORM_PROFILE_LOW_POWER:
>>>>> +             sel_prof = SMARTFAN_MODE_QUIET;
>>>>> +             break;
>>>>> +     case PLATFORM_PROFILE_BALANCED:
>>>>> +             sel_prof = SMARTFAN_MODE_BALANCED;
>>>>> +             break;
>>>>> +     case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>>>>> +             sel_prof = SMARTFAN_MODE_PERFORMANCE;
>>>>> +             break;
>>>>> +     case PLATFORM_PROFILE_PERFORMANCE:
>>>>> +             if (priv->extreme_supported) {
>>>>> +                     sel_prof = SMARTFAN_MODE_EXTREME;
>>>>> +                     break;
>>>>> +             }
>>>>> +             sel_prof = SMARTFAN_MODE_PERFORMANCE;
>>>>> +             break;
>>>>> +     case PLATFORM_PROFILE_CUSTOM:
>>>>> +             sel_prof = SMARTFAN_MODE_CUSTOM;
>>>>> +             break;
>>>>> +     default:
>>>>> +             return -EOPNOTSUPP;
>>>>> +     }
>>>>> +
>>>>> +     ret = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0,
>>>>> +                                           WMI_METHOD_ID_SMARTFAN_SET, sel_prof, NULL);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct dmi_system_id fwbug_list[] = {
>>>>> +     {
>>>>> +             .ident = "Legion Go 8APU1",
>>>>> +             .matches = {
>>>>> +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>>>> +                     DMI_MATCH(DMI_PRODUCT_VERSION, "Legion Go 8APU1"),
>>>>> +             },
>>>>> +             .driver_data = &quirk_no_extreme_bug,
>>>>> +     },
>>>>> +     {
>>>>> +             .ident = "Legion Go S 8ARP1",
>>>>> +             .matches = {
>>>>> +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>>>> +                     DMI_MATCH(DMI_PRODUCT_VERSION, "Legion Go S 8ARP1"),
>>>>> +             },
>>>>> +             .driver_data = &quirk_no_extreme_bug,
>>>>> +     },
>>>>> +     {
>>>>> +             .ident = "Legion Go S 8APU1",
>>>>> +             .matches = {
>>>>> +                     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>>>> +                     DMI_MATCH(DMI_PRODUCT_VERSION, "Legion Go S 8APU1"),
>>>>> +             },
>>>>> +             .driver_data = &quirk_no_extreme_bug,
>>>>> +     },
>>>>> +     {},
>>>>> +
>>>>> +};
>>>>> +
>>>>> +static bool extreme_supported(int profile_support_ver)
>>>>> +{
>>>>> +     const struct dmi_system_id *dmi_id;
>>>>> +     struct quirk_entry *quirks;
>>>>> +
>>>>> +     if (profile_support_ver < 6)
>>>>> +             return false;
>>>>> +
>>>>> +     dmi_id = dmi_first_match(fwbug_list);
>>>>> +     if (!dmi_id)
>>>>> +             return true;
>>>>> +
>>>>> +     quirks = dmi_id->driver_data;
>>>>> +     return quirks->extreme_supported;
>>>>> +}
>>>>> +
>>>>> +static int lenovo_wmi_platform_profile_probe(void *drvdata,
>>>>> +                                          unsigned long *choices)
>>>>> +{
>>>>> +     struct lenovo_wmi_gz_priv *priv = drvdata;
>>>>> +     enum platform_profile_option profile;
>>>> Unused variable, please remove.
>>>>
>>>>> +     int profile_support_ver;
>>>>> +     int ret;
>>>>> +
>>>>> +     ret = lenovo_wmi_gz_platform_profile_supported(priv,
>>>>> +                                                    &profile_support_ver);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     if (profile_support_ver < 1)
>>>>> +             return -ENODEV;
>>>>> +
>>>>> +     priv->extreme_supported = extreme_supported(profile_support_ver);
>>>>> +
>>>>> +     set_bit(PLATFORM_PROFILE_LOW_POWER, choices);
>>>>> +     set_bit(PLATFORM_PROFILE_BALANCED, choices);
>>>>> +     set_bit(PLATFORM_PROFILE_PERFORMANCE, choices);
>>>>> +     set_bit(PLATFORM_PROFILE_CUSTOM, choices);
>>>>> +
>>>>> +     if (priv->extreme_supported)
>>>>> +             set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, choices);
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct platform_profile_ops lenovo_wmi_gz_platform_profile_ops = {
>>>>> +     .probe = lenovo_wmi_platform_profile_probe,
>>>>> +     .profile_get = lenovo_wmi_gz_profile_get,
>>>>> +     .profile_set = lenovo_wmi_gz_profile_set,
>>>>> +};
>>>>> +
>>>>> +/* Notifier Methods */
>>>>> +int lenovo_wmi_gz_register_notifier(struct notifier_block *nb)
>>>>> +{
>>>>> +     guard(mutex)(&gz_chain_mutex);
>>>> The blocking notifier already does the locking itself. Please remove this mutex.
>>>>
>>> Good to know, will fix, ty.
>>>
>>>>> +     return blocking_notifier_chain_register(&gz_chain_head, nb);
>>>>> +}
>>>>> +EXPORT_SYMBOL_NS_GPL(lenovo_wmi_gz_register_notifier, "GZ_WMI");
>>>> Can you name the namespace similar to "LENOVO_GAMEZONE_WMI" please?
>>>>
>>> Acked
>>>
>>>>> +
>>>>> +int lenovo_wmi_gz_unregister_notifier(struct notifier_block *nb)
>>>>> +{
>>>>> +     guard(mutex)(&gz_chain_mutex);
>>>>> +     return blocking_notifier_chain_unregister(&gz_chain_head, nb);
>>>>> +}
>>>>> +EXPORT_SYMBOL_NS_GPL(lenovo_wmi_gz_unregister_notifier, "GZ_WMI");
>>>>> +
>>>>> +static void devm_lenovo_wmi_gz_unregister_notifier(void *data)
>>>>> +{
>>>>> +     struct notifier_block *nb = data;
>>>>> +
>>>>> +     lenovo_wmi_gz_unregister_notifier(nb);
>>>>> +}
>>>>> +
>>>>> +int devm_lenovo_wmi_gz_register_notifier(struct device *dev,
>>>>> +                                      struct notifier_block *nb)
>>>>> +{
>>>>> +     int ret;
>>>>> +
>>>>> +     ret = lenovo_wmi_gz_register_notifier(nb);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     return devm_add_action_or_reset(dev, devm_lenovo_wmi_gz_unregister_notifier, nb);
>>>>> +}
>>>>> +EXPORT_SYMBOL_NS_GPL(devm_lenovo_wmi_gz_register_notifier, "GZ_WMI");
>>>>> +
>>>>> +/* Driver Methods */
>>>>> +static void lenovo_wmi_gz_notify(struct wmi_device *wdev,
>>>>> +                              union acpi_object *obj)
>>>>> +{
>>>>> +     struct lenovo_wmi_gz_priv *tm_priv = dev_get_drvdata(&wdev->dev);
>>>>> +     struct lenovo_wmi_gz_priv *gz_priv =
>>>>> +             container_of(&gz_chain_head, struct lenovo_wmi_gz_priv, nhead);
>>>> I fear that this will not work because gz_chain_head is a global variable, not a field inside
>>>> struct lenovo_wmi_gz_priv. Also this would crash the kernel should the main gamezone driver be
>>>> unbound from its WMI device.
>>>>
>>>> I suggest you move the WMI driver for the WMI event into a separate module. Then you use another notifier
>>>> inside the new module to allow the gamezone driver to listen for events. For example this separate WMI event driver
>>>> could use the "val" argument inside blocking_notifier_call_chain() to specify the type of event (like THERMAL_MODE_CHANGED)
>>>> and the "v" argument to pass a pointer to a u32 variable containing the new thermal mode.
>>> I can do this, but we explicitly discussed doing it in one driver for
>>> all gamezone GUIDs. If I do this I'd like to confirm I won't need to
>>> revert on this later.
>>> As for naming, what would you suggest? Depending on scope it would
>>> either cover all lenovo_wmi-* events, or just the gamezone events.
>>>
>>> Kconfig: LENOVO_WMI_EVENTS | LENOVO_WMI_GAMEZONE_EVENTS
>>> .c: lenovo-wmi-events.c | lenovo-wmi-gamezone-events.c
>> I think there was a misunderstand here, with "one driver for all gamezone GUIDs" i meant
>> "one driver for all gamezone _event_ GUIDs". Sorry for that.
>>
>> Personally i would favor the lenovo-wmi-events name. With this we can add support for additional
>> WMI events later.
>>
> Okay, that sounds good to me.
> There is one thing I want to clarify for the gamezone driver. It will
> need to register two notifier blocks, correct? One for the "push"
> notification from events, and one for the "pull" notification call
> from other mode. My assumption is that we shouldn't use the same
> notifier_block on two chain heads, but I want to be sure before
> submitting.
>
> - Derek

Yes, using separate notification chains for WMI events and communication with
lenovo-wmi-other would make sense. This also means that you need to use separate
notifier blocks for each chain.

Thanks,
Armin Wolf

>>>> This also allows you to extend the separate WMI driver later to support more WMI event GUIDs.
>>> There are 4 more gamezone event GUIDs that the Legion Go doesn't
>>> implement, so that would be a good place for them. I haven't added
>>> them as I cannot test them with my hardware.
>> This is fine, getting only the platform profile interface to work is a good start.
>>
>>>>> +     int sel_prof;
>>>>> +     int ret;
>>>>> +
>>>>> +     if (obj->type != ACPI_TYPE_INTEGER)
>>>>> +             return;
>>>>> +
>>>>> +     switch (tm_priv->type) {
>>>>> +     case THERMAL_MODE:
>>>>> +             sel_prof = obj->integer.value;
>>>>> +             break;
>>>>> +     default:
>>>>> +             return;
>>>>> +     }
>>>>> +
>>>>> +     /* Update primary Gamezone instance */
>>>>> +     switch (sel_prof) {
>>>>> +     case SMARTFAN_MODE_QUIET:
>>>>> +             gz_priv->current_profile = PLATFORM_PROFILE_LOW_POWER;
>>>>> +             break;
>>>>> +     case SMARTFAN_MODE_BALANCED:
>>>>> +             gz_priv->current_profile = PLATFORM_PROFILE_BALANCED;
>>>>> +             break;
>>>>> +     case SMARTFAN_MODE_PERFORMANCE:
>>>>> +             if (gz_priv->extreme_supported) {
>>>>> +                     gz_priv->current_profile =
>>>>> +                             PLATFORM_PROFILE_BALANCED_PERFORMANCE;
>>>>> +                     break;
>>>>> +             }
>>>>> +             gz_priv->current_profile = PLATFORM_PROFILE_PERFORMANCE;
>>>>> +             break;
>>>>> +     case SMARTFAN_MODE_EXTREME:
>>>>> +             gz_priv->current_profile = PLATFORM_PROFILE_PERFORMANCE;
>>>>> +             break;
>>>>> +     case SMARTFAN_MODE_CUSTOM:
>>>>> +             gz_priv->current_profile = PLATFORM_PROFILE_CUSTOM;
>>>>> +             break;
>>>>> +     default:
>>>>> +             break;
>>>>> +     }
>>>> Please use platform_profile_notify() to notify userspace of the new platform profile settings.
>>> That makes sense.
>>>
>>>>> +
>>>>> +     ret = blocking_notifier_call_chain(&gz_chain_head, THERMAL_MODE_EVENT,
>>>>> +                                        &sel_prof);
>>>>> +     if (ret == NOTIFY_BAD)
>>>>> +             pr_err("Failed to send notification to call chain for WMI event %u\n",
>>>>> +                    tm_priv->type);
>>>>> +}
>>>>> +
>>>>> +static int lenovo_wmi_gz_probe(struct wmi_device *wdev, const void *context)
>>>>> +{
>>>>> +     struct lenovo_wmi_gz_priv *priv =
>>>>> +             devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>>>> Please do the call to devm_kzalloc() on a separate line:
>>>>
>>>>           struct lenovo_wmi_gz_priv *priv;
>>>>
>>>>           priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>>>>
>>> Understood
>>>
>>>>> +
>>>>> +     if (!priv)
>>>>> +             return -ENOMEM;
>>>>> +
>>>>> +     if (!context)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     priv->wdev = wdev;
>>>>> +     priv->type = *((enum lenovo_wmi_gz_type *)context);
>>>>> +
>>>>> +     dev_set_drvdata(&wdev->dev, priv);
>>>>> +
>>>>> +     if (priv->type != GAMEZONE_FULL)
>>>>> +             return 0;
>>>>> +
>>>>> +     priv->nhead = gz_chain_head;
>>>>> +     priv->ppdev = platform_profile_register(&wdev->dev, "lenovo-wmi-gamezone",
>>>>> +                                             priv, &lenovo_wmi_gz_platform_profile_ops);
>>>> Please check if platform_profile_register() was successful and return an error if not.
>>>>
>>> Will do, ty.
>>>
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct wmi_device_id lenovo_wmi_gz_id_table[] = {
>>>>> +     { GAMEZONE_WMI_DEVICE(LENOVO_GAMEZONE_GUID, GAMEZONE_FULL) },
>>>>> +     { GAMEZONE_WMI_DEVICE(THERMAL_MODE_EVENT_GUID, THERMAL_MODE) },
>>>>> +     {}
>>>>> +};
>>>>> +
>>>>> +static struct wmi_driver lenovo_wmi_gz_driver = {
>>>>> +     .driver = {
>>>>> +             .name = "lenovo_wmi_gamezone",
>>>>> +             .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>>>>> +     },
>>>>> +     .id_table = lenovo_wmi_gz_id_table,
>>>>> +     .probe = lenovo_wmi_gz_probe,
>>>>> +     .notify = lenovo_wmi_gz_notify,
>>>>> +     .no_singleton = true,
>>>>> +};
>>>>> +
>>>>> +module_wmi_driver(lenovo_wmi_gz_driver);
>>>>> +
>>>>> +MODULE_IMPORT_NS("LENOVO_WMI");
>>>>> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_gz_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.c b/drivers/platform/x86/lenovo-wmi.c
>>>>> new file mode 100644
>>>>> index 000000000000..0de2c37e69bd
>>>>> --- /dev/null
>>>>> +++ b/drivers/platform/x86/lenovo-wmi.c
>>>>> @@ -0,0 +1,77 @@
>>>>> +// 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 Method" 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 Method" 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 Mode" 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>
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include <linux/wmi.h>
>>>>> +#include "lenovo-wmi.h"
>>>>> +
>>>>> +/* wmidev_evaluate_method helper 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)
>>>>> +{
>>>> Please give this method a more descriptive name.
>>>>
>>> Okay. I followed the convention in asus_wmi here as it takes 2 args
>>> and _1 takes one arg. When I add fan profiles later I'll need one that
>>> takes one u64 arg as well, and I think some other GUID's I don't yet
>>> have implemented have 3 or 4 u16 args. Do you have a suggestion on how
>>> you'd prefer them named? My instinct would be to simplify to three,
>>> _u16, _u32, _u64, and pass 0 to unused args instead of having a second
>>> _1 helper.
>> Good question, would it make sense to just pass the arguments as a single byte buffer?
>>
>>>>> +     struct wmi_method_args args = { arg0, arg1 };
>>>>> +     struct acpi_buffer input = { (acpi_size)sizeof(args), &args };
>>>> Cast to acpi_size is unnecessary here.
>>> Acked
>>>
>>>>> +     struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>>>>> +     union acpi_object *ret_obj __free(kfree) = NULL;
>>>>> +     int err;
>>>>> +
>>>>> +     err = lenovo_wmidev_evaluate_method(wdev, instance, method_id, &input,
>>>>> +                                         &output);
>>>>> +
>>>>> +     if (err)
>>>>> +             return err;
>>>>> +
>>>>> +     if (retval) {
>>>>> +             ret_obj = (union acpi_object *)output.pointer;
>>>>> +             if (!ret_obj)
>>>>> +                     return -ENODATA;
>>>>> +
>>>>> +             if (ret_obj->type != ACPI_TYPE_INTEGER)
>>>>> +                     return -ENXIO;
>>>>> +
>>>>> +             *retval = (u32)ret_obj->integer.value;
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_NS_GPL(lenovo_wmidev_evaluate_method_2, "LENOVO_WMI");
>>>> Can you please rename the namespace to "LENOVO_WMI_HELPERS"?
>>> Yes
>>>
>>>>> +
>>>>> +int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance,
>>>>> +                                 u32 method_id, u32 arg0, u32 *retval)
>>>>> +{
>>>> Same as above.
>>>>
>>>>> +     return lenovo_wmidev_evaluate_method_2(wdev, instance, method_id, arg0,
>>>>> +                                            0, retval);
>>>>> +}
>>>>> +EXPORT_SYMBOL_NS_GPL(lenovo_wmidev_evaluate_method_1, "LENOVO_WMI");
>>>>> +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@...il.com>");
>>>>> +MODULE_DESCRIPTION("Lenovo WMI Common 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..113928b4fc0f
>>>>> --- /dev/null
>>>>> +++ b/drivers/platform/x86/lenovo-wmi.h
>>>>> @@ -0,0 +1,62 @@
>>>>> +/* 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 Method" 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 Method" 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 Mode" 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>
>>>>> + *
>>>>> + */
>>>>> +#include <linux/notifier.h>
>>>>> +#include <linux/platform_profile.h>
>>>>> +
>>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> This causes a build error on my machine. Please only use this macro inside source files
>>>> for modules and not inside header files.
>>> This also causes a build error on my machine inside the .c sources.
>>> I'm not sure why.
>> The reason for this is that some drivers including this header file also define this macro.
>> In general this macro should only be used inside source files for modules.
>>
>> Thanks,
>> Armin Wolf
>>
>>>>> +
>>>>> +#ifndef _LENOVO_WMI_H_
>>>>> +#define _LENOVO_WMI_H_
>>>>> +
>>>>> +#include <linux/bits.h>
>>>>> +#include <linux/types.h>
>>>>> +#include <linux/wmi.h>
>>>>> +
>>>>> +struct wmi_method_args {
>>>>> +     u32 arg0;
>>>>> +     u32 arg1;
>>>>> +};
>>>>> +
>>>>> +/* gamezone structs */
>>>>> +enum thermal_mode {
>>>>> +     SMARTFAN_MODE_QUIET = 0x01,
>>>>> +     SMARTFAN_MODE_BALANCED = 0x02,
>>>>> +     SMARTFAN_MODE_PERFORMANCE = 0x03,
>>>>> +     SMARTFAN_MODE_EXTREME = 0xE0, /* Ver 6+ */
>>>>> +     SMARTFAN_MODE_CUSTOM = 0xFF,
>>>>> +};
>>>>> +
>>>>> +enum lenovo_wmi_action {
>>>>> +     THERMAL_MODE_EVENT = 1,
>>>>> +};
>>>>> +
>>>>> +/* wmidev_evaluate_method helper functions */
>>>>> +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_1(struct wmi_device *wdev, u8 instance,
>>>>> +                                 u32 method_id, u32 arg0, u32 *retval);
>>>>> +
>>>>> +/* lenovo_wmi_gz_driver notifier functions */
>>>>> +int lenovo_wmi_gz_notifier_call(struct notifier_block *nb, unsigned long action,
>>>>> +                             enum platform_profile_option *profile);
>>>>> +int lenovo_wmi_gz_register_notifier(struct notifier_block *nb);
>>>>> +int lenovo_wmi_gz_unregister_notifier(struct notifier_block *nb);
>>>>> +int devm_lenovo_wmi_gz_register_notifier(struct device *dev,
>>>>> +                                      struct notifier_block *nb);
>>>> Can you please create a separate header file for each driver? Otherwise this header file
>>>> will contain many different things from different drivers, which will maybe not even be
>>>> available depending on the Kconfig settings.
>>> Can do.
>>>
>>> Cheers,
>>> - Derek
>>>
>>>> Thanks,
>>>> Armin Wolf
>>>>
>>>>> +#endif /* !_LENOVO_WMI_H_ */

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ