LGTM. Although patch is a bit more complicated.
I do not have time to test this today. I can try tomorrow.
On Fri, 28 Feb 2025 at 18:02, Mario Limonciello <superm1@xxxxxxxxxx> wrote:
>
> From: Mario Limonciello <mario.limonciello@xxxxxxx>
>
> 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.
>
> To allow two drivers to disagree, add support for "hidden choices".
> Hidden choices are platform profiles that a driver supports to be
> compatible with the platform profile of another driver.
>
> Fixes: 688834743d67 ("ACPI: platform_profile: Allow multiple handlers")
> Reported-by: Antheas Kapenekakis <lkml@xxxxxxxxxxx>
> Closes: https://lore.kernel.org/platform-driver-x86/e64b771e-3255-42ad-9257-5b8fc6c24ac9@xxxxxx/T/#mc068042dd29df36c16c8af92664860fc4763974b
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> Cc: "Luke D. Jones" <luke@xxxxxxxxxx>
> drivers/acpi/platform_profile.c | 94 +++++++++++++++++++++++++-------
> include/linux/platform_profile.h | 3 +
> 2 files changed, 76 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 2ad53cc6aae53..ef9444482db19 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -21,9 +21,15 @@ struct platform_profile_handler {
> struct device dev;
> int minor;
> unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> + unsigned long hidden_choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> const struct platform_profile_ops *ops;
> };
>
> +struct aggregate_choices_data {
> + unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> + int count;
> +};
> +
> static const char * const profile_names[] = {
> [PLATFORM_PROFILE_LOW_POWER] = "low-power",
> [PLATFORM_PROFILE_COOL] = "cool",
> @@ -73,7 +79,7 @@ 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))
> + if (!test_bit(*bit, handler->choices) && !test_bit(*bit, handler->hidden_choices))
> return -EOPNOTSUPP;
>
> return handler->ops->profile_set(dev, *bit);
> @@ -239,21 +245,44 @@ static const struct class platform_profile_class = {
> /**
> * _aggregate_choices - Aggregate the available profile choices
> * @dev: The device
> - * @data: The available profile choices
> + * @arg: struct aggregate_choices_data
> *
> * Return: 0 on success, -errno on failure
> */
> -static int _aggregate_choices(struct device *dev, void *data)
> +static int _aggregate_choices(struct device *dev, void *arg)
> {
> + unsigned long tmp[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> + struct aggregate_choices_data *data = arg;
> struct platform_profile_handler *handler;
> - unsigned long *aggregate = data;
>
> lockdep_assert_held(&profile_lock);
> handler = to_pprof_handler(dev);
> - if (test_bit(PLATFORM_PROFILE_LAST, aggregate))
> - bitmap_copy(aggregate, handler->choices, PLATFORM_PROFILE_LAST);
> + bitmap_or(tmp, handler->choices, handler->hidden_choices, PLATFORM_PROFILE_LAST);
> + if (test_bit(PLATFORM_PROFILE_LAST, data->aggregate))
> + bitmap_copy(data->aggregate, tmp, PLATFORM_PROFILE_LAST);
> else
> - bitmap_and(aggregate, handler->choices, aggregate, PLATFORM_PROFILE_LAST);
> + bitmap_and(data->aggregate, tmp, data->aggregate, PLATFORM_PROFILE_LAST);
> + data->count++;
> +
> + return 0;
> +}
> +
> +/**
> + * _remove_hidden_choices - Remove hidden choices from aggregate data
> + * @dev: The device
> + * @arg: struct aggregate_choices_data
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +static int _remove_hidden_choices(struct device *dev, void *arg)
> +{
> + struct aggregate_choices_data *data = arg;
> + struct platform_profile_handler *handler;
> +
> + lockdep_assert_held(&profile_lock);
> + handler = to_pprof_handler(dev);
> + bitmap_andnot(data->aggregate, handler->choices,
> + handler->hidden_choices, PLATFORM_PROFILE_LAST);
>
> return 0;
> }
> @@ -270,22 +299,31 @@ static ssize_t platform_profile_choices_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> + struct aggregate_choices_data data = {
> + .aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> + .count = 0,
> + };
> int err;
>
> - set_bit(PLATFORM_PROFILE_LAST, aggregate);
> + set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
> scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> err = class_for_each_device(&platform_profile_class, NULL,
> - aggregate, _aggregate_choices);
> + &data, _aggregate_choices);
> if (err)
> return err;
> + if (data.count == 1) {
> + err = class_for_each_device(&platform_profile_class, NULL,
> + &data, _remove_hidden_choices);
> + if (err)
> + return err;
> + }
> }
>
> /* no profile handler registered any more */
> - if (bitmap_empty(aggregate, PLATFORM_PROFILE_LAST))
> + if (bitmap_empty(data.aggregate, PLATFORM_PROFILE_LAST))
> return -EINVAL;
>
> - return _commmon_choices_show(aggregate, buf);
> + return _commmon_choices_show(data.aggregate, buf);
> }
>
> /**
> @@ -373,7 +411,10 @@ static ssize_t platform_profile_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> + struct aggregate_choices_data data = {
> + .aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> + .count = 0,
> + };
> int ret;
> int i;
>
> @@ -381,13 +422,13 @@ static ssize_t platform_profile_store(struct device *dev,
> i = sysfs_match_string(profile_names, buf);
> if (i < 0 || i == PLATFORM_PROFILE_CUSTOM)
> return -EINVAL;
> - set_bit(PLATFORM_PROFILE_LAST, choices);
> + set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
> scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> ret = class_for_each_device(&platform_profile_class, NULL,
> - choices, _aggregate_choices);
> + &data, _aggregate_choices);
> if (ret)
> return ret;
> - if (!test_bit(i, choices))
> + if (!test_bit(i, data.aggregate))
> return -EOPNOTSUPP;
>
> ret = class_for_each_device(&platform_profile_class, NULL, &i,
> @@ -453,12 +494,15 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
> */
> int platform_profile_cycle(void)
> {
> + struct aggregate_choices_data data = {
> + .aggregate = { [0 ... BITS_TO_LONGS(PLATFORM_PROFILE_LAST) - 1] = ~0UL },
> + .count = 0,
> + };
> enum platform_profile_option next = PLATFORM_PROFILE_LAST;
> enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
> - unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> int err;
>
> - set_bit(PLATFORM_PROFILE_LAST, choices);
> + set_bit(PLATFORM_PROFILE_LAST, data.aggregate);
> scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> err = class_for_each_device(&platform_profile_class, NULL,
> &profile, _aggregate_profiles);
> @@ -470,14 +514,14 @@ int platform_profile_cycle(void)
> return -EINVAL;
>
> err = class_for_each_device(&platform_profile_class, NULL,
> - choices, _aggregate_choices);
> + &data, _aggregate_choices);
> if (err)
> return err;
>
> /* never iterate into a custom if all drivers supported it */
> - clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
> + clear_bit(PLATFORM_PROFILE_CUSTOM, data.aggregate);
>
> - next = find_next_bit_wrap(choices,
> + next = find_next_bit_wrap(data.aggregate,
> PLATFORM_PROFILE_LAST,
> profile + 1);
>
> @@ -532,6 +576,14 @@ struct device *platform_profile_register(struct device *dev, const char *name,
> return ERR_PTR(-EINVAL);
> }
>
> + if (ops->hidden_choices) {
> + err = ops->hidden_choices(drvdata, pprof->hidden_choices);
> + if (err) {
> + dev_err(dev, "platform_profile hidden_choices failed\n");
> + return ERR_PTR(err);
> + }
> + }
> +
> guard(mutex)(&profile_lock);
>
> /* create class interface for individual handler */
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index 8ab5b0e8eb2c1..8c9df7dadd5d3 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -33,6 +33,8 @@ enum platform_profile_option {
> * @probe: Callback to setup choices available to the new class device. These
> * choices will only be enforced when setting a new profile, not when
> * getting the current one.
> + * @hidden_choices: Callback to setup choices that are not visible to the user
> + * but can be set by the driver.
> * @profile_get: Callback that will be called when showing the current platform
> * profile in sysfs.
> * @profile_set: Callback that will be called when storing a new platform
> @@ -40,6 +42,7 @@ enum platform_profile_option {
> */
> struct platform_profile_ops {
> int (*probe)(void *drvdata, unsigned long *choices);
> + int (*hidden_choices)(void *drvdata, unsigned long *choices);
> int (*profile_get)(struct device *dev, enum platform_profile_option *profile);
> int (*profile_set)(struct device *dev, enum platform_profile_option profile);
> };
> --
> 2.43.0
>
