Re: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/31/2024 15:55, Armin Wolf wrote:
> Am 31.10.24 um 05:09 schrieb Mario Limonciello:
> 
>> The "platform_profile" class device has the exact same semantics as the
>> platform profile files in /sys/firmware/acpi/ but it reflects values only
>> present for a single platform profile handler.
>>
>> The expectation is that legacy userspace can change the profile for all
>> handlers in /sys/firmware/acpi/platform_profile and can change it for
>> individual handlers by /sys/class/platform_profile/*.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/platform_profile.c  | 93 ++++++++++++++++++++++++++++----
>>   include/linux/platform_profile.h |  2 +
>>   2 files changed, 85 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/ 
>> platform_profile.c
>> index 9b681884ae324..1cc8182930dde 100644
>> --- a/drivers/acpi/platform_profile.c
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -24,13 +24,24 @@ static const char * const profile_names[] = {
>>   };
>>   static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>>
>> +static DEFINE_IDR(platform_profile_minor_idr);
>> +
>> +static const struct class platform_profile_class = {
>> +    .name = "platform-profile",
>> +};
>> +
>>   static bool platform_profile_is_registered(void)
>>   {
>>       lockdep_assert_held(&profile_lock);
>>       return !list_empty(&platform_profile_handler_list);
>>   }
>>
>> -static unsigned long platform_profile_get_choices(void)
>> +static bool platform_profile_is_class_device(struct device *dev)
>> +{
>> +    return dev && dev->class == &platform_profile_class;
>> +}
>> +
>> +static unsigned long platform_profile_get_choices(struct device *dev)
>>   {
>>       struct platform_profile_handler *handler;
>>       unsigned long aggregate = 0;
>> @@ -40,6 +51,9 @@ static unsigned long platform_profile_get_choices(void)
>>       list_for_each_entry(handler, &platform_profile_handler_list, 
>> list) {
>>           unsigned long individual = 0;
>>
>> +        /* if called from a class attribute then only match that one */
>> +        if (platform_profile_is_class_device(dev) && handler->dev != 
>> dev->parent)
>> +            continue;
> 
> I do not like how the sysfs attributes for the platform-profile class 
> are handled:
> 
> 1. We should use .dev_groups instead of manually registering the sysfs 
> attributes.
> 2. Can we name the sysfs attributes for the class a bit differently 
> ("profile_choices" and "profile")
>     and use separate store/show functions for those?

Sure.

> 3. Why do we still need platform_profile_handler_list?

The main reason for the list is for iteration and checking if it's empty.
I guess class_for_each_device() could serve the same purpose, but this 
patch probably needs to be way earlier in the series then.

> 
> This would allow us to get rid of platform_profile_is_class_device().
> 
>>           for_each_set_bit(i, handler->choices, PLATFORM_PROFILE_LAST)
>>               individual |= BIT(i);
>>           if (!aggregate)
>> @@ -51,7 +65,7 @@ static unsigned long platform_profile_get_choices(void)
>>       return aggregate;
>>   }
>>
>> -static int platform_profile_get_active(enum platform_profile_option 
>> *profile)
>> +static int platform_profile_get_active(struct device *dev, enum 
>> platform_profile_option *profile)
>>   {
>>       struct platform_profile_handler *handler;
>>       enum platform_profile_option active = PLATFORM_PROFILE_LAST;
>> @@ -60,6 +74,8 @@ static int platform_profile_get_active(enum 
>> platform_profile_option *profile)
>>
>>       lockdep_assert_held(&profile_lock);
>>       list_for_each_entry(handler, &platform_profile_handler_list, 
>> list) {
>> +        if (platform_profile_is_class_device(dev) && handler->dev != 
>> dev->parent)
>> +            continue;
>>           err = handler->profile_get(handler, &val);
>>           if (err) {
>>               pr_err("Failed to get profile for handler %s\n", 
>> handler->name);
>> @@ -69,6 +85,10 @@ static int platform_profile_get_active(enum 
>> platform_profile_option *profile)
>>           if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
>>               return -EINVAL;
>>
>> +        /*
>> +         * If the profiles are different for class devices then this 
>> must
>> +         * show "custom" to legacy sysfs interface
>> +         */
>>           if (active != val && active != PLATFORM_PROFILE_LAST) {
>>               *profile = PLATFORM_PROFILE_CUSTOM;
>>               return 0;
>> @@ -90,7 +110,7 @@ static ssize_t platform_profile_choices_show(struct 
>> device *dev,
>>       int i;
>>
>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock)
>> -        choices = platform_profile_get_choices();
>> +        choices = platform_profile_get_choices(dev);
>>
>>       for_each_set_bit(i, &choices, PLATFORM_PROFILE_LAST) {
>>           if (len == 0)
>> @@ -113,7 +133,7 @@ static ssize_t platform_profile_show(struct device 
>> *dev,
>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>           if (!platform_profile_is_registered())
>>               return -ENODEV;
>> -        err = platform_profile_get_active(&profile);
>> +        err = platform_profile_get_active(dev, &profile);
>>           if (err)
>>               return err;
>>       }
>> @@ -138,12 +158,22 @@ static ssize_t platform_profile_store(struct 
>> device *dev,
>>           if (!platform_profile_is_registered())
>>               return -ENODEV;
>>
>> -        /* Check that all handlers support this profile choice */
>> -        choices = platform_profile_get_choices();
>> +        /* don't allow setting custom to legacy sysfs interface */
>> +        if (!platform_profile_is_class_device(dev) &&
>> +             i == PLATFORM_PROFILE_CUSTOM) {
>> +            pr_warn("Custom profile not supported for legacy sysfs 
>> interface\n");
>> +            return -EINVAL;
>> +        }
>> +
>> +        /* Check that applicable handlers support this profile choice */
>> +        choices = platform_profile_get_choices(dev);
>>           if (!test_bit(i, &choices))
>>               return -EOPNOTSUPP;
>>
>>           list_for_each_entry(handler, &platform_profile_handler_list, 
>> list) {
>> +            if (platform_profile_is_class_device(dev) &&
>> +                handler->dev != dev->parent)
>> +                continue;
>>               err = handler->profile_set(handler, i);
>>               if (err) {
>>                   pr_err("Failed to set profile for handler %s\n", 
>> handler->name);
>> @@ -152,6 +182,9 @@ static ssize_t platform_profile_store(struct 
>> device *dev,
>>           }
>>           if (err) {
>>               list_for_each_entry_continue_reverse(handler, 
>> &platform_profile_handler_list, list) {
>> +                if (platform_profile_is_class_device(dev) &&
>> +                    handler->dev != dev->parent)
>> +                    continue;
>>                   if (handler->profile_set(handler, 
>> PLATFORM_PROFILE_BALANCED))
>>                       pr_err("Failed to revert profile for handler %s\n",
>>                              handler->name);
>> @@ -194,11 +227,11 @@ int platform_profile_cycle(void)
>>       int err;
>>
>>       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>> -        err = platform_profile_get_active(&profile);
>> +        err = platform_profile_get_active(NULL, &profile);
>>           if (err)
>>               return err;
>>
>> -        choices = platform_profile_get_choices();
>> +        choices = platform_profile_get_choices(NULL);
>>
>>           next = find_next_bit_wrap(&choices,
>>                         PLATFORM_PROFILE_LAST,
>> @@ -228,6 +261,7 @@ EXPORT_SYMBOL_GPL(platform_profile_cycle);
>>
>>   int platform_profile_register(struct platform_profile_handler *pprof)
>>   {
>> +    bool registered;
>>       int err;
>>
>>       /* Sanity check the profile handler */
>> @@ -250,14 +284,49 @@ int platform_profile_register(struct 
>> platform_profile_handler *pprof)
>>       if (cur_profile)
>>           return -EEXIST;
>>
>> -    err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>> +    registered = platform_profile_is_registered();
>> +    if (!registered) {
>> +        /* class for individual handlers */
>> +        err = class_register(&platform_profile_class);
>> +        if (err)
>> +            return err;
> 
> Why do we need to unregister the class here? From my point of view, 
> having a empty class if no
> platform profiles are registered is totally fine.

Hmm, OK.

> 
>> +        /* legacy sysfs files */
>> +        err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>> +        if (err)
>> +            goto cleanup_class;
>> +
>> +    }
>> +
>> +    /* create class interface for individual handler */
>> +    pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 
>> 0, GFP_KERNEL);
>> +    pprof->class_dev = device_create(&platform_profile_class, pprof- 
>> >dev,
>> +                     MKDEV(0, pprof->minor), NULL, "platform-profile- 
>> %s",
>> +                     pprof->name);
> 
> I would suggest that the name of the class devices should not contain 
> the platform profile name,
> as this would mean that two platform profile handlers cannot have the 
> same name.
> 
> Maybe using "platform-profile-<minor>" would be a better solution here? 
> The name can instead be
> read using an additional sysfs property.

Sure makes sense.

> 
> Thanks,
> Armin Wolf
> 
>> +    if (IS_ERR(pprof->class_dev)) {
>> +        err = PTR_ERR(pprof->class_dev);
>> +        goto cleanup_legacy;
>> +    }
>> +    err = sysfs_create_group(&pprof->class_dev->kobj, 
>> &platform_profile_group);
>>       if (err)
>> -        return err;
>> +        goto cleanup_device;
>> +
>>       list_add_tail(&pprof->list, &platform_profile_handler_list);
>>       sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>
>>       cur_profile = pprof;
>>       return 0;
>> +
>> +cleanup_device:
>> +    device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
>> +
>> +cleanup_legacy:
>> +    if (!registered)
>> +        sysfs_remove_group(acpi_kobj, &platform_profile_group);
>> +cleanup_class:
>> +    if (!registered)
>> +        class_unregister(&platform_profile_class);
>> +
>> +    return err;
>>   }
>>   EXPORT_SYMBOL_GPL(platform_profile_register);
>>
>> @@ -270,6 +339,10 @@ int platform_profile_remove(struct 
>> platform_profile_handler *pprof)
>>       cur_profile = NULL;
>>
>>       sysfs_notify(acpi_kobj, NULL, "platform_profile");
>> +
>> +    sysfs_remove_group(&pprof->class_dev->kobj, 
>> &platform_profile_group);
>> +    device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
>> +
>>       if (!platform_profile_is_registered())
>>           sysfs_remove_group(acpi_kobj, &platform_profile_group);
>>
>> diff --git a/include/linux/platform_profile.h b/include/linux/ 
>> platform_profile.h
>> index da009c8a402c9..764c4812ef759 100644
>> --- a/include/linux/platform_profile.h
>> +++ b/include/linux/platform_profile.h
>> @@ -30,6 +30,8 @@ enum platform_profile_option {
>>   struct platform_profile_handler {
>>       const char *name;
>>       struct device *dev;
>> +    struct device *class_dev;
>> +    int minor;
>>       unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>>       struct list_head list;
>>       int (*profile_get)(struct platform_profile_handler *pprof,