Re: [PATCH v2 1/5] platform profile: Separate handler locking from sysfs lifetime

[Rafael J. Wysocki (Intel)]

On Mon, Oct 19, 2026 at 10:51 AM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, 19 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@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/acpi/platform_profile.c | 75 ++++++++++++++++++---------------
> >  1 file changed, 42 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> > index 79d2e92e5c28..f98ce4cda02c 100644
> > --- a/drivers/acpi/platform_profile.c
> > +++ b/drivers/acpi/platform_profile.c
> > @@ -8,6 +8,7 @@
> >  #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>
> > @@ -15,8 +16,12 @@
> >  #define to_pprof_handler(d)  (container_of(d, struct platform_profile_handler, dev))
> >
> >  static DEFINE_MUTEX(profile_lock);
>
> Should definitely add comment to this lock as well to explain what it
> protects.

+1

> > +/* Serialize registration and sysfs updates without blocking sysfs callbacks. */
> > +static DEFINE_MUTEX(profile_registration_lock);
> > +static LIST_HEAD(profile_handlers);

Also, I think that the rest of the series is not going to get into 7.4
in any shape or form, so my suggestion would be to address the above
and send a new version of the $subject patch as a separate fix, and
defer the other patches in the series until 7.4-rc1 is out.