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

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

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

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

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,

Am 31.10.24 um 22:54 schrieb Mario Limonciello:

> 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.
>
Maybe we can introduce the class earlier. Basically we could:

1. Extend the public API.
2. Introduce the class infrastructure (but still block multiple handlers).
3. Introduce the ability to register multiple handlers.

This would allow for relying more on the class infrastructure for things like
device iteration, etc.

Thanks,
Armin Wolf

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

Hi Mario,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.12-rc5 next-20241101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/ACPI-platform-profile-Add-a-name-member-to-handlers/20241031-121650
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20241031040952.109057-21-mario.limonciello%40amd.com
patch subject: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers
config: i386-buildonly-randconfig-005-20241101 (https://download.01.org/0day-ci/archive/20241101/202411012227.46a4WcxB-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241101/202411012227.46a4WcxB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411012227.46a4WcxB-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/acpi/platform_profile.c:303:7: error: call to undeclared function 'MKDEV'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     303 |                                          MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
         |                                          ^
   drivers/acpi/platform_profile.c:344:42: error: call to undeclared function 'MKDEV'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     344 |         device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
         |                                                 ^
   2 errors generated.


vim +/MKDEV +303 drivers/acpi/platform_profile.c

   261	
   262	int platform_profile_register(struct platform_profile_handler *pprof)
   263	{
   264		bool registered;
   265		int err;
   266	
   267		/* Sanity check the profile handler */
   268		if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
   269		    !pprof->profile_set || !pprof->profile_get) {
   270			pr_err("platform_profile: handler is invalid\n");
   271			return -EINVAL;
   272		}
   273		if (!test_bit(PLATFORM_PROFILE_BALANCED, pprof->choices)) {
   274			pr_err("platform_profile: handler does not support balanced profile\n");
   275			return -EINVAL;
   276		}
   277		if (!pprof->dev) {
   278			pr_err("platform_profile: handler device is not set\n");
   279			return -EINVAL;
   280		}
   281	
   282		guard(mutex)(&profile_lock);
   283		/* We can only have one active profile */
   284		if (cur_profile)
   285			return -EEXIST;
   286	
   287		registered = platform_profile_is_registered();
   288		if (!registered) {
   289			/* class for individual handlers */
   290			err = class_register(&platform_profile_class);
   291			if (err)
   292				return err;
   293			/* legacy sysfs files */
   294			err = sysfs_create_group(acpi_kobj, &platform_profile_group);
   295			if (err)
   296				goto cleanup_class;
   297	
   298		}
   299	
   300		/* create class interface for individual handler */
   301		pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 0, GFP_KERNEL);
   302		pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
 > 303						 MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
   304						 pprof->name);
   305		if (IS_ERR(pprof->class_dev)) {
   306			err = PTR_ERR(pprof->class_dev);
   307			goto cleanup_legacy;
   308		}
   309		err = sysfs_create_group(&pprof->class_dev->kobj, &platform_profile_group);
   310		if (err)
   311			goto cleanup_device;
   312	
   313		list_add_tail(&pprof->list, &platform_profile_handler_list);
   314		sysfs_notify(acpi_kobj, NULL, "platform_profile");
   315	
   316		cur_profile = pprof;
   317		return 0;
   318	
   319	cleanup_device:
   320		device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
   321	
   322	cleanup_legacy:
   323		if (!registered)
   324			sysfs_remove_group(acpi_kobj, &platform_profile_group);
   325	cleanup_class:
   326		if (!registered)
   327			class_unregister(&platform_profile_class);
   328	
   329		return err;
   330	}
   331	EXPORT_SYMBOL_GPL(platform_profile_register);
   332

Hi Mario,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v6.12-rc5 next-20241101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/ACPI-platform-profile-Add-a-name-member-to-handlers/20241031-121650
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20241031040952.109057-21-mario.limonciello%40amd.com
patch subject: [PATCH v3 20/22] ACPI: platform_profile: Register class device for platform profile handlers
config: x86_64-buildonly-randconfig-005-20241101 (https://download.01.org/0day-ci/archive/20241101/202411012317.1pQLOspC-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241101/202411012317.1pQLOspC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411012317.1pQLOspC-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/acpi/platform_profile.c: In function 'platform_profile_register':
>> drivers/acpi/platform_profile.c:303:42: error: implicit declaration of function 'MKDEV' [-Werror=implicit-function-declaration]
     303 |                                          MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
         |                                          ^~~~~
   cc1: some warnings being treated as errors


vim +/MKDEV +303 drivers/acpi/platform_profile.c

   261	
   262	int platform_profile_register(struct platform_profile_handler *pprof)
   263	{
   264		bool registered;
   265		int err;
   266	
   267		/* Sanity check the profile handler */
   268		if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
   269		    !pprof->profile_set || !pprof->profile_get) {
   270			pr_err("platform_profile: handler is invalid\n");
   271			return -EINVAL;
   272		}
   273		if (!test_bit(PLATFORM_PROFILE_BALANCED, pprof->choices)) {
   274			pr_err("platform_profile: handler does not support balanced profile\n");
   275			return -EINVAL;
   276		}
   277		if (!pprof->dev) {
   278			pr_err("platform_profile: handler device is not set\n");
   279			return -EINVAL;
   280		}
   281	
   282		guard(mutex)(&profile_lock);
   283		/* We can only have one active profile */
   284		if (cur_profile)
   285			return -EEXIST;
   286	
   287		registered = platform_profile_is_registered();
   288		if (!registered) {
   289			/* class for individual handlers */
   290			err = class_register(&platform_profile_class);
   291			if (err)
   292				return err;
   293			/* legacy sysfs files */
   294			err = sysfs_create_group(acpi_kobj, &platform_profile_group);
   295			if (err)
   296				goto cleanup_class;
   297	
   298		}
   299	
   300		/* create class interface for individual handler */
   301		pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 0, GFP_KERNEL);
   302		pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
 > 303						 MKDEV(0, pprof->minor), NULL, "platform-profile-%s",
   304						 pprof->name);
   305		if (IS_ERR(pprof->class_dev)) {
   306			err = PTR_ERR(pprof->class_dev);
   307			goto cleanup_legacy;
   308		}
   309		err = sysfs_create_group(&pprof->class_dev->kobj, &platform_profile_group);
   310		if (err)
   311			goto cleanup_device;
   312	
   313		list_add_tail(&pprof->list, &platform_profile_handler_list);
   314		sysfs_notify(acpi_kobj, NULL, "platform_profile");
   315	
   316		cur_profile = pprof;
   317		return 0;
   318	
   319	cleanup_device:
   320		device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));
   321	
   322	cleanup_legacy:
   323		if (!registered)
   324			sysfs_remove_group(acpi_kobj, &platform_profile_group);
   325	cleanup_class:
   326		if (!registered)
   327			class_unregister(&platform_profile_class);
   328	
   329		return err;
   330	}
   331	EXPORT_SYMBOL_GPL(platform_profile_register);
   332

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

Am 02.11.24 um 03:13 schrieb Mark Pearson:

>
> 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@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
>
I agree the different handlers should have different names, but including the name inside the device name will:

- make users depend on the device name, which usually causes problems later
- cause a WARN() (if i remember correctly) should two handlers accidentally have the same name

Especially the last point can happen sooner than you think, for example if the same driver is instantiated twice.

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,