Re: [PATCH v4 12/20] ACPI: platform_profile: Add profile attribute for class interface

[Armin Wolf <W_Armin@gmx.de>]

Am 06.11.24 um 06:46 schrieb Mario Limonciello:

>
>
> On 11/5/24 22:10, Armin Wolf wrote:
>> Am 05.11.24 um 16:33 schrieb Mario Limonciello:
>>
>>> Reading and writing the `profile` sysfs file will use the callbacks for
>>> the platform profile handler to read or set the given profile.
>>>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>   drivers/acpi/platform_profile.c | 118
>>> ++++++++++++++++++++++++++++++++
>>>   1 file changed, 118 insertions(+)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/
>>> platform_profile.c
>>> index e1b6569c4ee70..79083d0bb22e3 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -65,6 +65,78 @@ static int _get_class_choices(struct device *dev,
>>> unsigned long *choices)
>>>       return 0;
>>>   }
>>>
>>> +/**
>>> + * _store_class_profile - Set the profile for a class device
>>> + * @dev: The class device
>>> + * @data: The profile to set
>>> + */
>>> +static int _store_class_profile(struct device *dev, void *data)
>>> +{
>>> +    enum platform_profile_option profile;
>>> +    unsigned long choices;
>>> +    int *i = (int *)data;
>>> +    int err;
>>> +
>>> +    err = _get_class_choices(dev, &choices);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> +        struct platform_profile_handler *handler;
>>> +
>>> +        if (!test_bit(*i, &choices))
>>> +            return -EOPNOTSUPP;
>>> +
>>> +        handler = dev_get_drvdata(dev);
>>> +        err = handler->profile_get(handler, &profile);
>>> +        if (err)
>>> +            return err;
>>> +
>>> +        err = handler->profile_set(handler, *i);
>>> +        if (err) {
>>> +            int recover_err;
>>> +
>>> +            dev_err(dev, "Failed to set profile: %d\n", err);
>>> +            recover_err = handler->profile_set(handler, profile);
>>> +            if (recover_err)
>>> +                dev_err(dev, "Failed to reset profile: %d\n",
>>> recover_err);
>>> +        }
>>
>> The whole recovery handling seems unnecessary to me. In setting the
>> platform profile fails, then
>> we should just return an error. The platform profile handler will
>> tell us the current platform
>> profile anyway.
>
> Sure, makes sense.  That also means no need to capture the profile
> before setting it.
>
>>
>>> + sysfs_notify(&handler->class_dev->kobj, NULL, "platform_profile");
>>> +        kobject_uevent(&handler->class_dev->kobj, KOBJ_CHANGE);
>>
>> Please avoid sending those events when the platform profile is
>> changed through the class sysfs interface.
>>
>>> +    }
>>> +
>>> +    sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>
>> Please avoid sending this event when the platform profile is changed
>> through the legacy sysfs interface.
>
> In both above cases - why?
>
> * If I change using class interface then that implicitly means that
> legacy interface changes.
> * If I change using legacy interface that implicitly means class
> interface changes too.
>
I meant that:
- if the profile is changed using the class interface then only the legacy interface should be notified
- if the profile is changed using the legacy interface then only the class interface should be notified

Thanks,
Armin Wolf

>>
>>> +    return err ? err : 0;
>>> +}
>>> +
>>> +/**
>>> + * get_class_profile - Show the current profile for a class device
>>> + * @dev: The class device
>>> + * @profile: The profile to return
>>> + * Return: 0 on success, -errno on failure
>>> + */
>>> +static int get_class_profile(struct device *dev,
>>> +                 enum platform_profile_option *profile)
>>> +{
>>> +    struct platform_profile_handler *handler;
>>> +    enum platform_profile_option val;
>>> +    int err;
>>> +
>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> +        handler = dev_get_drvdata(dev);
>>> +        err = handler->profile_get(handler, &val);
>>> +        if (err) {
>>> +            pr_err("Failed to get profile for handler %s\n",
>>> handler- >name);
>>> +            return err;
>>> +        }
>>> +    }
>>> +
>>> +    if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
>>> +        return -EINVAL;
>>> +    *profile = val;
>>> +
>>> +    return 0;
>>> +}
>>>
>>>   /**
>>>    * name_show - Show the name of the profile handler
>>> @@ -102,12 +174,58 @@ static ssize_t choices_show(struct device *dev,
>>>       return _commmon_choices_show(choices, buf);
>>>   }
>>>
>>> +/**
>>> + * profile_show - Show the current profile for a class device
>>> + * @dev: The device
>>> + * @attr: The attribute
>>> + * @buf: The buffer to write to
>>> + * Return: The number of bytes written
>>> + */
>>> +static ssize_t profile_show(struct device *dev,
>>> +                struct device_attribute *attr,
>>> +                char *buf)
>>> +{
>>> +    enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
>>> +    int err;
>>> +
>>> +    err = get_class_profile(dev, &profile);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    return sysfs_emit(buf, "%s\n", profile_names[profile]);
>>> +}
>>> +
>>> +/**
>>> + * profile_store - Set the profile for a class device
>>> + * @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 profile_store(struct device *dev,
>>> +                 struct device_attribute *attr,
>>> +                 const char *buf, size_t count)
>>> +{
>>> +    int i, ret;
>>> +
>>> +    i = sysfs_match_string(profile_names, buf);
>>> +    if (i < 0)
>>> +        return -EINVAL;
>>> +
>>> +    ret = _store_class_profile(dev, (void *)(long)&i);
>>
>> Please just pass &i.
>
> Ack.
>
>>
>> Thanks,
>> Armin Wolf
>>
>>> +
>>> +    return ret ? ret : count;
>>> +}
>>>
>>>   static DEVICE_ATTR_RO(name);
>>>   static DEVICE_ATTR_RO(choices);
>>> +static DEVICE_ATTR_RW(profile);
>>> +
>>>   static struct attribute *profile_attrs[] = {
>>>       &dev_attr_name.attr,
>>>       &dev_attr_choices.attr,
>>> +    &dev_attr_profile.attr,
>>>       NULL
>>>   };
>>>   ATTRIBUTE_GROUPS(profile);
>>
>