Message-ID: <7ebb09b7-337d-4a9c-ba5c-bed874209fd4@gmx.de>
Date: Thu, 9 Jan 2025 23:34:34 +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 3/4] platform/x86: Add Lenovo Capability Data 01 WMI
 Driver

Am 02.01.25 um 01:47 schrieb Derek J. Clark:

> Adds lenovo-wmi-capdata01.c which provides a driver for the
> LENOVO_CAPABILITY_DATA_01 WMI data block that comes on "Other Method"
> enabled hardware. Provides an interface for querying if a given
> attribute is supported by the hardware, as well as its default_value,
> max_value, min_value, and step increment.
>
> v2:
> - Use devm_kzalloc to ensure driver can be instanced, remove global
>    reference.
> - Ensure reverse Christmas tree for all variable declarations.
> - Remove extra whitespace.
> - Use guard(mutex) in all mutex instances, global mutex.
> - Use pr_fmt instead of adding the driver name to each pr_err.
> - Remove noisy pr_info usage.
> - Rename capdata_wmi to lenovo_wmi_cd01_priv and cd01_wmi to priv.
> - Use list to get the lenovo_wmi_cd01_priv instance in
>    lenovo_wmi_capdata01_get as none of the data provided by the macros
>    that will use it can pass a member of the struct for use in
>    container_of.
>
> Signed-off-by: Derek J. Clark <derekjohn.clark@...il.com>
> ---
>   MAINTAINERS                                 |   1 +
>   drivers/platform/x86/Kconfig                |  11 ++
>   drivers/platform/x86/Makefile               |   1 +
>   drivers/platform/x86/lenovo-wmi-capdata01.c | 131 ++++++++++++++++++++
>   drivers/platform/x86/lenovo-wmi.h           |  20 +++
>   5 files changed, 164 insertions(+)
>   create mode 100644 drivers/platform/x86/lenovo-wmi-capdata01.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8f8a6aec6b92..c9374c395905 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13038,6 +13038,7 @@ LENOVO WMI drivers
>   M:	Derek J. Clark <derekjohn.clark@...il.com>
>   L:	platform-driver-x86@...r.kernel.org
>   S:	Maintained
> +F:	drivers/platform/x86/lenovo-wmi-capdata01.c
>   F:	drivers/platform/x86/lenovo-wmi-gamezone.c
>   F:	drivers/platform/x86/lenovo-wmi.h
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 9a6ac7fdec9f..a2c1ab47ad9e 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -470,6 +470,17 @@ config LENOVO_WMI_GAMEZONE
>   	  To compile this driver as a module, choose M here: the module will
>   	  be called lenovo_wmi_gamezone.
>
> +config LENOVO_WMI_DATA01
> +	tristate "Lenovo Legion WMI capability Data 01 Driver"
> +	depends on ACPI_WMI
> +	help
> +	  Say Y here if you have a WMI aware Lenovo Legion device in the "Gaming Series"
> +	  line of hardware. This interface is a dependency for exposing tunable power
> +	  settings.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called lenovo_wmi_capdata01.

Could it be that the resulting module will be called lenovo-wmi-capdata01? Also if its a mere
dependency without any value when being used alone then it would make sense to hide it from
users by removing the help texts:

	config LENOVO_WMI_DATA01
		tristate
		depends on ACPI_WMI

> +
>   config IDEAPAD_LAPTOP
>   	tristate "Lenovo IdeaPad Laptop Extras"
>   	depends on ACPI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 7cb29a480ed2..6c96cc3f3855 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_YOGABOOK)		+= lenovo-yogabook.o
>   obj-$(CONFIG_YT2_1380)		+= lenovo-yoga-tab2-pro-1380-fastcharger.o
>   obj-$(CONFIG_LENOVO_WMI_CAMERA)	+= lenovo-wmi-camera.o
>   obj-$(CONFIG_LENOVO_WMI_GAMEZONE)	+= lenovo-wmi-gamezone.o
> +obj-$(CONFIG_LENOVO_WMI_DATA01)	+= lenovo-wmi-capdata01.o
>
>   # Intel
>   obj-y				+= intel/
> diff --git a/drivers/platform/x86/lenovo-wmi-capdata01.c b/drivers/platform/x86/lenovo-wmi-capdata01.c
> new file mode 100644
> index 000000000000..b10a6e4b320f
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-wmi-capdata01.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * LENOVO_CAPABILITY_DATA_01 WMI data block driver. This interface provides
> + * information on tunable attributes used by the "Other Method" WMI interface,
> + * including if it is supported by the hardware, the default_value, max_value,
> + * min_value, and step increment.
> + *
> + * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@...il.com>
> + */
> +
> +#include <linux/list.h>
> +#include "lenovo-wmi.h"
> +
> +#define LENOVO_CAPABILITY_DATA_01_GUID "7A8F5407-CB67-4D6E-B547-39B3BE018154"
> +
> +static DEFINE_MUTEX(cd01_call_mutex);

Does the WMI interface really rely on such mutual exclusion of callers? If no then
please remove this mutex.

> +static DEFINE_MUTEX(cd01_list_mutex);
> +static LIST_HEAD(cd01_wmi_list);
> +
> +static const struct wmi_device_id lenovo_wmi_capdata01_id_table[] = {
> +	{ LENOVO_CAPABILITY_DATA_01_GUID, NULL },
> +	{}
> +};
> +
> +struct lenovo_wmi_cd01_priv {
> +	struct wmi_device *wdev;
> +	struct list_head list;
> +};
> +
> +static inline struct lenovo_wmi_cd01_priv *get_first_wmi_priv(void)
> +{
> +	guard(mutex)(&cd01_list_mutex);
> +	return list_first_entry_or_null(&cd01_wmi_list,
> +					struct lenovo_wmi_cd01_priv, list);

Two things:

1. This will cause issues should a WMI device in this list be removed while a
consumer is using it. In this case you will need extend the scope of the list mutex.

2. Do multiple capdata01 WMI devices exist in any systems? If no then please consider
using the component framework (https://docs.kernel.org/driver-api/component.html) which
will simplify the interop between the consumer driver of capdata01 and this driver.

> +}
> +
> +int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id,
> +			     struct capability_data_01 *cap_data)
> +{
> +	u32 attribute_id = *(int *)&attr_id;

This will cause alignment issues, please use FIELD_GET()/FIELD_PREP() to construct a u32 to
pass to this function.

> +	struct lenovo_wmi_cd01_priv *priv;
> +	union acpi_object *ret_obj;
> +	int instance_idx;
> +	int count;
> +
> +	priv = get_first_wmi_priv();
> +	if (!priv)
> +		return -ENODEV;
> +
> +	guard(mutex)(&cd01_call_mutex);
> +	count = wmidev_instance_count(priv->wdev);
> +	pr_info("Got instance count: %u\n", count);
> +
> +	for (instance_idx = 0; instance_idx < count; instance_idx++) {
> +		ret_obj = wmidev_block_query(priv->wdev, instance_idx);
> +		if (!ret_obj) {
> +			pr_err("WMI Data block query failed.\n");
> +			continue;
> +		}
> +
> +		if (ret_obj->type != ACPI_TYPE_BUFFER) {
> +			pr_err("WMI Data block query returned wrong type.\n");
> +			kfree(ret_obj);
> +			continue;
> +		}
> +
> +		if (ret_obj->buffer.length != sizeof(*cap_data)) {
> +			pr_err("WMI Data block query returned wrong buffer length: %u vice expected %lu.\n",
> +			       ret_obj->buffer.length, sizeof(*cap_data));
> +			kfree(ret_obj);
> +			continue;
> +		}
> +
> +		memcpy(cap_data, ret_obj->buffer.pointer,
> +		       ret_obj->buffer.length);
> +		kfree(ret_obj);
> +
> +		if (cap_data->id != attribute_id)
> +			continue;
> +		break;
> +	}

Maybe it would make sense to read this data during device initialization and store it
inside an array? This way looking up capability data would be _much_ faster especially
since WMI calls are usually quite slow.

Also this function is way to noisy when it comes to error messages. Please leave this
to the caller of this function.

> +
> +	if (cap_data->id != attribute_id) {
> +		pr_err("Unable to find capability data for attribute_id %x\n",
> +		       attribute_id);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(lenovo_wmi_capdata01_get, "CAPDATA_WMI");
> +
> +static int lenovo_wmi_capdata01_probe(struct wmi_device *wdev,
> +				      const void *context)
> +
> +{
> +	struct lenovo_wmi_cd01_priv *priv;
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->wdev = wdev;
> +
> +	guard(mutex)(&cd01_list_mutex);
> +	list_add_tail(&priv->list, &cd01_wmi_list);
> +
> +	return 0;
> +}
> +
> +static void lenovo_wmi_capdata01_remove(struct wmi_device *wdev)
> +{
> +	struct lenovo_wmi_cd01_priv *priv = dev_get_drvdata(&wdev->dev);
> +
> +	guard(mutex)(&cd01_list_mutex);
> +	list_del(&priv->list);
> +}
> +
> +static struct wmi_driver lenovo_wmi_capdata01_driver = {
> +	.driver = { .name = "lenovo_wmi_capdata01" },

Please set ".probe_type = PROBE_PREFER_ASYNCHRONOUS".

> +	.id_table = lenovo_wmi_capdata01_id_table,
> +	.probe = lenovo_wmi_capdata01_probe,
> +	.remove = lenovo_wmi_capdata01_remove,

Please set ".no_singleton = true".

Thanks,
Armin Wolf

> +};
> +
> +module_wmi_driver(lenovo_wmi_capdata01_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_capdata01_id_table);
> +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@...il.com>");
> +MODULE_DESCRIPTION("Lenovo Capability Data 01 WMI Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h
> index 8a302c6c47cb..53cea84a956b 100644
> --- a/drivers/platform/x86/lenovo-wmi.h
> +++ b/drivers/platform/x86/lenovo-wmi.h
> @@ -36,6 +36,22 @@ struct wmi_method_args {
>   	u32 arg1;
>   };
>
> +struct lenovo_wmi_attr_id {
> +	u32 mode_id : 16; /* Fan profile */
> +	u32 feature_id : 8; /* Attribute (SPL/SPPT/...) */
> +	u32 device_id : 8; /* CPU/GPU/... */
> +} __packed;
> +
> +/* Data struct for LENOVO_CAPABILITY_DATA_01 */
> +struct capability_data_01 {
> +	u32 id;
> +	u32 supported;
> +	u32 default_value;
> +	u32 step;
> +	u32 min_value;
> +	u32 max_value;
> +};
> +
>   /* General Use functions */
>   static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance,
>   					 u32 method_id, struct acpi_buffer *in,
> @@ -102,4 +118,8 @@ int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance,
>   					       0, retval);
>   }
>
> +/* LENOVO_CAPABILITY_DATA_01 exported functions */
> +int lenovo_wmi_capdata01_get(struct lenovo_wmi_attr_id attr_id,
> +			     struct capability_data_01 *cap_data);
> +
>   #endif /* !_LENOVO_WMI_H_ */

Powered by blists - more mailing lists