| Message ID | 20241105153316.378-17-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add support for binding ACPI platform profile to multiple drivers | expand |
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. > + 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. > > 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. 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);
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. > >> + 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);
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); > >
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; + 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); 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"); + 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);