Re: [PATCH 3/9] platform/x86: lenovo-wmi-capdata: Defer mutex initialization

[Rong Zhang]

Hi Ilpo,

于 2026年10月6日 GMT+08:00 00:25:07，"Ilpo Järvinen" <ilpo.jarvinen@xxxxxxxxxxxxxxx> 写道：
> On Mon, 14 Sep 2026, Rong Zhang wrote:
> 
> > In the following changes, priv->list may be freed if the first call to
> > lwmi_cd_cache() fails due to WMI/ACPI errors, so the list_mutex must be
> > initialized after it in order not to break lockdep, as there is no
> > devm_mutex_destroy(). Considering that the first call to lwmi_cd_cache()
> > doesn't need serialization as there is no other reader or writer this
> > early, the initialization of list_mutex can be deferred.
> > 
> > Therefore, initialize list_mutex only after the first call to
> > lwmi_cd_cache() succeeds, otherwise it remains uninitialized and can be
> > devm_kfree()-ed.
> > 
> > Signed-off-by: Rong Zhang <i@xxxxxxxx>
> > ---
> >  drivers/platform/x86/lenovo/wmi-capdata.c | 80 +++++++++++++++++++++++--------
> >  1 file changed, 60 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
> > index 880ac444c206..0123ec8f7b53 100644
> > --- a/drivers/platform/x86/lenovo/wmi-capdata.c
> > +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
> > @@ -91,6 +91,7 @@ struct lwmi_cd_priv {
> >  	struct wmi_device *wdev;
> >  	struct cd_list *list;
> >  	struct dentry *debugfs_dir;
> > +	bool initialized;
> >  
> >  	/*
> >  	 * A capdata device may be a component master of another capdata device.
> > @@ -588,14 +589,14 @@ static void lwmi_cd_debugfs_remove(struct lwmi_cd_priv *priv)
> >  /* ======== WMI interface ======== */
> >  
> >  /**
> > - * lwmi_cd_cache() - Cache all WMI data block information
> > + * __lwmi_cd_cache() - Cache all WMI data block information locklessly
> >   * @priv: lenovo-wmi-capdata driver data.
> >   *
> > - * Loop through each WMI data block and cache the data.
> > + * Loop through each WMI data block and cache the data locklessly.
> >   *
> >   * Return: 0 on success, or an error.
> >   */
> > -static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> > +static int __lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >  {
> >  	size_t size;
> >  	int idx;
> > @@ -617,7 +618,6 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >  		return -EINVAL;
> >  	}
> >  
> > -	guard(mutex)(&priv->list->list_mutex);
> >  	for (idx = 0; idx < priv->list->count; idx++, p += size) {
> >  		union acpi_object *ret_obj __free(kfree) = NULL;
> >  
> > @@ -635,14 +635,37 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * lwmi_cd_cache() - Cache all WMI data block information
> > + * @priv: lenovo-wmi-capdata driver data.
> > + *
> > + * Loop through each WMI data block and cache the data.
> > + *
> > + * Return: 0 on success, or an error.
> > + */
> > +static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> > +{
> > +	if (!priv->initialized)
> > +		return __lwmi_cd_cache(priv);
> > +
> > +	switch (priv->info->type) {
> > +	case LENOVO_CAPABILITY_DATA_01:
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	guard(mutex)(&priv->list->list_mutex);
> > +	return __lwmi_cd_cache(priv);
> > +}
> > +
> >  /**
> >   * lwmi_cd_fan_list_alloc_cache() - Alloc and cache Fan Test Data list
> >   * @priv: lenovo-wmi-capdata driver data.
> > - * @listptr: Pointer to returned cd_list pointer.
> >   *
> >   * Return: count of fans found, or an error.
> >   */
> > -static int lwmi_cd_fan_list_alloc_cache(struct lwmi_cd_priv *priv, struct cd_list **listptr)
> > +static int lwmi_cd_fan_list_alloc_cache(struct lwmi_cd_priv *priv)
> >  {
> >  	struct cd_list *list;
> >  	size_t size;
> > @@ -688,6 +711,9 @@ static int lwmi_cd_fan_list_alloc_cache(struct lwmi_cd_priv *priv, struct cd_lis
> >  	if (!list)
> >  		return -ENOMEM;
> >  
> > +	list->count = count;
> > +	priv->list = list;
> > +
> >  	for (idx = 0; idx < count; idx++) {
> >  		/* Do not calculate array index using count, as it may be truncated. */
> >  		list->cd_fan[idx] = (struct capdata_fan) {
> > @@ -697,8 +723,7 @@ static int lwmi_cd_fan_list_alloc_cache(struct lwmi_cd_priv *priv, struct cd_lis
> >  		};
> >  	}
> >  
> > -	*listptr = list;
> > -	return count;
> > +	return 0;
> >  }
> >  
> >  /**
> > @@ -714,7 +739,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> >  {
> >  	struct cd_list *list;
> >  	size_t list_size;
> > -	int count, ret;
> > +	int count;
> >  
> >  	count = wmidev_instance_count(priv->wdev);
> >  
> > @@ -726,11 +751,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> >  		list_size = struct_size(list, cd01, count);
> >  		break;
> >  	case LENOVO_FAN_TEST_DATA:
> > -		count = lwmi_cd_fan_list_alloc_cache(priv, &list);
> > -		if (count < 0)
> > -			return count;
> > -
> > -		goto got_list;
> > +		return lwmi_cd_fan_list_alloc_cache(priv);
> >  	default:
> >  		return -EINVAL;
> >  	}
> > @@ -739,17 +760,32 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> >  	if (!list)
> >  		return -ENOMEM;
> >  
> > -got_list:
> > -	ret = devm_mutex_init(&priv->wdev->dev, &list->list_mutex);
> > -	if (ret)
> > -		return ret;
> > -
> >  	list->count = count;
> >  	priv->list = list;
> >  
> >  	return 0;
> >  }
> >  
> > +/**
> > + * lwmi_cd_finalize() - Finalize the capability data initialization
> > + * @priv: lenovo-wmi-capdata driver data.
> > + *
> > + * Return: 0 on success, or an error code.
> > + */
> > +static int lwmi_cd_finalize(struct lwmi_cd_priv *priv)
> > +{
> > +	int ret;
> > +
> > +	if (priv->list) {
> > +		ret = devm_mutex_init(&priv->wdev->dev, &priv->list->list_mutex);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	priv->initialized = 1;
> 
> Don't you need a barrier to ensure these are really done in the correct 
> order?
> 

Indeed. Will add appropriate release-acquire barriers.

ACK to all comments to other patches. Thanks a lot for reviewing!

Thanks,
Rong

> > +	return 0;
> > +}
> > +
> >  /**
> >   * lwmi_cd_setup() - Cache all WMI data block information
> >   * @priv: lenovo-wmi-capdata driver data.
> > @@ -768,7 +804,11 @@ static int lwmi_cd_setup(struct lwmi_cd_priv *priv)
> >  	if (ret)
> >  		return ret;
> >  
> > -	return lwmi_cd_cache(priv);
> > +	ret = lwmi_cd_cache(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return lwmi_cd_finalize(priv);
> >  }
> >  
> >  /**
> > 
> > 
>