Re: [PATCH 7/8] ACPI: platform_profile: Add support for multiple handlers

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/28/2024 11:51, Armin Wolf wrote:
> Am 28.10.24 um 15:10 schrieb Mario Limonciello:
> 
>> On 10/28/2024 06:01, Mark Pearson wrote:
>>> Hi Mario,
>>>
>>> On Fri, Oct 25, 2024, at 3:30 PM, Mario Limonciello wrote:
>>>> Multiple drivers may attempt to register platform profile handlers,
>>>> but only one may be registered and the behavior is non-deterministic
>>>> for which one wins.  It's mostly controlled by probing order.
>>>>
>>>> This can be problematic if one driver changes CPU settings and another
>>>> driver notifies the EC for changing fan curves.
>>>>
>>>> Modify the ACPI platform profile handler to let multiple drivers
>>>> register platform profile handlers and abstract this detail from 
>>>> userspace.
>>>>
>>>>  From userspace perspective the user will see profiles available across
>>>> both drivers.  However to avoid chaos only allow changing to profiles
>>>> that are common in both drivers.
>>>>
>>>> If any problems occur when changing profiles for any driver, then 
>>>> revert
>>>> back to the previous profile.
>>>>
>>>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>   drivers/acpi/platform_profile.c | 203 +++++++++++++++++ 
>>>> +--------------
>>>>   1 file changed, 117 insertions(+), 86 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/ 
>>>> platform_profile.c
>>>> index 091ca6941a925..915e3c49f0b5f 100644
>>>> --- a/drivers/acpi/platform_profile.c
>>>> +++ b/drivers/acpi/platform_profile.c
>>>> @@ -9,7 +9,6 @@
>>>>   #include <linux/platform_profile.h>
>>>>   #include <linux/sysfs.h>
>>>>
>>>> -static struct platform_profile_handler *cur_profile;
>>>>   static LIST_HEAD(platform_profile_handler_list);
>>>>   static DEFINE_MUTEX(profile_lock);
>>>>
>>>> @@ -36,26 +35,26 @@ static ssize_t platform_profile_choices_show(struct
>>>> device *dev,
>>>>                       struct device_attribute *attr,
>>>>                       char *buf)
>>>>   {
>>>> +    struct platform_profile_handler *handler;
>>>> +    unsigned long seen = 0;
>>>>       int len = 0;
>>>> -    int err, i;
>>>> -
>>>> -    err = mutex_lock_interruptible(&profile_lock);
>>>> -    if (err)
>>>> -        return err;
>>>> -
>>>> -    if (!cur_profile) {
>>>> -        mutex_unlock(&profile_lock);
>>>> -        return -ENODEV;
>>>> +    int i;
>>>> +
>>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS, 
>>>> &profile_lock) {
>>>> +        list_for_each_entry(handler, 
>>>> &platform_profile_handler_list, list) {
>>>> +            for_each_set_bit(i, handler->choices, 
>>>> PLATFORM_PROFILE_LAST) {
>>>> +                if (seen & BIT(i))
>>>> +                    continue;
>>>> +                if (len == 0)
>>>> +                    len += sysfs_emit_at(buf, len, "%s", 
>>>> profile_names[i]);
>>>> +                else
>>>> +                    len += sysfs_emit_at(buf, len, " %s", 
>>>> profile_names[i]);
>>>> +                seen |= BIT(i);
>>>> +            }
>>>> +        }
>>>>       }
>>>>
>>>> -    for_each_set_bit(i, cur_profile->choices, PLATFORM_PROFILE_LAST) {
>>>> -        if (len == 0)
>>>> -            len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
>>>> -        else
>>>> -            len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
>>>> -    }
>>>>       len += sysfs_emit_at(buf, len, "\n");
>>>> -    mutex_unlock(&profile_lock);
>>>>       return len;
>>>>   }
>>>>
>>>> @@ -64,22 +63,20 @@ static ssize_t platform_profile_show(struct 
>>>> device *dev,
>>>>                       char *buf)
>>>>   {
>>>>       enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
>>>> +    struct platform_profile_handler *handler;
>>>>       int err;
>>>>
>>>> -    err = mutex_lock_interruptible(&profile_lock);
>>>> -    if (err)
>>>> -        return err;
>>>>
>>>> -    if (!cur_profile) {
>>>> -        mutex_unlock(&profile_lock);
>>>> -        return -ENODEV;
>>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS, 
>>>> &profile_lock) {
>>>> +        if (!platform_profile_is_registered())
>>>> +            return -ENODEV;
>>>> +        list_for_each_entry(handler, 
>>>> &platform_profile_handler_list, list) {
>>>> +            err = handler->profile_get(handler, &profile);
>>>> +            if (err)
>>>> +                return err;
>>>> +        }
>>>>       }
>>>>
>>>> -    err = cur_profile->profile_get(cur_profile, &profile);
>>>> -    mutex_unlock(&profile_lock);
>>>> -    if (err)
>>>> -        return err;
>>>> -
>>>>       /* Check that profile is valid index */
>>>>       if (WARN_ON((profile < 0) || (profile >= 
>>>> ARRAY_SIZE(profile_names))))
>>>>           return -EIO;
>>>> @@ -91,37 +88,48 @@ static ssize_t platform_profile_store(struct 
>>>> device *dev,
>>>>                   struct device_attribute *attr,
>>>>                   const char *buf, size_t count)
>>>>   {
>>>> +    struct platform_profile_handler *handler;
>>>> +    enum platform_profile_option profile;
>>>>       int err, i;
>>>>
>>>> -    err = mutex_lock_interruptible(&profile_lock);
>>>> -    if (err)
>>>> -        return err;
>>>> -
>>>> -    if (!cur_profile) {
>>>> -        mutex_unlock(&profile_lock);
>>>> -        return -ENODEV;
>>>> -    }
>>>> -
>>>>       /* Scan for a matching profile */
>>>>       i = sysfs_match_string(profile_names, buf);
>>>>       if (i < 0) {
>>>> -        mutex_unlock(&profile_lock);
>>>>           return -EINVAL;
>>>>       }
>>>>
>>>> -    /* Check that platform supports this profile choice */
>>>> -    if (!test_bit(i, cur_profile->choices)) {
>>>> -        mutex_unlock(&profile_lock);
>>>> -        return -EOPNOTSUPP;
>>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS, 
>>>> &profile_lock) {
>>>> +        if (!platform_profile_is_registered())
>>>> +            return -ENODEV;
>>>> +
>>>> +        /* Check that all handlers support this profile choice */
>>>> +        list_for_each_entry(handler, 
>>>> &platform_profile_handler_list, list) {
>>>> +            if (!test_bit(i, handler->choices))
>>>> +                return -EOPNOTSUPP;
>>>> +
>>>> +            /* save the profile so that it can be reverted if 
>>>> necessary */
>>>> +            err = handler->profile_get(handler, &profile);
>>>> +            if (err)
>>>> +                return err;
>>>> +        }
>>>> +
>>>> +        list_for_each_entry(handler, 
>>>> &platform_profile_handler_list, list) {
>>>> +            err = handler->profile_set(handler, i);
>>>> +            if (err) {
>>>> +                pr_err("Failed to set profile for handler %s\n", 
>>>> handler->name);
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +        if (err) {
>>>> +            list_for_each_entry_continue_reverse(handler,
>>>> &platform_profile_handler_list, list) {
>>>> +                if (handler->profile_set(handler, profile))
>>>> +                    pr_err("Failed to revert profile for handler 
>>>> %s\n",
>>>> handler->name);
>>>> +            }
>>>> +            return err;
>>>> +        }
>>>>       }
>>>>
>>>> -    err = cur_profile->profile_set(cur_profile, i);
>>>> -    if (!err)
>>>> -        sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>>> -
>>>> -    mutex_unlock(&profile_lock);
>>>> -    if (err)
>>>> -        return err;
>>>> +    sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>>>       return count;
>>>>   }
>>>>
>>>> @@ -140,7 +148,8 @@ static const struct attribute_group
>>>> platform_profile_group = {
>>>>
>>>>   void platform_profile_notify(void)
>>>>   {
>>>> -    if (!cur_profile)
>>>> +    guard(mutex)(&profile_lock);
>>>> +    if (!platform_profile_is_registered())
>>>>           return;
>>>>       sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>>>   }
>>>> @@ -148,40 +157,65 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>>>>
>>>>   int platform_profile_cycle(void)
>>>>   {
>>>> +    struct platform_profile_handler *handler;
>>>>       enum platform_profile_option profile;
>>>> -    enum platform_profile_option next;
>>>> +    enum platform_profile_option next = PLATFORM_PROFILE_LAST;
>>>> +    enum platform_profile_option next2 = PLATFORM_PROFILE_LAST;
>>>>       int err;
>>>>
>>>> -    err = mutex_lock_interruptible(&profile_lock);
>>>> -    if (err)
>>>> -        return err;
>>>> -
>>>> -    if (!cur_profile) {
>>>> -        mutex_unlock(&profile_lock);
>>>> -        return -ENODEV;
>>>> -    }
>>>> -
>>>> -    err = cur_profile->profile_get(cur_profile, &profile);
>>>> -    if (err) {
>>>> -        mutex_unlock(&profile_lock);
>>>> -        return err;
>>>> -    }
>>>> -
>>>> -    next = find_next_bit_wrap(cur_profile->choices, 
>>>> PLATFORM_PROFILE_LAST,
>>>> -                  profile + 1);
>>>> -
>>>> -    if (WARN_ON(next == PLATFORM_PROFILE_LAST)) {
>>>> -        mutex_unlock(&profile_lock);
>>>> -        return -EINVAL;
>>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS, 
>>>> &profile_lock) {
>>>> +        /* first pass, make sure all handlers agree on the 
>>>> definition of
>>>> "next" profile */
>>>> +        list_for_each_entry(handler, 
>>>> &platform_profile_handler_list, list) {
>>>> +
>>>> +            err = handler->profile_get(handler, &profile);
>>>> +            if (err)
>>>> +                return err;
>>>> +
>>>> +            if (next == PLATFORM_PROFILE_LAST)
>>>> +                next = find_next_bit_wrap(handler->choices,
>>>> +                              PLATFORM_PROFILE_LAST,
>>>> +                              profile + 1);
>>>> +            else
>>>> +                next2 = find_next_bit_wrap(handler->choices,
>>>> +                               PLATFORM_PROFILE_LAST,
>>>> +                               profile + 1);
>>>> +
>>>> +            if (WARN_ON(next == PLATFORM_PROFILE_LAST))
>>>> +                return -EINVAL;
>>>> +
>>>> +            if (next2 == PLATFORM_PROFILE_LAST)
>>>> +                continue;
>>>> +
>>>> +            if (next != next2) {
>>>> +                pr_warn("Next profile to cycle to is ambiguous between
>>>> platform_profile handlers\n");
>>>> +                return -EINVAL;
>>>> +            }
>>>> +            next = next2;
>>>> +        }
>>>> +
>>>> +        /*
>>>> +         * Second pass: apply "next" to each handler
>>>> +         * If any failures occur unwind and revert all back to the 
>>>> original
>>>> profile
>>>> +         */
>>>> +        list_for_each_entry(handler, 
>>>> &platform_profile_handler_list, list) {
>>>> +            err = handler->profile_set(handler, next);
>>>> +            if (err) {
>>>> +                pr_err("Failed to set profile for handler %s\n", 
>>>> handler->name);
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +        if (err) {
>>>> +            list_for_each_entry_continue_reverse(handler,
>>>> &platform_profile_handler_list, list) {
>>>> +                err = handler->profile_set(handler, profile);
>>>> +                if (err)
>>>> +                    pr_err("Failed to revert profile for handler 
>>>> %s\n",
>>>> handler->name);
>>>> +            }
>>>> +        }
>>>>       }
>>>>
>>>> -    err = cur_profile->profile_set(cur_profile, next);
>>>> -    mutex_unlock(&profile_lock);
>>>> -
>>>> -    if (!err)
>>>> -        sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>>> +    sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>>>
>>>> -    return err;
>>>> +    return 0;
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(platform_profile_cycle);
>>>>
>>>> @@ -190,21 +224,19 @@ int platform_profile_register(struct
>>>> platform_profile_handler *pprof)
>>>>       int err;
>>>>
>>>>       guard(mutex)(&profile_lock);
>>>> -    /* We can only have one active profile */
>>>> -    if (cur_profile)
>>>> -        return -EEXIST;
>>>>
>>>>       /* Sanity check the profile handler field are set */
>>>>       if (!pprof || bitmap_empty(pprof->choices, 
>>>> PLATFORM_PROFILE_LAST) ||
>>>>           !pprof->profile_set || !pprof->profile_get)
>>>>           return -EINVAL;
>>>>
>>>> -    err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>>>> -    if (err)
>>>> -        return err;
>>>> +    if (!platform_profile_is_registered()) {
>>>> +        err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>>>> +        if (err)
>>>> +            return err;
>>>> +    }
>>>>       list_add_tail(&pprof->list, &platform_profile_handler_list);
>>>>
>>>> -    cur_profile = pprof;
>>>>       return 0;
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(platform_profile_register);
>>>> @@ -215,7 +247,6 @@ int platform_profile_remove(struct
>>>> platform_profile_handler *pprof)
>>>>
>>>>       list_del(&pprof->list);
>>>>
>>>> -    cur_profile = NULL;
>>>>       if (!platform_profile_is_registered())
>>>>           sysfs_remove_group(acpi_kobj, &platform_profile_group);
>>>>
>>>> -- 
>>>> 2.43.0
>>>
>>> I'm still going thru the code changes - but I'm a bit unsure on the 
>>> implementation itself.
>>
>> FYI, I split it up in v2 to make each chunk and intent behind it more 
>> manageable to review instead of patch 7 being "so" big.
>>
>> V2 covers some of the points below as well based on some feedback from 
>> Hans and Armin.
>>
>>>
>>> I'd expect that one of the advantages of having different profile 
>>> handlers register is that you could support extra & new profiles that 
>>> might be wanted. For example the recent discussion of the AMD handler 
>>> providing better tools to tweak advanced system settings for gaming 
>>> etc. Won't this approach limit that? You'll only be able to have 
>>> common settings.
>>
>> Well that RFC it turns out won't really be scalable because SPS is 
>> done differently in AMD Strix and newer.  I haven't revisited it yet.
>>
>> But yes this approach would conceptually limit that idea because 
>> common settings are all that is presented.
>>
>>>
>>> I find having a common profile and two different handlers a bit 
>>> tricky on how to handle. My concern is it can easily lead to conflict 
>>> in settings.
>>> If two handlers are doing different operations to provide the same 
>>> effect - then neither handler is (probably) providing what they think 
>>> is required. With your CPU vs EC example, the EC will often set CPU 
>>> clock thresholds and the CPU profile handler will be changing that. 
>>> If this is done I think it should be explicit to the user (admittedly 
>>> I'm doing this with my Lenovo hat on - but we certify our platforms 
>>> with our EC profile handler)
>>>
>>> I could see providing two separate handlers. e.g. balanced-A and 
>>> balanced-B (for driver-A and driver-B) and the user maybe choosing 
>>> which one they want (or both - though the user interface for that is 
>>> definitely tricky)
>>> But choosing one option for two different drivers seems confusing and 
>>> with unknown side-effects. I appreciate it's complicated by your 
>>> example wanting to add CPU and EC - I know how much work you've been 
>>> doing on the AMD CPU front which benefits all systems.
>>>
>>
>> Thinking through your comments I guess another way to approach this 
>> would be "per-driver" sysfs knobs.  Here's my thought.
>>
>> 1) /sys/firmware/acpi/platform_profile_choices would contain only 
>> things that are common and if there is something NOT common then also 
>> the string "custom".
>>
>> 2) /sys/firmware/acpi/platform_profile would accept writes for 
>> everything in platform profile choices except "custom".
>>
>> 3) Each driver handler would also export it's own sysfs files to 
>> represent the driver state.
>>
>> 3) If the user changed the main knob at /sys/firmware/acpi/ 
>> platform_profile then it would change all driver handlers.
>>
>> 4) If the user changed sysfs for any driver individually then the main 
>> knob /sys/firmware/acpi/platform_profile would export "custom".
>>
> Sound like a good idea to me. Maybe we can create a "platform-profile" 
> class for the per-driver sysfs interface?
> The legacy platform profile sysfs interface can then use 
> class_for_each_device() when getting/setting the current profiles.
> 
> For handling notifications we can add a notifier similar to the power 
> supply subsystem. The legacy platform profile sysfs interface
> can use this to receive notifications and forward those to the global 
> sysfs attrs.
> 
> In the end old userspace applications can continue to use the legacy 
> platform profile sysfs interface while new applications can
> use the platform-profile class.

OK, to keep sanity and keep it bisectable I think the way I want to do 
this is:
* keep 1-13 as is from the v2 series
* add custom profile and class handling code in
* rebase v2 14 and 15 on top of that.

Any opposition?

> 
> Thanks,
> Armin Wolf
> 
>> Hans what do you think?
>>
>>> Another concern - would this mean that another driver could limit the 
>>> options available? For instance if someone wrote a new 'mega-turbo' 
>>> only profile driver and it loaded - it would then mean no profiles 
>>> were available for anything as no profiles matched?
>>
>> Yes.  I don't think it's a problem in practice right now (as we only 
>> just recently have two drivers vying for this position), but it / 
>> could/ be something that happens.
>>
>>>
>>> Let me know if I've misunderstood the architecture. I didn't fully 
>>> get how the ASUS and Framework platforms were impacted in the intro 
>>> I'm afraid.
>>>
>>> Thanks!
>>> Mark
>>
>> Framework isn't affected, it was just showing that there are platforms 
>> that use the BIOS/EC notification concept and not just SPS values that 
>> the driver programs so it can't "go away" to solve this issue.
>>
>> ASUS is the only thing affected right now.
>>