Message-ID: <32a788aa-ddf0-4ae1-a51f-0c15de2056c9@gmx.de>
Date: Thu, 9 Jan 2025 23:11:49 +0100
From: Armin Wolf <W_Armin@....de>
To: "Derek J. Clark" <derekjohn.clark@...il.com>,
 Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: 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 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.

> +
>   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.

> +
> +#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.

> +
> +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.

> +{
> +	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.

> +	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.

> +		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.

> +
> +	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.

> +
> +	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.

> +	}
> +
> +	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.

> +
> +	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.

> +
> +	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.

> +	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.

> +	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.

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.

> +	.id_table = lenovo_wmi_gamezone_id_table,
> +	.probe = lenovo_wmi_gamezone_probe,
> +	.remove = lenovo_wmi_gamezone_remove,

Please set ".no_singleton = true" here.

> +};
> +
> +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.

> +
> +	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.

> +		}
> +		if (ret_obj->type != ACPI_TYPE_INTEGER) {
> +			pr_err("WMI query returnd ACPI object with wrong type.\n");
> +			kfree(ret_obj);
> +			return -EIO;

-ENXIO.

> +		}
> +		*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_ */

Powered by blists - more mailing lists