| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c4752d95-9615-4812-bbfa-7730f561517e@amd.com>
Date: Mon, 28 Oct 2024 12:01:46 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Armin Wolf <W_Armin@....de>, Mark Pearson <mpearson-lenovo@...ebb.ca>,
Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
Maximilian Luz <luzmaximilian@...il.com>, Lee Chun-Yi <jlee@...e.com>,
Shyam Sundar S K <Shyam-sundar.S-k@....com>,
Corentin Chary <corentin.chary@...il.com>, "Luke D . Jones"
<luke@...nes.dev>, Ike Panhc <ike.pan@...onical.com>,
Henrique de Moraes Holschuh <hmh@....eng.br>,
Alexis Belmonte <alexbelm48@...il.com>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Ai Chao <aichao@...inos.cn>, Gergo Koteles <soyer@....hu>,
open list <linux-kernel@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>,
"open list:THINKPAD ACPI EXTRAS DRIVER"
<ibm-acpi-devel@...ts.sourceforge.net>,
Matthew Schwartz <matthew.schwartz@...ux.dev>
Subject: Re: [PATCH 7/8] ACPI: platform_profile: Add support for multiple
handlers
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@...ux.dev>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@....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.
>>
Powered by blists - more mailing lists