| 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
| ||
|
Message-ID: <229b2b2e-8404-4589-b364-af4a4a4c9461@gmx.de>
Date: Tue, 11 Mar 2025 21:30:15 +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 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?
>>
>>> +
>>> +/* 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.
>> 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