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

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

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,