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

[PATCH v6 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.

 .../ABI/testing/sysfs-platform_profile        | 24 +++++++++++
 Documentation/userspace-api/index.rst         |  1 +
 .../userspace-api/sysfs-platform_profile.rst  | 43 +++++++++++++++++++
 3 files changed, 68 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..5ac14268585f
--- /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
+		available_profiles 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..56639d218003
--- /dev/null
+++ b/Documentation/userspace-api/sysfs-platform_profile.rst
@@ -0,0 +1,43 @@
+=====================================================================
+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
+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
+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 v6 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

 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..567b2320693a
--- /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 || !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_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 v6 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

 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 v6 2/3] ACPI: platform-profile: Add platform profile support

[Rafael J. Wysocki]

On Fri, Dec 11, 2020 at 3:15 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>

[cut]

> 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);

I'm not sure why this callback is necessary and, provided that there
is a good enough reason, why it cannot return an enum
platform_profile_option value.

In principle, if ->profile_set() returns 0, the requested profile can
be saved in a static var and then returned by subsequent "read"
operations.

> +       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_*/
> --

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

[Rafael J. Wysocki]

On Fri, Dec 11, 2020 at 3:15 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>
> ---
> 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.
>
>  .../ABI/testing/sysfs-platform_profile        | 24 +++++++++++
>  Documentation/userspace-api/index.rst         |  1 +
>  .../userspace-api/sysfs-platform_profile.rst  | 43 +++++++++++++++++++
>  3 files changed, 68 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..5ac14268585f
> --- /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
> +               available_profiles changes the profile to the new value.

IIUC s/available_profiles/platform_profile_choices/

> 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..56639d218003
> --- /dev/null
> +++ b/Documentation/userspace-api/sysfs-platform_profile.rst
> @@ -0,0 +1,43 @@
> +=====================================================================
> +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
> +profile is often automatically adjusted to the load by some

After reading this one more time it looks to me like referring to the
profile here is premature, it needs to be defined first.

Maybe say "The platform configuration is often ..."

Iit is also unclear what "the load" means here, so I would say
something like "adjusted to the current conditions" to avoid using
that term.

> +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.
> +
> +
> --

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

[Barnabás Pőcze]

2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:

> On Fri, Dec 11, 2020 at 3:15 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>
> [...]
> > +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);
>
> I'm not sure why this callback is necessary and, provided that there
> is a good enough reason, why it cannot return an enum
> platform_profile_option value.
>
> In principle, if ->profile_set() returns 0, the requested profile can
> be saved in a static var and then returned by subsequent "read"
> operations.
>

It is possible that the platform profile can be changed with (e.g.) a dedicated
button (commonly found on laptops), in which case there needs to be a mechanism
to retrieve the new profile, which would not be possible without introducing
something else in place of that getter - unless I'm missing something obvious.


Regards,
Barnabás Pőcze

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

[Rafael J. Wysocki]

On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
>
> > On Fri, Dec 11, 2020 at 3:15 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>
> > [...]
> > > +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);
> >
> > I'm not sure why this callback is necessary and, provided that there
> > is a good enough reason, why it cannot return an enum
> > platform_profile_option value.
> >
> > In principle, if ->profile_set() returns 0, the requested profile can
> > be saved in a static var and then returned by subsequent "read"
> > operations.
> >
>
> It is possible that the platform profile can be changed with (e.g.) a dedicated
> button (commonly found on laptops), in which case there needs to be a mechanism
> to retrieve the new profile, which would not be possible without introducing
> something else in place of that getter - unless I'm missing something obvious.

Fair enough.

The other question remains, then.

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

[Mark Pearson]

Hi Rafael,

On 16/12/2020 13:47, Rafael J. Wysocki wrote:
> On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>
>> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
>>
>>> On Fri, Dec 11, 2020 at 3:15 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>
>>> [...]
>>>> +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);
>>>
>>> I'm not sure why this callback is necessary and, provided that there
>>> is a good enough reason, why it cannot return an enum
>>> platform_profile_option value.
>>>
>>> In principle, if ->profile_set() returns 0, the requested profile can
>>> be saved in a static var and then returned by subsequent "read"
>>> operations.
>>>
>>
>> It is possible that the platform profile can be changed with (e.g.) a dedicated
>> button (commonly found on laptops), in which case there needs to be a mechanism
>> to retrieve the new profile, which would not be possible without introducing
>> something else in place of that getter - unless I'm missing something obvious.
> 
> Fair enough.
> 
> The other question remains, then.
> 
My thinking here that I shouldn't make assumptions for future platform
implementations - there may be valid cases in the future where being
able to return an error condition if there was an error would be useful.

Just trying to keep this somewhat future proof. Returning an error
condition seemed a useful thing to have available.

Mark

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

[Mark Pearson]

Hi Rafael,

On 16/12/2020 13:34, Rafael J. Wysocki wrote:
> On Fri, Dec 11, 2020 at 3:15 AM Mark Pearson <markpearson@lenovo.com> wrote:
>>
<snip>
>>
>> diff --git a/Documentation/ABI/testing/sysfs-platform_profile b/Documentation/ABI/testing/sysfs-platform_profile
>> new file mode 100644
>> index 000000000000..5ac14268585f
>> --- /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
>> +               available_profiles changes the profile to the new value.
> 
> IIUC s/available_profiles/platform_profile_choices/
Yep - I missed this one. Thank you, I'll update
> 
>> 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..56639d218003
>> --- /dev/null
>> +++ b/Documentation/userspace-api/sysfs-platform_profile.rst
>> @@ -0,0 +1,43 @@
>> +=====================================================================
>> +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
>> +profile is often automatically adjusted to the load by some
> 
> After reading this one more time it looks to me like referring to the
> profile here is premature, it needs to be defined first.
> 
> Maybe say "The platform configuration is often ..."
> 
> Iit is also unclear what "the load" means here, so I would say
> something like "adjusted to the current conditions" to avoid using
> that term.
I'm good with both those suggestions. Thank you. I'll update

Mark

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

[Rafael J. Wysocki]

On Wed, Dec 16, 2020 at 8:19 PM Mark Pearson <markpearson@lenovo.com> wrote:
>
> Hi Rafael,
>
> On 16/12/2020 13:47, Rafael J. Wysocki wrote:
> > On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>
> >> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
> >>
> >>> On Fri, Dec 11, 2020 at 3:15 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>
> >>> [...]
> >>>> +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);
> >>>
> >>> I'm not sure why this callback is necessary and, provided that there
> >>> is a good enough reason, why it cannot return an enum
> >>> platform_profile_option value.
> >>>
> >>> In principle, if ->profile_set() returns 0, the requested profile can
> >>> be saved in a static var and then returned by subsequent "read"
> >>> operations.
> >>>
> >>
> >> It is possible that the platform profile can be changed with (e.g.) a dedicated
> >> button (commonly found on laptops), in which case there needs to be a mechanism
> >> to retrieve the new profile, which would not be possible without introducing
> >> something else in place of that getter - unless I'm missing something obvious.
> >
> > Fair enough.
> >
> > The other question remains, then.
> >
> My thinking here that I shouldn't make assumptions for future platform
> implementations - there may be valid cases in the future where being
> able to return an error condition if there was an error would be useful.
>
> Just trying to keep this somewhat future proof. Returning an error
> condition seemed a useful thing to have available.

You can still return an error while returning a platform_profile_option value.

Just add a special value representing an error to that set.

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

[Mark Pearson]

On 16/12/2020 14:50, Rafael J. Wysocki wrote:
> On Wed, Dec 16, 2020 at 8:19 PM Mark Pearson <markpearson@lenovo.com> wrote:
>>
>> Hi Rafael,
>>
>> On 16/12/2020 13:47, Rafael J. Wysocki wrote:
>>> On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>>>
>>>> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
>>>>
>>>>> On Fri, Dec 11, 2020 at 3:15 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>
>>>>> [...]
>>>>>> +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);
>>>>>
>>>>> I'm not sure why this callback is necessary and, provided that there
>>>>> is a good enough reason, why it cannot return an enum
>>>>> platform_profile_option value.
>>>>>
>>>>> In principle, if ->profile_set() returns 0, the requested profile can
>>>>> be saved in a static var and then returned by subsequent "read"
>>>>> operations.
>>>>>
>>>>
>>>> It is possible that the platform profile can be changed with (e.g.) a dedicated
>>>> button (commonly found on laptops), in which case there needs to be a mechanism
>>>> to retrieve the new profile, which would not be possible without introducing
>>>> something else in place of that getter - unless I'm missing something obvious.
>>>
>>> Fair enough.
>>>
>>> The other question remains, then.
>>>
>> My thinking here that I shouldn't make assumptions for future platform
>> implementations - there may be valid cases in the future where being
>> able to return an error condition if there was an error would be useful.
>>
>> Just trying to keep this somewhat future proof. Returning an error
>> condition seemed a useful thing to have available.
> 
> You can still return an error while returning a platform_profile_option value.
> 
> Just add a special value representing an error to that set.
> 
I'd like to understand what is better about that approach than having an
error and a returnable parameter?

I'm probably missing something obvious but if I add an extra
platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
error case (and return just an enum platform_profile_option) it seems I
lose the granularity of being able to return a specific error condition.
It doesn't feel like an improvement.

Let me know what I'm missing.

Thanks
Mark

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

[Rafael J. Wysocki]

On Wed, Dec 16, 2020 at 10:36 PM Mark Pearson <markpearson@lenovo.com> wrote:
>
>
> On 16/12/2020 14:50, Rafael J. Wysocki wrote:
> > On Wed, Dec 16, 2020 at 8:19 PM Mark Pearson <markpearson@lenovo.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >> On 16/12/2020 13:47, Rafael J. Wysocki wrote:
> >>> On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>>>
> >>>> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
> >>>>
> >>>>> On Fri, Dec 11, 2020 at 3:15 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>
> >>>>> [...]
> >>>>>> +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);
> >>>>>
> >>>>> I'm not sure why this callback is necessary and, provided that there
> >>>>> is a good enough reason, why it cannot return an enum
> >>>>> platform_profile_option value.
> >>>>>
> >>>>> In principle, if ->profile_set() returns 0, the requested profile can
> >>>>> be saved in a static var and then returned by subsequent "read"
> >>>>> operations.
> >>>>>
> >>>>
> >>>> It is possible that the platform profile can be changed with (e.g.) a dedicated
> >>>> button (commonly found on laptops), in which case there needs to be a mechanism
> >>>> to retrieve the new profile, which would not be possible without introducing
> >>>> something else in place of that getter - unless I'm missing something obvious.
> >>>
> >>> Fair enough.
> >>>
> >>> The other question remains, then.
> >>>
> >> My thinking here that I shouldn't make assumptions for future platform
> >> implementations - there may be valid cases in the future where being
> >> able to return an error condition if there was an error would be useful.
> >>
> >> Just trying to keep this somewhat future proof. Returning an error
> >> condition seemed a useful thing to have available.
> >
> > You can still return an error while returning a platform_profile_option value.
> >
> > Just add a special value representing an error to that set.
> >
> I'd like to understand what is better about that approach than having an
> error and a returnable parameter?
>
> I'm probably missing something obvious but if I add an extra
> platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
> error case (and return just an enum platform_profile_option) it seems I
> lose the granularity of being able to return a specific error condition.
> It doesn't feel like an improvement.

And what's the user expected to do about the different error codes
that can be returned?

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

[Mark Pearson]

On 17/12/2020 08:36, Rafael J. Wysocki wrote:
> On Wed, Dec 16, 2020 at 10:36 PM Mark Pearson <markpearson@lenovo.com> wrote:
>>
>>
>> On 16/12/2020 14:50, Rafael J. Wysocki wrote:
>>> On Wed, Dec 16, 2020 at 8:19 PM Mark Pearson <markpearson@lenovo.com> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> On 16/12/2020 13:47, Rafael J. Wysocki wrote:
>>>>> On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>>>>>
>>>>>> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
>>>>>>
>>>>>>> On Fri, Dec 11, 2020 at 3:15 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>
>>>>>>> [...]
>>>>>>>> +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);
>>>>>>>
>>>>>>> I'm not sure why this callback is necessary and, provided that there
>>>>>>> is a good enough reason, why it cannot return an enum
>>>>>>> platform_profile_option value.
>>>>>>>
>>>>>>> In principle, if ->profile_set() returns 0, the requested profile can
>>>>>>> be saved in a static var and then returned by subsequent "read"
>>>>>>> operations.
>>>>>>>
>>>>>>
>>>>>> It is possible that the platform profile can be changed with (e.g.) a dedicated
>>>>>> button (commonly found on laptops), in which case there needs to be a mechanism
>>>>>> to retrieve the new profile, which would not be possible without introducing
>>>>>> something else in place of that getter - unless I'm missing something obvious.
>>>>>
>>>>> Fair enough.
>>>>>
>>>>> The other question remains, then.
>>>>>
>>>> My thinking here that I shouldn't make assumptions for future platform
>>>> implementations - there may be valid cases in the future where being
>>>> able to return an error condition if there was an error would be useful.
>>>>
>>>> Just trying to keep this somewhat future proof. Returning an error
>>>> condition seemed a useful thing to have available.
>>>
>>> You can still return an error while returning a platform_profile_option value.
>>>
>>> Just add a special value representing an error to that set.
>>>
>> I'd like to understand what is better about that approach than having an
>> error and a returnable parameter?
>>
>> I'm probably missing something obvious but if I add an extra
>> platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
>> error case (and return just an enum platform_profile_option) it seems I
>> lose the granularity of being able to return a specific error condition.
>> It doesn't feel like an improvement.
> 
> And what's the user expected to do about the different error codes
> that can be returned?
> 

From my experiences when something fails I would rather have more
information than less. I'm probably over thinking it though.

Looking at it again, from my Lenovo platform profile implementation
perspective I have no objections. It's hard to argue a point without
having a hard requirement or example :) I'll go ahead with your
suggestion of just returning the profile.

Thank you for the review
Mark

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

[Barnabás Pőcze]

2020. december 17., csütörtök 14:36 keltezéssel, Rafael J. Wysocki <rafael@kernel.org> írta:

> [...]
> > >> My thinking here that I shouldn't make assumptions for future platform
> > >> implementations - there may be valid cases in the future where being
> > >> able to return an error condition if there was an error would be useful.
> > >>
> > >> Just trying to keep this somewhat future proof. Returning an error
> > >> condition seemed a useful thing to have available.
> > >
> > > You can still return an error while returning a platform_profile_option value.
> > >
> > > Just add a special value representing an error to that set.
> > >
> > I'd like to understand what is better about that approach than having an
> > error and a returnable parameter?
> >
> > I'm probably missing something obvious but if I add an extra
> > platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
> > error case (and return just an enum platform_profile_option) it seems I
> > lose the granularity of being able to return a specific error condition.
> > It doesn't feel like an improvement.
>
> And what's the user expected to do about the different error codes
> that can be returned?

Even assuming the users of the API cannot or will not handle different errors
differently, who would benefit from getting rid of this information which may be
an aid in debugging for those who know what they're looking at?

And if a new enum value is introduced to signal an error condition, how is that
going to be communicated to the users? A magic string like "error" or "-1"?
Or under a single errno like -EIO? Personally, I believe neither of these two
solutions are better than returning the actual errno which is deemed most
appropriate by the platform profile handler. Or am I missing a third way?


Regards,
Barnabás Pőcze

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

[Rafael J. Wysocki]

On Thu, Dec 17, 2020 at 4:02 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> 2020. december 17., csütörtök 14:36 keltezéssel, Rafael J. Wysocki <rafael@kernel.org> írta:
>
> > [...]
> > > >> My thinking here that I shouldn't make assumptions for future platform
> > > >> implementations - there may be valid cases in the future where being
> > > >> able to return an error condition if there was an error would be useful.
> > > >>
> > > >> Just trying to keep this somewhat future proof. Returning an error
> > > >> condition seemed a useful thing to have available.
> > > >
> > > > You can still return an error while returning a platform_profile_option value.
> > > >
> > > > Just add a special value representing an error to that set.
> > > >
> > > I'd like to understand what is better about that approach than having an
> > > error and a returnable parameter?
> > >
> > > I'm probably missing something obvious but if I add an extra
> > > platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
> > > error case (and return just an enum platform_profile_option) it seems I
> > > lose the granularity of being able to return a specific error condition.
> > > It doesn't feel like an improvement.
> >
> > And what's the user expected to do about the different error codes
> > that can be returned?
>
> Even assuming the users of the API cannot or will not handle different errors
> differently, who would benefit from getting rid of this information which may be
> an aid in debugging for those who know what they're looking at?
>
> And if a new enum value is introduced to signal an error condition, how is that
> going to be communicated to the users? A magic string like "error" or "-1"?
> Or under a single errno like -EIO?

Yes.

> Personally, I believe neither of these two
> solutions are better than returning the actual errno which is deemed most
> appropriate by the platform profile handler. Or am I missing a third way?

Can we please defer this until we actually have a driver needing to
return different error values from ->get_profile() (and I find it hard
to believe that there will be any drivers like that)?

Let's do the simpler thing until we have a real need to do the more
complicated one.

Otherwise we're preparing for things that may never happen.

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

[Maximilian Luz]

On 12/17/20 4:09 PM, Rafael J. Wysocki wrote:
> On Thu, Dec 17, 2020 at 4:02 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>
>> 2020. december 17., csütörtök 14:36 keltezéssel, Rafael J. Wysocki <rafael@kernel.org> írta:
>>
>>> [...]
>>>>>> My thinking here that I shouldn't make assumptions for future platform
>>>>>> implementations - there may be valid cases in the future where being
>>>>>> able to return an error condition if there was an error would be useful.
>>>>>>
>>>>>> Just trying to keep this somewhat future proof. Returning an error
>>>>>> condition seemed a useful thing to have available.
>>>>>
>>>>> You can still return an error while returning a platform_profile_option value.
>>>>>
>>>>> Just add a special value representing an error to that set.
>>>>>
>>>> I'd like to understand what is better about that approach than having an
>>>> error and a returnable parameter?
>>>>
>>>> I'm probably missing something obvious but if I add an extra
>>>> platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
>>>> error case (and return just an enum platform_profile_option) it seems I
>>>> lose the granularity of being able to return a specific error condition.
>>>> It doesn't feel like an improvement.
>>>
>>> And what's the user expected to do about the different error codes
>>> that can be returned?
>>
>> Even assuming the users of the API cannot or will not handle different errors
>> differently, who would benefit from getting rid of this information which may be
>> an aid in debugging for those who know what they're looking at?
>>
>> And if a new enum value is introduced to signal an error condition, how is that
>> going to be communicated to the users? A magic string like "error" or "-1"?
>> Or under a single errno like -EIO?
> 
> Yes.
> 
>> Personally, I believe neither of these two
>> solutions are better than returning the actual errno which is deemed most
>> appropriate by the platform profile handler. Or am I missing a third way?
> 
> Can we please defer this until we actually have a driver needing to
> return different error values from ->get_profile() (and I find it hard
> to believe that there will be any drivers like that)?

I can maybe give an example. On Microsoft Surface devices, the
performance mode / platform profile is set via a request to the embedded
controller and can be queried the same way. This request can fail (most
notably with ETIMEDOUT and EREMOTEIO). I think at least being able to
show users an error in this case would be helpful.

A driver for that is currently at [1], but I haven't adapted that yet to
the platform profile patchset.

On the other hand, I can also query and store the profile when loading
the driver and then update it when it's changed. That'd require that no
one else changes the profile, but I think we can safely assume that.

[1]: https://github.com/linux-surface/surface-aggregator-module/blob/master/module/src/clients/surface_perfmode.c,

Regards,
Max

> 
> Let's do the simpler thing until we have a real need to do the more
> complicated one.
> 
> Otherwise we're preparing for things that may never happen.
> 

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

[Rafael J. Wysocki]

On Thu, Dec 17, 2020 at 4:18 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
>
>
> On 12/17/20 4:09 PM, Rafael J. Wysocki wrote:
> > On Thu, Dec 17, 2020 at 4:02 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>
> >> 2020. december 17., csütörtök 14:36 keltezéssel, Rafael J. Wysocki <rafael@kernel.org> írta:
> >>
> >>> [...]
> >>>>>> My thinking here that I shouldn't make assumptions for future platform
> >>>>>> implementations - there may be valid cases in the future where being
> >>>>>> able to return an error condition if there was an error would be useful.
> >>>>>>
> >>>>>> Just trying to keep this somewhat future proof. Returning an error
> >>>>>> condition seemed a useful thing to have available.
> >>>>>
> >>>>> You can still return an error while returning a platform_profile_option value.
> >>>>>
> >>>>> Just add a special value representing an error to that set.
> >>>>>
> >>>> I'd like to understand what is better about that approach than having an
> >>>> error and a returnable parameter?
> >>>>
> >>>> I'm probably missing something obvious but if I add an extra
> >>>> platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
> >>>> error case (and return just an enum platform_profile_option) it seems I
> >>>> lose the granularity of being able to return a specific error condition.
> >>>> It doesn't feel like an improvement.
> >>>
> >>> And what's the user expected to do about the different error codes
> >>> that can be returned?
> >>
> >> Even assuming the users of the API cannot or will not handle different errors
> >> differently, who would benefit from getting rid of this information which may be
> >> an aid in debugging for those who know what they're looking at?
> >>
> >> And if a new enum value is introduced to signal an error condition, how is that
> >> going to be communicated to the users? A magic string like "error" or "-1"?
> >> Or under a single errno like -EIO?
> >
> > Yes.
> >
> >> Personally, I believe neither of these two
> >> solutions are better than returning the actual errno which is deemed most
> >> appropriate by the platform profile handler. Or am I missing a third way?
> >
> > Can we please defer this until we actually have a driver needing to
> > return different error values from ->get_profile() (and I find it hard
> > to believe that there will be any drivers like that)?
>
> I can maybe give an example. On Microsoft Surface devices, the
> performance mode / platform profile is set via a request to the embedded
> controller and can be queried the same way. This request can fail (most
> notably with ETIMEDOUT and EREMOTEIO). I think at least being able to
> show users an error in this case would be helpful.

OK, fair enough, let's keep it the way it is.