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

[PATCH v5 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

 .../ABI/testing/sysfs-platform_profile.rst    | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform_profile.rst

diff --git a/Documentation/ABI/testing/sysfs-platform_profile.rst b/Documentation/ABI/testing/sysfs-platform_profile.rst
new file mode 100644
index 000000000000..5f7b2a94409b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform_profile.rst
@@ -0,0 +1,66 @@
+=======================================================================
+ 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 this API 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 documentation.
+
+: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.
-- 
2.28.0

[PATCH v5 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

 drivers/acpi/Kconfig             |  14 +++
 drivers/acpi/Makefile            |   1 +
 drivers/acpi/platform_profile.c  | 181 +++++++++++++++++++++++++++++++
 include/linux/platform_profile.h |  39 +++++++
 4 files changed, 235 insertions(+)
 create mode 100644 drivers/acpi/platform_profile.c
 create mode 100644 include/linux/platform_profile.h

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index edf1558c1105..c1ca6255ff85 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -326,6 +326,20 @@ config ACPI_THERMAL
 	  To compile this driver as a module, choose M here:
 	  the module will be called thermal.
 
+config ACPI_PLATFORM_PROFILE
+	tristate "ACPI Platform Profile Driver"
+	default y
+	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..1bc092359e35
--- /dev/null
+++ b/drivers/acpi/platform_profile.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* Platform profile sysfs interface */
+
+#include <linux/acpi.h>
+#include <linux/bits.h>
+#include <linux/init.h>
+#include <linux/mutex.h>
+#include <linux/platform_profile.h>
+#include <linux/sysfs.h>
+
+static const struct platform_profile_handler *cur_profile;
+static DEFINE_MUTEX(profile_lock);
+
+static const char * const profile_names[] = {
+	[platform_profile_low] = "low-power",
+	[platform_profile_cool] = "cool",
+	[platform_profile_quiet] = "quiet",
+	[platform_profile_balanced] = "balanced",
+	[platform_profile_perform] = "performance",
+};
+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_unregister(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_unregister);
+
+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..f2e1b1c90482
--- /dev/null
+++ b/include/linux/platform_profile.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Platform profile sysfs interface
+ *
+ * See Documentation/ABI/testing/sysfs-platform_profile.rst for more
+ * information.
+ */
+
+#ifndef _PLATFORM_PROFILE_H_
+#define _PLATFORM_PROFILE_H_
+
+#include <linux/bitops.h>
+
+/*
+ * If more options are added please update profile_names
+ * array in platform-profile.c and sysfs-platform-profile.rst
+ * documentation.
+ */
+
+enum platform_profile_option {
+	platform_profile_low,
+	platform_profile_cool,
+	platform_profile_quiet,
+	platform_profile_balanced,
+	platform_profile_perform,
+	platform_profile_last, /*must always be last */
+};
+
+struct platform_profile_handler {
+	unsigned long choices[BITS_TO_LONGS(platform_profile_last)];
+	int (*profile_get)(enum platform_profile_option *profile);
+	int (*profile_set)(enum platform_profile_option profile);
+};
+
+int platform_profile_register(const struct platform_profile_handler *pprof);
+int platform_profile_unregister(void);
+void platform_profile_notify(void);
+
+#endif  /*_PLATFORM_PROFILE_H_*/
-- 
2.28.0

[PATCH v5 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

 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..40b2dece9b0e 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_low:
+		*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_unregister();
+	}
+}
+
+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 v5 1/3] Documentation: Add documentation for new platform_profile sysfs attribute

[Hans de Goede]

Hi,

On 12/2/20 6:11 PM, Mark Pearson 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>

Thank you, patches 1 and 2 look good to me now, you may add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

To patch 2 (since I'm co-author of patch 1 it would be a bit weird
to add it there too).

Rafael, it would be great if you pick up patches 1 and 2 for merging
into 5.11 (assuming that you agree that they are ready) then I will merge
patch 3 once 5.11-rc1 is out.

Regards,

Hans



> ---
> Changes in v2:
>  - updated to rst format
> Changes in v3, v4 & v5:
>  - version bump along with rest of patch series
> 
>  .../ABI/testing/sysfs-platform_profile.rst    | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform_profile.rst
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform_profile.rst b/Documentation/ABI/testing/sysfs-platform_profile.rst
> new file mode 100644
> index 000000000000..5f7b2a94409b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform_profile.rst
> @@ -0,0 +1,66 @@
> +=======================================================================
> + 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 this API 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 documentation.
> +
> +: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.
> 

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

[Rafael J. Wysocki]

On Thu, Dec 3, 2020 at 10:46 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/2/20 6:11 PM, Mark Pearson 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>
>
> Thank you, patches 1 and 2 look good to me now, you may add my:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
> To patch 2 (since I'm co-author of patch 1 it would be a bit weird
> to add it there too).
>
> Rafael, it would be great if you pick up patches 1 and 2 for merging
> into 5.11 (assuming that you agree that they are ready) then I will merge
> patch 3 once 5.11-rc1 is out.

I've applied patch [1/2] (as 5.11-rc material) for now, but I still
needed to fix it up somewhat.  Please check the result in my
bleeding-edge branch.

I'll get to the other patch tomorrow.

Thanks!

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

[Hans de Goede]

Hi,

On 12/7/20 8:29 PM, Rafael J. Wysocki wrote:
> On Thu, Dec 3, 2020 at 10:46 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 12/2/20 6:11 PM, Mark Pearson 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>
>>
>> Thank you, patches 1 and 2 look good to me now, you may add my:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>> To patch 2 (since I'm co-author of patch 1 it would be a bit weird
>> to add it there too).
>>
>> Rafael, it would be great if you pick up patches 1 and 2 for merging
>> into 5.11 (assuming that you agree that they are ready) then I will merge
>> patch 3 once 5.11-rc1 is out.
> 
> I've applied patch [1/2] (as 5.11-rc material) for now, but I still
> needed to fix it up somewhat.  Please check the result in my
> bleeding-edge branch.

Thank you.

> I'll get to the other patch tomorrow.

The other patch likely conflicts with a bunch of other thinkpad_acpi
changes already in pdx86/for-next; and I still need to review v5 of
it, so please do not apply it.

I will pick it up after 5.11-rc1 and send it out as part of a
pull-req for rc2.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 12/8/20 10:11 AM, Hans de Goede wrote:
> Hi,
> 
> On 12/7/20 8:29 PM, Rafael J. Wysocki wrote:
>> On Thu, Dec 3, 2020 at 10:46 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 12/2/20 6:11 PM, Mark Pearson 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>
>>>
>>> Thank you, patches 1 and 2 look good to me now, you may add my:
>>>
>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> To patch 2 (since I'm co-author of patch 1 it would be a bit weird
>>> to add it there too).
>>>
>>> Rafael, it would be great if you pick up patches 1 and 2 for merging
>>> into 5.11 (assuming that you agree that they are ready) then I will merge
>>> patch 3 once 5.11-rc1 is out.
>>
>> I've applied patch [1/2] (as 5.11-rc material) for now, but I still
>> needed to fix it up somewhat.  Please check the result in my
>> bleeding-edge branch.
> 
> Thank you.
> 
>> I'll get to the other patch tomorrow.
> 
> The other patch likely conflicts with a bunch of other thinkpad_acpi
> changes already in pdx86/for-next; and I still need to review v5 of
> it, so please do not apply it.
> 
> I will pick it up after 5.11-rc1 and send it out as part of a
> pull-req for rc2.

Sorry I misread what you wrote, when you said you "applied patch [1/2]",
I now see that you have only merged:

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

And that the other patch which you intend to merge is not the
thinkpad_acpi patch but:

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

So please ignore what I said before.

###

I checked out the fixed up version of:
"[PATCH v5 1/3] Documentation: Add documentation for new platform_profile sysfs attribute"

Which you merged and it looks good to me, thanks.

Regards,

Hans

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

[Rafael J. Wysocki]

On Wed, Dec 2, 2020 at 6:16 PM 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
>
>  drivers/acpi/Kconfig             |  14 +++
>  drivers/acpi/Makefile            |   1 +
>  drivers/acpi/platform_profile.c  | 181 +++++++++++++++++++++++++++++++
>  include/linux/platform_profile.h |  39 +++++++
>  4 files changed, 235 insertions(+)
>  create mode 100644 drivers/acpi/platform_profile.c
>  create mode 100644 include/linux/platform_profile.h
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index edf1558c1105..c1ca6255ff85 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -326,6 +326,20 @@ config ACPI_THERMAL
>           To compile this driver as a module, choose M here:
>           the module will be called thermal.
>
> +config ACPI_PLATFORM_PROFILE
> +       tristate "ACPI Platform Profile Driver"
> +       default y

default m

> +       help
> +         This driver adds support for platform-profiles on platforms that
> +         support it.

Empty line here, please.

> +         Platform-profiles can be used to control the platform behaviour. For
> +         example whether to operate in a lower power mode, in a higher
> +         power performance mode or between the two.

And here.

> +         This driver provides the sysfs interface and is used as the registration
> +         point for platform specific drivers.

And here.

> +         Which profiles are supported is determined on a per-platform basis and
> +         should be obtained from the platform specific driver.
> +
>  config ACPI_CUSTOM_DSDT_FILE
>         string "Custom DSDT Table file to include"
>         default ""
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 44e412506317..c64a8af106c0 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -78,6 +78,7 @@ obj-$(CONFIG_ACPI_PCI_SLOT)   += pci_slot.o
>  obj-$(CONFIG_ACPI_PROCESSOR)   += processor.o
>  obj-$(CONFIG_ACPI)             += container.o
>  obj-$(CONFIG_ACPI_THERMAL)     += thermal.o
> +obj-$(CONFIG_ACPI_PLATFORM_PROFILE)    += platform_profile.o
>  obj-$(CONFIG_ACPI_NFIT)                += nfit/
>  obj-$(CONFIG_ACPI_NUMA)                += numa/
>  obj-$(CONFIG_ACPI)             += acpi_memhotplug.o
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> new file mode 100644
> index 000000000000..1bc092359e35
> --- /dev/null
> +++ b/drivers/acpi/platform_profile.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/* Platform profile sysfs interface */
> +
> +#include <linux/acpi.h>
> +#include <linux/bits.h>
> +#include <linux/init.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_profile.h>
> +#include <linux/sysfs.h>
> +
> +static const struct platform_profile_handler *cur_profile;
> +static DEFINE_MUTEX(profile_lock);
> +
> +static const char * const profile_names[] = {
> +       [platform_profile_low] = "low-power",
> +       [platform_profile_cool] = "cool",
> +       [platform_profile_quiet] = "quiet",
> +       [platform_profile_balanced] = "balanced",
> +       [platform_profile_perform] = "performance",

The enum values in upper case, please.

> +};
> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last);
> +
> +static ssize_t platform_profile_choices_show(struct device *dev,
> +                                       struct device_attribute *attr,
> +                                       char *buf)
> +{
> +       int len = 0;
> +       int err, i;
> +
> +       err = mutex_lock_interruptible(&profile_lock);

Why interruptible?

And why is the lock needed in the first place?

> +       if (err)
> +               return err;
> +
> +       if (!cur_profile) {
> +               mutex_unlock(&profile_lock);
> +               return -ENODEV;
> +       }
> +
> +       for_each_set_bit(i, cur_profile->choices, platform_profile_last) {
> +               if (len == 0)
> +                       len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
> +               else
> +                       len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
> +       }
> +       len += sysfs_emit_at(buf, len, "\n");
> +       mutex_unlock(&profile_lock);
> +       return len;
> +}
> +
> +static ssize_t platform_profile_show(struct device *dev,
> +                                       struct device_attribute *attr,
> +                                       char *buf)
> +{
> +       enum platform_profile_option profile = platform_profile_balanced;
> +       int err;
> +
> +       err = mutex_lock_interruptible(&profile_lock);
> +       if (err)
> +               return err;
> +
> +       if (!cur_profile) {
> +               mutex_unlock(&profile_lock);
> +               return -ENODEV;
> +       }
> +
> +       err = cur_profile->profile_get(&profile);

In which cases this can fail?

> +       mutex_unlock(&profile_lock);
> +       if (err)
> +               return err;
> +
> +       /* Check that profile is valid index */
> +       if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
> +               return -EIO;
> +
> +       return sysfs_emit(buf, "%s\n", profile_names[profile]);
> +}
> +
> +static ssize_t platform_profile_store(struct device *dev,
> +                           struct device_attribute *attr,
> +                           const char *buf, size_t count)
> +{
> +       int err, i;
> +
> +       err = mutex_lock_interruptible(&profile_lock);
> +       if (err)
> +               return err;
> +
> +       if (!cur_profile) {
> +               mutex_unlock(&profile_lock);
> +               return -ENODEV;
> +       }
> +
> +       /* Scan for a matching profile */
> +       i = sysfs_match_string(profile_names, buf);
> +       if (i < 0) {
> +               mutex_unlock(&profile_lock);
> +               return -EINVAL;
> +       }
> +
> +       /* Check that platform supports this profile choice */
> +       if (!test_bit(i, cur_profile->choices)) {
> +               mutex_unlock(&profile_lock);
> +               return -EOPNOTSUPP;
> +       }
> +
> +       err = cur_profile->profile_set(i);

What if this gets a signal in the middle of the ->profile_set()
execution?  Is this always guaranteed to work?

> +       mutex_unlock(&profile_lock);
> +       if (err)
> +               return err;
> +       return count;
> +}
> +
> +static DEVICE_ATTR_RO(platform_profile_choices);
> +static DEVICE_ATTR_RW(platform_profile);
> +
> +static struct attribute *platform_profile_attrs[] = {
> +       &dev_attr_platform_profile_choices.attr,
> +       &dev_attr_platform_profile.attr,
> +       NULL
> +};
> +
> +static const struct attribute_group platform_profile_group = {
> +       .attrs = platform_profile_attrs
> +};
> +
> +void platform_profile_notify(void)
> +{
> +       if (!cur_profile)
> +               return;
> +       sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +}
> +EXPORT_SYMBOL_GPL(platform_profile_notify);
> +
> +int platform_profile_register(const struct platform_profile_handler *pprof)
> +{
> +       int err;
> +
> +       mutex_lock(&profile_lock);
> +       /* We can only have one active profile */
> +       if (cur_profile) {
> +               mutex_unlock(&profile_lock);
> +               return -EEXIST;
> +       }
> +
> +       /* Sanity check the profile handler field are set */
> +       if (!pprof || !pprof->choices || !pprof->profile_set ||
> +                       !pprof->profile_get) {
> +               mutex_unlock(&profile_lock);
> +               return -EINVAL;
> +       }
> +
> +       err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> +       if (err) {
> +               mutex_unlock(&profile_lock);
> +               return err;
> +       }
> +
> +       cur_profile = pprof;
> +       mutex_unlock(&profile_lock);
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(platform_profile_register);
> +
> +int platform_profile_unregister(void)

"Unregister" functions typically take an argument pointing to the
target object, so something like platform_profile_remove() may be a
better choice here.

> +{
> +       mutex_lock(&profile_lock);
> +       if (!cur_profile) {
> +               mutex_unlock(&profile_lock);
> +               return -ENODEV;
> +       }
> +
> +       sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +       cur_profile = NULL;
> +       mutex_unlock(&profile_lock);
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(platform_profile_unregister);
> +
> +MODULE_AUTHOR("Mark Pearson <markpearson@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..f2e1b1c90482
> --- /dev/null
> +++ b/include/linux/platform_profile.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Platform profile sysfs interface
> + *
> + * See Documentation/ABI/testing/sysfs-platform_profile.rst for more
> + * information.
> + */
> +
> +#ifndef _PLATFORM_PROFILE_H_
> +#define _PLATFORM_PROFILE_H_
> +
> +#include <linux/bitops.h>
> +
> +/*
> + * If more options are added please update profile_names
> + * array in platform-profile.c and sysfs-platform-profile.rst
> + * documentation.
> + */
> +
> +enum platform_profile_option {
> +       platform_profile_low,
> +       platform_profile_cool,
> +       platform_profile_quiet,
> +       platform_profile_balanced,
> +       platform_profile_perform,
> +       platform_profile_last, /*must always be last */
> +};
> +
> +struct platform_profile_handler {
> +       unsigned long choices[BITS_TO_LONGS(platform_profile_last)];
> +       int (*profile_get)(enum platform_profile_option *profile);
> +       int (*profile_set)(enum platform_profile_option profile);
> +};
> +
> +int platform_profile_register(const struct platform_profile_handler *pprof);
> +int platform_profile_unregister(void);
> +void platform_profile_notify(void);
> +
> +#endif  /*_PLATFORM_PROFILE_H_*/
> --

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

[Rafael J. Wysocki]

On Tue, Dec 8, 2020 at 7:26 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@lenovo.com> wrote:

[cut]

> > +
> > +static ssize_t platform_profile_store(struct device *dev,
> > +                           struct device_attribute *attr,
> > +                           const char *buf, size_t count)
> > +{
> > +       int err, i;
> > +
> > +       err = mutex_lock_interruptible(&profile_lock);
> > +       if (err)
> > +               return err;
> > +
> > +       if (!cur_profile) {
> > +               mutex_unlock(&profile_lock);
> > +               return -ENODEV;
> > +       }
> > +
> > +       /* Scan for a matching profile */
> > +       i = sysfs_match_string(profile_names, buf);
> > +       if (i < 0) {
> > +               mutex_unlock(&profile_lock);
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* Check that platform supports this profile choice */
> > +       if (!test_bit(i, cur_profile->choices)) {
> > +               mutex_unlock(&profile_lock);
> > +               return -EOPNOTSUPP;
> > +       }
> > +
> > +       err = cur_profile->profile_set(i);
>
> What if this gets a signal in the middle of the ->profile_set()
> execution?  Is this always guaranteed to work?

I got this backwards, sorry.

The "interruptible" variant is used to allow the waiters to be
interrupted, so I guess the concern is that ->profile_set() may get
stuck or just take too much time?

> > +       mutex_unlock(&profile_lock);
> > +       if (err)
> > +               return err;
> > +       return count;
> > +}
> > +

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

[Mark Pearson]

Hi Rafael,

Thanks for the review - a couple of questions (and a bunch of acks) below

On 08/12/2020 13:26, Rafael J. Wysocki wrote:
> On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@lenovo.com> wrote:
>>
<snip>
>>
>>  drivers/acpi/Kconfig             |  14 +++
>>  drivers/acpi/Makefile            |   1 +
>>  drivers/acpi/platform_profile.c  | 181 +++++++++++++++++++++++++++++++
>>  include/linux/platform_profile.h |  39 +++++++
>>  4 files changed, 235 insertions(+)
>>  create mode 100644 drivers/acpi/platform_profile.c
>>  create mode 100644 include/linux/platform_profile.h
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index edf1558c1105..c1ca6255ff85 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -326,6 +326,20 @@ config ACPI_THERMAL
>>           To compile this driver as a module, choose M here:
>>           the module will be called thermal.
>>
>> +config ACPI_PLATFORM_PROFILE
>> +       tristate "ACPI Platform Profile Driver"
>> +       default y
> 
> default m
OK
> 
>> +       help
>> +         This driver adds support for platform-profiles on platforms that
>> +         support it.
> 
> Empty line here, please.
Ack
> 
>> +         Platform-profiles can be used to control the platform behaviour. For
>> +         example whether to operate in a lower power mode, in a higher
>> +         power performance mode or between the two.
> 
> And here.
Ack
> 
>> +         This driver provides the sysfs interface and is used as the registration
>> +         point for platform specific drivers.
> 
> And here.
Ack
> 
>> +         Which profiles are supported is determined on a per-platform basis and
>> +         should be obtained from the platform specific driver.
>> +
>>  config ACPI_CUSTOM_DSDT_FILE
>>         string "Custom DSDT Table file to include"
>>         default ""
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 44e412506317..c64a8af106c0 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -78,6 +78,7 @@ obj-$(CONFIG_ACPI_PCI_SLOT)   += pci_slot.o
>>  obj-$(CONFIG_ACPI_PROCESSOR)   += processor.o
>>  obj-$(CONFIG_ACPI)             += container.o
>>  obj-$(CONFIG_ACPI_THERMAL)     += thermal.o
>> +obj-$(CONFIG_ACPI_PLATFORM_PROFILE)    += platform_profile.o
>>  obj-$(CONFIG_ACPI_NFIT)                += nfit/
>>  obj-$(CONFIG_ACPI_NUMA)                += numa/
>>  obj-$(CONFIG_ACPI)             += acpi_memhotplug.o
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> new file mode 100644
>> index 000000000000..1bc092359e35
>> --- /dev/null
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -0,0 +1,181 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +/* Platform profile sysfs interface */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/bits.h>
>> +#include <linux/init.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_profile.h>
>> +#include <linux/sysfs.h>
>> +
>> +static const struct platform_profile_handler *cur_profile;
>> +static DEFINE_MUTEX(profile_lock);
>> +
>> +static const char * const profile_names[] = {
>> +       [platform_profile_low] = "low-power",
>> +       [platform_profile_cool] = "cool",
>> +       [platform_profile_quiet] = "quiet",
>> +       [platform_profile_balanced] = "balanced",
>> +       [platform_profile_perform] = "performance",
> 
> The enum values in upper case, please.
Sorry, I'm a bit confused here - do you mean change to "Low-power" or
something else (maybe PLATFORM_PROFILE_LOW?)

Just want to make sure I'm getting it correct. If I change the strings
it will impact patch1 in the series which is integrated into your
bleeding-edge branch.

> 
>> +};
>> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last);
>> +
>> +static ssize_t platform_profile_choices_show(struct device *dev,
>> +                                       struct device_attribute *attr,
>> +                                       char *buf)
>> +{
>> +       int len = 0;
>> +       int err, i;
>> +
>> +       err = mutex_lock_interruptible(&profile_lock);
> 
> Why interruptible?
> 
> And why is the lock needed in the first place?

My thinking was that I don't know what happens when I hand over to thhe
platform driver who does the get/set, so having a lock to prevent a get
whilst a set is in operation seemed like a good idea.

It was interruptible as a suggestion in an earlier reivew as the
preferred way of doing these things for functions that could be called
by user space.

Do you think the lock is a problem?

> 
>> +       if (err)
>> +               return err;
>> +
>> +       if (!cur_profile) {
>> +               mutex_unlock(&profile_lock);
>> +               return -ENODEV;
>> +       }
>> +
>> +       for_each_set_bit(i, cur_profile->choices, platform_profile_last) {
>> +               if (len == 0)
>> +                       len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
>> +               else
>> +                       len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
>> +       }
>> +       len += sysfs_emit_at(buf, len, "\n");
>> +       mutex_unlock(&profile_lock);
>> +       return len;
>> +}
>> +
>> +static ssize_t platform_profile_show(struct device *dev,
>> +                                       struct device_attribute *attr,
>> +                                       char *buf)
>> +{
>> +       enum platform_profile_option profile = platform_profile_balanced;
>> +       int err;
>> +
>> +       err = mutex_lock_interruptible(&profile_lock);
>> +       if (err)
>> +               return err;
>> +
>> +       if (!cur_profile) {
>> +               mutex_unlock(&profile_lock);
>> +               return -ENODEV;
>> +       }
>> +
>> +       err = cur_profile->profile_get(&profile);
> 
> In which cases this can fail?
I'm not sure - but as this is supposed to be vendor agnostic I can't
foresee what might be wanted or could happen on various hardware. I
agree a failure is probably unlikely in the Lenovo case where we're
doing an ACPI call, but is there any issue in handling error codes?
It doesn't seem to gain much by removing it and may have future impacts.
> 
>> +       mutex_unlock(&profile_lock);
>> +       if (err)
>> +               return err;
>> +
>> +       /* Check that profile is valid index */
>> +       if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
>> +               return -EIO;
>> +
>> +       return sysfs_emit(buf, "%s\n", profile_names[profile]);
>> +}
>> +
>> +static ssize_t platform_profile_store(struct device *dev,
>> +                           struct device_attribute *attr,
>> +                           const char *buf, size_t count)
>> +{
>> +       int err, i;
>> +
>> +       err = mutex_lock_interruptible(&profile_lock);
>> +       if (err)
>> +               return err;
>> +
>> +       if (!cur_profile) {
>> +               mutex_unlock(&profile_lock);
>> +               return -ENODEV;
>> +       }
>> +
>> +       /* Scan for a matching profile */
>> +       i = sysfs_match_string(profile_names, buf);
>> +       if (i < 0) {
>> +               mutex_unlock(&profile_lock);
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* Check that platform supports this profile choice */
>> +       if (!test_bit(i, cur_profile->choices)) {
>> +               mutex_unlock(&profile_lock);
>> +               return -EOPNOTSUPP;
>> +       }
>> +
>> +       err = cur_profile->profile_set(i);
> 
> What if this gets a signal in the middle of the ->profile_set()
> execution?  Is this always guaranteed to work?
I'm afraid I don't know the answer to this one. What would be the
recommendation to cover this event?

> 
>> +       mutex_unlock(&profile_lock);
>> +       if (err)
>> +               return err;
>> +       return count;
>> +}
>> +
>> +static DEVICE_ATTR_RO(platform_profile_choices);
>> +static DEVICE_ATTR_RW(platform_profile);
>> +
>> +static struct attribute *platform_profile_attrs[] = {
>> +       &dev_attr_platform_profile_choices.attr,
>> +       &dev_attr_platform_profile.attr,
>> +       NULL
>> +};
>> +
>> +static const struct attribute_group platform_profile_group = {
>> +       .attrs = platform_profile_attrs
>> +};
>> +
>> +void platform_profile_notify(void)
>> +{
>> +       if (!cur_profile)
>> +               return;
>> +       sysfs_notify(acpi_kobj, NULL, "platform_profile");
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_notify);
>> +
>> +int platform_profile_register(const struct platform_profile_handler *pprof)
>> +{
>> +       int err;
>> +
>> +       mutex_lock(&profile_lock);
>> +       /* We can only have one active profile */
>> +       if (cur_profile) {
>> +               mutex_unlock(&profile_lock);
>> +               return -EEXIST;
>> +       }
>> +
>> +       /* Sanity check the profile handler field are set */
>> +       if (!pprof || !pprof->choices || !pprof->profile_set ||
>> +                       !pprof->profile_get) {
>> +               mutex_unlock(&profile_lock);
>> +               return -EINVAL;
>> +       }
>> +
>> +       err = sysfs_create_group(acpi_kobj, &platform_profile_group);
>> +       if (err) {
>> +               mutex_unlock(&profile_lock);
>> +               return err;
>> +       }
>> +
>> +       cur_profile = pprof;
>> +       mutex_unlock(&profile_lock);
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_register);
>> +
>> +int platform_profile_unregister(void)
> 
> "Unregister" functions typically take an argument pointing to the
> target object, so something like platform_profile_remove() may be a
> better choice here.
Sure - happy to change that

> 
>> +{
>> +       mutex_lock(&profile_lock);
>> +       if (!cur_profile) {
>> +               mutex_unlock(&profile_lock);
>> +               return -ENODEV;
>> +       }
>> +
>> +       sysfs_remove_group(acpi_kobj, &platform_profile_group);
>> +       cur_profile = NULL;
>> +       mutex_unlock(&profile_lock);
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_unregister);
>> +
>> +MODULE_AUTHOR("Mark Pearson <markpearson@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..f2e1b1c90482
>> --- /dev/null
>> +++ b/include/linux/platform_profile.h
>> @@ -0,0 +1,39 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Platform profile sysfs interface
>> + *
>> + * See Documentation/ABI/testing/sysfs-platform_profile.rst for more
>> + * information.
>> + */
>> +
>> +#ifndef _PLATFORM_PROFILE_H_
>> +#define _PLATFORM_PROFILE_H_
>> +
>> +#include <linux/bitops.h>
>> +
>> +/*
>> + * If more options are added please update profile_names
>> + * array in platform-profile.c and sysfs-platform-profile.rst
>> + * documentation.
>> + */
>> +
>> +enum platform_profile_option {
>> +       platform_profile_low,
>> +       platform_profile_cool,
>> +       platform_profile_quiet,
>> +       platform_profile_balanced,
>> +       platform_profile_perform,
>> +       platform_profile_last, /*must always be last */
>> +};
>> +
>> +struct platform_profile_handler {
>> +       unsigned long choices[BITS_TO_LONGS(platform_profile_last)];
>> +       int (*profile_get)(enum platform_profile_option *profile);
>> +       int (*profile_set)(enum platform_profile_option profile);
>> +};
>> +
>> +int platform_profile_register(const struct platform_profile_handler *pprof);
>> +int platform_profile_unregister(void);
>> +void platform_profile_notify(void);
>> +
>> +#endif  /*_PLATFORM_PROFILE_H_*/
>> --
Thanks
Mark

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

[Rafael J. Wysocki]

On Tue, Dec 8, 2020 at 7:54 PM Mark Pearson <markpearson@lenovo.com> wrote:
>
> Hi Rafael,
>
> Thanks for the review - a couple of questions (and a bunch of acks) below
>
> On 08/12/2020 13:26, Rafael J. Wysocki wrote:
> > On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@lenovo.com> wrote:

[cut]

> >> +static const struct platform_profile_handler *cur_profile;
> >> +static DEFINE_MUTEX(profile_lock);
> >> +
> >> +static const char * const profile_names[] = {
> >> +       [platform_profile_low] = "low-power",
> >> +       [platform_profile_cool] = "cool",
> >> +       [platform_profile_quiet] = "quiet",
> >> +       [platform_profile_balanced] = "balanced",
> >> +       [platform_profile_perform] = "performance",
> >
> > The enum values in upper case, please.
> Sorry, I'm a bit confused here - do you mean change to "Low-power" or
> something else (maybe PLATFORM_PROFILE_LOW?)

platform_profile_low -> PLATFORM_PROFILE_LOW
platform_profile_cool -> PLATFORM_PROFILE_COOL

etc.

> Just want to make sure I'm getting it correct. If I change the strings
> it will impact patch1 in the series which is integrated into your
> bleeding-edge branch.
>
> >
> >> +};
> >> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last);
> >> +
> >> +static ssize_t platform_profile_choices_show(struct device *dev,
> >> +                                       struct device_attribute *attr,
> >> +                                       char *buf)
> >> +{
> >> +       int len = 0;
> >> +       int err, i;
> >> +
> >> +       err = mutex_lock_interruptible(&profile_lock);
> >
> > Why interruptible?
> >
> > And why is the lock needed in the first place?
>
> My thinking was that I don't know what happens when I hand over to thhe
> platform driver who does the get/set, so having a lock to prevent a get
> whilst a set is in operation seemed like a good idea.

Taking it over get/set probably is (and you need to protect the
cur_profile pointer from concurrent updates).

And here you need to ensure that the cur_profile object doesn't go
away while this is running.  So that's why.

> It was interruptible as a suggestion in an earlier reivew as the
> preferred way of doing these things for functions that could be called
> by user space.

Well, it is not used consistently this way at least.  But OK.

> Do you think the lock is a problem?

No, it isn't in principle.

> >
> >> +       if (err)
> >> +               return err;
> >> +
> >> +       if (!cur_profile) {
> >> +               mutex_unlock(&profile_lock);
> >> +               return -ENODEV;
> >> +       }
> >> +
> >> +       for_each_set_bit(i, cur_profile->choices, platform_profile_last) {
> >> +               if (len == 0)
> >> +                       len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
> >> +               else
> >> +                       len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
> >> +       }
> >> +       len += sysfs_emit_at(buf, len, "\n");
> >> +       mutex_unlock(&profile_lock);
> >> +       return len;
> >> +}
> >> +
> >> +static ssize_t platform_profile_show(struct device *dev,
> >> +                                       struct device_attribute *attr,
> >> +                                       char *buf)
> >> +{
> >> +       enum platform_profile_option profile = platform_profile_balanced;
> >> +       int err;
> >> +
> >> +       err = mutex_lock_interruptible(&profile_lock);
> >> +       if (err)
> >> +               return err;
> >> +
> >> +       if (!cur_profile) {
> >> +               mutex_unlock(&profile_lock);
> >> +               return -ENODEV;
> >> +       }
> >> +
> >> +       err = cur_profile->profile_get(&profile);
> >
> > In which cases this can fail?
> I'm not sure - but as this is supposed to be vendor agnostic I can't
> foresee what might be wanted or could happen on various hardware.

It returns the index of the current profile AFAICS, so I don't really
see a reason for it to fail.

Moreover, the index could be maintained by the common code along with
the cur_profile pointer, couldn't it?

> I agree a failure is probably unlikely in the Lenovo case where we're
> doing an ACPI call, but is there any issue in handling error codes?
> It doesn't seem to gain much by removing it and may have future impacts.
> >
> >> +       mutex_unlock(&profile_lock);
> >> +       if (err)
> >> +               return err;
> >> +
> >> +       /* Check that profile is valid index */
> >> +       if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names))))
> >> +               return -EIO;
> >> +
> >> +       return sysfs_emit(buf, "%s\n", profile_names[profile]);
> >> +}
> >> +
> >> +static ssize_t platform_profile_store(struct device *dev,
> >> +                           struct device_attribute *attr,
> >> +                           const char *buf, size_t count)
> >> +{
> >> +       int err, i;
> >> +
> >> +       err = mutex_lock_interruptible(&profile_lock);
> >> +       if (err)
> >> +               return err;
> >> +
> >> +       if (!cur_profile) {
> >> +               mutex_unlock(&profile_lock);
> >> +               return -ENODEV;
> >> +       }
> >> +
> >> +       /* Scan for a matching profile */
> >> +       i = sysfs_match_string(profile_names, buf);
> >> +       if (i < 0) {
> >> +               mutex_unlock(&profile_lock);
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       /* Check that platform supports this profile choice */
> >> +       if (!test_bit(i, cur_profile->choices)) {
> >> +               mutex_unlock(&profile_lock);
> >> +               return -EOPNOTSUPP;
> >> +       }
> >> +
> >> +       err = cur_profile->profile_set(i);
> >
> > What if this gets a signal in the middle of the ->profile_set()
> > execution?  Is this always guaranteed to work?
> I'm afraid I don't know the answer to this one. What would be the
> recommendation to cover this event?

Never mind, this was a mistake of mine.

> >
> >> +       mutex_unlock(&profile_lock);
> >> +       if (err)
> >> +               return err;
> >> +       return count;
> >> +}
> >> +
> >> +static DEVICE_ATTR_RO(platform_profile_choices);
> >> +static DEVICE_ATTR_RW(platform_profile);
> >> +
> >> +static struct attribute *platform_profile_attrs[] = {
> >> +       &dev_attr_platform_profile_choices.attr,
> >> +       &dev_attr_platform_profile.attr,
> >> +       NULL
> >> +};
> >> +
> >> +static const struct attribute_group platform_profile_group = {
> >> +       .attrs = platform_profile_attrs
> >> +};
> >> +
> >> +void platform_profile_notify(void)
> >> +{
> >> +       if (!cur_profile)
> >> +               return;
> >> +       sysfs_notify(acpi_kobj, NULL, "platform_profile");
> >> +}
> >> +EXPORT_SYMBOL_GPL(platform_profile_notify);
> >> +
> >> +int platform_profile_register(const struct platform_profile_handler *pprof)
> >> +{
> >> +       int err;
> >> +
> >> +       mutex_lock(&profile_lock);
> >> +       /* We can only have one active profile */
> >> +       if (cur_profile) {
> >> +               mutex_unlock(&profile_lock);
> >> +               return -EEXIST;
> >> +       }
> >> +
> >> +       /* Sanity check the profile handler field are set */
> >> +       if (!pprof || !pprof->choices || !pprof->profile_set ||
> >> +                       !pprof->profile_get) {
> >> +               mutex_unlock(&profile_lock);
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> >> +       if (err) {
> >> +               mutex_unlock(&profile_lock);
> >> +               return err;
> >> +       }
> >> +
> >> +       cur_profile = pprof;
> >> +       mutex_unlock(&profile_lock);
> >> +       return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(platform_profile_register);
> >> +
> >> +int platform_profile_unregister(void)
> >
> > "Unregister" functions typically take an argument pointing to the
> > target object, so something like platform_profile_remove() may be a
> > better choice here.
> Sure - happy to change that
>
> >
> >> +{
> >> +       mutex_lock(&profile_lock);
> >> +       if (!cur_profile) {
> >> +               mutex_unlock(&profile_lock);
> >> +               return -ENODEV;
> >> +       }
> >> +
> >> +       sysfs_remove_group(acpi_kobj, &platform_profile_group);
> >> +       cur_profile = NULL;
> >> +       mutex_unlock(&profile_lock);
> >> +       return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(platform_profile_unregister);
> >> +
> >> +MODULE_AUTHOR("Mark Pearson <markpearson@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..f2e1b1c90482
> >> --- /dev/null
> >> +++ b/include/linux/platform_profile.h
> >> @@ -0,0 +1,39 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Platform profile sysfs interface
> >> + *
> >> + * See Documentation/ABI/testing/sysfs-platform_profile.rst for more
> >> + * information.
> >> + */
> >> +
> >> +#ifndef _PLATFORM_PROFILE_H_
> >> +#define _PLATFORM_PROFILE_H_
> >> +
> >> +#include <linux/bitops.h>
> >> +
> >> +/*
> >> + * If more options are added please update profile_names
> >> + * array in platform-profile.c and sysfs-platform-profile.rst
> >> + * documentation.
> >> + */
> >> +
> >> +enum platform_profile_option {
> >> +       platform_profile_low,
> >> +       platform_profile_cool,
> >> +       platform_profile_quiet,
> >> +       platform_profile_balanced,
> >> +       platform_profile_perform,
> >> +       platform_profile_last, /*must always be last */

So please use upper-case names in this list.

> >> +};
> >> +
> >> +struct platform_profile_handler {
> >> +       unsigned long choices[BITS_TO_LONGS(platform_profile_last)];
> >> +       int (*profile_get)(enum platform_profile_option *profile);
> >> +       int (*profile_set)(enum platform_profile_option profile);
> >> +};
> >> +
> >> +int platform_profile_register(const struct platform_profile_handler *pprof);
> >> +int platform_profile_unregister(void);
> >> +void platform_profile_notify(void);
> >> +
> >> +#endif  /*_PLATFORM_PROFILE_H_*/
> >> --

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

[Mark Pearson]

On 08/12/2020 14:18, Rafael J. Wysocki wrote:
> On Tue, Dec 8, 2020 at 7:54 PM Mark Pearson <markpearson@lenovo.com> wrote:
>>
>> Hi Rafael,
>>
>> Thanks for the review - a couple of questions (and a bunch of acks) below
>>
>> On 08/12/2020 13:26, Rafael J. Wysocki wrote:
>>> On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@lenovo.com> wrote:
> 
> [cut]
> 
>>>> +static const struct platform_profile_handler *cur_profile;
>>>> +static DEFINE_MUTEX(profile_lock);
>>>> +
>>>> +static const char * const profile_names[] = {
>>>> +       [platform_profile_low] = "low-power",
>>>> +       [platform_profile_cool] = "cool",
>>>> +       [platform_profile_quiet] = "quiet",
>>>> +       [platform_profile_balanced] = "balanced",
>>>> +       [platform_profile_perform] = "performance",
>>>
>>> The enum values in upper case, please.
>> Sorry, I'm a bit confused here - do you mean change to "Low-power" or
>> something else (maybe PLATFORM_PROFILE_LOW?)
> 
> platform_profile_low -> PLATFORM_PROFILE_LOW
> platform_profile_cool -> PLATFORM_PROFILE_COOL
> 
> etc.
> 
Got it - I'll update. Thanks for the clarification

>> Just want to make sure I'm getting it correct. If I change the strings
>> it will impact patch1 in the series which is integrated into your
>> bleeding-edge branch.
>>
>>>
>>>> +};
>>>> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last);
>>>> +
>>>> +static ssize_t platform_profile_choices_show(struct device *dev,
>>>> +                                       struct device_attribute *attr,
>>>> +                                       char *buf)
>>>> +{
>>>> +       int len = 0;
>>>> +       int err, i;
>>>> +
>>>> +       err = mutex_lock_interruptible(&profile_lock);
>>>
>>> Why interruptible?
>>>
>>> And why is the lock needed in the first place?
>>
>> My thinking was that I don't know what happens when I hand over to thhe
>> platform driver who does the get/set, so having a lock to prevent a get
>> whilst a set is in operation seemed like a good idea.
> 
> Taking it over get/set probably is (and you need to protect the
> cur_profile pointer from concurrent updates).
> 
> And here you need to ensure that the cur_profile object doesn't go
> away while this is running.  So that's why.
> 
>> It was interruptible as a suggestion in an earlier reivew as the
>> preferred way of doing these things for functions that could be called
>> by user space.
> 
> Well, it is not used consistently this way at least.  But OK.

That was based on review comments - interruptible used where it could be
accessed from user space and not where it couldn't. Hans made some good
notes about it previously.
> 
>> Do you think the lock is a problem?
> 
> No, it isn't in principle.
> 
>>>
>>>> +       if (err)
>>>> +               return err;
>>>> +
>>>> +       if (!cur_profile) {
>>>> +               mutex_unlock(&profile_lock);
>>>> +               return -ENODEV;
>>>> +       }
>>>> +
>>>> +       for_each_set_bit(i, cur_profile->choices, platform_profile_last) {
>>>> +               if (len == 0)
>>>> +                       len += sysfs_emit_at(buf, len, "%s", profile_names[i]);
>>>> +               else
>>>> +                       len += sysfs_emit_at(buf, len, " %s", profile_names[i]);
>>>> +       }
>>>> +       len += sysfs_emit_at(buf, len, "\n");
>>>> +       mutex_unlock(&profile_lock);
>>>> +       return len;
>>>> +}
>>>> +
>>>> +static ssize_t platform_profile_show(struct device *dev,
>>>> +                                       struct device_attribute *attr,
>>>> +                                       char *buf)
>>>> +{
>>>> +       enum platform_profile_option profile = platform_profile_balanced;
>>>> +       int err;
>>>> +
>>>> +       err = mutex_lock_interruptible(&profile_lock);
>>>> +       if (err)
>>>> +               return err;
>>>> +
>>>> +       if (!cur_profile) {
>>>> +               mutex_unlock(&profile_lock);
>>>> +               return -ENODEV;
>>>> +       }
>>>> +
>>>> +       err = cur_profile->profile_get(&profile);
>>>
>>> In which cases this can fail?
>> I'm not sure - but as this is supposed to be vendor agnostic I can't
>> foresee what might be wanted or could happen on various hardware.
> 
> It returns the index of the current profile AFAICS, so I don't really
> see a reason for it to fail.
> 
> Moreover, the index could be maintained by the common code along with
> the cur_profile pointer, couldn't it?
OK - I see your point.

I think that it's good to check for an error from the driver and to not
display a potentially incorrect value in the case of an error.

I double checked and in the Lenovo case (patch 3) all of this works
exactly as you describe (so my previous comment was incorrect). The
value is cached in thinkpad_acpi and there can't be an error returned.
But it seems wrong to make assumptions on others wanting to do the same
in the future as their implementation might have different constraints.

Let me know if you feel strongly about this and I can update, from my
point of view I lean towards leaving it as it is but it doesn't impact
the Lenovo implementation.

> 
<snip>>>>> +
>>>> +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 */
> 
> So please use upper-case names in this list.
> 
Ack

Thanks for the clarifications
Mark