On Tue, 20 Oct 2026, 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 Reviewed-by: Ilpo Järvinen -- i. > --- > 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 > #include > #include > +#include > #include > #include > #include > > #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 >