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

[Mario Limonciello <mario.limonciello@amd.com>]

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.

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