Am 07.11.24 um 23:05 schrieb Mario Limonciello:
> On 11/7/2024 02:58, Armin Wolf wrote:
>> Am 07.11.24 um 07:02 schrieb Mario Limonciello:
>>
>>> As multiple platform profile handlers might not all support the same
>>> profile, cycling to the next profile could have a different result
>>> depending on what handler are registered.
>>>
>>> Check what is active and supported by all handlers to decide what
>>> to do.
>>>
>>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>> v5:
>>> * Adjust mutex use
>>> ---
>>> drivers/acpi/platform_profile.c | 23 ++++++++++++++---------
>>> 1 file changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/
>>> platform_profile.c
>>> index 7f302ac4d3779..2c466f2d16b42 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -411,34 +411,39 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>>>
>>> int platform_profile_cycle(void)
>>> {
>>> + enum platform_profile_option next = PLATFORM_PROFILE_LAST;
>>> enum platform_profile_option profile;
>>> - enum platform_profile_option next;
>>> + unsigned long choices;
>>> int err;
>>>
>>> if (!class_is_registered(&platform_profile_class))
>>> return -ENODEV;
>>>
>>> scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> - if (!cur_profile)
>>> - return -ENODEV;
>>> + err = class_for_each_device(&platform_profile_class, NULL,
>>> + &profile, _aggregate_profiles);
>>> + if (err)
>>> + return err;
>>>
>>> - err = cur_profile->profile_get(cur_profile, &profile);
>>> + err = class_for_each_device(&platform_profile_class, NULL,
>>> + &choices, _aggregate_choices);
>>> if (err)
>>> return err;
>>>
>>> - next = find_next_bit_wrap(cur_profile->choices,
>>> PLATFORM_PROFILE_LAST,
>>> + next = find_next_bit_wrap(&choices,
>>> + PLATFORM_PROFILE_LAST,
>>> profile + 1);
>>
>> Could it be that this would lead to be "custom" profile being
>> selected under some conditions?
>
> Yeah, you're right. If all drivers supported custom then this could
> happen. I'll clear custom like this:
>
> choices &= ~BIT(PLATFORM_PROFILE_CUSTOM);
>
Sound good to me.
>> Also _aggregate_profiles() expects profile to be initialized with
>> PLATFORM_PROFILE_LAST.
>
> Will correct initialization in platform_profile_cycle() to this.
>
> enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
>
> But this also raises a good point. If _aggregate_profiles() returns
> custom then this should be an error because next profile is undefined.
> So I'll catch that like this.
> err = class_for_each_device()
> if (err)
> return err;
> if (profile == PLATFORM_PROFILE_CUSTOM)
> return -EINVAL;
Good point, please also check if profile == PLATFORM_PROFILE_LAST in case no platform profile handlers are currently installed.
Thanks,
Armin Wol
>>
>> Thanks,
>> Armin Wolf
>>
>>>
>>> - if (WARN_ON(next == PLATFORM_PROFILE_LAST))
>>> - return -EINVAL;
>>> + err = class_for_each_device(&platform_profile_class, NULL,
>>> &next,
>>> + _store_class_profile);
>>>
>>> - err = cur_profile->profile_set(cur_profile, next);
>>> if (err)
>>> return err;
>>> }
>>>
>>> sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>> - return 0;
>>> +
>>> + return err;
>>> }
>>> EXPORT_SYMBOL_GPL(platform_profile_cycle);
>>>
>
>