Re: [PATCH v3] platform profile: Separate handler locking from sysfs lifetime

[Rafael J. Wysocki (Intel)]

On Tue, Oct 20, 2026 at 11:55 PM Mark Pearson <mpearson-lenovo@xxxxxxxxx> wrote:
>
> On Tue, Oct 20, 2026, at 10:45 AM, Daniel Lezcano wrote:
> > Registration and removal currently hold profile_lock across
> > device_unregister() and sysfs_update_group(). These operations can remove
> > sysfs attributes and wait for their active callbacks to finish. The
> > profile read and write callbacks acquire profile_lock themselves, so a
> > concurrent access can deadlock:
> >
> >   Removal/update path             Active sysfs callback
> >   -------------------             ---------------------
> >   acquire profile_lock            hold kernfs active reference
> >   remove a sysfs attribute         wait for profile_lock
> >   wait for its active references
> >
> > The removal path cannot finish until the callback returns, and the
> > callback cannot return until the removal path releases profile_lock.
> > This applies both to per-provider device teardown and to updates of the
> > legacy aggregate attribute group.
> >
> > Commit 041142d7d252 ("ACPI: platform-profile: Fix possible deadlock in
> > platform_profile_remove()") previously fixed this lock ordering by moving
> > sysfs_remove_group() outside profile_lock. Commit 55b1e68134bf ("ACPI:
> > platform_profile: Use guard(mutex) for register/unregister") reintroduced
> > the issue by extending the mutex guard over sysfs_remove_group(). Later,
> > commit 77be5cacb2c2 ("ACPI: platform_profile: Create class for ACPI
> > platform profile") replaced that teardown with device_unregister() and
> > sysfs_update_group(), still under the same mutex, extending the problem
> > to class-device teardown and aggregate attribute updates.
> >
> > Simply dropping profile_lock around these operations is insufficient.
> > Aggregate operations currently discover providers through the class
> > device list. A provider being unregistered could therefore still be
> > selected for an aggregate read, write or cycle while its sysfs files are
> > being torn down. Concurrent registrations and removals would also race
> > when updating aggregate attribute visibility.
> >
> > Introduce profile_registration_lock to serialize registration, removal
> > and aggregate sysfs updates. Sysfs callbacks never acquire this mutex.
> > Retain profile_lock for profile operations and use it to protect an
> > explicit list of handlers participating in the aggregate interface.
> >
> > Add a handler to that list only after device_register() succeeds. On
> > removal, unlink it under profile_lock before calling device_unregister().
> > Acquiring profile_lock first waits for any aggregate operation already
> > using the handler to complete; unlinking prevents later aggregate
> > operations from finding it. Release profile_lock before draining sysfs
> > callbacks, allowing active per-provider callbacks to complete before the
> > device is released. Apply the same unlink-before-unregister ordering to
> > registration error cleanup.
> >
> > Walk this list for aggregate reads, writes and profile cycling, and use
> > its emptiness to determine aggregate attribute visibility. The class
> > devices remain the per-provider userspace interface.
> >
> > There can now be an interval between removing the last handler from the
> > list and removing the aggregate attributes. Explicitly reject choices
> > reads and writes during that interval: the intersection bitmap starts
> > with every bit set, so an empty traversal must not advertise all profiles
> > or report a successful write without updating a provider.
> >
> > This separates aggregate membership from class device teardown while
> > preserving the existing class and ACPI sysfs interfaces.
> >
> > Fixes: 55b1e68134bf ("ACPI: platform_profile: Use guard(mutex) for
> > register/unregister")
> > Assisted-by: OpenCode:gpt-6.1-sol
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxxxxxxxx>
> > ---
> > Changes in v3:
> >   - Send the locking fix as a standalone patch, independently of the
> >     firmware-agnostic extraction and legacy-interface deprecation
> > series.
> >   - Document that profile_lock protects handler-list membership and
> >     serializes profile operations against each other and handler
> > removal.
> >     Explain why it must not be held across sysfs teardown.
> >
> > Changes in v2:
> >   - Expand the deadlock and handler-lifetime explanation.
> >   - Add the Fixes tag and describe the earlier fix, regression and later
> >     extension to class-device teardown and sysfs group updates.
> >
> >  drivers/acpi/platform_profile.c | 80 ++++++++++++++++++++++++-----------------
> >  1 file changed, 47 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> > index 79d2e92e5c28..d79080a20e21 100644
> > --- a/drivers/acpi/platform_profile.c
> > +++ b/drivers/acpi/platform_profile.c
> > @@ -8,15 +8,25 @@
> >  #include <linux/bits.h>
> >  #include <linux/cleanup.h>
> >  #include <linux/init.h>
> > +#include <linux/list.h>
> >  #include <linux/mutex.h>
> >  #include <linux/platform_profile.h>
> >  #include <linux/sysfs.h>
> >
> >  #define to_pprof_handler(d)  (container_of(d, struct
> > platform_profile_handler, dev))
> >
> > +/*
> > + * Protects profile_handlers membership and serializes profile
> > operations
> > + * against each other and handler removal. Never held across sysfs
> > teardown,
> > + * which can wait for callbacks that acquire this lock.
> > + */
> >  static DEFINE_MUTEX(profile_lock);
> > +/* Serialize registration and sysfs updates without blocking sysfs
> > callbacks. */
> > +static DEFINE_MUTEX(profile_registration_lock);
> > +static LIST_HEAD(profile_handlers);
> >
> >  struct platform_profile_handler {
> > +     struct list_head node;
> >       const char *name;
> >       struct device dev;
> >       int minor;
> > @@ -44,6 +54,22 @@ static_assert(ARRAY_SIZE(profile_names) ==
> > PLATFORM_PROFILE_LAST);
> >
> >  static DEFINE_IDA(platform_profile_ida);
> >
> > +static int profile_walk_handlers(void *data,
> > +                              int (*fn)(struct device *dev, void *data))
> > +{
> > +     struct platform_profile_handler *handler;
> > +     int ret;
> > +
> > +     lockdep_assert_held(&profile_lock);
> > +     list_for_each_entry(handler, &profile_handlers, node) {
> > +             ret = fn(&handler->dev, data);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  /**
> >   * _commmon_choices_show - Show the available profile choices
> >   * @choices: The available profile choices
> > @@ -306,20 +332,18 @@ static ssize_t
> > platform_profile_choices_show(struct kobject *kobj,
> >       int err;
> >
> >       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> > -             err = class_for_each_device(&platform_profile_class, NULL,
> > -                                         &data, _aggregate_choices);
> > +             err = profile_walk_handlers(&data, _aggregate_choices);
> >               if (err)
> >                       return err;
> >               if (data.count == 1) {
> > -                     err = class_for_each_device(&platform_profile_class, NULL,
> > -                                                 &data, _remove_hidden_choices);
> > +                     err = profile_walk_handlers(&data, _remove_hidden_choices);
> >                       if (err)
> >                               return err;
> >               }
> >       }
> >
> >       /* no profile handler registered any more */
> > -     if (bitmap_empty(data.aggregate, PLATFORM_PROFILE_LAST))
> > +     if (!data.count || bitmap_empty(data.aggregate, PLATFORM_PROFILE_LAST))
> >               return -EINVAL;
> >
> >       return _commmon_choices_show(data.aggregate, buf);
> > @@ -384,8 +408,7 @@ static ssize_t platform_profile_show(struct kobject *kobj,
> >       int err;
> >
> >       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> > -             err = class_for_each_device(&platform_profile_class, NULL,
> > -                                         &profile, _aggregate_profiles);
> > +             err = profile_walk_handlers(&profile, _aggregate_profiles);
> >               if (err)
> >                       return err;
> >       }
> > @@ -423,15 +446,13 @@ static ssize_t platform_profile_store(struct
> > kobject *kobj,
> >               return -EINVAL;
> >
> >       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> > -             ret = class_for_each_device(&platform_profile_class, NULL,
> > -                                         &data, _aggregate_choices);
> > +             ret = profile_walk_handlers(&data, _aggregate_choices);
> >               if (ret)
> >                       return ret;
> > -             if (!test_bit(i, data.aggregate))
> > +             if (!data.count || !test_bit(i, data.aggregate))
> >                       return -EOPNOTSUPP;
> >
> > -             ret = class_for_each_device(&platform_profile_class, NULL, &i,
> > -                                         _store_and_notify);
> > +             ret = profile_walk_handlers(&i, _store_and_notify);
> >               if (ret)
> >                       return ret;
> >       }
> > @@ -450,22 +471,11 @@ static struct attribute *platform_profile_attrs[] = {
> >       NULL
> >  };
> >
> > -static int profile_class_registered(struct device *dev, const void
> > *data)
> > -{
> > -     return 1;
> > -}
> > -
> >  static umode_t profile_class_is_visible(struct kobject *kobj, struct
> > attribute *attr, int idx)
> >  {
> > -     struct device *dev;
> > -
> > -     dev = class_find_device(&platform_profile_class, NULL, NULL,
> > profile_class_registered);
> > -     if (!dev)
> > -             return 0;
> > -
> > -     put_device(dev);
> > +     guard(mutex)(&profile_lock);
> >
> > -     return attr->mode;
> > +     return list_empty(&profile_handlers) ? 0 : attr->mode;
> >  }
> >
> >  static const struct attribute_group platform_profile_group = {
> > @@ -502,8 +512,7 @@ int platform_profile_cycle(void)
> >       int err;
> >
> >       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> > -             err = class_for_each_device(&platform_profile_class, NULL,
> > -                                         &profile, _aggregate_profiles);
> > +             err = profile_walk_handlers(&profile, _aggregate_profiles);
> >               if (err)
> >                       return err;
> >
> > @@ -512,8 +521,7 @@ int platform_profile_cycle(void)
> >                   profile == PLATFORM_PROFILE_LAST)
> >                       return -EINVAL;
> >
> > -             err = class_for_each_device(&platform_profile_class, NULL,
> > -                                         &data, _aggregate_choices);
> > +             err = profile_walk_handlers(&data, _aggregate_choices);
> >               if (err)
> >                       return err;
> >
> > @@ -525,8 +533,7 @@ int platform_profile_cycle(void)
> >                                         PLATFORM_PROFILE_LAST,
> >                                         profile + 1);
> >
> > -             err = class_for_each_device(&platform_profile_class, NULL, &next,
> > -                                         _store_and_notify);
> > +             err = profile_walk_handlers(&next, _store_and_notify);
> >
> >               if (err)
> >                       return err;
> > @@ -583,7 +590,7 @@ struct device *platform_profile_register(struct
> > device *dev, const char *name,
> >               }
> >       }
> >
> > -     guard(mutex)(&profile_lock);
> > +     guard(mutex)(&profile_registration_lock);
> >
> >       /* create class interface for individual handler */
> >       minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
> > @@ -604,6 +611,8 @@ struct device *platform_profile_register(struct
> > device *dev, const char *name,
> >               put_device(ppdev);
> >               goto cleanup_ida;
> >       }
> > +     scoped_guard(mutex, &profile_lock)
> > +             list_add_tail(&to_pprof_handler(ppdev)->node, &profile_handlers);
> >
> >       sysfs_notify(acpi_kobj, NULL, "platform_profile");
> >
> > @@ -614,6 +623,8 @@ struct device *platform_profile_register(struct
> > device *dev, const char *name,
> >       return ppdev;
> >
> >  cleanup_cur:
> > +     scoped_guard(mutex, &profile_lock)
> > +             list_del(&to_pprof_handler(ppdev)->node);
> >       device_unregister(ppdev);
> >
> >  cleanup_ida:
> > @@ -636,7 +647,10 @@ void platform_profile_remove(struct device *dev)
> >
> >       pprof = to_pprof_handler(dev);
> >
> > -     guard(mutex)(&profile_lock);
> > +     guard(mutex)(&profile_registration_lock);
> > +
> > +     scoped_guard(mutex, &profile_lock)
> > +             list_del(&pprof->node);
> >
> >       ida_free(&platform_profile_ida, pprof->minor);
> >       device_unregister(&pprof->dev);
> >
> > base-commit: d0cd1e850e1602729dd60dc3c5fd96f80e0cdc5d
>
> Looks good to me.
> Reviewed-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>

Applied as 7.4-rc material with some minor tweaks:

https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?id=b66621ad2a8fb553c4cb75b9d69fead4f0532933