Re: [PATCH 2/3] platform/x86: lenovo-wmi-other: Balance IDA id allocation and free

[Ilpo Järvinen]

On Thu, 2 Apr 2026, Rong Zhang wrote:

> Currently, the IDA id is only freed on wmi-other device removal or
> failure to create firmware-attributes device, kset, or attributes. It
> leaks IDA ids if the wmi-other device is bound multiple times, as the
> unbind callback never frees the previously allocated IDA id.
> Additionally, if the wmi-other device has failed to create a
> firmware-attributes device before it gets removed, the wmi-device
> removal callback double frees the same IDA id.
> 
> These bugs were found by sashiko.dev [1].
> 
> Fix them by moving ida_free() into lwmi_om_fw_attr_remove() so it is
> balanced with ida_alloc() in lwmi_om_fw_attr_add(). With them fixed,
> properly set and utilize the validity of priv->ida_id to balance
> firmware-attributes registration and removal, without relying on
> propagating the registration error to the component framework, which is
> more reliable and aligns with the hwmon device registration and removal
> sequences.
> 
> No functional change intended.
> 
> Fixes: edc4b183b794 ("platform/x86: Add Lenovo Other Mode WMI Driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Link: https://sashiko.dev/#/patchset/20260331181208.421552-1-derekjohn.clark%40gmail.com [1]
> Signed-off-by: Rong Zhang <i@xxxxxxxx>
> ---
>  drivers/platform/x86/lenovo/wmi-other.c | 34 +++++++++++++++----------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> index 6040f45aa2b0..b47418df099f 100644
> --- a/drivers/platform/x86/lenovo/wmi-other.c
> +++ b/drivers/platform/x86/lenovo/wmi-other.c
> @@ -957,17 +957,17 @@ static struct capdata01_attr_group cd01_attr_groups[] = {
>  /**
>   * lwmi_om_fw_attr_add() - Register all firmware_attributes_class members
>   * @priv: The Other Mode driver data.
> - *
> - * Return: Either 0, or an error code.
>   */
> -static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
> +static void lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
>  {
>  	unsigned int i;
>  	int err;
>  
>  	priv->ida_id = ida_alloc(&lwmi_om_ida, GFP_KERNEL);
> -	if (priv->ida_id < 0)
> -		return priv->ida_id;
> +	if (priv->ida_id < 0) {
> +		err = priv->ida_id;

This looks a bit backwards. It would be better to do:

	err = ida_alloc(&lwmi_om_ida, GFP_KERNEL);
	if (err < 0)
> +		goto err;

	priv->ida_id = err;

...This, btw, tells us why "ret" would have been superior name for the 
generic return variable as it does not carry "error" connotation.

> +	}
>  
>  	priv->fw_attr_dev = device_create(&firmware_attributes_class, NULL,
>  					  MKDEV(0, 0), NULL, "%s-%u",
> @@ -993,7 +993,7 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
>  
>  		cd01_attr_groups[i].tunable_attr->dev = &priv->wdev->dev;
>  	}
> -	return 0;
> +	return;
>  
>  err_remove_groups:
>  	while (i--)
> @@ -1007,7 +1007,12 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
>  
>  err_free_ida:
>  	ida_free(&lwmi_om_ida, priv->ida_id);
> -	return err;
> +
> +err:
> +	priv->ida_id = -EIDRM;
> +
> +	dev_warn(&priv->wdev->dev,
> +		 "failed to register firmware-attributes device: %d\n", err);
>  }
>  
>  /**
> @@ -1016,12 +1021,17 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
>   */
>  static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
>  {
> +	if (priv->ida_id < 0)
> +		return;
> +
>  	for (unsigned int i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; i++)
>  		sysfs_remove_group(&priv->fw_attr_kset->kobj,
>  				   cd01_attr_groups[i].attr_group);
>  
>  	kset_unregister(priv->fw_attr_kset);
>  	device_unregister(priv->fw_attr_dev);
> +	ida_free(&lwmi_om_ida, priv->ida_id);
> +	priv->ida_id = -EIDRM;
>  }
>  
>  /* ======== Self (master: lenovo-wmi-other) ======== */
> @@ -1063,7 +1073,9 @@ static int lwmi_om_master_bind(struct device *dev)
>  
>  	lwmi_om_fan_info_collect_cd00(priv);
>  
> -	return lwmi_om_fw_attr_add(priv);
> +	lwmi_om_fw_attr_add(priv);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -1115,13 +1127,7 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
>  
>  static void lwmi_other_remove(struct wmi_device *wdev)
>  {
> -	struct lwmi_om_priv *priv = dev_get_drvdata(&wdev->dev);
> -
>  	component_master_del(&wdev->dev, &lwmi_om_master_ops);
> -
> -	/* No IDA to free if the driver is never bound to its components. */
> -	if (priv->ida_id >= 0)
> -		ida_free(&lwmi_om_ida, priv->ida_id);
>  }
>  
>  static const struct wmi_device_id lwmi_other_id_table[] = {
> 

-- 
 i.