Message-ID: <a339b4b5-c7ca-4875-adb8-183539be563f@gmx.de>
Date: Fri, 2 May 2025 04:18:40 +0200
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 v6 4/6] platform/x86: Add Lenovo WMI Capability Data 01
 Driver

Am 28.04.25 um 03:18 schrieb Derek J. Clark:

> Adds lenovo-wmi-capdata01 driver which provides the
> LENOVO_CAPABILITY_DATA_01 WMI data block that comes on "Other Mode"
> 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.
>
> Reviewed-by: Armin Wolf <W_Armin@....de>
> Signed-off-by: Derek J. Clark <derekjohn.clark@...il.com>
> ---
> v6:
>   - Recache capabiltiy data on ACPI AC events to ensure accutare
>     max_value.
>   - Fix typos and rewordings from v5 review.
> v5:
>   - Return to cache at device initialization. On component bind, pass a
>     pointer to lenovo-wmi-other.
>   - Fixes from v4 review.
> v4:
>   - Make driver data a private struct, remove references from Other Mode
>     driver.
>   - Don't cache data at device initialization. Instead, on component bind,
>     cache the data on a member variable of the Other Mode driver data
>     passed as a void pointer.
>   - Add header file for capdata01 structs.
>   - Add new struct to pass capdata01 array data and array length to Other
>     Mode.
> v3:
> - Add as component to lenovo-wmi-other driver.
> 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 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.
>   MAINTAINERS                                 |   1 +
>   drivers/platform/x86/Kconfig                |   4 +
>   drivers/platform/x86/Makefile               |   1 +
>   drivers/platform/x86/lenovo-wmi-capdata01.c | 272 ++++++++++++++++++++
>   drivers/platform/x86/lenovo-wmi-capdata01.h |  29 +++
>   5 files changed, 307 insertions(+)
>   create mode 100644 drivers/platform/x86/lenovo-wmi-capdata01.c
>   create mode 100644 drivers/platform/x86/lenovo-wmi-capdata01.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2b4b06e81192..1b22e41cc730 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13164,6 +13164,7 @@ 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-capdata01.*
>   F:	drivers/platform/x86/lenovo-wmi-events.*
>   F:	drivers/platform/x86/lenovo-wmi-helpers.*
>   
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 13b8f4ac5dc5..64663667f0cb 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -467,6 +467,10 @@ config LENOVO_WMI_HELPERS
>   	tristate
>   	depends on ACPI_WMI
>   
> +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 fc039839286a..7a35c77221b7 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -69,6 +69,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_DATA01)	+= lenovo-wmi-capdata01.o
>   obj-$(CONFIG_LENOVO_WMI_EVENTS)	+= lenovo-wmi-events.o
>   obj-$(CONFIG_LENOVO_WMI_HELPERS)	+= lenovo-wmi-helpers.o
>   
> diff --git a/drivers/platform/x86/lenovo-wmi-capdata01.c b/drivers/platform/x86/lenovo-wmi-capdata01.c
> new file mode 100644
> index 000000000000..841d4a37249b
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-wmi-capdata01.c
> @@ -0,0 +1,272 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Lenovo Capability Data 01 WMI Data Block driver.
> + *
> + * Lenovo Capability Data 01 provides information on tunable attributes used by
> + * the "Other Mode" WMI interface. The data includes if the attribute is
> + * supported by the hardware, the default_value, max_value, min_value, and step
> + * increment. Each attibute has multiple pages, one for each of the thermal
> + * modes managed by the Gamezone interface.
> + *
> + * Copyright(C) 2025 Derek J. Clark <derekjohn.clark@...il.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/cleanup.h>
> +#include <linux/component.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/export.h>
> +#include <linux/gfp_types.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/overflow.h>
> +#include <linux/types.h>
> +#include <linux/wmi.h>
> +
> +#include "lenovo-wmi-capdata01.h"
> +
> +#define LENOVO_CAPABILITY_DATA_01_GUID "7A8F5407-CB67-4D6E-B547-39B3BE018154"
> +
> +#define ACPI_AC_NOTIFY_STATUS 0x80
> +
> +static DEFINE_MUTEX(list_mutex);

Hi,

why not moving this mutex inside struct cd01_list?

> +
> +struct lwmi_cd01_priv {
> +	struct notifier_block acpi_nb; /* ACPI events */
> +	struct wmi_device *wdev;
> +	struct cd01_list *list;
> +};
> +
> +/**
> + * lwmi_cd01_component_bind() - Bind component to master device.
> + * @cd01_dev: Pointer to the lenovo-wmi-capdata01 driver parent device.
> + * @om_dev: Pointer to the lenovo-wmi-other driver parent device.
> + * @data: capdata01_list object pointer used to return the capability data.
> + *
> + * On lenovo-wmi-other's master bind, provide a pointer to the local capdata01
> + * list. This is used to call lwmi_cd01_get_data to look up attribute data
> + * from the lenovo-wmi-other driver.
> + *
> +:* Return: 0 on success, or an error code.
> + */
> +static int lwmi_cd01_component_bind(struct device *cd01_dev,
> +				    struct device *om_dev, void *data)
> +{
> +	struct lwmi_cd01_priv *priv = dev_get_drvdata(cd01_dev);
> +	struct cd01_list **cd01_list = data;
> +
> +	if (!priv->list)
> +		return -ENODEV;
> +
> +	*cd01_list = priv->list;
> +
> +	return 0;
> +}
> +
> +static const struct component_ops lwmi_cd01_component_ops = {
> +	.bind = lwmi_cd01_component_bind,
> +};
> +
> +/**
> + * lwmi_cd01_get_data - Get the data of the specified attribute
> + * @dev: The lenovo-wmi-capdata01 parent device.
> + * @tunable_attr: The attribute to be populated.
> + *
> + * Retrieves the capability data 01 struct pointer for the given
> + * attribute for its specified thermal mode.
> + *
> + * Return: Either a pointer to capability data, or NULL.
> + */
> +struct capdata01 *lwmi_cd01_get_data(struct cd01_list *list, u32 attribute_id)
> +{
> +	u8 idx;
> +
> +	guard(mutex)(&list_mutex);
> +	for (idx = 0; idx < list->count; idx++) {
> +		if (list->data[idx].id != attribute_id)
> +			continue;
> +		return &list->data[idx];

This might cause issues should lwmi_cd01_cache() be called when the caller of this function
currently accesses the returned data.

Maybe it would make sense to simply copy the struct capdata01? It is rather small, so the overhead
would be negligible.

I envision something like this:

int lwmi_cd01_get_data(struct cd01_list *list, u32 attribute_id, struct capdata01 *output);

In this case the resulting capdata is copied into "output".

> +	}
> +	return NULL;
> +}
> +EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD01");
> +
> +/**
> + * lwmi_cd01_setup() - Cache all WMI data block information
> + * @priv: lenovo-wmi-capdata01 driver data.
> + *
> + * Loop through each WMI data block and cache the data.
> + *
> + * Return: 0 on success, or an error.
> + */
> +static int lwmi_cd01_cache(struct lwmi_cd01_priv *priv)
> +{
> +	int idx;
> +
> +	guard(mutex)(&list_mutex);
> +	for (idx = 0; idx < priv->list->count; idx++) {
> +		union acpi_object *ret_obj __free(kfree) = NULL;
> +
> +		ret_obj = wmidev_block_query(priv->wdev, idx);
> +		if (!ret_obj)
> +			return -ENODEV;
> +
> +		if (ret_obj->type != ACPI_TYPE_BUFFER ||
> +		    ret_obj->buffer.length < sizeof(priv->list->data[idx]))
> +			continue;
> +
> +		memcpy(&priv->list->data[idx], ret_obj->buffer.pointer,
> +		       ret_obj->buffer.length);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * lwmi_cd01_alloc() - Allocate a cd01_list struct in drvdata
> + * @priv: lenovo-wmi-capdata01 driver data.
> + *
> + * Allocate a cd01_list struct large enough to contain data from all WMI data
> + * blocks provided by the interface.
> + *
> + * Return: 0 on success, or an error.
> + */
> +static int lwmi_cd01_alloc(struct lwmi_cd01_priv *priv)
> +{
> +	struct cd01_list *list;
> +	size_t list_size;
> +	int count;
> +
> +	count = wmidev_instance_count(priv->wdev);
> +	list_size = struct_size(list, data, count);
> +
> +	guard(mutex)(&list_mutex);
> +	list = devm_kzalloc(&priv->wdev->dev, list_size, GFP_KERNEL);
> +	if (!list)
> +		return -ENOMEM;
> +
> +	list->count = count;
> +	priv->list = list;
> +
> +	return 0;
> +}
> +
> +/**
> + * lwmi_cd01_setup() - Cache all WMI data block information
> + * @priv: lenovo-wmi-capdata01 driver data.
> + *
> + * Allocate a cd01_list struct large enough to contain data from all WMI data
> + * blocks provided by the interface. Then loop through each data block and
> + * cache the data.
> + *
> + * Return: 0 on success, or an error code.
> + */
> +static int lwmi_cd01_setup(struct lwmi_cd01_priv *priv)
> +{
> +	int ret;
> +
> +	ret = lwmi_cd01_alloc(priv);
> +	if (ret)
> +		return ret;
> +
> +	return lwmi_cd01_cache(priv);
> +}
> +
> +/**
> + * lwmi_cd01_notifier_call() - Call method for lenovo-wmi-capdata01 driver notifier.
> + * block call chain.
> + * @nb: The notifier_block registered to lenovo-wmi-events driver.
> + * @action: Unused.
> + * @data: The ACPI event.
> + *
> + * For LWMI_EVENT_THERMAL_MODE, set current_mode and notify platform_profile
> + * of a change.
> + *
> + * Return: notifier_block status.
> + */
> +static int lwmi_cd01_notifier_call(struct notifier_block *nb, unsigned long action,
> +				   void *data)
> +{
> +	struct acpi_bus_event *event = (struct acpi_bus_event *)data;

Unnecessary cast, please drop.

> +	struct lwmi_cd01_priv *priv;
> +	int ret;
> +
> +
> +	priv = container_of(nb, struct lwmi_cd01_priv, acpi_nb);
> +
> +	switch (event->type) {
> +	case ACPI_AC_NOTIFY_STATUS:

You should also check the the "device_class" of the ACPI event (search for ACPI_AC_CLASS)
because "type" is not guaranteed to be unique between different events.

> +		ret = lwmi_cd01_cache(priv);
> +		if (ret)
> +			return NOTIFY_BAD;
> +
> +		return NOTIFY_OK;
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +}
> +
> +static int lwmi_cd01_probe(struct wmi_device *wdev, const void *context)
> +
> +{
> +	struct lwmi_cd01_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->wdev = wdev;
> +	dev_set_drvdata(&wdev->dev, priv);
> +
> +	ret = lwmi_cd01_setup(priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
> +	register_acpi_notifier(&priv->acpi_nb);
> +
> +	return component_add(&wdev->dev, &lwmi_cd01_component_ops);

You need to unregister the ACPI notifier when component_add() fails.

> +}
> +
> +static void lwmi_cd01_remove(struct wmi_device *wdev)
> +{
> +	component_del(&wdev->dev, &lwmi_cd01_component_ops);

Same as above. You could use devm_add_action_or_reset() to fix both cases.

> +}
> +
> +static const struct wmi_device_id lwmi_cd01_id_table[] = {
> +	{ LENOVO_CAPABILITY_DATA_01_GUID, NULL },
> +	{}
> +};
> +
> +static struct wmi_driver lwmi_cd01_driver = {
> +	.driver = {
> +		.name = "lenovo_wmi_cd01",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +	.id_table = lwmi_cd01_id_table,
> +	.probe = lwmi_cd01_probe,
> +	.remove = lwmi_cd01_remove,
> +	.no_singleton = true,
> +};
> +
> +/**
> + * lwmi_cd01_match() - Match rule for the master driver.
> + * @dev: Pointer to the capability data 01 parent device.
> + * @data: Unused void pointer for passing match criteria.
> + *
> + * Return: int.
> + */
> +int lwmi_cd01_match(struct device *dev, void *data)
> +{
> +	return dev->driver == &lwmi_cd01_driver.driver;
> +}
> +EXPORT_SYMBOL_NS_GPL(lwmi_cd01_match, "LENOVO_WMI_CD01");
> +
> +module_wmi_driver(lwmi_cd01_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, lwmi_cd01_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-capdata01.h b/drivers/platform/x86/lenovo-wmi-capdata01.h
> new file mode 100644
> index 000000000000..ed4f3d86464d
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-wmi-capdata01.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +/* Copyright(C) 2025 Derek J. Clark <derekjohn.clark@...il.com> */
> +
> +#ifndef _LENOVO_WMI_CAPDATA01_H_
> +#define _LENOVO_WMI_CAPDATA01_H_
> +
> +#include <linux/types.h>
> +
> +struct device;
> +
> +struct capdata01 {
> +	u32 id;
> +	u32 supported;
> +	u32 default_value;
> +	u32 step;
> +	u32 min_value;
> +	u32 max_value;
> +};
> +
> +struct cd01_list {
> +	u8 count;
> +	struct capdata01 data[];
> +};

Since client driver are not directly accessing this struct anyway please keep its definition private
and only provide an incomplete definition in this header:

	struct cd01_list *;

Thanks,
Armin Wolf

> +
> +struct capdata01 *lwmi_cd01_get_data(struct cd01_list *list, u32 attribute_id);
> +int lwmi_cd01_match(struct device *dev, void *data);
> +
> +#endif /* !_LENOVO_WMI_CAPDATA01_H_ */

Powered by blists - more mailing lists