[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <61e17bf7-b549-4345-9d4f-be14e13b34af@gmx.de>
Date: Tue, 25 Nov 2025 03:08:55 +0100
From: Armin Wolf <W_Armin@....de>
To: Rong Zhang <i@...g.moe>, Mark Pearson <mpearson-lenovo@...ebb.ca>,
"Derek J. Clark" <derekjohn.clark@...il.com>,
Hans de Goede <hansg@...nel.org>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Guenter Roeck <linux@...ck-us.net>, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v6 6/7] platform/x86: lenovo-wmi-capdata: Wire up Fan Test
Data
Am 23.11.25 um 16:02 schrieb Rong Zhang:
> Hi Armin,
>
> On Sun, 2025-11-23 at 04:36 +0100, Armin Wolf wrote:
>> Am 22.11.25 um 19:44 schrieb Rong Zhang:
>>
>>> A capdata00 attribute (0x04050000) describes the presence of Fan Test
>>> Data. Query it, and bind Fan Test Data as a component of capdata00
>>> accordingly. The component master of capdata00 may pass a callback while
>>> binding to retrieve fan info from Fan Test Data.
>>>
>>> Summarizing this scheme:
>>>
>>> lenovo-wmi-other <-> capdata00 <-> capdata_fan
>>> |- master |- component
>>> |- sub-master
>>> |- sub-component
>>>
>>> The callback will be called once both the master and the sub-component
>>> are bound to the sub-master (component).
>>>
>>> This scheme is essential to solve these issues:
>>> - The component framework only supports one aggregation per master
>>> - A binding is only established until all components are found
>>> - The Fan Test Data interface may be missing on some devices
>>> - To get rid of queries for the presense of WMI GUIDs
>>>
>>> capdata00 is registered as a component and a sub-master on probe,
>>> instead of chaining the registerations in one's bind callback. This is
>>> because calling (un)registration methods of the component framework
>>> causes deadlock in (un)bind callbacks, i.e., it's impossible to register
>>> capdata00 as a sub-master/component in its component/sub-master bind
>>> callback, and vice versa.
>> I spend some time trying to understand all of this, and i have come to the conclusion
>> that while passing a callback between both component masters, it still is not exactly
>> clean.
> Could you elaborate? Thanks.
>
>> Can you instead use devm_lwmi_om_register_notifier() and define another event for querying
>> the fan data? This way we could ditch the component submaster with all of the callback madness.
> Thanks for your review and suggestion.
>
> At first I thought the notifier approach was nice, however...
>
> There are two directions to use the notifier framework:
> (1) lenovo-wmi-other calls blocking_notifier_call_chain() to query
> capdata_fan
> (2) capdata_fan calls blocking_notifier_call_chain() to provide data
> to lenovo-wmi-other
>
> Both requires some assumptions on probing/binding sequence:
> (1) capdata_fan must be probed before lenovo-wmi-other calling
> blocking_notifier_call_chain(), otherwise lenovo-wmi-other will
> mistakenly believe capdata_fan is missing
> (2) lenovo-wmi-other must be probed before capdata_fan calling
> blocking_notifier_call_chain(), otherwise lenovo-wmi-other will
> wait for fan info forever
>
> So eventually we need *both directions* to break the assumptions on
> probing/binding sequence. Moreover, some extra synchronization
> primitives may be required to make sure the binding between lenovo-wmi-
> other and capdata00&01 won't be unbind while registering the HWMON
> device.
>
>> Sorry for being the one for suggesting the submaster approach in the first place, i did not
>> know that the component framework is so limited when it comes to nested component masters :/
> I'd say the sub-master approach is still the least ugly one.
>
> The callback is always called in a bind callback. Hence, the component
> framework protects our sanity, so we don't need to worry about the
> other side of our component chain being messed up while handling one
> side. The approach doesn't need any assumption on binding sequence as
> the corresponding bind/unbind callbacks effectively create a state
> machine.
Fine, if you prefer the submaster approach, then ignore my suggestion. As long as
you do not use wmi_has_guid(), i am happy.
Thanks,
Armin Wolf
>> If Mark is OK with using the submaster approach then you can ignore the above suggestion.
>>
>> Thanks,
>> Armin Wolf
> Thanks,
> Rong
>
>>> Signed-off-by: Rong Zhang <i@...g.moe>
>>> ---
>>> Changes in v6:
>>> - Fix the error path of component_add(capdata00)
>>> - Elaborate the design in commit message
>>>
>>> Changes in v5:
>>> - Fix missing include (thanks kernel test robot)
>>>
>>> Changes in v4:
>>> - New patch in the series (thanks Armin Wolf's inspiration)
>>> - Get rid of wmi_has_guid() (see also [PATCH v4 3/7])
>>> ---
>>> drivers/platform/x86/lenovo/wmi-capdata.c | 265 +++++++++++++++++++++-
>>> drivers/platform/x86/lenovo/wmi-capdata.h | 20 ++
>>> drivers/platform/x86/lenovo/wmi-other.c | 5 -
>>> 3 files changed, 283 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
>>> index e6392357395c..8760f8c071ca 100644
>>> --- a/drivers/platform/x86/lenovo/wmi-capdata.c
>>> +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
>>> @@ -27,6 +27,7 @@
>>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>
>>> #include <linux/acpi.h>
>>> +#include <linux/bitfield.h>
>>> #include <linux/cleanup.h>
>>> #include <linux/component.h>
>>> #include <linux/container_of.h>
>>> @@ -50,10 +51,17 @@
>>> #define ACPI_AC_CLASS "ac_adapter"
>>> #define ACPI_AC_NOTIFY_STATUS 0x80
>>>
>>> +#define LWMI_FEATURE_ID_FAN_TEST 0x05
>>> +
>>> +#define LWMI_ATTR_ID_FAN_TEST \
>>> + (FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, LWMI_DEVICE_ID_FAN) | \
>>> + FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, LWMI_FEATURE_ID_FAN_TEST))
>>> +
>>> enum lwmi_cd_type {
>>> LENOVO_CAPABILITY_DATA_00,
>>> LENOVO_CAPABILITY_DATA_01,
>>> LENOVO_FAN_TEST_DATA,
>>> + CD_TYPE_NONE = -1,
>>> };
>>>
>>> #define LWMI_CD_TABLE_ITEM(_type) \
>>> @@ -75,6 +83,20 @@ struct lwmi_cd_priv {
>>> struct notifier_block acpi_nb; /* ACPI events */
>>> struct wmi_device *wdev;
>>> struct cd_list *list;
>>> +
>>> + /*
>>> + * A capdata device may be a component master of another capdata device.
>>> + * E.g., lenovo-wmi-other <-> capdata00 <-> capdata_fan
>>> + * |- master |- component
>>> + * |- sub-master
>>> + * |- sub-component
>>> + */
>>> + struct lwmi_cd_sub_master_priv {
>>> + struct device *master_dev;
>>> + cd_list_cb_t master_cb;
>>> + struct cd_list *sub_component_list; /* ERR_PTR(-ENODEV) implies no sub-component. */
>>> + bool registered; /* Has the sub-master been registered? */
>>> + } *sub_master;
>>> };
>>>
>>> struct cd_list {
>>> @@ -125,7 +147,7 @@ void lwmi_cd_match_add_all(struct device *master, struct component_match **match
>>> return;
>>>
>>> for (i = 0; i < ARRAY_SIZE(lwmi_cd_table); i++) {
>>> - /* Skip optional interfaces. */
>>> + /* Skip sub-components. */
>>> if (lwmi_cd_table[i].type == LENOVO_FAN_TEST_DATA)
>>> continue;
>>>
>>> @@ -137,6 +159,56 @@ void lwmi_cd_match_add_all(struct device *master, struct component_match **match
>>> }
>>> EXPORT_SYMBOL_NS_GPL(lwmi_cd_match_add_all, "LENOVO_WMI_CD");
>>>
>>> +/**
>>> + * lwmi_cd_call_master_cb() - Call the master callback for the sub-component.
>>> + * @priv: Pointer to the capability data private data.
>>> + *
>>> + * Call the master callback and pass the sub-component list to it if the
>>> + * dependency chain (master <-> sub-master <-> sub-component) is complete.
>>> + */
>>> +static void lwmi_cd_call_master_cb(struct lwmi_cd_priv *priv)
>>> +{
>>> + struct cd_list *sub_component_list = priv->sub_master->sub_component_list;
>>> +
>>> + /*
>>> + * Call the callback only if the dependency chain is ready:
>>> + * - Binding between master and sub-master: fills master_dev and master_cb
>>> + * - Binding between sub-master and sub-component: fills sub_component_list
>>> + *
>>> + * If a binding has been unbound before the other binding is bound, the
>>> + * corresponding members filled by the former are guaranteed to be cleared.
>>> + *
>>> + * This function is only called in bind callbacks, and the component
>>> + * framework guarantees bind/unbind callbacks may never execute
>>> + * simultaneously, which implies that it's impossible to have a race
>>> + * condition.
>>> + *
>>> + * Hence, this check is sufficient to ensure that the callback is called
>>> + * at most once and with the correct state, without relying on a specific
>>> + * sequence of binding establishment.
>>> + */
>>> + if (!sub_component_list ||
>>> + !priv->sub_master->master_dev ||
>>> + !priv->sub_master->master_cb)
>>> + return;
>>> +
>>> + if (PTR_ERR(sub_component_list) == -ENODEV)
>>> + sub_component_list = NULL;
>>> + else if (WARN_ON(IS_ERR(sub_component_list)))
>>> + return;
>>> +
>>> + priv->sub_master->master_cb(priv->sub_master->master_dev,
>>> + sub_component_list);
>>> +
>>> + /*
>>> + * Prevent "unbind and rebind" sequences from userspace from calling the
>>> + * callback twice.
>>> + */
>>> + priv->sub_master->master_cb = NULL;
>>> + priv->sub_master->master_dev = NULL;
>>> + priv->sub_master->sub_component_list = NULL;
>>> +}
>>> +
>>> /**
>>> * lwmi_cd_component_bind() - Bind component to master device.
>>> * @cd_dev: Pointer to the lenovo-wmi-capdata driver parent device.
>>> @@ -147,6 +219,8 @@ EXPORT_SYMBOL_NS_GPL(lwmi_cd_match_add_all, "LENOVO_WMI_CD");
>>> * list. This is used to call lwmi_cd*_get_data to look up attribute data
>>> * from the lenovo-wmi-other driver.
>>> *
>>> + * If cd_dev is a sub-master, try to call the master callback.
>>> + *
>>> * Return: 0
>>> */
>>> static int lwmi_cd_component_bind(struct device *cd_dev,
>>> @@ -158,6 +232,11 @@ static int lwmi_cd_component_bind(struct device *cd_dev,
>>> switch (priv->list->type) {
>>> case LENOVO_CAPABILITY_DATA_00:
>>> binder->cd00_list = priv->list;
>>> +
>>> + priv->sub_master->master_dev = om_dev;
>>> + priv->sub_master->master_cb = binder->cd_fan_list_cb;
>>> + lwmi_cd_call_master_cb(priv);
>>> +
>>> break;
>>> case LENOVO_CAPABILITY_DATA_01:
>>> binder->cd01_list = priv->list;
>>> @@ -169,8 +248,167 @@ static int lwmi_cd_component_bind(struct device *cd_dev,
>>> return 0;
>>> }
>>>
>>> +/**
>>> + * lwmi_cd_component_unbind() - Unbind component to master device.
>>> + * @cd_dev: Pointer to the lenovo-wmi-capdata driver parent device.
>>> + * @om_dev: Pointer to the lenovo-wmi-other driver parent device.
>>> + * @data: Unused.
>>> + *
>>> + * If cd_dev is a sub-master, clear the collected data from the master device to
>>> + * prevent the binding establishment between the sub-master and the sub-
>>> + * component (if it's about to happen) from calling the master callback.
>>> + */
>>> +static void lwmi_cd_component_unbind(struct device *cd_dev,
>>> + struct device *om_dev, void *data)
>>> +{
>>> + struct lwmi_cd_priv *priv = dev_get_drvdata(cd_dev);
>>> +
>>> + switch (priv->list->type) {
>>> + case LENOVO_CAPABILITY_DATA_00:
>>> + priv->sub_master->master_dev = NULL;
>>> + priv->sub_master->master_cb = NULL;
>>> + return;
>>> + default:
>>> + return;
>>> + }
>>> +}
>>> +
>>> static const struct component_ops lwmi_cd_component_ops = {
>>> .bind = lwmi_cd_component_bind,
>>> + .unbind = lwmi_cd_component_unbind,
>>> +};
>>> +
>>> +/**
>>> + * lwmi_cd_sub_master_bind() - Bind sub-component of sub-master device
>>> + * @dev: The sub-master capdata basic device.
>>> + *
>>> + * Call component_bind_all to bind the sub-component device to the sub-master
>>> + * device. On success, collect the pointer to the sub-component list and try
>>> + * to call the master callback.
>>> + *
>>> + * Return: 0 on success, or an error code.
>>> + */
>>> +static int lwmi_cd_sub_master_bind(struct device *dev)
>>> +{
>>> + struct lwmi_cd_priv *priv = dev_get_drvdata(dev);
>>> + struct cd_list *sub_component_list;
>>> + int ret;
>>> +
>>> + ret = component_bind_all(dev, &sub_component_list);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + priv->sub_master->sub_component_list = sub_component_list;
>>> + lwmi_cd_call_master_cb(priv);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * lwmi_cd_sub_master_unbind() - Unbind sub-component of sub-master device
>>> + * @dev: The sub-master capdata basic device
>>> + *
>>> + * Clear the collected pointer to the sub-component list to prevent the binding
>>> + * establishment between the sub-master and the sub-component (if it's about to
>>> + * happen) from calling the master callback. Then, call component_unbind_all to
>>> + * unbind the sub-component device from the sub-master device.
>>> + */
>>> +static void lwmi_cd_sub_master_unbind(struct device *dev)
>>> +{
>>> + struct lwmi_cd_priv *priv = dev_get_drvdata(dev);
>>> +
>>> + priv->sub_master->sub_component_list = NULL;
>>> +
>>> + component_unbind_all(dev, NULL);
>>> +}
>>> +
>>> +static const struct component_master_ops lwmi_cd_sub_master_ops = {
>>> + .bind = lwmi_cd_sub_master_bind,
>>> + .unbind = lwmi_cd_sub_master_unbind,
>>> +};
>>> +
>>> +/**
>>> + * lwmi_cd_sub_master_add() - Register a sub-master with its sub-component
>>> + * @priv: Pointer to the sub-master capdata device private data.
>>> + * @sub_component_type: Type of the sub-component.
>>> + *
>>> + * Match the sub-component type and register the current capdata device as a
>>> + * sub-master. If the given sub-component type is CD_TYPE_NONE, mark the sub-
>>> + * component as non-existent without registering sub-master.
>>> + *
>>> + * Return: 0 on success, or an error code.
>>> + */
>>> +static int lwmi_cd_sub_master_add(struct lwmi_cd_priv *priv,
>>> + enum lwmi_cd_type sub_component_type)
>>> +{
>>> + struct component_match *master_match = NULL;
>>> + int ret;
>>> +
>>> + priv->sub_master = devm_kzalloc(&priv->wdev->dev, sizeof(*priv->sub_master), GFP_KERNEL);
>>> + if (!priv->sub_master)
>>> + return -ENOMEM;
>>> +
>>> + if (sub_component_type == CD_TYPE_NONE) {
>>> + /* The master callback will be called with NULL on bind. */
>>> + priv->sub_master->sub_component_list = ERR_PTR(-ENODEV);
>>> + priv->sub_master->registered = false;
>>> + return 0;
>>> + }
>>> +
>>> + /*
>>> + * lwmi_cd_match() needs a pointer to enum lwmi_cd_type, but on-stack
>>> + * data cannot be used here. Steal one from lwmi_cd_table.
>>> + */
>>> + component_match_add(&priv->wdev->dev, &master_match, lwmi_cd_match,
>>> + (void *)&lwmi_cd_table[sub_component_type].type);
>>> + if (IS_ERR(master_match))
>>> + return PTR_ERR(master_match);
>>> +
>>> + ret = component_master_add_with_match(&priv->wdev->dev, &lwmi_cd_sub_master_ops,
>>> + master_match);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + priv->sub_master->registered = true;
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * lwmi_cd_sub_master_del() - Unregister a sub-master if it's registered
>>> + * @priv: Pointer to the sub-master capdata device private data.
>>> + */
>>> +static void lwmi_cd_sub_master_del(struct lwmi_cd_priv *priv)
>>> +{
>>> + if (priv->sub_master->registered) {
>>> + component_master_del(&priv->wdev->dev, &lwmi_cd_sub_master_ops);
>>> + priv->sub_master->registered = false;
>>> + }
>>> +}
>>> +
>>> +/**
>>> + * lwmi_cd_sub_component_bind() - Bind sub-component to sub-master device.
>>> + * @sc_dev: Pointer to the sub-component capdata parent device.
>>> + * @sm_dev: Pointer to the sub-master capdata parent device.
>>> + * @data: Pointer used to return the capability data list pointer.
>>> + *
>>> + * On sub-master's bind, provide a pointer to the local capdata list.
>>> + * This is used by the sub-master to call the master callback.
>>> + *
>>> + * Return: 0
>>> + */
>>> +static int lwmi_cd_sub_component_bind(struct device *sc_dev,
>>> + struct device *sm_dev, void *data)
>>> +{
>>> + struct lwmi_cd_priv *priv = dev_get_drvdata(sc_dev);
>>> + struct cd_list **listp = data;
>>> +
>>> + *listp = priv->list;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct component_ops lwmi_cd_sub_component_ops = {
>>> + .bind = lwmi_cd_sub_component_bind,
>>> };
>>>
>>> /**
>>> @@ -470,9 +708,28 @@ static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
>>> goto out;
>>>
>>> switch (info->type) {
>>> - case LENOVO_CAPABILITY_DATA_00:
>>> + case LENOVO_CAPABILITY_DATA_00: {
>>> + enum lwmi_cd_type sub_component_type = LENOVO_FAN_TEST_DATA;
>>> + struct capdata00 capdata00;
>>> +
>>> + ret = lwmi_cd00_get_data(priv->list, LWMI_ATTR_ID_FAN_TEST, &capdata00);
>>> + if (ret || !(capdata00.supported & LWMI_SUPP_VALID)) {
>>> + dev_dbg(&wdev->dev, "capdata00 declares no fan test support\n");
>>> + sub_component_type = CD_TYPE_NONE;
>>> + }
>>> +
>>> + /* Sub-master (capdata00) <-> sub-component (capdata_fan) */
>>> + ret = lwmi_cd_sub_master_add(priv, sub_component_type);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + /* Master (lenovo-wmi-other) <-> sub-master (capdata00) */
>>> ret = component_add(&wdev->dev, &lwmi_cd_component_ops);
>>> + if (ret)
>>> + lwmi_cd_sub_master_del(priv);
>>> +
>>> goto out;
>>> + }
>>> case LENOVO_CAPABILITY_DATA_01:
>>> priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
>>>
>>> @@ -488,6 +745,7 @@ static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
>>> ret = component_add(&wdev->dev, &lwmi_cd_component_ops);
>>> goto out;
>>> case LENOVO_FAN_TEST_DATA:
>>> + ret = component_add(&wdev->dev, &lwmi_cd_sub_component_ops);
>>> goto out;
>>> default:
>>> return -EINVAL;
>>> @@ -509,10 +767,13 @@ static void lwmi_cd_remove(struct wmi_device *wdev)
>>>
>>> switch (priv->list->type) {
>>> case LENOVO_CAPABILITY_DATA_00:
>>> + lwmi_cd_sub_master_del(priv);
>>> + fallthrough;
>>> case LENOVO_CAPABILITY_DATA_01:
>>> component_del(&wdev->dev, &lwmi_cd_component_ops);
>>> break;
>>> case LENOVO_FAN_TEST_DATA:
>>> + component_del(&wdev->dev, &lwmi_cd_sub_component_ops);
>>> break;
>>> default:
>>> WARN_ON(1);
>>> diff --git a/drivers/platform/x86/lenovo/wmi-capdata.h b/drivers/platform/x86/lenovo/wmi-capdata.h
>>> index 38af4c4e4ef4..59ca3b3e5760 100644
>>> --- a/drivers/platform/x86/lenovo/wmi-capdata.h
>>> +++ b/drivers/platform/x86/lenovo/wmi-capdata.h
>>> @@ -5,8 +5,20 @@
>>> #ifndef _LENOVO_WMI_CAPDATA_H_
>>> #define _LENOVO_WMI_CAPDATA_H_
>>>
>>> +#include <linux/bits.h>
>>> #include <linux/types.h>
>>>
>>> +#define LWMI_SUPP_VALID BIT(0)
>>> +#define LWMI_SUPP_MAY_GET (LWMI_SUPP_VALID | BIT(1))
>>> +#define LWMI_SUPP_MAY_SET (LWMI_SUPP_VALID | BIT(2))
>>> +
>>> +#define LWMI_ATTR_DEV_ID_MASK GENMASK(31, 24)
>>> +#define LWMI_ATTR_FEAT_ID_MASK GENMASK(23, 16)
>>> +#define LWMI_ATTR_MODE_ID_MASK GENMASK(15, 8)
>>> +#define LWMI_ATTR_TYPE_ID_MASK GENMASK(7, 0)
>>> +
>>> +#define LWMI_DEVICE_ID_FAN 0x04
>>> +
>>> struct component_match;
>>> struct device;
>>> struct cd_list;
>>> @@ -32,9 +44,17 @@ struct capdata_fan {
>>> u32 max_rpm;
>>> };
>>>
>>> +typedef void (*cd_list_cb_t)(struct device *master_dev, struct cd_list *cd_list);
>>> +
>>> struct lwmi_cd_binder {
>>> struct cd_list *cd00_list;
>>> struct cd_list *cd01_list;
>>> + /*
>>> + * May be called during or after the bind callback.
>>> + * Will be called with NULL if capdata_fan does not exist.
>>> + * The pointer is only valid in the callback; never keep it for later use!
>>> + */
>>> + cd_list_cb_t cd_fan_list_cb;
>>> };
>>>
>>> void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr);
>>> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
>>> index f2e1e34d58a9..b3adcc2804fa 100644
>>> --- a/drivers/platform/x86/lenovo/wmi-other.c
>>> +++ b/drivers/platform/x86/lenovo/wmi-other.c
>>> @@ -54,11 +54,6 @@
>>> #define LWMI_FEATURE_VALUE_GET 17
>>> #define LWMI_FEATURE_VALUE_SET 18
>>>
>>> -#define LWMI_ATTR_DEV_ID_MASK GENMASK(31, 24)
>>> -#define LWMI_ATTR_FEAT_ID_MASK GENMASK(23, 16)
>>> -#define LWMI_ATTR_MODE_ID_MASK GENMASK(15, 8)
>>> -#define LWMI_ATTR_TYPE_ID_MASK GENMASK(7, 0)
>>> -
>>> #define LWMI_OM_FW_ATTR_BASE_PATH "lenovo-wmi-other"
>>>
>>> static BLOCKING_NOTIFIER_HEAD(om_chain_head);
Powered by blists - more mailing lists