Hi Rafael,
Thanks for the review - a couple of questions (and a bunch of acks) below
On 08/12/2020 13:26, Rafael J. Wysocki wrote:
> On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote:
>>
<snip>
>>
>> drivers/acpi/Kconfig | 14 +++
>> drivers/acpi/Makefile | 1 +
>> drivers/acpi/platform_profile.c | 181 +++++++++++++++++++++++++++++++
>> include/linux/platform_profile.h | 39 +++++++
>> 4 files changed, 235 insertions(+)
>> create mode 100644 drivers/acpi/platform_profile.c
>> create mode 100644 include/linux/platform_profile.h
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index edf1558c1105..c1ca6255ff85 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -326,6 +326,20 @@ config ACPI_THERMAL
>> To compile this driver as a module, choose M here:
>> the module will be called thermal.
>>
>> +config ACPI_PLATFORM_PROFILE
>> + tristate "ACPI Platform Profile Driver"
>> + default y
>
> default m
OK
>
>> + help
>> + This driver adds support for platform-profiles on platforms that
>> + support it.
>
> Empty line here, please.
Ack
>
>> + Platform-profiles can be used to control the platform behaviour. For
>> + example whether to operate in a lower power mode, in a higher
>> + power performance mode or between the two.
>
> And here.
Ack
>
>> + This driver provides the sysfs interface and is used as the registration
>> + point for platform specific drivers.
>
> And here.
Ack
>
>> + Which profiles are supported is determined on a per-platform basis and
>> + should be obtained from the platform specific driver.
>> +
>> config ACPI_CUSTOM_DSDT_FILE
>> string "Custom DSDT Table file to include"
>> default ""
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 44e412506317..c64a8af106c0 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -78,6 +78,7 @@ obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o
>> obj-$(CONFIG_ACPI_PROCESSOR) += processor.o
>> obj-$(CONFIG_ACPI) += container.o
>> obj-$(CONFIG_ACPI_THERMAL) += thermal.o
>> +obj-$(CONFIG_ACPI_PLATFORM_PROFILE) += platform_profile.o
>> obj-$(CONFIG_ACPI_NFIT) += nfit/
>> obj-$(CONFIG_ACPI_NUMA) += numa/
>> obj-$(CONFIG_ACPI) += acpi_memhotplug.o
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> new file mode 100644
>> index 000000000000..1bc092359e35
>> --- /dev/null
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -0,0 +1,181 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +/* Platform profile sysfs interface */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/bits.h>
>> +#include <linux/init.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_profile.h>
>> +#include <linux/sysfs.h>
>> +
>> +static const struct platform_profile_handler *cur_profile;
>> +static DEFINE_MUTEX(profile_lock);
>> +
>> +static const char * const profile_names[] = {
>> + [platform_profile_low] = "low-power",
>> + [platform_profile_cool] = "cool",
>> + [platform_profile_quiet] = "quiet",
>> + [platform_profile_balanced] = "balanced",
>> + [platform_profile_perform] = "performance",
>
> The enum values in upper case, please.
Sorry, I'm a bit confused here - do you mean change to "Low-power" or
something else (maybe PLATFORM_PROFILE_LOW?)
Just want to make sure I'm getting it correct. If I change the strings
it will impact patch1 in the series which is integrated into your
bleeding-edge branch.
>
>> +};
>> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last);
>> +
>> +static ssize_t platform_profile_choices_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + int len = 0;
>> + int err, i;
>> +
>> + err = mutex_lock_interruptible(&profile_lock);
>
> Why interruptible?
>
> And why is the lock needed in the first place?
My thinking was that I don't know what happens when I hand over to thhe
platform driver who does the get/set, so having a lock to prevent a get
whilst a set is in operation seemed like a good idea.
It was interruptible as a suggestion in an earlier reivew as the
preferred way of doing these things for functions that could be called
by user space.
Do you think the lock is a problem?
>
>> + if (err)
>> + return err;
>> +
>> + if (!cur_profile) {
>> + mutex_unlock(&profile_lock);
>> + return -ENODEV;
>> + }
>> +
>> + for_each_set_bit(i, cur_profile->choices, platform_profile_last) {
>> + if (len == 0)
>> + len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
>> + else
>> + len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
>> + }
>> + len += sysfs_emit_at(buf, len, "\n");
>> + mutex_unlock(&profile_lock);
>> + return len;
>> +}
>> +
>> +static ssize_t platform_profile_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + enum platform_profile_option profile = platform_profile_balanced;
>> + int err;
>> +
>> + err = mutex_lock_interruptible(&profile_lock);
>> + if (err)
>> + return err;
>> +
>> + if (!cur_profile) {
>> + mutex_unlock(&profile_lock);
>> + return -ENODEV;
>> + }
>> +
>> + err = cur_profile->profile_get(&profile);
>
> In which cases this can fail?
I'm not sure - but as this is supposed to be vendor agnostic I can't
foresee what might be wanted or could happen on various hardware. I
agree a failure is probably unlikely in the Lenovo case where we're
doing an ACPI call, but is there any issue in handling error codes?
It doesn't seem to gain much by removing it and may have future impacts.
>
>> + mutex_unlock(&profile_lock);
>> + if (err)
>> + return err;
>> +
>> + /* Check that profile is valid index */
>> + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
>> + return -EIO;
>> +
>> + return sysfs_emit(buf, "%s\n", profile_names[profile]);
>> +}
>> +
>> +static ssize_t platform_profile_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + 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;
>> + }
>> +
>> + err = cur_profile->profile_set(i);
>
> What if this gets a signal in the middle of the ->profile_set()
> execution? Is this always guaranteed to work?
I'm afraid I don't know the answer to this one. What would be the
recommendation to cover this event?
>
>> + mutex_unlock(&profile_lock);
>> + if (err)
>> + return err;
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR_RO(platform_profile_choices);
>> +static DEVICE_ATTR_RW(platform_profile);
>> +
>> +static struct attribute *platform_profile_attrs[] = {
>> + &dev_attr_platform_profile_choices.attr,
>> + &dev_attr_platform_profile.attr,
>> + NULL
>> +};
>> +
>> +static const struct attribute_group platform_profile_group = {
>> + .attrs = platform_profile_attrs
>> +};
>> +
>> +void platform_profile_notify(void)
>> +{
>> + if (!cur_profile)
>> + return;
>> + sysfs_notify(acpi_kobj, NULL, "platform_profile");
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_notify);
>> +
>> +int platform_profile_register(const struct platform_profile_handler *pprof)
>> +{
>> + int err;
>> +
>> + mutex_lock(&profile_lock);
>> + /* We can only have one active profile */
>> + if (cur_profile) {
>> + mutex_unlock(&profile_lock);
>> + return -EEXIST;
>> + }
>> +
>> + /* Sanity check the profile handler field are set */
>> + if (!pprof || !pprof->choices || !pprof->profile_set ||
>> + !pprof->profile_get) {
>> + mutex_unlock(&profile_lock);
>> + return -EINVAL;
>> + }
>> +
>> + err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>> + if (err) {
>> + mutex_unlock(&profile_lock);
>> + return err;
>> + }
>> +
>> + cur_profile = pprof;
>> + mutex_unlock(&profile_lock);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_register);
>> +
>> +int platform_profile_unregister(void)
>
> "Unregister" functions typically take an argument pointing to the
> target object, so something like platform_profile_remove() may be a
> better choice here.
Sure - happy to change that
>
>> +{
>> + mutex_lock(&profile_lock);
>> + if (!cur_profile) {
>> + mutex_unlock(&profile_lock);
>> + return -ENODEV;
>> + }
>> +
>> + sysfs_remove_group(acpi_kobj, &platform_profile_group);
>> + cur_profile = NULL;
>> + mutex_unlock(&profile_lock);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_unregister);
>> +
>> +MODULE_AUTHOR("Mark Pearson <markpearson@xxxxxxxxxx>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
>> new file mode 100644
>> index 000000000000..f2e1b1c90482
>> --- /dev/null
>> +++ b/include/linux/platform_profile.h
>> @@ -0,0 +1,39 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Platform profile sysfs interface
>> + *
>> + * See Documentation/ABI/testing/sysfs-platform_profile.rst for more
>> + * information.
>> + */
>> +
>> +#ifndef _PLATFORM_PROFILE_H_
>> +#define _PLATFORM_PROFILE_H_
>> +
>> +#include <linux/bitops.h>
>> +
>> +/*
>> + * If more options are added please update profile_names
>> + * array in platform-profile.c and sysfs-platform-profile.rst
>> + * documentation.
>> + */
>> +
>> +enum platform_profile_option {
>> + platform_profile_low,
>> + platform_profile_cool,
>> + platform_profile_quiet,
>> + platform_profile_balanced,
>> + platform_profile_perform,
>> + platform_profile_last, /*must always be last */
>> +};
>> +
>> +struct platform_profile_handler {
>> + unsigned long choices[BITS_TO_LONGS(platform_profile_last)];
>> + int (*profile_get)(enum platform_profile_option *profile);
>> + int (*profile_set)(enum platform_profile_option profile);
>> +};
>> +
>> +int platform_profile_register(const struct platform_profile_handler *pprof);
>> +int platform_profile_unregister(void);
>> +void platform_profile_notify(void);
>> +
>> +#endif /*_PLATFORM_PROFILE_H_*/
>> --
Thanks
Mark
