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

[Armin Wolf <W_Armin@gmx.de>]

Am 07.11.24 um 22:41 schrieb Mario Limonciello:

> On 11/7/2024 02:34, Armin Wolf wrote:
>> Am 07.11.24 um 07:02 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.
>>>
>>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>> v5:
>>>   * Drop recovery flow
>>>   * Don't get profile before setting (not needed)
>>>   * Simplify casting for call to _store_class_profile()
>>>   * Only notify legacy interface of changes
>>>   * Adjust mutex use
>>> ---
>>>   drivers/acpi/platform_profile.c | 110
>>> ++++++++++++++++++++++++++++++++
>>>   1 file changed, 110 insertions(+)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/
>>> platform_profile.c
>>> index 5e0bb91c5f451..35e0e8f666072 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -65,6 +65,62 @@ 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)
>>> +{
>>> +    struct platform_profile_handler *handler;
>>> +    unsigned long choices;
>>> +    int *i = (int *)data;
>>> +    int err;
>>> +
>>> +    err = _get_class_choices(dev, &choices);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    lockdep_assert_held(&profile_lock);
>>> +    if (!test_bit(*i, &choices))
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    handler = dev_get_drvdata(dev);
>>> +    err = handler->profile_set(handler, *i);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    return err ? err : 0;
>>
>> Please just return 0 here.
>>
>>> +}
>>> +
>>> +/**
>>> + * 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;
>>> +
>>> +    lockdep_assert_held(&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
>>>    * @dev: The device
>>> @@ -106,12 +162,66 @@ 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;
>>> +
>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> +        err = get_class_profile(dev, &profile);
>>> +        if (err)
>>> +            return err;
>>> +    }
>>> +
>>> +    return sysfs_emit(buf, "%s\n", profile_names[profile]);
>>
>> AFAIK we do not need to take the mutex here, since querying the
>> current platform profile
>> should not change any state.
>
> I think it's still needed, in case someone attempts to unload the driver
> at the same time as it's being read.  It's not static information
> because it needs to use the function pointer into the driver to get it.
>
> This will protect from that occurring.
>
> That's the same reason I was thinking name needed protection too.
>
I see, good point.

Thanks,
Armin Wolf

>>
>> Thanks,
>> Armin Wolf
>>
>>> +}
>>> +
>>> +/**
>>> + * 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;
>>> +
>>> +    scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> +        ret = _store_class_profile(dev, &i);
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>> +    sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>> +
>>> +    return 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);
>