Message-ID: <89fe1a59-506b-46c5-be5f-5440ff405abc@gmx.de>
Date: Fri, 1 Nov 2024 02:54:58 +0100
From: Armin Wolf <W_Armin@....de>
To: Mario Limonciello <mario.limonciello@....com>,
 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

Am 31.10.24 um 22:54 schrieb Mario Limonciello:

> 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.
>
Maybe we can introduce the class earlier. Basically we could:

1. Extend the public API.
2. Introduce the class infrastructure (but still block multiple handlers).
3. Introduce the ability to register multiple handlers.

This would allow for relying more on the class infrastructure for things like
device iteration, etc.

Thanks,
Armin Wolf

>>
>> 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