| Message ID | 20241028020131.8031-13-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add support for binding ACPI platform profile to multiple drivers | expand |
On Sun, 27 Oct 2024, Mario Limonciello wrote: > If for any reason multiple profile handlers don't agree on the profile > set for the system then the value shown in sysfs can be wrong. > > Explicitly check that they match. > > Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/acpi/platform_profile.c | 61 ++++++++++++++++++++++++--------- > 1 file changed, 45 insertions(+), 16 deletions(-) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index db2ebd0393cf7..d22c4eb5f0c36 100644 > --- a/drivers/acpi/platform_profile.c > +++ b/drivers/acpi/platform_profile.c > @@ -51,6 +51,45 @@ static unsigned long platform_profile_get_choices(void) > return seen; > } > > +/* expected to be called under mutex */ Don't add comments like this but enforce it with a lockdep annotation. "mutex" would have been too vague anyway :-). > +static int platform_profile_get_active(enum platform_profile_option *profile) > +{ > + struct platform_profile_handler *handler; > + enum platform_profile_option active = PLATFORM_PROFILE_LAST; > + enum platform_profile_option active2 = PLATFORM_PROFILE_LAST; > + int err; > + > + list_for_each_entry(handler, &platform_profile_handler_list, list) { > + if (active == PLATFORM_PROFILE_LAST) > + err = handler->profile_get(handler, &active); > + else > + err = handler->profile_get(handler, &active2); > + if (err) { > + pr_err("Failed to get profile for handler %s\n", handler->name); > + return err; > + } > + > + if (WARN_ON(active == PLATFORM_PROFILE_LAST)) > + return -EINVAL; > + if (active2 == PLATFORM_PROFILE_LAST) > + continue; > + > + if (active != active2) { > + pr_warn("Profile handlers don't agree on current profile\n"); > + return -EINVAL; > + } > + active = active2; This looked very confusing (IMO). How about this: enum platform_profile_option active = PLATFORM_PROFILE_LAST; enum platform_profile_option val; ... err = handler->profile_get(handler, &val); if (err) { pr_err(...); return err; } if (WARN_ON(val == PLATFORM_PROFILE_LAST)) return -EINVAL; if (active != val && active != PLATFORM_PROFILE_LAST) { pr_warn("Profile handlers don't agree on current profile\n"); return -EINVAL; } active = val; > + } > + > + /* Check that profile is valid index */ > + if (WARN_ON((active < 0) || (active >= ARRAY_SIZE(profile_names)))) What does that < 0 check do? Should it be checked right after reading profile_get()? Or perhaps check both of these right there? > + return -EIO; > + > + *profile = active; > + > + return 0; > +} > + > static ssize_t platform_profile_choices_show(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -80,24 +119,14 @@ static ssize_t platform_profile_show(struct device *dev, > enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED; > int err; > > - err = mutex_lock_interruptible(&profile_lock); > - if (err) > - return err; > - > - if (!cur_profile) { > - mutex_unlock(&profile_lock); > - return -ENODEV; > + scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) { scoped_cond_guard() conversion should be made in the guard patch? > + if (!platform_profile_is_registered()) > + return -ENODEV; > + err = platform_profile_get_active(&profile); > + if (err) > + return err; > } > > - err = cur_profile->profile_get(cur_profile, &profile); > - mutex_unlock(&profile_lock); > - if (err) > - return err; > - > - /* Check that profile is valid index */ > - if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) > - return -EIO; > - > return sysfs_emit(buf, "%s\n", profile_names[profile]); > } > >
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index db2ebd0393cf7..d22c4eb5f0c36 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -51,6 +51,45 @@ static unsigned long platform_profile_get_choices(void) return seen; } +/* expected to be called under mutex */ +static int platform_profile_get_active(enum platform_profile_option *profile) +{ + struct platform_profile_handler *handler; + enum platform_profile_option active = PLATFORM_PROFILE_LAST; + enum platform_profile_option active2 = PLATFORM_PROFILE_LAST; + int err; + + list_for_each_entry(handler, &platform_profile_handler_list, list) { + if (active == PLATFORM_PROFILE_LAST) + err = handler->profile_get(handler, &active); + else + err = handler->profile_get(handler, &active2); + if (err) { + pr_err("Failed to get profile for handler %s\n", handler->name); + return err; + } + + if (WARN_ON(active == PLATFORM_PROFILE_LAST)) + return -EINVAL; + if (active2 == PLATFORM_PROFILE_LAST) + continue; + + if (active != active2) { + pr_warn("Profile handlers don't agree on current profile\n"); + return -EINVAL; + } + active = active2; + } + + /* Check that profile is valid index */ + if (WARN_ON((active < 0) || (active >= ARRAY_SIZE(profile_names)))) + return -EIO; + + *profile = active; + + return 0; +} + static ssize_t platform_profile_choices_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -80,24 +119,14 @@ static ssize_t platform_profile_show(struct device *dev, enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED; int err; - err = mutex_lock_interruptible(&profile_lock); - if (err) - return err; - - if (!cur_profile) { - mutex_unlock(&profile_lock); - return -ENODEV; + scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) { + if (!platform_profile_is_registered()) + return -ENODEV; + err = platform_profile_get_active(&profile); + if (err) + return err; } - err = cur_profile->profile_get(cur_profile, &profile); - mutex_unlock(&profile_lock); - if (err) - return err; - - /* Check that profile is valid index */ - if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) - return -EIO; - return sysfs_emit(buf, "%s\n", profile_names[profile]); }