| Message ID | 20250304064745.1073770-2-superm1@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add quiet/low power compat code | expand |
On Tue, 4 Mar 2025 at 07:48, Mario Limonciello <superm1@kernel.org> wrote: > > From: Mario Limonciello <mario.limonciello@amd.com> > > When two drivers don't support all the same profiles the legacy interface > only exports the common profiles. > > This causes problems for cases where one driver uses low-power but another > uses quiet because the result is that neither is exported to sysfs. > > If one platform profile handler supports quiet and the other > supports low power treat them as the same for the purpose of > the sysfs interface. > > Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers") > Reported-by: Antheas Kapenekakis <lkml@antheas.dev> > Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@gmx.de/T/#mc068042dd29df36c16c8af92664860fc4763974b > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/acpi/platform_profile.c | 38 ++++++++++++++++++++++++++++++--- > 1 file changed, 35 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > index 2ad53cc6aae53..d9a7cc5891734 100644 > --- a/drivers/acpi/platform_profile.c > +++ b/drivers/acpi/platform_profile.c > @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) > > lockdep_assert_held(&profile_lock); > handler = to_pprof_handler(dev); > - if (!test_bit(*bit, handler->choices)) > - return -EOPNOTSUPP; > + if (!test_bit(*bit, handler->choices)) { > + switch (*bit) { > + case PLATFORM_PROFILE_QUIET: > + *bit = PLATFORM_PROFILE_LOW_POWER; > + break; > + case PLATFORM_PROFILE_LOW_POWER: > + *bit = PLATFORM_PROFILE_QUIET; > + break; > + default: > + return -EOPNOTSUPP; > + } > + if (!test_bit(*bit, handler->choices)) > + return -EOPNOTSUPP; > + } > > return handler->ops->profile_set(dev, *bit); > } > @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) > handler = to_pprof_handler(dev); > if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) > bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); > - else > + else { > + /* treat quiet and low power the same for aggregation purposes */ > + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && > + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) > + set_bit(PLATFORM_PROFILE_QUIET, aggregate); > + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && > + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) > + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); > bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); > + } So you end up showing both? If that's the case, isn't it equivalent to just make amd-pmf show both quiet and low-power? I guess it is not ideal for framework devices. But if asus devices end up showing both, then it should be ok for framework devices to show both. I like the behavior of the V1 personally. > return 0; > } > @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) > if (err) > return err; > > + /* treat low-power and quiet as the same */ > + if ((*profile == PLATFORM_PROFILE_LOW_POWER && > + val == PLATFORM_PROFILE_QUIET) || > + (*profile == PLATFORM_PROFILE_QUIET && > + val == PLATFORM_PROFILE_LOW_POWER)) > + *profile = val; > + > if (*profile != PLATFORM_PROFILE_LAST && *profile != val) > *profile = PLATFORM_PROFILE_CUSTOM; > else > @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, > dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); > return ERR_PTR(-EINVAL); > } > + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && > + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { > + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); > + return ERR_PTR(-EINVAL); > + } Can you avoid failing here? It caused a lot of issues in the past (the WMI driver bails). a dev_err should be enough. Since you do not fail maybe it can be increased to dev_crit. There is at least one driver that implements both currently, and a fix would have to precede this patch. > > guard(mutex)(&profile_lock); > > -- > 2.43.0 >
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index 2ad53cc6aae53..d9a7cc5891734 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -73,8 +73,20 @@ static int _store_class_profile(struct device *dev, void *data) lockdep_assert_held(&profile_lock); handler = to_pprof_handler(dev); - if (!test_bit(*bit, handler->choices)) - return -EOPNOTSUPP; + if (!test_bit(*bit, handler->choices)) { + switch (*bit) { + case PLATFORM_PROFILE_QUIET: + *bit = PLATFORM_PROFILE_LOW_POWER; + break; + case PLATFORM_PROFILE_LOW_POWER: + *bit = PLATFORM_PROFILE_QUIET; + break; + default: + return -EOPNOTSUPP; + } + if (!test_bit(*bit, handler->choices)) + return -EOPNOTSUPP; + } return handler->ops->profile_set(dev, *bit); } @@ -252,8 +264,16 @@ static int _aggregate_choices(struct device *dev, void *data) handler = to_pprof_handler(dev); if (test_bit(PLATFORM_PROFILE_LAST, aggregate)) bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST); - else + else { + /* treat quiet and low power the same for aggregation purposes */ + if (test_bit(PLATFORM_PROFILE_QUIET, handler->choices) && + test_bit(PLATFORM_PROFILE_LOW_POWER, aggregate)) + set_bit(PLATFORM_PROFILE_QUIET, aggregate); + else if (test_bit(PLATFORM_PROFILE_LOW_POWER, handler->choices) && + test_bit(PLATFORM_PROFILE_QUIET, aggregate)) + set_bit(PLATFORM_PROFILE_LOW_POWER, aggregate); bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST); + } return 0; } @@ -305,6 +325,13 @@ static int _aggregate_profiles(struct device *dev, void *data) if (err) return err; + /* treat low-power and quiet as the same */ + if ((*profile == PLATFORM_PROFILE_LOW_POWER && + val == PLATFORM_PROFILE_QUIET) || + (*profile == PLATFORM_PROFILE_QUIET && + val == PLATFORM_PROFILE_LOW_POWER)) + *profile = val; + if (*profile != PLATFORM_PROFILE_LAST && *profile != val) *profile = PLATFORM_PROFILE_CUSTOM; else @@ -531,6 +558,11 @@ struct device *platform_profile_register(struct device *dev, const char *name, dev_err(dev, "Failed to register platform_profile class device with empty choices\n"); return ERR_PTR(-EINVAL); } + if (test_bit(PLATFORM_PROFILE_QUIET, pprof->choices) && + test_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices)) { + dev_err(dev, "Failed to register platform_profile class device with both quiet and low-power\n"); + return ERR_PTR(-EINVAL); + } guard(mutex)(&profile_lock);