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.limonciello@xxxxxxx>
>> ---
>> 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,
