| 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: <d9a19f6f-59bc-4ed0-8f9f-8a4fca195d80@amd.com>
Date: Thu, 31 Oct 2024 16:54:51 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Armin Wolf <W_Armin@....de>, 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>,
"open list:ACPI" <linux-acpi@...r.kernel.org>,
"open list:MICROSOFT SURFACE PLATFORM PROFILE DRIVER"
<platform-driver-x86@...r.kernel.org>,
"open list:THINKPAD ACPI EXTRAS DRIVER"
<ibm-acpi-devel@...ts.sourceforge.net>,
Mark Pearson <mpearson-lenovo@...ebb.ca>,
Matthew Schwartz <matthew.schwartz@...ux.dev>
Subject: Re: [PATCH v3 20/22] ACPI: platform_profile: Register class device
for platform profile handlers
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@....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,
Powered by blists - more mailing lists