| Message ID | 20241105153316.378-19-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: > Multiple drivers may attempt to register platform profile handlers, > but only one may be registered and the behavior is non-deterministic > for which one wins. It's mostly controlled by probing order. > > This can be problematic if one driver changes CPU settings and another > driver notifies the EC for changing fan curves. > > Modify the ACPI platform profile handler to let multiple drivers > register platform profile handlers and abstract this detail from userspace. > > To avoid undefined behaviors only offer profiles that are commonly > advertised across multiple handlers. > > If any problems occur when changing profiles for any driver, then revert > back to the balanced profile, which is now required. Reviewed-by: Armin Wolf <W_Armin@gmx.de> > Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev> > Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/acpi/platform_profile.c | 12 ++---------- > 1 file changed, 2 insertions(+), 10 deletions(-) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index 568485e285061..b9eb25f58a2a2 100644 > --- a/drivers/acpi/platform_profile.c > +++ b/drivers/acpi/platform_profile.c > @@ -10,7 +10,6 @@ > #include <linux/platform_profile.h> > #include <linux/sysfs.h> > > -static struct platform_profile_handler *cur_profile; > static DEFINE_MUTEX(profile_lock); > > static const char * const profile_names[] = { > @@ -368,8 +367,7 @@ static const struct attribute_group platform_profile_group = { > > void platform_profile_notify(void) > { > - if (!cur_profile) > - return; > + guard(mutex)(&profile_lock); > if (!class_is_registered(&platform_profile_class)) > return; > sysfs_notify(acpi_kobj, NULL, "platform_profile"); > @@ -428,9 +426,6 @@ int platform_profile_register(struct platform_profile_handler *pprof) > } > > guard(mutex)(&profile_lock); > - /* We can only have one active profile */ > - if (cur_profile) > - return -EEXIST; > > if (!class_is_registered(&platform_profile_class)) { > /* class for individual handlers */ > @@ -451,9 +446,9 @@ int platform_profile_register(struct platform_profile_handler *pprof) > if (IS_ERR(pprof->class_dev)) > return PTR_ERR(pprof->class_dev); > dev_set_drvdata(pprof->class_dev, pprof); > + > sysfs_notify(acpi_kobj, NULL, "platform_profile"); > > - cur_profile = pprof; > return 0; > > cleanup_class: > @@ -467,13 +462,10 @@ int platform_profile_remove(struct platform_profile_handler *pprof) > { > guard(mutex)(&profile_lock); > > - cur_profile = NULL; > - > sysfs_notify(acpi_kobj, NULL, "platform_profile"); > > device_destroy(&platform_profile_class, MKDEV(0, pprof->minor)); > > - cur_profile = NULL; > return 0; > } > EXPORT_SYMBOL_GPL(platform_profile_remove);
Am 06.11.24 um 20:21 schrieb Armin Wolf: > Am 05.11.24 um 16:33 schrieb Mario Limonciello: > >> Multiple drivers may attempt to register platform profile handlers, >> but only one may be registered and the behavior is non-deterministic >> for which one wins. It's mostly controlled by probing order. >> >> This can be problematic if one driver changes CPU settings and another >> driver notifies the EC for changing fan curves. >> >> Modify the ACPI platform profile handler to let multiple drivers >> register platform profile handlers and abstract this detail from >> userspace. >> >> To avoid undefined behaviors only offer profiles that are commonly >> advertised across multiple handlers. >> >> If any problems occur when changing profiles for any driver, then revert >> back to the balanced profile, which is now required. > > Reviewed-by: Armin Wolf <W_Armin@gmx.de> > I just noticed that the following text might need to be removed: "If any problems occur when changing profiles for any driver, then revert back to the balanced profile, which is now required." Thanks, Armin Wolf >> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev> >> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> drivers/acpi/platform_profile.c | 12 ++---------- >> 1 file changed, 2 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/acpi/platform_profile.c >> b/drivers/acpi/platform_profile.c >> index 568485e285061..b9eb25f58a2a2 100644 >> --- a/drivers/acpi/platform_profile.c >> +++ b/drivers/acpi/platform_profile.c >> @@ -10,7 +10,6 @@ >> #include <linux/platform_profile.h> >> #include <linux/sysfs.h> >> >> -static struct platform_profile_handler *cur_profile; >> static DEFINE_MUTEX(profile_lock); >> >> static const char * const profile_names[] = { >> @@ -368,8 +367,7 @@ static const struct attribute_group >> platform_profile_group = { >> >> void platform_profile_notify(void) >> { >> - if (!cur_profile) >> - return; >> + guard(mutex)(&profile_lock); >> if (!class_is_registered(&platform_profile_class)) >> return; >> sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> @@ -428,9 +426,6 @@ int platform_profile_register(struct >> platform_profile_handler *pprof) >> } >> >> guard(mutex)(&profile_lock); >> - /* We can only have one active profile */ >> - if (cur_profile) >> - return -EEXIST; >> >> if (!class_is_registered(&platform_profile_class)) { >> /* class for individual handlers */ >> @@ -451,9 +446,9 @@ int platform_profile_register(struct >> platform_profile_handler *pprof) >> if (IS_ERR(pprof->class_dev)) >> return PTR_ERR(pprof->class_dev); >> dev_set_drvdata(pprof->class_dev, pprof); >> + >> sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> >> - cur_profile = pprof; >> return 0; >> >> cleanup_class: >> @@ -467,13 +462,10 @@ int platform_profile_remove(struct >> platform_profile_handler *pprof) >> { >> guard(mutex)(&profile_lock); >> >> - cur_profile = NULL; >> - >> sysfs_notify(acpi_kobj, NULL, "platform_profile"); >> >> device_destroy(&platform_profile_class, MKDEV(0, pprof->minor)); >> >> - cur_profile = NULL; >> return 0; >> } >> EXPORT_SYMBOL_GPL(platform_profile_remove); >
On 11/6/2024 13:33, Armin Wolf wrote: > Am 06.11.24 um 20:21 schrieb Armin Wolf: > >> Am 05.11.24 um 16:33 schrieb Mario Limonciello: >> >>> Multiple drivers may attempt to register platform profile handlers, >>> but only one may be registered and the behavior is non-deterministic >>> for which one wins. It's mostly controlled by probing order. >>> >>> This can be problematic if one driver changes CPU settings and another >>> driver notifies the EC for changing fan curves. >>> >>> Modify the ACPI platform profile handler to let multiple drivers >>> register platform profile handlers and abstract this detail from >>> userspace. >>> >>> To avoid undefined behaviors only offer profiles that are commonly >>> advertised across multiple handlers. >>> >>> If any problems occur when changing profiles for any driver, then revert >>> back to the balanced profile, which is now required. >> >> Reviewed-by: Armin Wolf <W_Armin@gmx.de> >> > I just noticed that the following text might need to be removed: > > "If any problems occur when changing profiles for any driver, then revert > back to the balanced profile, which is now required." Good catch, thanks. > > Thanks, > Armin Wolf > >>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev> >>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>> --- >>> drivers/acpi/platform_profile.c | 12 ++---------- >>> 1 file changed, 2 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/acpi/platform_profile.c >>> b/drivers/acpi/platform_profile.c >>> index 568485e285061..b9eb25f58a2a2 100644 >>> --- a/drivers/acpi/platform_profile.c >>> +++ b/drivers/acpi/platform_profile.c >>> @@ -10,7 +10,6 @@ >>> #include <linux/platform_profile.h> >>> #include <linux/sysfs.h> >>> >>> -static struct platform_profile_handler *cur_profile; >>> static DEFINE_MUTEX(profile_lock); >>> >>> static const char * const profile_names[] = { >>> @@ -368,8 +367,7 @@ static const struct attribute_group >>> platform_profile_group = { >>> >>> void platform_profile_notify(void) >>> { >>> - if (!cur_profile) >>> - return; >>> + guard(mutex)(&profile_lock); >>> if (!class_is_registered(&platform_profile_class)) >>> return; >>> sysfs_notify(acpi_kobj, NULL, "platform_profile"); >>> @@ -428,9 +426,6 @@ int platform_profile_register(struct >>> platform_profile_handler *pprof) >>> } >>> >>> guard(mutex)(&profile_lock); >>> - /* We can only have one active profile */ >>> - if (cur_profile) >>> - return -EEXIST; >>> >>> if (!class_is_registered(&platform_profile_class)) { >>> /* class for individual handlers */ >>> @@ -451,9 +446,9 @@ int platform_profile_register(struct >>> platform_profile_handler *pprof) >>> if (IS_ERR(pprof->class_dev)) >>> return PTR_ERR(pprof->class_dev); >>> dev_set_drvdata(pprof->class_dev, pprof); >>> + >>> sysfs_notify(acpi_kobj, NULL, "platform_profile"); >>> >>> - cur_profile = pprof; >>> return 0; >>> >>> cleanup_class: >>> @@ -467,13 +462,10 @@ int platform_profile_remove(struct >>> platform_profile_handler *pprof) >>> { >>> guard(mutex)(&profile_lock); >>> >>> - cur_profile = NULL; >>> - >>> sysfs_notify(acpi_kobj, NULL, "platform_profile"); >>> >>> device_destroy(&platform_profile_class, MKDEV(0, pprof->minor)); >>> >>> - cur_profile = NULL; >>> return 0; >>> } >>> EXPORT_SYMBOL_GPL(platform_profile_remove); >>
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index 568485e285061..b9eb25f58a2a2 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -10,7 +10,6 @@ #include <linux/platform_profile.h> #include <linux/sysfs.h> -static struct platform_profile_handler *cur_profile; static DEFINE_MUTEX(profile_lock); static const char * const profile_names[] = { @@ -368,8 +367,7 @@ static const struct attribute_group platform_profile_group = { void platform_profile_notify(void) { - if (!cur_profile) - return; + guard(mutex)(&profile_lock); if (!class_is_registered(&platform_profile_class)) return; sysfs_notify(acpi_kobj, NULL, "platform_profile"); @@ -428,9 +426,6 @@ int platform_profile_register(struct platform_profile_handler *pprof) } guard(mutex)(&profile_lock); - /* We can only have one active profile */ - if (cur_profile) - return -EEXIST; if (!class_is_registered(&platform_profile_class)) { /* class for individual handlers */ @@ -451,9 +446,9 @@ int platform_profile_register(struct platform_profile_handler *pprof) if (IS_ERR(pprof->class_dev)) return PTR_ERR(pprof->class_dev); dev_set_drvdata(pprof->class_dev, pprof); + sysfs_notify(acpi_kobj, NULL, "platform_profile"); - cur_profile = pprof; return 0; cleanup_class: @@ -467,13 +462,10 @@ int platform_profile_remove(struct platform_profile_handler *pprof) { guard(mutex)(&profile_lock); - cur_profile = NULL; - sysfs_notify(acpi_kobj, NULL, "platform_profile"); device_destroy(&platform_profile_class, MKDEV(0, pprof->minor)); - cur_profile = NULL; return 0; } EXPORT_SYMBOL_GPL(platform_profile_remove);