On Thu, Oct 31, 2024, at 4:55 PM, 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.limoncie...@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?
> 3. Why do we still need platform_profile_handler_list?
>
> 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.
>
>> +            /* 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.
>
> Thanks,
> Armin Wolf
>

I'm still getting my head around this patch (it's late and I'm a bit brain-dead 
this evening) - but isn't it a good thing to force the different profile 
handlers to have different names?

I like the platform-profile-<name> approach, it makes it simpler for users to 
know if they're changing a platform vendors profile, a CPU vendors profile, or 
something else. If profiles have the same name it would become quite confusing.

Mark

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


_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Reply via email to