On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote:
>
> This is the initial implementation of the platform-profile feature.
> It provides the details discussed and outlined in the
> sysfs-platform_profile document.
>
> Many modern systems have the ability to modify the operating profile to
> control aspects like fan speed, temperature and power levels. This
> module provides a common sysfs interface that platform modules can register
> against to control their individual profile options.
>
> Signed-off-by: Mark Pearson <markpearson@xxxxxxxxxx>
> ---
> Changes in v2:
> Address (hopefully) all recommendations from review including:
> - reorder includes list alphabetically
> - make globals statics and use const as required
> - change profile name scanning to use full string
> - clean up profile name lists to remove unwanted additions
> - use sysfs_emit and sysfs_emit_at appropriately (much nicer!)
> - improve error handling. Return errors to user in all cases and use
> better error codes where appropriate (ENOOPSUPP)
> - clean up sysfs output for better readability
> - formatting fixes where needed
> - improve structure and enum names to be clearer
> - remove cur_profile field from structure. It is now local to the
> actual platform driver file (patch 3 in series)
> - improve checking so if future profile options are added profile_names
> will be updated as well.
> - move CONFIG option next to ACPI_THERMAL as it seemed slightly related
> - removed MAINTAINERS update as not appropriate (note warning message
> is seen when running checkpatch)
>
> Changes in v3:
> - Add missed platform_profile.h file
>
> Changes in v4:
> - Clean up duplicate entry in Kconfig file
> - Add linux/bits.h to include list
> - Remove unnecessary items from include list
> - Make cur_profile const
> - Clean up comments
> - formatting clean-ups
> - add checking of profile return value to show function
> - add checking to store to see if it's a supported profile
> - revert ENOTSUPP change in store function
> - improved error checking in profile registration
> - improved profile naming (now platform_profile_*)
>
> Changes in v5:
> - correct 'balance' to 'balanced' to be consistent with documentation
> - add WARN_ON when checking profile index in show function
> - switch mutex_lock_interruptible back to mutex_lock where appropriate
> - add 'platform_profile_last' as final entry in profile entry. Update
> implementation to use this appropriately
> - Use BITS_TO_LONG and appropriate access functions for choices field
> - Correct error handling as recommended
> - Sanity check profile fields on registration
> - Remove unnecessary init and exit functions
>
> 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
> + help
> + This driver adds support for platform-profiles on platforms that
> + support it.
Empty line here, please.
> + 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.
> + This driver provides the sysfs interface and is used as the registration
> + point for platform specific drivers.
And here.
> + 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.
> +};
> +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?
> + 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?
> + 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?
> + 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.
> +{
> + 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_*/
> --
