| Message ID | 20241119171739.77028-14-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for binding ACPI platform profile to multiple drivers | expand |
On Tue, 19 Nov 2024, Mario Limonciello wrote: > 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> > --- > v7: > * Remove extra handler set > * Remove err variable > v6: > * Fix return > 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 | 100 ++++++++++++++++++++++++++++++++ > 1 file changed, 100 insertions(+) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index 9d6ead043994c..1530e6096cd39 100644 > --- a/drivers/acpi/platform_profile.c > +++ b/drivers/acpi/platform_profile.c > @@ -46,6 +46,52 @@ static ssize_t _commmon_choices_show(unsigned long *choices, char *buf) > return len; > } > > +/** > + * _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; > + int *i = (int *)data; I don't like using "i" as the variable name for this. > + lockdep_assert_held(&profile_lock); > + handler = dev_get_drvdata(dev); > + if (!test_bit(*i, handler->choices)) > + return -EOPNOTSUPP; > + > + return handler->profile_set(handler, *i); > +} > + > +/** > + * 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 > @@ -77,12 +123,66 @@ static ssize_t choices_show(struct device *dev, > return _commmon_choices_show(handler->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]); > +} > + > +/** > + * 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); Please consider if there's a better name for the variable. > + 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, I started to wonder if "choices" is good name for the other attribute as it is the set of values "profile" accepts? Should they be bound by the naming too like "profile_choices" or something along those lines so the connection between the two is very evident? > NULL > }; > ATTRIBUTE_GROUPS(profile); >
I don't know why you dropped Mario and the list, I reinstanstated those two. On Wed, 20 Nov 2024, Mark Pearson wrote: > On Wed, Nov 20, 2024, at 9:56 AM, Ilpo Järvinen wrote: > > On Tue, 19 Nov 2024, Mario Limonciello wrote: > > > >> 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> > >> --- > >> v7: > >> * Remove extra handler set > >> * Remove err variable > >> v6: > >> * Fix return > >> 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 | 100 ++++++++++++++++++++++++++++++++ > >> 1 file changed, 100 insertions(+) > >> > >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > >> index 9d6ead043994c..1530e6096cd39 100644 > >> --- a/drivers/acpi/platform_profile.c > >> +++ b/drivers/acpi/platform_profile.c > >> static struct attribute *profile_attrs[] = { > >> &dev_attr_name.attr, > >> &dev_attr_choices.attr, > >> + &dev_attr_profile.attr, > > > > I started to wonder if "choices" is good name for the other attribute as > > it is the set of values "profile" accepts? Should they be bound by the > > naming too like "profile_choices" or something along those lines so the > > connection between the two is very evident? > > > Wouldn't it be weird to not have it in sync with the main sysfs entry > (which I don't think we can change at that point without messing up > userspace). > > I think it would be more confusing to have different naming as it would > imply they're different things. Ah, I didn't realize there's a pre-existing convention. Then just disregard what I suggested.
On Thu, Nov 21, 2024, at 6:10 AM, Ilpo Järvinen wrote: > I don't know why you dropped Mario and the list, I reinstanstated those > two. Unintentional - sorry. Hit the reply button instead of reply-all and didn't notice. Doh. > > On Wed, 20 Nov 2024, Mark Pearson wrote: >> On Wed, Nov 20, 2024, at 9:56 AM, Ilpo Järvinen wrote: >> > On Tue, 19 Nov 2024, Mario Limonciello wrote: >> > >> >> 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> >> >> --- >> >> v7: >> >> * Remove extra handler set >> >> * Remove err variable >> >> v6: >> >> * Fix return >> >> 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 | 100 ++++++++++++++++++++++++++++++++ >> >> 1 file changed, 100 insertions(+) >> >> >> >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >> >> index 9d6ead043994c..1530e6096cd39 100644 >> >> --- a/drivers/acpi/platform_profile.c >> >> +++ b/drivers/acpi/platform_profile.c > >> >> static struct attribute *profile_attrs[] = { >> >> &dev_attr_name.attr, >> >> &dev_attr_choices.attr, >> >> + &dev_attr_profile.attr, >> > >> > I started to wonder if "choices" is good name for the other attribute as >> > it is the set of values "profile" accepts? Should they be bound by the >> > naming too like "profile_choices" or something along those lines so the >> > connection between the two is very evident? >> > >> Wouldn't it be weird to not have it in sync with the main sysfs entry >> (which I don't think we can change at that point without messing up >> userspace). >> >> I think it would be more confusing to have different naming as it would >> imply they're different things. > > Ah, I didn't realize there's a pre-existing convention. Then just > disregard what I suggested. > No idea if it's a convention - I just would think it would be confusing for users. Thanks Mark
Am 21.11.24 um 15:27 schrieb Mark Pearson: > On Thu, Nov 21, 2024, at 6:10 AM, Ilpo Järvinen wrote: >> I don't know why you dropped Mario and the list, I reinstanstated those >> two. > Unintentional - sorry. Hit the reply button instead of reply-all and didn't notice. Doh. > >> On Wed, 20 Nov 2024, Mark Pearson wrote: >>> On Wed, Nov 20, 2024, at 9:56 AM, Ilpo Järvinen wrote: >>>> On Tue, 19 Nov 2024, Mario Limonciello wrote: >>>> >>>>> 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> >>>>> --- >>>>> v7: >>>>> * Remove extra handler set >>>>> * Remove err variable >>>>> v6: >>>>> * Fix return >>>>> 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 | 100 ++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 100 insertions(+) >>>>> >>>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >>>>> index 9d6ead043994c..1530e6096cd39 100644 >>>>> --- a/drivers/acpi/platform_profile.c >>>>> +++ b/drivers/acpi/platform_profile.c >>>>> static struct attribute *profile_attrs[] = { >>>>> &dev_attr_name.attr, >>>>> &dev_attr_choices.attr, >>>>> + &dev_attr_profile.attr, >>>> I started to wonder if "choices" is good name for the other attribute as >>>> it is the set of values "profile" accepts? Should they be bound by the >>>> naming too like "profile_choices" or something along those lines so the >>>> connection between the two is very evident? >>>> >>> Wouldn't it be weird to not have it in sync with the main sysfs entry >>> (which I don't think we can change at that point without messing up >>> userspace). >>> >>> I think it would be more confusing to have different naming as it would >>> imply they're different things. >> Ah, I didn't realize there's a pre-existing convention. Then just >> disregard what I suggested. >> > No idea if it's a convention - I just would think it would be confusing for users. > > Thanks > Mark I personally would prefer the attribute name "choices", but i would also accept if the attribute was named "profile_choices". I think adding the "profile" prefix to the attribute when it is already handled by the platform-profile class is silly. Either way: Reviewed-by: Armin Wolf <W_Armin@gmx.de>
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index 9d6ead043994c..1530e6096cd39 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -46,6 +46,52 @@ static ssize_t _commmon_choices_show(unsigned long *choices, char *buf) return len; } +/** + * _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; + int *i = (int *)data; + + lockdep_assert_held(&profile_lock); + handler = dev_get_drvdata(dev); + if (!test_bit(*i, handler->choices)) + return -EOPNOTSUPP; + + return handler->profile_set(handler, *i); +} + +/** + * 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 @@ -77,12 +123,66 @@ static ssize_t choices_show(struct device *dev, return _commmon_choices_show(handler->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]); +} + +/** + * 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);