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

[Derek John Clark]

On Thu, Apr 9, 2026 at 5:30 AM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> 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.
>

I've taken a note of this. In the next series I'll add a patch to
rename any "err" to "ret" for all lenovo wmi drivers. This series is
already getting out of hand though so I don't want to introduce more
complexity at this time. For now I'll just invert the logic as stated.

Thanks,
Derek

> > +     }
> >
> >       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.
>