Re: [PATCH v4 16/20] ACPI: platform_profile: Make sure all profile handlers agree on profile

[Armin Wolf <W_Armin@gmx.de>]

Am 06.11.24 um 22:25 schrieb Mario Limonciello:

> On 11/6/2024 14:58, Armin Wolf wrote:
>> Am 05.11.24 um 16:33 schrieb Mario Limonciello:
>>
>>> If for any reason multiple profile handlers don't agree on the profile
>>> return the custom profile.
>>>
>>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>   drivers/acpi/platform_profile.c | 84
>>> +++++++++++++++++++++------------
>>>   1 file changed, 53 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/
>>> platform_profile.c
>>> index d8c2d195106c2..7861fccc2e58c 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -280,55 +280,77 @@ static ssize_t
>>> platform_profile_choices_show(struct device *dev,
>>>       return _commmon_choices_show(aggregate, buf);
>>>   }
>>>
>>> -static ssize_t platform_profile_show(struct device *dev,
>>> -                    struct device_attribute *attr,
>>> -                    char *buf)
>>> +/**
>>> + * _aggregate_profiles - Aggregate the profiles for legacy sysfs
>>> interface
>>> + * @dev: The device
>>> + * @data: The profile to return
>>> + * Return: 0 on success, -errno on failure
>>> + */
>>> +static int _aggregate_profiles(struct device *dev, void *data)
>>>   {
>>> -    enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
>>> +    enum platform_profile_option *profile = data;
>>> +    enum platform_profile_option val;
>>>       int err;
>>>
>>> -    scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> -        if (!cur_profile)
>>> -            return -ENODEV;
>>> +    err = get_class_profile(dev, &val);
>>> +    if (err)
>>> +        return err;
>>>
>>> -        err = cur_profile->profile_get(cur_profile, &profile);
>>> -        if (err)
>>> -            return err;
>>> -    }
>>> +    if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
>>> +        *profile = PLATFORM_PROFILE_CUSTOM;
>>> +    else
>>> +        *profile = val;
>>>
>>> -    /* Check that profile is valid index */
>>> -    if (WARN_ON((profile < 0) || (profile >=
>>> ARRAY_SIZE(profile_names))))
>>> -        return -EIO;
>>
>> Please check the returned value from profile_get() first before doing
>> the custom platform
>> profile check.
>
> You might have missed it - that's part of get_class_profile() already.
>
You are right, my bad.

>>
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + * platform_profile_show - Show the current profile for legacy
>>> sysfs interface
>>> + * @dev: The device
>>> + * @attr: The attribute
>>> + * @buf: The buffer to write to
>>> + * Return: The number of bytes written
>>> + */
>>> +static ssize_t platform_profile_show(struct device *dev,
>>> +                     struct device_attribute *attr,
>>> +                     char *buf)
>>> +{
>>> +    enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
>>> +    int err;
>>> +
>>> +    err = class_for_each_device(&platform_profile_class, NULL,
>>> +                    &profile, _aggregate_profiles);
>>
>> Missing error handling.
>>
>
> Right, got it.
>
>>>
>>>       return sysfs_emit(buf, "%s\n", profile_names[profile]);
>>>   }
>>>
>>> +/**
>>> + * platform_profile_store - Set the profile for legacy sysfs interface
>>> + * @dev: The device
>>> + * @attr: The attribute
>>> + * @buf: The buffer to read from
>>> + * @count: The number of bytes to read
>>> + * Return: The number of bytes read
>>> + */
>>>   static ssize_t platform_profile_store(struct device *dev,
>>> -                struct device_attribute *attr,
>>> -                const char *buf, size_t count)
>>> +                      struct device_attribute *attr,
>>> +                      const char *buf, size_t count)
>>>   {
>>> -    int err, i;
>>> +    int ret;
>>> +    int i;
>>>
>>>       /* Scan for a matching profile */
>>>       i = sysfs_match_string(profile_names, buf);
>>>       if (i < 0)
>>>           return -EINVAL;
>>> -
>>> -    scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> -        if (!cur_profile)
>>> -            return -ENODEV;
>>> -
>>> -        /* Check that platform supports this profile choice */
>>> -        if (!test_bit(i, cur_profile->choices))
>>> -            return -EOPNOTSUPP;
>>> -
>>> -        err = cur_profile->profile_set(cur_profile, i);
>>> -        if (err)
>>> -            return err;
>>> +    if (i == PLATFORM_PROFILE_CUSTOM) {
>>> +        pr_warn("Custom profile not supported for legacy sysfs
>>> interface\n");
>>
>> This would allow userspace applications to spam the kernel log. Please
>> just return -EINVAL here
>> and document this special case inside the interface specification.
>>
> OK.
>
>> Thanks,
>> Armin Wolf
>>
>>> +        return -EINVAL;
>>>       }
>>>
>>> -    sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>> -    return count;
>>> +    ret = class_for_each_device(&platform_profile_class, NULL, &i,
>>> _store_class_profile);
>>> +
>>> +    return ret ? ret : count;
>>>   }
>>>
>>>   static DEVICE_ATTR_RO(platform_profile_choices);
>
>