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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 11469 bytes --]

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 <daniel.lezcano@oss.qualcomm.com>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 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 <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
>