Message-ID: <667b29838d39f7bac2a754c4f0720e33ead556f8.camel@rong.moe>
Date: Sun, 23 Nov 2025 23:02:04 +0800
From: Rong Zhang <i@...g.moe>
To: Armin Wolf <W_Armin@....de>, 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

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.

> 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