[PATCH v8 1/3] Documentation: Add documentation for new platform_profile sysfs attribute

[PATCH v8 1/3] Documentation: Add documentation for new platform_profile sysfs attribute

[Mark Pearson]

On modern systems the platform performance, temperature, fan and other
hardware related characteristics are often dynamically configurable. The
profile is often automatically adjusted to the load by some
automatic-mechanism (which may very well live outside the kernel).

These auto platform-adjustment mechanisms often can be configured with
one of several 'platform-profiles', with either a bias towards low-power
consumption or towards performance (and higher power consumption and
thermals).

Introduce a new platform_profile sysfs API which offers a generic API for
selecting the performance-profile of these automatic-mechanisms.

Co-developed-by: Mark Pearson <markpearson@lenovo.com>
Signed-off-by: Mark Pearson <markpearson@lenovo.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
 - updated to rst format
Changes in v3, v4, v5
 - version bump along with rest of patch series
Changes in v6:
 - Split sysfs-platform_profile.rs into ABI text and then admin guide in
   userspace-api section. Hope this is correct - I'm guessing a bit.
Changes in v7:
 - Correct available_choices to platform_profile_choices
 - Improve phrasing as recommended by review
Changes in v8:
 - Removed unnecessary empty lines at end of file

 .../ABI/testing/sysfs-platform_profile        | 24 +++++++++++
 Documentation/userspace-api/index.rst         |  1 +
 .../userspace-api/sysfs-platform_profile.rst  | 42 +++++++++++++++++++
 3 files changed, 67 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform_profile
 create mode 100644 Documentation/userspace-api/sysfs-platform_profile.rst

diff --git a/Documentation/ABI/testing/sysfs-platform_profile b/Documentation/ABI/testing/sysfs-platform_profile
new file mode 100644
index 000000000000..9d6b89b66cca
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform_profile
@@ -0,0 +1,24 @@
+What:		/sys/firmware/acpi/platform_profile_choices
+Date:		October 2020
+Contact:	Hans de Goede <hdegoede@redhat.com>
+Description:	This file contains a space-separated list of profiles supported for this device.
+
+		Drivers must use the following standard profile-names:
+
+		============	============================================
+		low-power	Low power consumption
+		cool		Cooler operation
+		quiet		Quieter operation
+		balanced	Balance between low power consumption and performance
+		performance	High performance operation
+		============	============================================
+
+		Userspace may expect drivers to offer more than one of these
+		standard profile names.
+
+What:		/sys/firmware/acpi/platform_profile
+Date:		October 2020
+Contact:	Hans de Goede <hdegoede@redhat.com>
+Description:	Reading this file gives the current selected profile for this
+		device. Writing this file with one of the strings from
+		platform_profile_choices changes the profile to the new value.
diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
index acd2cc2a538d..d29b020e5622 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -24,6 +24,7 @@ place where this information is gathered.
    ioctl/index
    iommu
    media/index
+   sysfs-platform_profile
 
 .. only::  subproject and html
 
diff --git a/Documentation/userspace-api/sysfs-platform_profile.rst b/Documentation/userspace-api/sysfs-platform_profile.rst
new file mode 100644
index 000000000000..c33a71263d9e
--- /dev/null
+++ b/Documentation/userspace-api/sysfs-platform_profile.rst
@@ -0,0 +1,42 @@
+=====================================================================
+Platform Profile Selection (e.g. /sys/firmware/acpi/platform_profile)
+=====================================================================
+
+On modern systems the platform performance, temperature, fan and other
+hardware related characteristics are often dynamically configurable. The
+platform configuration is often automatically adjusted to the current
+conditions by some automatic mechanism (which may very well live outside
+the kernel).
+
+These auto platform adjustment mechanisms often can be configured with
+one of several platform profiles, with either a bias towards low power
+operation or towards performance.
+
+The purpose of the platform_profile attribute is to offer a generic sysfs
+API for selecting the platform profile of these automatic mechanisms.
+
+Note that this API is only for selecting the platform profile, it is
+NOT a goal of this API to allow monitoring the resulting performance
+characteristics. Monitoring performance is best done with device/vendor
+specific tools such as e.g. turbostat.
+
+Specifically when selecting a high performance profile the actual achieved
+performance may be limited by various factors such as: the heat generated
+by other components, room temperature, free air flow at the bottom of a
+laptop, etc. It is explicitly NOT a goal of this API to let userspace know
+about any sub-optimal conditions which are impeding reaching the requested
+performance level.
+
+Since numbers on their own cannot represent the multiple variables that a
+profile will adjust (power consumption, heat generation, etc) this API
+uses strings to describe the various profiles. To make sure that userspace
+gets a consistent experience the sysfs-platform_profile ABI document defines
+a fixed set of profile names. Drivers *must* map their internal profile
+representation onto this fixed set.
+
+If there is no good match when mapping then a new profile name may be
+added. Drivers which wish to introduce new profile names must:
+
+ 1. Explain why the existing profile names canot be used.
+ 2. Add the new profile name, along with a clear description of the
+    expected behaviour, to the sysfs-platform_profile ABI documentation.
-- 
2.28.0

[PATCH v8 2/3] ACPI: platform-profile: Add platform profile support

[Mark Pearson]

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@lenovo.com>
---
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

Changed in v6:
 - Change default build option to 'm' and clean in formating in Kconfig
 - Change enums to be capitalised as requested
 - Rename unregister function to remove

Changes in v7
 - version bump along with rest of patch series

Changes in v8:
 - correct checking for empty choices bitmap in register function

 drivers/acpi/Kconfig             |  17 +++
 drivers/acpi/Makefile            |   1 +
 drivers/acpi/platform_profile.c  | 181 +++++++++++++++++++++++++++++++
 include/linux/platform_profile.h |  39 +++++++
 4 files changed, 238 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..5ddff93e38c2 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -326,6 +326,23 @@ 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 m
+	help
+	  This driver adds support for platform-profiles on platforms that
+	  support it.
+
+	  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.
+
+	  This driver provides the sysfs interface and is used as the registration
+	  point for platform specific drivers.
+
+	  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..f65aa8cd2185
--- /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",
+};
+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);
+	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);
+	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);
+	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 || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
+		!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_remove(void)
+{
+	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_remove);
+
+MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
new file mode 100644
index 000000000000..9a1e2abd7602
--- /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_remove(void);
+void platform_profile_notify(void);
+
+#endif  /*_PLATFORM_PROFILE_H_*/
-- 
2.28.0

Re: [PATCH v8 2/3] ACPI: platform-profile: Add platform profile support

[Rafael J. Wysocki]

On Wed, Dec 30, 2020 at 1:24 AM Mark Pearson <markpearson@lenovo.com> 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@lenovo.com>
> ---
> 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
>
> Changed in v6:
>  - Change default build option to 'm' and clean in formating in Kconfig
>  - Change enums to be capitalised as requested
>  - Rename unregister function to remove
>
> Changes in v7
>  - version bump along with rest of patch series
>
> Changes in v8:
>  - correct checking for empty choices bitmap in register function

Applied with the Hans' R-by and I replaced PLATFORM_PROFILE_LOW and
PLATFORM_PROFILE_PERFORM with PLATFORM_PROFILE_LOW_POWER and
PLATFORM_PROFILE_PERFORMANCE, respectively.

Thanks!

>  drivers/acpi/Kconfig             |  17 +++
>  drivers/acpi/Makefile            |   1 +
>  drivers/acpi/platform_profile.c  | 181 +++++++++++++++++++++++++++++++
>  include/linux/platform_profile.h |  39 +++++++
>  4 files changed, 238 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..5ddff93e38c2 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -326,6 +326,23 @@ 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 m
> +       help
> +         This driver adds support for platform-profiles on platforms that
> +         support it.
> +
> +         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.
> +
> +         This driver provides the sysfs interface and is used as the registration
> +         point for platform specific drivers.
> +
> +         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..f65aa8cd2185
> --- /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",
> +};
> +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);
> +       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);
> +       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);
> +       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 || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
> +               !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_remove(void)
> +{
> +       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_remove);
> +
> +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> new file mode 100644
> index 000000000000..9a1e2abd7602
> --- /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_remove(void);
> +void platform_profile_notify(void);
> +
> +#endif  /*_PLATFORM_PROFILE_H_*/
> --
> 2.28.0
>

[PATCH v8 3/3] platform/x86: thinkpad_acpi: Add platform profile support

[Mark Pearson]

Add support to thinkpad_acpi for Lenovo platforms that have DYTC
version 5 support or newer to use the platform profile feature.

This will allow users to determine and control the platform modes
between low-power, balanced operation and performance modes.

Signed-off-by: Mark Pearson <markpearson@lenovo.com>
---
Changes in v2:
 Address (hopefully) all recommendations from review including:
 - use IS_ENABLED instead of IS_DEFINED
 - update driver to work with all the fixes in platform_profile update
 - improve error handling for invalid inputs
 - move tracking of current profile mode into this driver

Changes in v3:
 - version update for patch series

Changes in v4:
 - Rebase on top of palm sensor patch which led to a little bit of file
   restructuring/clean up
 - Use BIT macro where applicable
 - Formatting fixes
 - Check sysfs node created on exit function
 - implement and use DYTC_SET_COMMAND macro
 - in case of failure setting performance mode make sure CQL mode is
   enabled again before returning.
 - Clean up initialisation and error handling code

Changes in v5:
 - Use atomic_int with ignoring events
 - Add mutex when accessing ACPI information 
 - Clean up registration code
 - Use cached value in profile_get function
 - Add dytc_cql_command function to clean up and provide common handler
 - Update to work with changes in platform_profile implementation

Changes in v6:
 - Update file to build against update in v6 platform_profile

Changes in v7 & v8:
 - version bump along with rest of patch series

 drivers/platform/x86/thinkpad_acpi.c | 292 ++++++++++++++++++++++++++-
 1 file changed, 286 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 6a4c54db38fb..e08b3548afd1 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -66,6 +66,7 @@
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include <linux/power_supply.h>
+#include <linux/platform_profile.h>
 #include <sound/core.h>
 #include <sound/control.h>
 #include <sound/initval.h>
@@ -9843,16 +9844,27 @@ static bool has_lapsensor;
 static bool palm_state;
 static bool lap_state;
 
-static int lapsensor_get(bool *present, bool *state)
+static int dytc_command(int command, int *output)
 {
 	acpi_handle dytc_handle;
-	int output;
 
-	*present = false;
-	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle)))
+	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
+		/* Platform doesn't support DYTC */
 		return -ENODEV;
-	if (!acpi_evalf(dytc_handle, &output, NULL, "dd", DYTC_CMD_GET))
+	}
+	if (!acpi_evalf(dytc_handle, output, NULL, "dd", command))
 		return -EIO;
+	return 0;
+}
+
+static int lapsensor_get(bool *present, bool *state)
+{
+	int output, err;
+
+	*present = false;
+	err = dytc_command(DYTC_CMD_GET, &output);
+	if (err)
+		return err;
 
 	*present = true; /*If we get his far, we have lapmode support*/
 	*state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false;
@@ -9971,6 +9983,262 @@ static struct ibm_struct proxsensor_driver_data = {
 	.exit = proxsensor_exit,
 };
 
+#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
+
+/*************************************************************************
+ * DYTC Platform Profile interface
+ */
+
+#define DYTC_CMD_QUERY        0 /* To get DYTC status - enable/revision */
+#define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
+#define DYTC_CMD_RESET    0x1ff /* To reset back to default */
+
+#define DYTC_QUERY_ENABLE_BIT 8  /* Bit        8 - 0 = disabled, 1 = enabled */
+#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
+#define DYTC_QUERY_REV_BIT    28 /* Bits 28 - 31 - revision */
+
+#define DYTC_GET_FUNCTION_BIT 8  /* Bits  8-11 - function setting */
+#define DYTC_GET_MODE_BIT     12 /* Bits 12-15 - mode setting */
+
+#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - function setting */
+#define DYTC_SET_MODE_BIT     16 /* Bits 16-19 - mode setting */
+#define DYTC_SET_VALID_BIT    20 /* Bit     20 - 1 = on, 0 = off */
+
+#define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
+#define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
+#define DYTC_FUNCTION_MMC     11 /* Function = 11, desk mode */
+
+#define DYTC_MODE_PERFORM     2  /* High power mode aka performance */
+#define DYTC_MODE_QUIET       3  /* Low power mode aka quiet */
+#define DYTC_MODE_BALANCE   0xF  /* Default mode aka balanced */
+
+#define DYTC_SET_COMMAND(function, mode, on) \
+	(DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \
+	 (mode) << DYTC_SET_MODE_BIT | \
+	 (on) << DYTC_SET_VALID_BIT)
+
+#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 0)
+
+#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 1)
+
+static bool dytc_profile_available;
+static enum platform_profile_option dytc_current_profile;
+static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
+static DEFINE_MUTEX(dytc_mutex);
+
+static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
+{
+	switch (dytcmode) {
+	case DYTC_MODE_QUIET:
+		*profile = PLATFORM_PROFILE_LOW;
+		break;
+	case DYTC_MODE_BALANCE:
+		*profile =  PLATFORM_PROFILE_BALANCED;
+		break;
+	case DYTC_MODE_PERFORM:
+		*profile =  PLATFORM_PROFILE_PERFORM;
+		break;
+	default: /* Unknown mode */
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
+{
+	switch (profile) {
+	case PLATFORM_PROFILE_QUIET:
+		*perfmode = DYTC_MODE_QUIET;
+		break;
+	case PLATFORM_PROFILE_BALANCED:
+		*perfmode = DYTC_MODE_BALANCE;
+		break;
+	case PLATFORM_PROFILE_PERFORM:
+		*perfmode = DYTC_MODE_PERFORM;
+		break;
+	default: /* Unknown profile */
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+/*
+ * dytc_profile_get: Function to register with platform_profile
+ * handler. Returns current platform profile.
+ */
+int dytc_profile_get(enum platform_profile_option *profile)
+{
+	*profile = dytc_current_profile;
+	return 0;
+}
+
+/*
+ * Helper function - check if we are in CQL mode and if we are
+ *  -  disable CQL,
+ *  - run the command
+ *  - enable CQL
+ *  If not in CQL mode, just run the command
+ */
+int dytc_cql_command(int command, int *output)
+{
+	int err, cmd_err, dummy;
+	int cur_funcmode;
+
+	/* Determine if we are in CQL mode. This alters the commands we do */
+	err = dytc_command(DYTC_CMD_GET, output);
+	if (err)
+		return err;
+
+	cur_funcmode = (*output >> DYTC_GET_FUNCTION_BIT) & 0xF;
+	/* Check if we're OK to return immediately */
+	if ((command == DYTC_CMD_GET) && (cur_funcmode != DYTC_FUNCTION_CQL))
+		return 0;
+
+	if (cur_funcmode == DYTC_FUNCTION_CQL) {
+		atomic_inc(&dytc_ignore_event);
+		err = dytc_command(DYTC_DISABLE_CQL, &dummy);
+		if (err)
+			return err;
+	}
+
+	cmd_err = dytc_command(command,	output);
+	/* Check return condition after we've restored CQL state */
+
+	if (cur_funcmode == DYTC_FUNCTION_CQL) {
+		err = dytc_command(DYTC_ENABLE_CQL, &dummy);
+		if (err)
+			return err;
+	}
+
+	return cmd_err;
+}
+
+/*
+ * dytc_profile_set: Function to register with platform_profile
+ * handler. Sets current platform profile.
+ */
+int dytc_profile_set(enum platform_profile_option profile)
+{
+	int output;
+	int err;
+
+	if (!dytc_profile_available)
+		return -ENODEV;
+
+	err = mutex_lock_interruptible(&dytc_mutex);
+	if (err)
+		return err;
+
+	if (profile == PLATFORM_PROFILE_BALANCED) {
+		/* To get back to balanced mode we just issue a reset command */
+		err = dytc_command(DYTC_CMD_RESET, &output);
+		if (err)
+			goto unlock;
+	} else {
+		int perfmode;
+
+		err = convert_profile_to_dytc(profile, &perfmode);
+		if (err)
+			goto unlock;
+
+		/* Determine if we are in CQL mode. This alters the commands we do */
+		err = dytc_cql_command(DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1), &output);
+		if (err)
+			goto unlock;
+	}
+	/* Success - update current profile */
+	dytc_current_profile = profile;
+unlock:
+	mutex_unlock(&dytc_mutex);
+	return err;
+}
+
+static void dytc_profile_refresh(void)
+{
+	enum platform_profile_option profile;
+	int output, err;
+	int perfmode;
+
+	mutex_lock(&dytc_mutex);
+	err = dytc_cql_command(DYTC_CMD_GET, &output);
+	mutex_unlock(&dytc_mutex);
+	if (err)
+		return;
+
+	perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
+	convert_dytc_to_profile(perfmode, &profile);
+	if (profile != dytc_current_profile) {
+		dytc_current_profile = profile;
+		platform_profile_notify();
+	}
+}
+
+static struct platform_profile_handler dytc_profile = {
+	.profile_get = dytc_profile_get,
+	.profile_set = dytc_profile_set,
+};
+
+static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
+{
+	int err, output;
+
+	/* Setup supported modes */
+	set_bit(PLATFORM_PROFILE_LOW,      dytc_profile.choices);
+	set_bit(PLATFORM_PROFILE_BALANCED, dytc_profile.choices);
+	set_bit(PLATFORM_PROFILE_PERFORM,  dytc_profile.choices);
+
+	dytc_profile_available = false;
+	err = dytc_command(DYTC_CMD_QUERY, &output);
+	/*
+	 * If support isn't available (ENODEV) then don't return an error
+	 * and don't create the sysfs group
+	 */
+	if (err == -ENODEV)
+		return 0;
+	/* For all other errors we can flag the failure */
+	if (err)
+		return err;
+
+	/* Check DYTC is enabled and supports mode setting */
+	if (output & BIT(DYTC_QUERY_ENABLE_BIT)) {
+		/* Only DYTC v5.0 and later has this feature. */
+		int dytc_version;
+
+		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
+		if (dytc_version >= 5) {
+			dbg_printk(TPACPI_DBG_INIT,
+				   "DYTC version %d: thermal mode available\n", dytc_version);
+			/* Create platform_profile structure and register */
+			err = platform_profile_register(&dytc_profile);
+			/*
+			 * If for some reason platform_profiles aren't enabled
+			 * don't quit terminally.
+			 */
+			if (err)
+				return 0;
+
+			dytc_profile_available = true;
+			/* Ensure initial values are correct */
+			dytc_profile_refresh();
+		}
+	}
+	return 0;
+}
+
+static void dytc_profile_exit(void)
+{
+	if (dytc_profile_available) {
+		dytc_profile_available = false;
+		platform_profile_remove();
+	}
+}
+
+static struct ibm_struct  dytc_profile_driver_data = {
+	.name = "dytc-profile",
+	.exit = dytc_profile_exit,
+};
+#endif /* CONFIG_ACPI_PLATFORM_PROFILE */
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -10019,8 +10287,14 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 		mutex_unlock(&kbdlight_mutex);
 	}
 
-	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
+	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) {
 		lapsensor_refresh();
+#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
+		/* If we are already accessing DYTC then skip dytc update */
+		if (!atomic_add_unless(&dytc_ignore_event, -1, 0))
+			dytc_profile_refresh();
+#endif
+	}
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
@@ -10463,6 +10737,12 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_proxsensor_init,
 		.data = &proxsensor_driver_data,
 	},
+#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
+	{
+		.init = tpacpi_dytc_profile_init,
+		.data = &dytc_profile_driver_data,
+	},
+#endif
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.28.0

Re: [PATCH v8 3/3] platform/x86: thinkpad_acpi: Add platform profile support

[Hans de Goede]

Hi,

Some review remarks inline, mostly things I noticed while running
Rafael's bleeding-edge branch which has 1/3 and 2/3 of this series
combined with this patch.

On 12/30/20 1:18 AM, Mark Pearson wrote:
> Add support to thinkpad_acpi for Lenovo platforms that have DYTC
> version 5 support or newer to use the platform profile feature.
> 
> This will allow users to determine and control the platform modes
> between low-power, balanced operation and performance modes.
> 
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
> ---
> Changes in v2:
>  Address (hopefully) all recommendations from review including:
>  - use IS_ENABLED instead of IS_DEFINED
>  - update driver to work with all the fixes in platform_profile update
>  - improve error handling for invalid inputs
>  - move tracking of current profile mode into this driver
> 
> Changes in v3:
>  - version update for patch series
> 
> Changes in v4:
>  - Rebase on top of palm sensor patch which led to a little bit of file
>    restructuring/clean up
>  - Use BIT macro where applicable
>  - Formatting fixes
>  - Check sysfs node created on exit function
>  - implement and use DYTC_SET_COMMAND macro
>  - in case of failure setting performance mode make sure CQL mode is
>    enabled again before returning.
>  - Clean up initialisation and error handling code
> 
> Changes in v5:
>  - Use atomic_int with ignoring events
>  - Add mutex when accessing ACPI information 
>  - Clean up registration code
>  - Use cached value in profile_get function
>  - Add dytc_cql_command function to clean up and provide common handler
>  - Update to work with changes in platform_profile implementation
> 
> Changes in v6:
>  - Update file to build against update in v6 platform_profile
> 
> Changes in v7 & v8:
>  - version bump along with rest of patch series
> 
>  drivers/platform/x86/thinkpad_acpi.c | 292 ++++++++++++++++++++++++++-
>  1 file changed, 286 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 6a4c54db38fb..e08b3548afd1 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -66,6 +66,7 @@
>  #include <linux/acpi.h>
>  #include <linux/pci.h>
>  #include <linux/power_supply.h>
> +#include <linux/platform_profile.h>
>  #include <sound/core.h>
>  #include <sound/control.h>
>  #include <sound/initval.h>
> @@ -9843,16 +9844,27 @@ static bool has_lapsensor;
>  static bool palm_state;
>  static bool lap_state;
>  
> -static int lapsensor_get(bool *present, bool *state)
> +static int dytc_command(int command, int *output)
>  {
>  	acpi_handle dytc_handle;
> -	int output;
>  
> -	*present = false;
> -	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle)))
> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) {
> +		/* Platform doesn't support DYTC */
>  		return -ENODEV;
> -	if (!acpi_evalf(dytc_handle, &output, NULL, "dd", DYTC_CMD_GET))
> +	}
> +	if (!acpi_evalf(dytc_handle, output, NULL, "dd", command))
>  		return -EIO;
> +	return 0;
> +}
> +
> +static int lapsensor_get(bool *present, bool *state)
> +{
> +	int output, err;
> +
> +	*present = false;
> +	err = dytc_command(DYTC_CMD_GET, &output);
> +	if (err)
> +		return err;
>  
>  	*present = true; /*If we get his far, we have lapmode support*/
>  	*state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false;
> @@ -9971,6 +9983,262 @@ static struct ibm_struct proxsensor_driver_data = {
>  	.exit = proxsensor_exit,
>  };
>  
> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
> +
> +/*************************************************************************
> + * DYTC Platform Profile interface
> + */
> +
> +#define DYTC_CMD_QUERY        0 /* To get DYTC status - enable/revision */
> +#define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
> +#define DYTC_CMD_RESET    0x1ff /* To reset back to default */
> +
> +#define DYTC_QUERY_ENABLE_BIT 8  /* Bit        8 - 0 = disabled, 1 = enabled */
> +#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
> +#define DYTC_QUERY_REV_BIT    28 /* Bits 28 - 31 - revision */
> +
> +#define DYTC_GET_FUNCTION_BIT 8  /* Bits  8-11 - function setting */
> +#define DYTC_GET_MODE_BIT     12 /* Bits 12-15 - mode setting */
> +
> +#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - function setting */
> +#define DYTC_SET_MODE_BIT     16 /* Bits 16-19 - mode setting */
> +#define DYTC_SET_VALID_BIT    20 /* Bit     20 - 1 = on, 0 = off */
> +
> +#define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
> +#define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
> +#define DYTC_FUNCTION_MMC     11 /* Function = 11, desk mode */
> +
> +#define DYTC_MODE_PERFORM     2  /* High power mode aka performance */
> +#define DYTC_MODE_QUIET       3  /* Low power mode aka quiet */
> +#define DYTC_MODE_BALANCE   0xF  /* Default mode aka balanced */
> +
> +#define DYTC_SET_COMMAND(function, mode, on) \
> +	(DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \
> +	 (mode) << DYTC_SET_MODE_BIT | \
> +	 (on) << DYTC_SET_VALID_BIT)
> +
> +#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 0)
> +
> +#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 1)
> +
> +static bool dytc_profile_available;
> +static enum platform_profile_option dytc_current_profile;
> +static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
> +static DEFINE_MUTEX(dytc_mutex);
> +
> +static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
> +{
> +	switch (dytcmode) {
> +	case DYTC_MODE_QUIET:
> +		*profile = PLATFORM_PROFILE_LOW;

This needs to be PLATFORM_PROFILE_LOW_POWER now, due to changes Rafael made while
merging 2/3 (more instances of this below).

> +		break;
> +	case DYTC_MODE_BALANCE:
> +		*profile =  PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case DYTC_MODE_PERFORM:
> +		*profile =  PLATFORM_PROFILE_PERFORM;

This needs to be PLATFORM_PROFILE_PERFORMANCE now, due to changes Rafael made while
merging 2/3 (more instances of this below).

> +		break;
> +	default: /* Unknown mode */
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
> +{
> +	switch (profile) {
> +	case PLATFORM_PROFILE_QUIET:

QUIET ? Above you translate DYTC_MODE_QUIET to PLATFORM_PROFILE_LOW_POWER and when
setting the choices bits you also use PLATFORM_PROFILE_LOW_POWER, so you should use
PLATFORM_PROFILE_LOW_POWER here too. Or replace all of them with PLATFORM_PROFILE_QUIET,
which might be better if the internal Lenovo docs describe this setting as quiet.

> +		*perfmode = DYTC_MODE_QUIET;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		*perfmode = DYTC_MODE_BALANCE;
> +		break;
> +	case PLATFORM_PROFILE_PERFORM:
> +		*perfmode = DYTC_MODE_PERFORM;

PERFORMANCE (as above).

> +		break;
> +	default: /* Unknown profile */
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * dytc_profile_get: Function to register with platform_profile
> + * handler. Returns current platform profile.
> + */
> +int dytc_profile_get(enum platform_profile_option *profile)
> +{
> +	*profile = dytc_current_profile;
> +	return 0;
> +}
> +
> +/*
> + * Helper function - check if we are in CQL mode and if we are
> + *  -  disable CQL,
> + *  - run the command
> + *  - enable CQL
> + *  If not in CQL mode, just run the command
> + */
> +int dytc_cql_command(int command, int *output)
> +{
> +	int err, cmd_err, dummy;
> +	int cur_funcmode;
> +
> +	/* Determine if we are in CQL mode. This alters the commands we do */
> +	err = dytc_command(DYTC_CMD_GET, output);
> +	if (err)
> +		return err;
> +
> +	cur_funcmode = (*output >> DYTC_GET_FUNCTION_BIT) & 0xF;
> +	/* Check if we're OK to return immediately */
> +	if ((command == DYTC_CMD_GET) && (cur_funcmode != DYTC_FUNCTION_CQL))
> +		return 0;
> +
> +	if (cur_funcmode == DYTC_FUNCTION_CQL) {
> +		atomic_inc(&dytc_ignore_event);
> +		err = dytc_command(DYTC_DISABLE_CQL, &dummy);
> +		if (err)
> +			return err;
> +	}
> +
> +	cmd_err = dytc_command(command,	output);
> +	/* Check return condition after we've restored CQL state */
> +
> +	if (cur_funcmode == DYTC_FUNCTION_CQL) {
> +		err = dytc_command(DYTC_ENABLE_CQL, &dummy);
> +		if (err)
> +			return err;
> +	}
> +
> +	return cmd_err;
> +}
> +
> +/*
> + * dytc_profile_set: Function to register with platform_profile
> + * handler. Sets current platform profile.
> + */
> +int dytc_profile_set(enum platform_profile_option profile)
> +{
> +	int output;
> +	int err;
> +
> +	if (!dytc_profile_available)
> +		return -ENODEV;
> +
> +	err = mutex_lock_interruptible(&dytc_mutex);
> +	if (err)
> +		return err;
> +
> +	if (profile == PLATFORM_PROFILE_BALANCED) {
> +		/* To get back to balanced mode we just issue a reset command */
> +		err = dytc_command(DYTC_CMD_RESET, &output);
> +		if (err)
> +			goto unlock;
> +	} else {
> +		int perfmode;
> +
> +		err = convert_profile_to_dytc(profile, &perfmode);
> +		if (err)
> +			goto unlock;
> +
> +		/* Determine if we are in CQL mode. This alters the commands we do */
> +		err = dytc_cql_command(DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1), &output);
> +		if (err)
> +			goto unlock;
> +	}
> +	/* Success - update current profile */
> +	dytc_current_profile = profile;
> +unlock:
> +	mutex_unlock(&dytc_mutex);
> +	return err;
> +}
> +
> +static void dytc_profile_refresh(void)
> +{
> +	enum platform_profile_option profile;
> +	int output, err;
> +	int perfmode;
> +
> +	mutex_lock(&dytc_mutex);
> +	err = dytc_cql_command(DYTC_CMD_GET, &output);
> +	mutex_unlock(&dytc_mutex);
> +	if (err)
> +		return;
> +
> +	perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
> +	convert_dytc_to_profile(perfmode, &profile);
> +	if (profile != dytc_current_profile) {
> +		dytc_current_profile = profile;
> +		platform_profile_notify();
> +	}
> +}
> +
> +static struct platform_profile_handler dytc_profile = {
> +	.profile_get = dytc_profile_get,
> +	.profile_set = dytc_profile_set,
> +};
> +
> +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
> +{
> +	int err, output;
> +
> +	/* Setup supported modes */
> +	set_bit(PLATFORM_PROFILE_LOW,      dytc_profile.choices);
> +	set_bit(PLATFORM_PROFILE_BALANCED, dytc_profile.choices);
> +	set_bit(PLATFORM_PROFILE_PERFORM,  dytc_profile.choices);

Same changes necessary as discussed above.

Regards,

Hans



> +
> +	dytc_profile_available = false;
> +	err = dytc_command(DYTC_CMD_QUERY, &output);
> +	/*
> +	 * If support isn't available (ENODEV) then don't return an error
> +	 * and don't create the sysfs group
> +	 */
> +	if (err == -ENODEV)
> +		return 0;
> +	/* For all other errors we can flag the failure */
> +	if (err)
> +		return err;
> +
> +	/* Check DYTC is enabled and supports mode setting */
> +	if (output & BIT(DYTC_QUERY_ENABLE_BIT)) {
> +		/* Only DYTC v5.0 and later has this feature. */
> +		int dytc_version;
> +
> +		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
> +		if (dytc_version >= 5) {
> +			dbg_printk(TPACPI_DBG_INIT,
> +				   "DYTC version %d: thermal mode available\n", dytc_version);
> +			/* Create platform_profile structure and register */
> +			err = platform_profile_register(&dytc_profile);
> +			/*
> +			 * If for some reason platform_profiles aren't enabled
> +			 * don't quit terminally.
> +			 */
> +			if (err)
> +				return 0;
> +
> +			dytc_profile_available = true;
> +			/* Ensure initial values are correct */
> +			dytc_profile_refresh();
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void dytc_profile_exit(void)
> +{
> +	if (dytc_profile_available) {
> +		dytc_profile_available = false;
> +		platform_profile_remove();
> +	}
> +}
> +
> +static struct ibm_struct  dytc_profile_driver_data = {
> +	.name = "dytc-profile",
> +	.exit = dytc_profile_exit,
> +};
> +#endif /* CONFIG_ACPI_PLATFORM_PROFILE */
> +
>  /****************************************************************************
>   ****************************************************************************
>   *
> @@ -10019,8 +10287,14 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
>  		mutex_unlock(&kbdlight_mutex);
>  	}
>  
> -	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED)
> +	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) {
>  		lapsensor_refresh();
> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
> +		/* If we are already accessing DYTC then skip dytc update */
> +		if (!atomic_add_unless(&dytc_ignore_event, -1, 0))
> +			dytc_profile_refresh();
> +#endif
> +	}
>  }
>  
>  static void hotkey_driver_event(const unsigned int scancode)
> @@ -10463,6 +10737,12 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>  		.init = tpacpi_proxsensor_init,
>  		.data = &proxsensor_driver_data,
>  	},
> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
> +	{
> +		.init = tpacpi_dytc_profile_init,
> +		.data = &dytc_profile_driver_data,
> +	},
> +#endif
>  };
>  
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> 

Re: [PATCH v8 1/3] Documentation: Add documentation for new platform_profile sysfs attribute

[Rafael J. Wysocki]

On Wed, Dec 30, 2020 at 1:24 AM Mark Pearson <markpearson@lenovo.com> wrote:
>
> On modern systems the platform performance, temperature, fan and other
> hardware related characteristics are often dynamically configurable. The
> profile is often automatically adjusted to the load by some
> automatic-mechanism (which may very well live outside the kernel).
>
> These auto platform-adjustment mechanisms often can be configured with
> one of several 'platform-profiles', with either a bias towards low-power
> consumption or towards performance (and higher power consumption and
> thermals).
>
> Introduce a new platform_profile sysfs API which offers a generic API for
> selecting the performance-profile of these automatic-mechanisms.
>
> Co-developed-by: Mark Pearson <markpearson@lenovo.com>
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied, thanks!

> ---
> Changes in v2:
>  - updated to rst format
> Changes in v3, v4, v5
>  - version bump along with rest of patch series
> Changes in v6:
>  - Split sysfs-platform_profile.rs into ABI text and then admin guide in
>    userspace-api section. Hope this is correct - I'm guessing a bit.
> Changes in v7:
>  - Correct available_choices to platform_profile_choices
>  - Improve phrasing as recommended by review
> Changes in v8:
>  - Removed unnecessary empty lines at end of file
>
>  .../ABI/testing/sysfs-platform_profile        | 24 +++++++++++
>  Documentation/userspace-api/index.rst         |  1 +
>  .../userspace-api/sysfs-platform_profile.rst  | 42 +++++++++++++++++++
>  3 files changed, 67 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform_profile
>  create mode 100644 Documentation/userspace-api/sysfs-platform_profile.rst
>
> diff --git a/Documentation/ABI/testing/sysfs-platform_profile b/Documentation/ABI/testing/sysfs-platform_profile
> new file mode 100644
> index 000000000000..9d6b89b66cca
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform_profile
> @@ -0,0 +1,24 @@
> +What:          /sys/firmware/acpi/platform_profile_choices
> +Date:          October 2020
> +Contact:       Hans de Goede <hdegoede@redhat.com>
> +Description:   This file contains a space-separated list of profiles supported for this device.
> +
> +               Drivers must use the following standard profile-names:
> +
> +               ============    ============================================
> +               low-power       Low power consumption
> +               cool            Cooler operation
> +               quiet           Quieter operation
> +               balanced        Balance between low power consumption and performance
> +               performance     High performance operation
> +               ============    ============================================
> +
> +               Userspace may expect drivers to offer more than one of these
> +               standard profile names.
> +
> +What:          /sys/firmware/acpi/platform_profile
> +Date:          October 2020
> +Contact:       Hans de Goede <hdegoede@redhat.com>
> +Description:   Reading this file gives the current selected profile for this
> +               device. Writing this file with one of the strings from
> +               platform_profile_choices changes the profile to the new value.
> diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
> index acd2cc2a538d..d29b020e5622 100644
> --- a/Documentation/userspace-api/index.rst
> +++ b/Documentation/userspace-api/index.rst
> @@ -24,6 +24,7 @@ place where this information is gathered.
>     ioctl/index
>     iommu
>     media/index
> +   sysfs-platform_profile
>
>  .. only::  subproject and html
>
> diff --git a/Documentation/userspace-api/sysfs-platform_profile.rst b/Documentation/userspace-api/sysfs-platform_profile.rst
> new file mode 100644
> index 000000000000..c33a71263d9e
> --- /dev/null
> +++ b/Documentation/userspace-api/sysfs-platform_profile.rst
> @@ -0,0 +1,42 @@
> +=====================================================================
> +Platform Profile Selection (e.g. /sys/firmware/acpi/platform_profile)
> +=====================================================================
> +
> +On modern systems the platform performance, temperature, fan and other
> +hardware related characteristics are often dynamically configurable. The
> +platform configuration is often automatically adjusted to the current
> +conditions by some automatic mechanism (which may very well live outside
> +the kernel).
> +
> +These auto platform adjustment mechanisms often can be configured with
> +one of several platform profiles, with either a bias towards low power
> +operation or towards performance.
> +
> +The purpose of the platform_profile attribute is to offer a generic sysfs
> +API for selecting the platform profile of these automatic mechanisms.
> +
> +Note that this API is only for selecting the platform profile, it is
> +NOT a goal of this API to allow monitoring the resulting performance
> +characteristics. Monitoring performance is best done with device/vendor
> +specific tools such as e.g. turbostat.
> +
> +Specifically when selecting a high performance profile the actual achieved
> +performance may be limited by various factors such as: the heat generated
> +by other components, room temperature, free air flow at the bottom of a
> +laptop, etc. It is explicitly NOT a goal of this API to let userspace know
> +about any sub-optimal conditions which are impeding reaching the requested
> +performance level.
> +
> +Since numbers on their own cannot represent the multiple variables that a
> +profile will adjust (power consumption, heat generation, etc) this API
> +uses strings to describe the various profiles. To make sure that userspace
> +gets a consistent experience the sysfs-platform_profile ABI document defines
> +a fixed set of profile names. Drivers *must* map their internal profile
> +representation onto this fixed set.
> +
> +If there is no good match when mapping then a new profile name may be
> +added. Drivers which wish to introduce new profile names must:
> +
> + 1. Explain why the existing profile names canot be used.
> + 2. Add the new profile name, along with a clear description of the
> +    expected behaviour, to the sysfs-platform_profile ABI documentation.
> --
> 2.28.0
>