| Message ID | 20241028020131.8031-12-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for binding ACPI platform profile to multiple drivers | expand |
On Tue, 29 Oct 2024, Mario Limonciello wrote: > On 10/29/2024 05:22, Ilpo Järvinen wrote: > > On Sun, 27 Oct 2024, Mario Limonciello wrote: > > > > > If multiple platform profile handlers have been registered then when > > > setting a profile verify that all profile handlers support the requested > > > profile and set it to each handler. > > > > > > If this fails for any given handler, revert all profile handlers back to > > > balanced and log an error into the kernel ring buffer. > > > > > > Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev> > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > > --- > > > drivers/acpi/platform_profile.c | 47 ++++++++++++++++++--------------- > > > 1 file changed, 26 insertions(+), 21 deletions(-) > > > > > > diff --git a/drivers/acpi/platform_profile.c > > > b/drivers/acpi/platform_profile.c > > > index a83842f05022b..db2ebd0393cf7 100644 > > > --- a/drivers/acpi/platform_profile.c > > > +++ b/drivers/acpi/platform_profile.c > > > @@ -105,37 +105,42 @@ static ssize_t platform_profile_store(struct device > > > *dev, > > > struct device_attribute *attr, > > > const char *buf, size_t count) > > > { > > > + struct platform_profile_handler *handler; > > > + unsigned long choices; > > > int err, i; > > > - err = mutex_lock_interruptible(&profile_lock); > > > - if (err) > > > - return err; > > > - > > > - if (!cur_profile) { > > > - mutex_unlock(&profile_lock); > > > - return -ENODEV; > > > - } > > > - > > > /* Scan for a matching profile */ > > > i = sysfs_match_string(profile_names, buf); > > > if (i < 0) { > > > - mutex_unlock(&profile_lock); > > > return -EINVAL; > > > } > > > - /* Check that platform supports this profile choice */ > > > - if (!test_bit(i, cur_profile->choices)) { > > > - mutex_unlock(&profile_lock); > > > - return -EOPNOTSUPP; > > > - } > > > + scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) { > > > > You made guard() conversions in the earlier patch but for some reason > > left scoped_cond_guard() ones mixed into other changes still. Is there > > a very good reason for that? > > > > Using scoped_cond_guard() requires changing the indentation which meant a bit > of back and forth with code coming and going. If you think it makes more > sense to split up even considering the indentation changes I'll do another set > of patches for the scoped_cond_guard changes only. There are ways to combat indentation changes while reviewing. However, it's a strange argument to bring up because now there are indentation changes in these patches exactly because you chose to make the scoped_cond_guard() change "while at it" rather than in a separate patch. I believe the patches will become cleaner and easier to review if you do scoped_cond_guard() change separate from any other logic changes.
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index a83842f05022b..db2ebd0393cf7 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -105,37 +105,42 @@ static ssize_t platform_profile_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { + struct platform_profile_handler *handler; + unsigned long choices; int err, i; - err = mutex_lock_interruptible(&profile_lock); - if (err) - return err; - - if (!cur_profile) { - mutex_unlock(&profile_lock); - return -ENODEV; - } - /* Scan for a matching profile */ i = sysfs_match_string(profile_names, buf); if (i < 0) { - mutex_unlock(&profile_lock); return -EINVAL; } - /* Check that platform supports this profile choice */ - if (!test_bit(i, cur_profile->choices)) { - mutex_unlock(&profile_lock); - return -EOPNOTSUPP; - } + scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) { + if (!platform_profile_is_registered()) + return -ENODEV; - err = cur_profile->profile_set(cur_profile, i); - if (!err) - sysfs_notify(acpi_kobj, NULL, "platform_profile"); + /* Check that all handlers support this profile choice */ + choices = platform_profile_get_choices(); + if (!test_bit(i, &choices)) + return -EOPNOTSUPP; + + list_for_each_entry(handler, &platform_profile_handler_list, list) { + err = handler->profile_set(handler, i); + if (err) { + pr_err("Failed to set profile for handler %s\n", handler->name); + break; + } + } + if (err) { + list_for_each_entry_continue_reverse(handler, &platform_profile_handler_list, list) { + if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED)) + pr_err("Failed to revert profile for handler %s\n", handler->name); + } + return err; + } + } - mutex_unlock(&profile_lock); - if (err) - return err; + sysfs_notify(acpi_kobj, NULL, "platform_profile"); return count; }