| Message ID | 20241031040952.109057-17-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Wed, 30 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 | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index 90cbc0de4d5bc..c2bb325ba531c 100644 > --- a/drivers/acpi/platform_profile.c > +++ b/drivers/acpi/platform_profile.c > @@ -99,6 +99,8 @@ 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; > > /* Scan for a matching profile */ > @@ -107,16 +109,29 @@ static ssize_t platform_profile_store(struct device *dev, > return -EINVAL; > > scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) { > - if (!cur_profile) > + if (!platform_profile_is_registered()) > return -ENODEV; > > - /* Check that platform supports this profile choice */ > - if (!test_bit(i, cur_profile->choices)) > + /* Check that all handlers support this profile choice */ > + choices = platform_profile_get_choices(); > + if (!test_bit(i, &choices)) > return -EOPNOTSUPP; > > - err = cur_profile->profile_set(cur_profile, i); > - if (err) > + 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) { Too long line. This looks an error rollback though so instead of break inside the loop you could goto into a label at the end of the function and have much less indentation to begin with. > + if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED)) > + pr_err("Failed to revert profile for handler %s\n", > + handler->name); > + } > return err; > + } > } > > sysfs_notify(acpi_kobj, NULL, "platform_profile"); >
On 10/31/2024 05:25, Ilpo Järvinen wrote: > On Wed, 30 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 | 25 ++++++++++++++++++++----- >> 1 file changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >> index 90cbc0de4d5bc..c2bb325ba531c 100644 >> --- a/drivers/acpi/platform_profile.c >> +++ b/drivers/acpi/platform_profile.c >> @@ -99,6 +99,8 @@ 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; >> >> /* Scan for a matching profile */ >> @@ -107,16 +109,29 @@ static ssize_t platform_profile_store(struct device *dev, >> return -EINVAL; >> >> scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) { >> - if (!cur_profile) >> + if (!platform_profile_is_registered()) >> return -ENODEV; >> >> - /* Check that platform supports this profile choice */ >> - if (!test_bit(i, cur_profile->choices)) >> + /* Check that all handlers support this profile choice */ >> + choices = platform_profile_get_choices(); >> + if (!test_bit(i, &choices)) >> return -EOPNOTSUPP; >> >> - err = cur_profile->profile_set(cur_profile, i); >> - if (err) >> + 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) { > > Too long line. > > This looks an error rollback though so instead of break inside the loop > you could goto into a label at the end of the function and have much less > indentation to begin with. How does the scoped_cond_guard interact with a goto? With the jump I had guessed it goes out of scope, but I wasn't really sure what the compiler does. I guess in the goto label I'll need another scoped_cond_guard()? > >> + if (handler->profile_set(handler, PLATFORM_PROFILE_BALANCED)) >> + pr_err("Failed to revert profile for handler %s\n", >> + handler->name); >> + } >> return err; >> + } >> } >> >> sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> >
On Thu, 31 Oct 2024, Mario Limonciello wrote: > On 10/31/2024 05:25, Ilpo Järvinen wrote: > > On Wed, 30 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 | 25 ++++++++++++++++++++----- > > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/acpi/platform_profile.c > > > b/drivers/acpi/platform_profile.c > > > index 90cbc0de4d5bc..c2bb325ba531c 100644 > > > --- a/drivers/acpi/platform_profile.c > > > +++ b/drivers/acpi/platform_profile.c > > > @@ -99,6 +99,8 @@ 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; > > > /* Scan for a matching profile */ > > > @@ -107,16 +109,29 @@ static ssize_t platform_profile_store(struct device > > > *dev, > > > return -EINVAL; > > > scoped_cond_guard(mutex_intr, return -ERESTARTSYS, > > > &profile_lock) { > > > - if (!cur_profile) > > > + if (!platform_profile_is_registered()) > > > return -ENODEV; > > > - /* Check that platform supports this profile choice */ > > > - if (!test_bit(i, cur_profile->choices)) > > > + /* Check that all handlers support this profile choice */ > > > + choices = platform_profile_get_choices(); > > > + if (!test_bit(i, &choices)) > > > return -EOPNOTSUPP; > > > - err = cur_profile->profile_set(cur_profile, i); > > > - if (err) > > > + 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) { > > > > Too long line. > > > > This looks an error rollback though so instead of break inside the loop > > you could goto into a label at the end of the function and have much less > > indentation to begin with. > > How does the scoped_cond_guard interact with a goto? With the jump I had > guessed it goes out of scope, but I wasn't really sure what the compiler does. > > I guess in the goto label I'll need another scoped_cond_guard()? Ah, the scope problem is a good point. Perhaps you could instead add e.g. platform_profile_reset_default() and call that before break, both patches that had the rollback did the same thing anyway so it can be reused too.
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index 90cbc0de4d5bc..c2bb325ba531c 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -99,6 +99,8 @@ 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; /* Scan for a matching profile */ @@ -107,16 +109,29 @@ static ssize_t platform_profile_store(struct device *dev, return -EINVAL; scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) { - if (!cur_profile) + if (!platform_profile_is_registered()) return -ENODEV; - /* Check that platform supports this profile choice */ - if (!test_bit(i, cur_profile->choices)) + /* Check that all handlers support this profile choice */ + choices = platform_profile_get_choices(); + if (!test_bit(i, &choices)) return -EOPNOTSUPP; - err = cur_profile->profile_set(cur_profile, i); - if (err) + 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; + } } sysfs_notify(acpi_kobj, NULL, "platform_profile");