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

[PATCH v4 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:
 - 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 v4 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_*)

 drivers/acpi/Kconfig             |  14 ++
 drivers/acpi/Makefile            |   1 +
 drivers/acpi/platform_profile.c  | 215 +++++++++++++++++++++++++++++++
 include/linux/platform_profile.h |  36 ++++++
 4 files changed, 266 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..678cb4596ada
--- /dev/null
+++ b/drivers/acpi/platform_profile.c
@@ -0,0 +1,215 @@
+// 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_balance] = "balance",
+	[platform_profile_perform] = "performance",
+};
+static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1);
+
+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;
+	}
+
+	if (!cur_profile->choices) {
+		mutex_unlock(&profile_lock);
+		return sysfs_emit(buf, "\n");
+	}
+
+	for (i = 0; i < ARRAY_SIZE(profile_names); i++) {
+		if (cur_profile->choices & BIT(i)) {
+			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_balance;
+	int err;
+
+	err = mutex_lock_interruptible(&profile_lock);
+	if (err)
+		return err;
+
+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -ENODEV;
+	}
+
+	if (!cur_profile->profile_get) {
+		mutex_unlock(&profile_lock);
+		return -EOPNOTSUPP;
+	}
+
+	err = cur_profile->profile_get(&profile);
+	mutex_unlock(&profile_lock);
+	if (err < 0)
+		return err;
+
+	/* Check that profile is valid index */
+	if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))
+		return sysfs_emit(buf, "\n");
+
+	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;
+	}
+
+	if (!cur_profile->profile_set) {
+		mutex_unlock(&profile_lock);
+		return -EOPNOTSUPP;
+	}
+
+	/* 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 (!(cur_profile->choices & BIT(i))) {
+		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;
+
+	err = mutex_lock_interruptible(&profile_lock);
+	if (err)
+		return err;
+
+	/* We can only have one active profile */
+	if (cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -EEXIST;
+	}
+
+	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)
+{
+	int err;
+
+	err = mutex_lock_interruptible(&profile_lock);
+	if (err)
+		return err;
+
+	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);
+
+static int __init platform_profile_init(void)
+{
+	return 0;
+}
+module_init(platform_profile_init);
+
+static void __exit platform_profile_exit(void)
+{
+	/* Check if we have a registered profile, and clean up */
+	if (cur_profile) {
+		sysfs_remove_group(acpi_kobj, &platform_profile_group);
+		cur_profile = NULL;
+	}
+}
+module_exit(platform_profile_exit);
+
+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..33ccd40bb9cf
--- /dev/null
+++ b/include/linux/platform_profile.h
@@ -0,0 +1,36 @@
+/* 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_
+
+/*
+ * 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_balance,
+	platform_profile_perform,
+};
+
+struct platform_profile_handler {
+	unsigned int choices; /* Bitmap of available choices */
+	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

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

[Barnabás Pőcze]

Hi


2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta:

> [...]
+static const char * const profile_names[] = {
+	[platform_profile_low] = "low-power",
+	[platform_profile_cool] = "cool",
+	[platform_profile_quiet] = "quiet",
+	[platform_profile_balance] = "balance",

Documentation says "balanced".


+	[platform_profile_perform] = "performance",
+};
> [...]
> +static ssize_t platform_profile_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	enum platform_profile_option profile = platform_profile_balance;
> +	int err;
> +
> +	err = mutex_lock_interruptible(&profile_lock);
> +	if (err)
> +		return err;
> +
> +	if (!cur_profile) {
> +		mutex_unlock(&profile_lock);
> +		return -ENODEV;
> +	}
> +
> +	if (!cur_profile->profile_get) {
> +		mutex_unlock(&profile_lock);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	err = cur_profile->profile_get(&profile);
> +	mutex_unlock(&profile_lock);
> +	if (err < 0)
> +		return err;
> +

In `platform_profile_store()`, you do
```
err = cur_profile->profile_set(i);
if (err)
  return err;
```
but here you do `if (err < 0)`, why?


> +	/* Check that profile is valid index */
> +	if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))
> +		return sysfs_emit(buf, "\n");
> +

I'd write `if (WARN_ON(profile < 0 ....))` since that is serious error in my
opinion which should be logged. I am also not sure if


> +	return sysfs_emit(buf, "%s\n", profile_names[profile]);
> +}
> [...]
> +int platform_profile_unregister(void)
> +{
> +	int err;
> +
> +	err = mutex_lock_interruptible(&profile_lock);
> +	if (err)
> +		return err;
> +

I know it was me who said to prefer `mutex_lock_interruptible()`, but in this
particular instance I believe `mutex_lock()` would be preferable to avoid the case
where the module unloading is interrupted, and thus the profile handler is not
unregistered properly. This could be handled in each module that uses this
interface, however, I believe it'd be better to handle it here.


> +	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;
> +}
> [...]


Regards,
Barnabás Pőcze

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

[Mark Pearson]

Thanks Barnabas,

On 2020-11-27 2:14 p.m., Barnabás Pőcze wrote:
> Hi
> 
> 
> 2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta:
> 
>> [...]
> +static const char * const profile_names[] = {
> +	[platform_profile_low] = "low-power",
> +	[platform_profile_cool] = "cool",
> +	[platform_profile_quiet] = "quiet",
> +	[platform_profile_balance] = "balance",
> 
> Documentation says "balanced".
Ah, my bad. I will correct.
> 
> 
> +	[platform_profile_perform] = "performance",
> +};
>> [...]
>> +static ssize_t platform_profile_show(struct device *dev,
>> +					struct device_attribute *attr,
>> +					char *buf)
>> +{
>> +	enum platform_profile_option profile = platform_profile_balance;
>> +	int err;
>> +
>> +	err = mutex_lock_interruptible(&profile_lock);
>> +	if (err)
>> +		return err;
>> +
>> +	if (!cur_profile) {
>> +		mutex_unlock(&profile_lock);
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!cur_profile->profile_get) {
>> +		mutex_unlock(&profile_lock);
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	err = cur_profile->profile_get(&profile);
>> +	mutex_unlock(&profile_lock);
>> +	if (err < 0)
>> +		return err;
>> +
> 
> In `platform_profile_store()`, you do
> ```
> err = cur_profile->profile_set(i);
> if (err)
>    return err;
> ```
> but here you do `if (err < 0)`, why?
At one point I had this function returning the profile, but then it 
ended up getting messy for handling errors. I changed it but didn't 
change the error handling - I'll tidy this up. Thanks!
> 
> 
>> +	/* Check that profile is valid index */
>> +	if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))
>> +		return sysfs_emit(buf, "\n");
>> +
> 
> I'd write `if (WARN_ON(profile < 0 ....))` since that is serious error in my
> opinion which should be logged. I am also not sure if
Will do. I've not had any experience with the WARN_ON macro before - 
thanks for the suggestion.
> 
> 
>> +	return sysfs_emit(buf, "%s\n", profile_names[profile]);
>> +}
>> [...]
>> +int platform_profile_unregister(void)
>> +{
>> +	int err;
>> +
>> +	err = mutex_lock_interruptible(&profile_lock);
>> +	if (err)
>> +		return err;
>> +
> 
> I know it was me who said to prefer `mutex_lock_interruptible()`, but in this
> particular instance I believe `mutex_lock()` would be preferable to avoid the case
> where the module unloading is interrupted, and thus the profile handler is not
> unregistered properly. This could be handled in each module that uses this
> interface, however, I believe it'd be better to handle it here.
Agreed (and I know Hans makes the same point in his email). Your 
suggestion makes sense, I'll switch back to mutex_lock for the register 
and unregister functions.
> 
> 
>> +	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;
>> +}
>> [...]
> 
> 
> Regards,
> Barnabás Pőcze
> 
Thank you for the review!
Mark

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

[Hans de Goede]

Hi,

On 11/26/20 5:51 PM, Mark Pearson 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_*)
> 
>  drivers/acpi/Kconfig             |  14 ++
>  drivers/acpi/Makefile            |   1 +
>  drivers/acpi/platform_profile.c  | 215 +++++++++++++++++++++++++++++++
>  include/linux/platform_profile.h |  36 ++++++
>  4 files changed, 266 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..678cb4596ada
> --- /dev/null
> +++ b/drivers/acpi/platform_profile.c
> @@ -0,0 +1,215 @@
> +// 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_balance] = "balance",

As Barnabás already pointed out the docs added in patch 1 say "balanced"
and here you use "balance" both in the enum value/label as we as in
the actual string. I have a slight preference for balanced, but the
most important thing here is being consistent everywhere.

> +	[platform_profile_perform] = "performance",
> +};
> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1);

It would be better to add an extra member/entry at the end of the enum
named platform_profile_no_profiles; and then use that instead of
platform_profile_perform+1. Also see below where I use this too.

> +
> +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;
> +	}
> +
> +	if (!cur_profile->choices) {
> +		mutex_unlock(&profile_lock);
> +		return sysfs_emit(buf, "\n");
> +	}

If choices is empty, the for below will never print anything and
the end result is still just emitting "\n", so this whole if
block is unnecessary and can be removed.

> +
> +	for (i = 0; i < ARRAY_SIZE(profile_names); i++) {
> +		if (cur_profile->choices & BIT(i)) {

Please change the type of choices to an unsigned long array, like this:

	unsigned long choices[BITS_TO_LONGS(platform_profile_no_profiles)];

And then replace the for + if with:

	for_each_set_bit(i, cur_profile->choices, platform_profile_no_profiles) {

This has 2 advantages:
1. Using for_each_set_bit is nicer then open coding it
2. If we ever go over 32 profile choices this will keep working automatically
   without needing any adjustment to the code.

Note this requires include/linux/bitops.h (in include/linux/platform_profile.h,
since you will be using BITS_TO_LONGS there).

> +			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_balance;
> +	int err;
> +
> +	err = mutex_lock_interruptible(&profile_lock);
> +	if (err)
> +		return err;
> +
> +	if (!cur_profile) {
> +		mutex_unlock(&profile_lock);
> +		return -ENODEV;
> +	}
> +
> +	if (!cur_profile->profile_get) {
> +		mutex_unlock(&profile_lock);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	err = cur_profile->profile_get(&profile);
> +	mutex_unlock(&profile_lock);
> +	if (err < 0)
> +		return err;
> +
> +	/* Check that profile is valid index */
> +	if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))
> +		return sysfs_emit(buf, "\n");

IMHO it would be better to do "return -EIO" here, since there
clearly is something wrong in this case.

> +
> +	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;
> +	}
> +
> +	if (!cur_profile->profile_set) {
> +		mutex_unlock(&profile_lock);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/* 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 (!(cur_profile->choices & BIT(i))) {
> +		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;

Maybe sanity check the platform_profile_handler a bit here,
I think it would be ok to check that choices, profile_set and profile_get
are all not 0 / NULL here and otherwise just return -EINVAL; Doing so
allows making the code above a bit simpler, also removing some exit
paths which require an unlock before exiting.

> +
> +	err = mutex_lock_interruptible(&profile_lock);
> +	if (err)
> +		return err;

Please use a regular mutex_lock here, this is called during
driver probing, so no need to handle Ctrl+C and other signals.

> +
> +	/* We can only have one active profile */
> +	if (cur_profile) {
> +		mutex_unlock(&profile_lock);
> +		return -EEXIST;
> +	}
> +
> +	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)
> +{
> +	int err;
> +
> +	err = mutex_lock_interruptible(&profile_lock);
> +	if (err)
> +		return err;

Also please use a regular mutex_lock here.

> +
> +	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);
> +
> +static int __init platform_profile_init(void)
> +{
> +	return 0;
> +}
> +module_init(platform_profile_init);

AFAIK you do not need to provide a module_init function, since this
is a no-op you can just leave it out.

> +static void __exit platform_profile_exit(void)
> +{
> +	/* Check if we have a registered profile, and clean up */
> +	if (cur_profile) {
> +		sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +		cur_profile = NULL;
> +	}
> +}
> +module_exit(platform_profile_exit);

Any driver/module registering a platform_profile will depend on
symbols from this module, so this module can only be unloaded (aka exit)
if that other driver/module has already been unloaded. Unloading that
other module should have already unregistered the cur_profile provider,
otherwise things like cur_profile->profile_set will now point to no
longer loaded code which would be really really bad. So you can drop
this exit function entirely.

Sorry for not spotting some of these sooner. Still nothing really big,
so hopefully you will be able to post a v5 addressing these soonish and
then with some luck we can at least get patches 1 and 2 merged by Rafael
in time for 5.11 (assuming Rafael is happy with v5). And then I can merge
patch 3 once 5.11-rc1 is out.

Regards,

Hans






> +
> +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..33ccd40bb9cf
> --- /dev/null
> +++ b/include/linux/platform_profile.h
> @@ -0,0 +1,36 @@
> +/* 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_
> +
> +/*
> + * 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_balance,
> +	platform_profile_perform,
> +};
> +
> +struct platform_profile_handler {
> +	unsigned int choices; /* Bitmap of available choices */
> +	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 v4 2/3] ACPI: platform-profile: Add platform profile support

[Barnabás Pőcze]

Hi


2020. november 28., szombat 15:08 keltezéssel, Hans de Goede írta:

> [...]
> > +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1);
>
> It would be better to add an extra member/entry at the end of the enum
> named platform_profile_no_profiles; and then use that instead of
> platform_profile_perform+1. Also see below where I use this too.
>

I'm not sure if it's just me, but when I read "no_profiles", then "number of probiles"
is not the first thing that comes to mind, maybe _end, _last, _max, etc.
would be harder to mistake for something else? What do you think?


> > +
> > +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;
> > +	}
> > +
> > +	if (!cur_profile->choices) {
> > +		mutex_unlock(&profile_lock);
> > +		return sysfs_emit(buf, "\n");
> > +	}
>
> If choices is empty, the for below will never print anything and
> the end result is still just emitting "\n", so this whole if
> block is unnecessary and can be removed.
>
> > +
> > +	for (i = 0; i < ARRAY_SIZE(profile_names); i++) {
> > +		if (cur_profile->choices & BIT(i)) {
>
> Please change the type of choices to an unsigned long array, like this:
>
> 	unsigned long choices[BITS_TO_LONGS(platform_profile_no_profiles)];
>

Sorry for the ignorant question, but does this play well with initialization?
Can something like this be done easily?
```
struct platform_profile_handler pph = {
  .choices = BIT(...) | BIT(...) | etc.
};
```
Or do you need to use something like `__set_bit()` in a function?


> [...]
> > +int platform_profile_register(const struct platform_profile_handler *pprof)
> > +{
> > +	int err;
>
> Maybe sanity check the platform_profile_handler a bit here,
> I think it would be ok to check that choices, profile_set and profile_get
> are all not 0 / NULL here and otherwise just return -EINVAL; Doing so
> allows making the code above a bit simpler, also removing some exit
> paths which require an unlock before exiting.
>
> > +
> > +	err = mutex_lock_interruptible(&profile_lock);
> > +	if (err)
> > +		return err;
>
> Please use a regular mutex_lock here, this is called during
> driver probing, so no need to handle Ctrl+C and other signals.
> [...]

I believe insmod/modprobe can be interrupted,
but I agree that's not really of any concern here.



Regards,
Barnabás Pőcze

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

[Mark Pearson]

On 2020-11-28 10:37 a.m., Barnabás Pőcze wrote:
> Hi
> 
> 
> 2020. november 28., szombat 15:08 keltezéssel, Hans de Goede írta:
> 
>> [...]
>>> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1);
>>
>> It would be better to add an extra member/entry at the end of the enum
>> named platform_profile_no_profiles; and then use that instead of
>> platform_profile_perform+1. Also see below where I use this too.
>>
> 
> I'm not sure if it's just me, but when I read "no_profiles", then "number of probiles"
> is not the first thing that comes to mind, maybe _end, _last, _max, etc.
> would be harder to mistake for something else? What do you think?
> 
FWIW - my vote would be platform_profile_last, it just seems to fit well 
when reading the code.

Mark

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

[Hans de Goede]

Hi,

On 11/29/20 2:19 AM, Mark Pearson wrote:
> 
> 
> On 2020-11-28 10:37 a.m., Barnabás Pőcze wrote:
>> Hi
>>
>>
>> 2020. november 28., szombat 15:08 keltezéssel, Hans de Goede írta:
>>
>>> [...]
>>>> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1);
>>>
>>> It would be better to add an extra member/entry at the end of the enum
>>> named platform_profile_no_profiles; and then use that instead of
>>> platform_profile_perform+1. Also see below where I use this too.
>>>
>>
>> I'm not sure if it's just me, but when I read "no_profiles", then "number of probiles"
>> is not the first thing that comes to mind, maybe _end, _last, _max, etc.
>> would be harder to mistake for something else? What do you think?
>>
> FWIW - my vote would be platform_profile_last, it just seems to fit well when reading the code.

I'm fine with platform_profile_last, or iow please feel free to paint this
bikeshed in any color you like :)

Regards,

Hans

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

[Mark Pearson]

Thanks Hans

On 2020-11-28 9:08 a.m., Hans de Goede wrote:
> Hi,
> 
> On 11/26/20 5:51 PM, Mark Pearson wrote:

<snip>

>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> new file mode 100644
>> index 000000000000..678cb4596ada
>> --- /dev/null
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -0,0 +1,215 @@
>> +// 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_balance] = "balance",
> 
> As Barnabás already pointed out the docs added in patch 1 say "balanced"
> and here you use "balance" both in the enum value/label as we as in
> the actual string. I have a slight preference for balanced, but the
> most important thing here is being consistent everywhere.
Agreed - I'll fix, my mistake.
> 
>> +	[platform_profile_perform] = "performance",
>> +};
>> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1);
> 
> It would be better to add an extra member/entry at the end of the enum
> named platform_profile_no_profiles; and then use that instead of
> platform_profile_perform+1. Also see below where I use this too.
Makes sense. Will do (and update the documentation too)
> 
>> +
>> +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;
>> +	}
>> +
>> +	if (!cur_profile->choices) {
>> +		mutex_unlock(&profile_lock);
>> +		return sysfs_emit(buf, "\n");
>> +	}
> 
> If choices is empty, the for below will never print anything and
> the end result is still just emitting "\n", so this whole if
> block is unnecessary and can be removed.
Agreed - I'll remove
> 
>> +
>> +	for (i = 0; i < ARRAY_SIZE(profile_names); i++) {
>> +		if (cur_profile->choices & BIT(i)) {
> 
> Please change the type of choices to an unsigned long array, like this:
> 
> 	unsigned long choices[BITS_TO_LONGS(platform_profile_no_profiles)];
> 
> And then replace the for + if with:
> 
> 	for_each_set_bit(i, cur_profile->choices, platform_profile_no_profiles) {
> 
> This has 2 advantages:
> 1. Using for_each_set_bit is nicer then open coding it
> 2. If we ever go over 32 profile choices this will keep working automatically
>     without needing any adjustment to the code.
> 
> Note this requires include/linux/bitops.h (in include/linux/platform_profile.h,
> since you will be using BITS_TO_LONGS there).

Neat - I hadn't come across this before. I'll give it a go. Thank you.
> 
>> +			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_balance;
>> +	int err;
>> +
>> +	err = mutex_lock_interruptible(&profile_lock);
>> +	if (err)
>> +		return err;
>> +
>> +	if (!cur_profile) {
>> +		mutex_unlock(&profile_lock);
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!cur_profile->profile_get) {
>> +		mutex_unlock(&profile_lock);
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	err = cur_profile->profile_get(&profile);
>> +	mutex_unlock(&profile_lock);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	/* Check that profile is valid index */
>> +	if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))
>> +		return sysfs_emit(buf, "\n");
> 
> IMHO it would be better to do "return -EIO" here, since there
> clearly is something wrong in this case.
Agreed. I'll correct this.
> 
<snip>
>> +
>> +int platform_profile_register(const struct platform_profile_handler *pprof)
>> +{
>> +	int err;
> 
> Maybe sanity check the platform_profile_handler a bit here,
> I think it would be ok to check that choices, profile_set and profile_get
> are all not 0 / NULL here and otherwise just return -EINVAL; Doing so
> allows making the code above a bit simpler, also removing some exit
> paths which require an unlock before exiting.
Makes sense - will do.
> 
>> +
>> +	err = mutex_lock_interruptible(&profile_lock);
>> +	if (err)
>> +		return err;
> 
> Please use a regular mutex_lock here, this is called during
> driver probing, so no need to handle Ctrl+C and other signals.
Will do
> 
>> +
>> +	/* We can only have one active profile */
>> +	if (cur_profile) {
>> +		mutex_unlock(&profile_lock);
>> +		return -EEXIST;
>> +	}
>> +
>> +	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)
>> +{
>> +	int err;
>> +
>> +	err = mutex_lock_interruptible(&profile_lock);
>> +	if (err)
>> +		return err;
> 
> Also please use a regular mutex_lock here.
Ack
> 
>> +
>> +	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);
>> +
>> +static int __init platform_profile_init(void)
>> +{
>> +	return 0;
>> +}
>> +module_init(platform_profile_init);
> 
> AFAIK you do not need to provide a module_init function, since this
> is a no-op you can just leave it out.
Ah - I never realised you could just not have init and exit functions. 
I'll remove.
> 
>> +static void __exit platform_profile_exit(void)
>> +{
>> +	/* Check if we have a registered profile, and clean up */
>> +	if (cur_profile) {
>> +		sysfs_remove_group(acpi_kobj, &platform_profile_group);
>> +		cur_profile = NULL;
>> +	}
>> +}
>> +module_exit(platform_profile_exit);
> 
> Any driver/module registering a platform_profile will depend on
> symbols from this module, so this module can only be unloaded (aka exit)
> if that other driver/module has already been unloaded. Unloading that
> other module should have already unregistered the cur_profile provider,
> otherwise things like cur_profile->profile_set will now point to no
> longer loaded code which would be really really bad. So you can drop
> this exit function entirely.
Will do
> 
> Sorry for not spotting some of these sooner. Still nothing really big,
> so hopefully you will be able to post a v5 addressing these soonish and
> then with some luck we can at least get patches 1 and 2 merged by Rafael
> in time for 5.11 (assuming Rafael is happy with v5). And then I can merge
> patch 3 once 5.11-rc1 is out.
> 
Sounds good - I'll get working on those ASAP (and as an advance note - 
I've read ahead to your other email and I'm on board with that plan)

Thanks for the review - really appreciated.
Mark

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

  drivers/platform/x86/thinkpad_acpi.c | 306 ++++++++++++++++++++++++++-
 1 file changed, 305 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 6a4c54db38fb..8463170391f5 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -72,6 +72,7 @@
 #include <linux/uaccess.h>
 #include <acpi/battery.h>
 #include <acpi/video.h>
+#include <linux/platform_profile.h>
 
 /* ThinkPad CMOS commands */
 #define TP_CMOS_VOLUME_DOWN	0
@@ -9971,6 +9972,296 @@ 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 balance */
+
+#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_ignore_next_event;
+static bool dytc_profile_available;
+static enum platform_profile_option dytc_current_profile;
+
+static int dytc_command(int command, int *output)
+{
+	acpi_handle 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", command))
+		return -EIO;
+	return 0;
+}
+
+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_balance;
+		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_balance:
+		*perfmode = DYTC_MODE_BALANCE;
+		break;
+	case platform_profile_perform:
+		*perfmode = DYTC_MODE_PERFORM;
+		break;
+	default: /* Unknown profile */
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int dytc_perfmode_get(int *perfmode, int *funcmode)
+{
+	int output, err, cmd_err;
+
+	if (!dytc_profile_available)
+		return -ENODEV;
+
+	err = dytc_command(DYTC_CMD_GET, &output);
+	if (err)
+		return err;
+
+	*funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
+	if (*funcmode == DYTC_FUNCTION_CQL) {
+		int dummy;
+		/*
+		 * We can't get the mode when in CQL mode - so we disable CQL
+		 * mode retrieve the mode and then enable it again.
+		 * As disabling/enabling CQL triggers an event we set a flag to
+		 * ignore these events. This will be cleared by the event handler
+		 */
+		dytc_ignore_next_event = true;
+		err = dytc_command(DYTC_DISABLE_CQL, &dummy);
+		if (err)
+			return err;
+
+		cmd_err = dytc_command(DYTC_CMD_GET, &output);
+		/* Check return condition after we've restored CQL state */
+
+		/* Again ignore this event */
+		dytc_ignore_next_event = true;
+		err = dytc_command(DYTC_ENABLE_CQL, &dummy);
+		if (err)
+			return err;
+		if (cmd_err)
+			return cmd_err;
+	}
+	*perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
+	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)
+{
+	int funcmode, perfmode;
+	int err;
+
+	err = dytc_perfmode_get(&perfmode, &funcmode);
+	if (err)
+		return err;
+
+	/* Convert Lenovo DYTC profile to platform_profile */
+	err = convert_dytc_to_profile(perfmode, profile);
+	if (err)
+		return err;
+
+	dytc_current_profile = *profile;
+	return 0;
+}
+
+/*
+ * dytc_profile_set: Function to register with platform_profile
+ * handler. Sets current platform profile.
+ */
+int dytc_profile_set(enum platform_profile_option profile)
+{
+	int cur_perfmode, cur_funcmode;
+	int output;
+	int err;
+
+	if (!dytc_profile_available)
+		return -ENODEV;
+
+	if (profile == platform_profile_balance) {
+		/* To get back to balance mode we just issue a reset command */
+		err = dytc_command(DYTC_CMD_RESET, &output);
+		if (err)
+			return err;
+	} else {
+		int perfmode;
+		int cmd_err;
+
+		err = convert_profile_to_dytc(profile, &perfmode);
+		if (err)
+			return err;
+
+		/* Determine if we are in CQL mode. This alters the commands we do */
+		err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode);
+		if (err)
+			return err;
+
+		if (cur_funcmode == DYTC_FUNCTION_CQL) {
+			/* To set the mode we need to disable CQL first*/
+			dytc_ignore_next_event = true; /* Ignore event */
+			err = dytc_command(DYTC_DISABLE_CQL, &output);
+			if (err)
+				return err;
+		}
+		cmd_err = dytc_command(DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1),
+				&output);
+		/* Check return condition after we've restored CQL state */
+
+		if (cur_funcmode == DYTC_FUNCTION_CQL) {
+			dytc_ignore_next_event = true; /* Ignore event */
+			err = dytc_command(DYTC_ENABLE_CQL, &output);
+			if (err)
+				return err;
+		}
+		if (cmd_err)
+			return cmd_err;
+	}
+	/* Success - update current profile */
+	dytc_current_profile = profile;
+	return 0;
+}
+
+static void dytc_profile_refresh(void)
+{
+	enum platform_profile_option profile;
+	int perfmode, funcmode;
+	int err;
+
+	err = dytc_perfmode_get(&perfmode, &funcmode);
+	if (err)
+		return;
+
+	err = 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 = {
+	.choices = BIT(platform_profile_low) |
+		BIT(platform_profile_balance) |
+		BIT(platform_profile_perform),
+	.profile_get = dytc_profile_get,
+	.profile_set = dytc_profile_set,
+};
+
+static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
+{
+	int err, output;
+
+	dytc_profile_available = false;
+	dytc_ignore_next_event = 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 */
+			do {
+				err = platform_profile_register(&dytc_profile);
+			} while (err == -EINTR);
+			/*
+			 * If for some reason platform_profiles aren't enabled
+			 * don't quit terminally.
+			 */
+			if (err)
+				return 0;
+			dytc_profile_available = true;
+		}
+	}
+	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 +10310,15 @@ 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 (dytc_ignore_next_event)
+			dytc_ignore_next_event = false; /*clear setting*/
+		else
+			dytc_profile_refresh();
+#endif
+	}
 }
 
 static void hotkey_driver_event(const unsigned int scancode)
@@ -10463,6 +10761,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 v4 3/3] platform/x86: thinkpad_acpi: Add platform profile support

[Barnabás Pőcze]

Hi


2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta:

> [...]
> +static bool dytc_ignore_next_event;

As a sidenote: can the profile switching be triggered by means that's not the
`/sys/firmware/acpi/platform_profile` attribute (e.g. a physical button)?
Because if so, then I'm not sure if `dytc_ignore_next_event` achieves its purpose
robustly, although I believe it won't cause issues in practice. I think it could
be made more robust using a mutex to serialize and synchronize access to the DYTC
interface, but I'm not sure if the effort is worth it.


> +static bool dytc_profile_available;
> +static enum platform_profile_option dytc_current_profile;
> [...]
> +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
> +{
> +	int err, output;
> +
> +	dytc_profile_available = false;
> +	dytc_ignore_next_event = 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 */
> +			do {
> +				err = platform_profile_register(&dytc_profile);
> +			} while (err == -EINTR);
> [...]

I'm wondering if this loop is really necessary?


Regards,
Barnabás Pőcze

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

[Hans de Goede]

Hi,

On 11/27/20 8:22 PM, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta:
> 
>> [...]
>> +static bool dytc_ignore_next_event;
> 
> As a sidenote: can the profile switching be triggered by means that's not the
> `/sys/firmware/acpi/platform_profile` attribute (e.g. a physical button)?
> Because if so, then I'm not sure if `dytc_ignore_next_event` achieves its purpose
> robustly, although I believe it won't cause issues in practice. I think it could
> be made more robust using a mutex to serialize and synchronize access to the DYTC
> interface, but I'm not sure if the effort is worth it.
> 
> 
>> +static bool dytc_profile_available;
>> +static enum platform_profile_option dytc_current_profile;
>> [...]
>> +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>> +{
>> +	int err, output;
>> +
>> +	dytc_profile_available = false;
>> +	dytc_ignore_next_event = 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 */
>> +			do {
>> +				err = platform_profile_register(&dytc_profile);
>> +			} while (err == -EINTR);
>> [...]
> 
> I'm wondering if this loop is really necessary?

It is the result of using mutex_interruptible inside platform_profile_register(),
once that is fixed (as I just requested in my review of patch 2/3) then this loop
can go away.

Regards,

Hans

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

[Barnabás Pőcze]

Hi


2020. november 28., szombat 16:00 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:

> [...]
> >> +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
> >> +{
> >> +	int err, output;
> >> +
> >> +	dytc_profile_available = false;
> >> +	dytc_ignore_next_event = 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 */
> >> +			do {
> >> +				err = platform_profile_register(&dytc_profile);
> >> +			} while (err == -EINTR);
> >> [...]
> >
> > I'm wondering if this loop is really necessary?
>
> It is the result of using mutex_interruptible inside platform_profile_register(),
> once that is fixed (as I just requested in my review of patch 2/3) then this loop
> can go away.
>

Thank you, I see that, my question should've been "why not simply fail and
return the error?", but with your requested change the question is moot.


Regards,
Barnabás Pőcze

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

[Mark Pearson]

Hi Barnabas,

On 2020-11-27 2:22 p.m., Barnabás Pőcze wrote:
> Hi
> 
> 
> 2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta:
> 
>> [...]
>> +static bool dytc_ignore_next_event;
> 
> As a sidenote: can the profile switching be triggered by means that's not the
> `/sys/firmware/acpi/platform_profile` attribute (e.g. a physical button)?
> Because if so, then I'm not sure if `dytc_ignore_next_event` achieves its purpose
> robustly, although I believe it won't cause issues in practice. I think it could
> be made more robust using a mutex to serialize and synchronize access to the DYTC
> interface, but I'm not sure if the effort is worth it.
> 

A user can do FN+L, FN+M, FN+H to switch mode (though hopefully with 
this API and the support in user space they won't need to do that any more).

So I think you're right about this area having issues, and Hans picks up 
on this too. I had avoided a mutex as I thought that would cause 
problems in the event handler. In Han's email he suggests an atomic int 
and I think that could work nicely but will have to try it out and see.

Regardless - I agree this area needs some work and I'll look into it

Thanks!
Mark

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

[Hans de Goede]

Hi,

On 11/26/20 5:51 PM, Mark Pearson wrote:
> Add support to thinkpad_acpi for Lenovo platforms that have DYTC
> version 5 support or newer to use the platform profile feature.
> 
> This will allow users to determine and control the platform modes
> between low-power, balanced operation and performance modes.
> 
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
> ---
> Changes in v2:
>  Address (hopefully) all recommendations from review including:
>  - use IS_ENABLED instead of IS_DEFINED
>  - update driver to work with all the fixes in platform_profile update
>  - improve error handling for invalid inputs
>  - move tracking of current profile mode into this driver
> 
> Changes in v3:
>  - version update for patch series
> 
> Changes in v4:
>  - Rebase on top of palm sensor patch which led to a little bit of file
>    restructuring/clean up
>  - Use BIT macro where applicable
>  - Formatting fixes
>  - Check sysfs node created on exit function
>  - implement and use DYTC_SET_COMMAND macro
>  - in case of failure setting performance mode make sure CQL mode is
>    enabled again before returning.
>  - Clean up initialisation and error handling code
> 
>   drivers/platform/x86/thinkpad_acpi.c | 306 ++++++++++++++++++++++++++-
>  1 file changed, 305 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 6a4c54db38fb..8463170391f5 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -72,6 +72,7 @@
>  #include <linux/uaccess.h>
>  #include <acpi/battery.h>
>  #include <acpi/video.h>
> +#include <linux/platform_profile.h>

Please group this together with the other linux/foo.h includes.

>  
>  /* ThinkPad CMOS commands */
>  #define TP_CMOS_VOLUME_DOWN	0
> @@ -9971,6 +9972,296 @@ 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 balance */
> +
> +#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_ignore_next_event;
> +static bool dytc_profile_available;
> +static enum platform_profile_option dytc_current_profile;
> +
> +static int dytc_command(int command, int *output)
> +{
> +	acpi_handle 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", command))
> +		return -EIO;
> +	return 0;
> +}
> +
> +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_balance;
> +		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_balance:
> +		*perfmode = DYTC_MODE_BALANCE;
> +		break;
> +	case platform_profile_perform:
> +		*perfmode = DYTC_MODE_PERFORM;
> +		break;
> +	default: /* Unknown profile */
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int dytc_perfmode_get(int *perfmode, int *funcmode)
> +{
> +	int output, err, cmd_err;
> +
> +	if (!dytc_profile_available)
> +		return -ENODEV;
> +
> +	err = dytc_command(DYTC_CMD_GET, &output);
> +	if (err)
> +		return err;
> +
> +	*funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
> +	if (*funcmode == DYTC_FUNCTION_CQL) {
> +		int dummy;
> +		/*
> +		 * We can't get the mode when in CQL mode - so we disable CQL
> +		 * mode retrieve the mode and then enable it again.
> +		 * As disabling/enabling CQL triggers an event we set a flag to
> +		 * ignore these events. This will be cleared by the event handler
> +		 */
> +		dytc_ignore_next_event = true;
> +		err = dytc_command(DYTC_DISABLE_CQL, &dummy);
> +		if (err)
> +			return err;
> +
> +		cmd_err = dytc_command(DYTC_CMD_GET, &output);
> +		/* Check return condition after we've restored CQL state */
> +
> +		/* Again ignore this event */
> +		dytc_ignore_next_event = true;

Are we sure the event-handler will have run before we do this second
setting of the ignore_next_event bool? Maybe make it an atomic integer
and increment / decrement the variable ?

E.g.:

Declaration:

static atomic_t dytc_ignore_next_event = ATOMIC_INIT();

Ignore next event:
		atomic_inc(&dytc_ignore_next_event);
		
Check if event should be ignored:

		if (!atomic_add_unless(&dytc_ignore_next_event, -1, 0))
			dytc_profile_refresh();

Note atomic_add_unless may needs some explanation, it adds -1 unless
the atomic_t already contains 0. And it returns true if the addition
was done. so if it returns true then dytc_ignore_next_event was not 0
(it might be zero afterwards).

IOW if atomic_add_unless returns true then dytc_ignore_next_event was true,
so then we should NOT continue with the refresh.




> +		err = dytc_command(DYTC_ENABLE_CQL, &dummy);
> +		if (err)
> +			return err;
> +		if (cmd_err)
> +			return cmd_err;
> +	}
> +	*perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
> +	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)
> +{
> +	int funcmode, perfmode;
> +	int err;
> +
> +	err = dytc_perfmode_get(&perfmode, &funcmode);
> +	if (err)
> +		return err;

Can't we used a cached value here ? I presume we get an
event when this is changed by the hotkey ? Esp. with the
whole enable/disable CQL dance getting the value seems a
bit expensive, so using a cached value might be better?

> +
> +	/* Convert Lenovo DYTC profile to platform_profile */
> +	err = convert_dytc_to_profile(perfmode, profile);
> +	if (err)
> +		return err;
> +
> +	dytc_current_profile = *profile;
> +	return 0;
> +}
> +
> +/*
> + * dytc_profile_set: Function to register with platform_profile
> + * handler. Sets current platform profile.
> + */
> +int dytc_profile_set(enum platform_profile_option profile)
> +{
> +	int cur_perfmode, cur_funcmode;
> +	int output;
> +	int err;
> +
> +	if (!dytc_profile_available)
> +		return -ENODEV;
> +
> +	if (profile == platform_profile_balance) {
> +		/* To get back to balance mode we just issue a reset command */
> +		err = dytc_command(DYTC_CMD_RESET, &output);
> +		if (err)
> +			return err;
> +	} else {
> +		int perfmode;
> +		int cmd_err;
> +
> +		err = convert_profile_to_dytc(profile, &perfmode);
> +		if (err)
> +			return err;
> +
> +		/* Determine if we are in CQL mode. This alters the commands we do */
> +		err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode);
> +		if (err)
> +			return err;
> +
> +		if (cur_funcmode == DYTC_FUNCTION_CQL) {
> +			/* To set the mode we need to disable CQL first*/
> +			dytc_ignore_next_event = true; /* Ignore event */
> +			err = dytc_command(DYTC_DISABLE_CQL, &output);
> +			if (err)
> +				return err;
> +		}

This seems somewhat duplicated from the get() code-path. Also you already doing
a DYTC_DISABLE_CQL and DYTC_ENABLE_CQL in dytc_perfmode_get, which is not necessary
to just get the funcmode which is all you need here AFAICT.

IOW it seems that when CQL is active you are now doing:

1. dytc_perfmode_get() calls DYTC_CMD_GET
2. dytc_perfmode_get() calls DYTC_DISABLE_CQL
3. dytc_perfmode_get() calls DYTC_CMD_GET again, result is ignored (not used by dytc_profile_set)
4. dytc_perfmode_get() calls DYTC_ENABLE_CQL
5. dytc_profile_set() calls DYTC_DISABLE_CQL
6. dytc_profile_set() calls DYTC_SET_COMMAND
7. dytc_profile_set() calls DYTC_ENABLE_CQL

And you can really skip step 2-4 here.

I think it would be good to add a bunch of helpers:

1. dytc_get_modes() -> DYTC_CMD_GET wrapper gets both modes, sets perfmode
to -1 when funcmode is CQL
2. dytc_disable_cql_if_necessary() which takes funcmode as argument and is
a no-op when funcmode != CQL
3. dytc_re_enable_cql_if_necessary() idem.

And then the flow in dytc_perfmode_get could look something like this
(pseudo code minus error handling):

	dytc_get_modes(&funcmode, &perfmode)
	if (funcmode != CQL) /* or alternatively check for perfmode != -1 */
		return success;

	dytc_disable_cql_if_necessary(funcmode);
	dytc_get_modes(NULL, &perfmode);
	dytc_disable_cql_if_necessary(funcmode);

And in the non-balanced path of dytc_profile_set:

	dytc_get_modes(&funcmode, NULL)

	dytc_disable_cql_if_necessary(funcmode);
	dytc_set_mode(...);
	dytc_disable_cql_if_necessary(funcmode);

Note the NULL could be a dummy, but I find NULL a bit cleaner
(at the cost of having to check for it in dytc_get_modes).

This is is just from a quick peek, when you implement this
it might turn out to be less then ideal, IOW this is just
a suggestion, feel free to deviate.

###

Since this will require a bit of work, timing wise (wrt the 5.11 merge-window)
it might be best to just keep this patch as is for v5, and only change
patch 1 and 2 of the set, so that those will hopefully be ready for
merging in time for the 5.11 window. I plan to pick this one up
once 5.11-rc1 is out (and has the necessary ACPI bits) so we have some
more time to get this one in shape.

For patch 1/2 the most important thing is to have a consumer of the
new internal APIs (almost) ready and this code fulfills that in
its current form.

Regards,

Hans









> +		cmd_err = dytc_command(DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1),
> +				&output);
> +		/* Check return condition after we've restored CQL state */
> +
> +		if (cur_funcmode == DYTC_FUNCTION_CQL) {
> +			dytc_ignore_next_event = true; /* Ignore event */
> +			err = dytc_command(DYTC_ENABLE_CQL, &output);
> +			if (err)
> +				return err;
> +		}
> +		if (cmd_err)
> +			return cmd_err;
> +	}
> +	/* Success - update current profile */
> +	dytc_current_profile = profile;
> +	return 0;
> +}
> +
> +static void dytc_profile_refresh(void)
> +{
> +	enum platform_profile_option profile;
> +	int perfmode, funcmode;
> +	int err;
> +
> +	err = dytc_perfmode_get(&perfmode, &funcmode);
> +	if (err)
> +		return;
> +
> +	err = 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 = {
> +	.choices = BIT(platform_profile_low) |
> +		BIT(platform_profile_balance) |
> +		BIT(platform_profile_perform),
> +	.profile_get = dytc_profile_get,
> +	.profile_set = dytc_profile_set,
> +};
> +
> +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
> +{
> +	int err, output;
> +
> +	dytc_profile_available = false;
> +	dytc_ignore_next_event = 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 */
> +			do {
> +				err = platform_profile_register(&dytc_profile);
> +			} while (err == -EINTR);
> +			/*
> +			 * If for some reason platform_profiles aren't enabled
> +			 * don't quit terminally.
> +			 */
> +			if (err)
> +				return 0;
> +			dytc_profile_available = true;
> +		}
> +	}
> +	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 +10310,15 @@ 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 (dytc_ignore_next_event)
> +			dytc_ignore_next_event = false; /*clear setting*/
> +		else
> +			dytc_profile_refresh();
> +#endif
> +	}
>  }
>  
>  static void hotkey_driver_event(const unsigned int scancode)
> @@ -10463,6 +10761,12 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>  		.init = tpacpi_proxsensor_init,
>  		.data = &proxsensor_driver_data,
>  	},
> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE)
> +	{
> +		.init = tpacpi_dytc_profile_init,
> +		.data = &dytc_profile_driver_data,
> +	},
> +#endif
>  };
>  
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> 

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

[Mark Pearson]

Hi Hans,

Sorry for the slow reply on this one - I went and did some 
investigation/testing first (and the US came back from Thanksgiving with 
a vengence of meetings....)

On 28/11/2020 09:55, Hans de Goede wrote:
> Hi,
> 
> On 11/26/20 5:51 PM, Mark Pearson wrote:
<snip>
>>
>>    drivers/platform/x86/thinkpad_acpi.c | 306 ++++++++++++++++++++++++++-
>>   1 file changed, 305 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 6a4c54db38fb..8463170391f5 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -72,6 +72,7 @@
>>   #include <linux/uaccess.h>
>>   #include <acpi/battery.h>
>>   #include <acpi/video.h>
>> +#include <linux/platform_profile.h>
> 
> Please group this together with the other linux/foo.h includes.
Ack.

Is it OK if I tidy up the list to be alphabetical as seems generally 
preferred, or would you rather I didn't mess with it apart from the one 
small adjustment?
> 
>>   
<snip>
>> +}
>> +
>> +static int dytc_perfmode_get(int *perfmode, int *funcmode)
>> +{
>> +	int output, err, cmd_err;
>> +
>> +	if (!dytc_profile_available)
>> +		return -ENODEV;
>> +
>> +	err = dytc_command(DYTC_CMD_GET, &output);
>> +	if (err)
>> +		return err;
>> +
>> +	*funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
>> +	if (*funcmode == DYTC_FUNCTION_CQL) {
>> +		int dummy;
>> +		/*
>> +		 * We can't get the mode when in CQL mode - so we disable CQL
>> +		 * mode retrieve the mode and then enable it again.
>> +		 * As disabling/enabling CQL triggers an event we set a flag to
>> +		 * ignore these events. This will be cleared by the event handler
>> +		 */
>> +		dytc_ignore_next_event = true;
>> +		err = dytc_command(DYTC_DISABLE_CQL, &dummy);
>> +		if (err)
>> +			return err;
>> +
>> +		cmd_err = dytc_command(DYTC_CMD_GET, &output);
>> +		/* Check return condition after we've restored CQL state */
>> +
>> +		/* Again ignore this event */
>> +		dytc_ignore_next_event = true;
> 
> Are we sure the event-handler will have run before we do this second
> setting of the ignore_next_event bool? Maybe make it an atomic integer
> and increment / decrement the variable ?
> 
> E.g.:
> 
> Declaration:
> 
> static atomic_t dytc_ignore_next_event = ATOMIC_INIT();
> 
> Ignore next event:
> 		atomic_inc(&dytc_ignore_next_event);
> 		
> Check if event should be ignored:
> 
> 		if (!atomic_add_unless(&dytc_ignore_next_event, -1, 0))
> 			dytc_profile_refresh();
> 
> Note atomic_add_unless may needs some explanation, it adds -1 unless
> the atomic_t already contains 0. And it returns true if the addition
> was done. so if it returns true then dytc_ignore_next_event was not 0
> (it might be zero afterwards).
> 
> IOW if atomic_add_unless returns true then dytc_ignore_next_event was true,
> so then we should NOT continue with the refresh.
> 
In my testing the event handler always ran first, but the atomic 
approach is much nicer - thank you for the suggestion.
I've played a bit with this and tried a few things over the last few 
days and it has been working nicely
One thing I noticed is I think I need to add a mutex to protect so that 
a FN key press won't interfere with a user space access and vice-versa.

> 
> 
> 
>> +		err = dytc_command(DYTC_ENABLE_CQL, &dummy);
>> +		if (err)
>> +			return err;
>> +		if (cmd_err)
>> +			return cmd_err;
>> +	}
>> +	*perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
>> +	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)
>> +{
>> +	int funcmode, perfmode;
>> +	int err;
>> +
>> +	err = dytc_perfmode_get(&perfmode, &funcmode);
>> +	if (err)
>> +		return err;
> 
> Can't we used a cached value here ? I presume we get an
> event when this is changed by the hotkey ? Esp. with the
> whole enable/disable CQL dance getting the value seems a
> bit expensive, so using a cached value might be better?

Agreed - I'll implement.
> 
>> +
>> +	/* Convert Lenovo DYTC profile to platform_profile */
>> +	err = convert_dytc_to_profile(perfmode, profile);
>> +	if (err)
>> +		return err;
>> +
>> +	dytc_current_profile = *profile;
>> +	return 0;
>> +}
>> +
>> +/*
>> + * dytc_profile_set: Function to register with platform_profile
>> + * handler. Sets current platform profile.
>> + */
>> +int dytc_profile_set(enum platform_profile_option profile)
>> +{
>> +	int cur_perfmode, cur_funcmode;
>> +	int output;
>> +	int err;
>> +
>> +	if (!dytc_profile_available)
>> +		return -ENODEV;
>> +
>> +	if (profile == platform_profile_balance) {
>> +		/* To get back to balance mode we just issue a reset command */
>> +		err = dytc_command(DYTC_CMD_RESET, &output);
>> +		if (err)
>> +			return err;
>> +	} else {
>> +		int perfmode;
>> +		int cmd_err;
>> +
>> +		err = convert_profile_to_dytc(profile, &perfmode);
>> +		if (err)
>> +			return err;
>> +
>> +		/* Determine if we are in CQL mode. This alters the commands we do */
>> +		err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode);
>> +		if (err)
>> +			return err;
>> +
>> +		if (cur_funcmode == DYTC_FUNCTION_CQL) {
>> +			/* To set the mode we need to disable CQL first*/
>> +			dytc_ignore_next_event = true; /* Ignore event */
>> +			err = dytc_command(DYTC_DISABLE_CQL, &output);
>> +			if (err)
>> +				return err;
>> +		}
> 
> This seems somewhat duplicated from the get() code-path. Also you already doing
> a DYTC_DISABLE_CQL and DYTC_ENABLE_CQL in dytc_perfmode_get, which is not necessary
> to just get the funcmode which is all you need here AFAICT.
> 
> IOW it seems that when CQL is active you are now doing:
> 
> 1. dytc_perfmode_get() calls DYTC_CMD_GET
> 2. dytc_perfmode_get() calls DYTC_DISABLE_CQL
> 3. dytc_perfmode_get() calls DYTC_CMD_GET again, result is ignored (not used by dytc_profile_set)
> 4. dytc_perfmode_get() calls DYTC_ENABLE_CQL
> 5. dytc_profile_set() calls DYTC_DISABLE_CQL
> 6. dytc_profile_set() calls DYTC_SET_COMMAND
> 7. dytc_profile_set() calls DYTC_ENABLE_CQL
> 
> And you can really skip step 2-4 here.
> 
> I think it would be good to add a bunch of helpers:
> 
> 1. dytc_get_modes() -> DYTC_CMD_GET wrapper gets both modes, sets perfmode
> to -1 when funcmode is CQL
> 2. dytc_disable_cql_if_necessary() which takes funcmode as argument and is
> a no-op when funcmode != CQL
> 3. dytc_re_enable_cql_if_necessary() idem.
> 
> And then the flow in dytc_perfmode_get could look something like this
> (pseudo code minus error handling):
> 
> 	dytc_get_modes(&funcmode, &perfmode)
> 	if (funcmode != CQL) /* or alternatively check for perfmode != -1 */
> 		return success;
> 
> 	dytc_disable_cql_if_necessary(funcmode);
> 	dytc_get_modes(NULL, &perfmode);
> 	dytc_disable_cql_if_necessary(funcmode);
> 
> And in the non-balanced path of dytc_profile_set:
> 
> 	dytc_get_modes(&funcmode, NULL)
> 
> 	dytc_disable_cql_if_necessary(funcmode);
> 	dytc_set_mode(...);
> 	dytc_disable_cql_if_necessary(funcmode);
> 
> Note the NULL could be a dummy, but I find NULL a bit cleaner
> (at the cost of having to check for it in dytc_get_modes).
> 
> This is is just from a quick peek, when you implement this
> it might turn out to be less then ideal, IOW this is just
> a suggestion, feel free to deviate.

Agreed - and thank you for the suggestions. I did prototype a similar 
method and it has worked out nicely. I've got a bit more cleanup but the 
code is better than it was.
> 
> ###
> 
> Since this will require a bit of work, timing wise (wrt the 5.11 merge-window)
> it might be best to just keep this patch as is for v5, and only change
> patch 1 and 2 of the set, so that those will hopefully be ready for
> merging in time for the 5.11 window. I plan to pick this one up
> once 5.11-rc1 is out (and has the necessary ACPI bits) so we have some
> more time to get this one in shape.
> 
> For patch 1/2 the most important thing is to have a consumer of the
> new internal APIs (almost) ready and this code fulfills that in
> its current form.
OK - I think that makes sense. Just curious though - will you then just 
accept the platform_profile pieces (1 & 2)? Would it make it easier if I 
just push the updated first two patches and drop thinkpad_acpi.c for now 
(it will follow shortly, but is going to be a couple more days) or would 
you rather have everything and just pick the bits you want?

I've got the v5 ready (I think) for the platform profile and am still 
working on thinkpad_acpi.c with the improvements from above. I think 
I'll be a couple more days there.

Thank you!
Mark

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

[Hans de Goede]

Hi,

On 12/1/20 5:51 PM, Mark Pearson wrote:
> Hi Hans,
> 
> Sorry for the slow reply on this one - I went and did some investigation/testing first (and the US came back from Thanksgiving with a vengence of meetings....)
> 
> On 28/11/2020 09:55, Hans de Goede wrote:
>> Hi,
>>
>> On 11/26/20 5:51 PM, Mark Pearson wrote:
> <snip>
>>>
>>>    drivers/platform/x86/thinkpad_acpi.c | 306 ++++++++++++++++++++++++++-
>>>   1 file changed, 305 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>>> index 6a4c54db38fb..8463170391f5 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -72,6 +72,7 @@
>>>   #include <linux/uaccess.h>
>>>   #include <acpi/battery.h>
>>>   #include <acpi/video.h>
>>> +#include <linux/platform_profile.h>
>>
>> Please group this together with the other linux/foo.h includes.
> Ack.
> 
> Is it OK if I tidy up the list to be alphabetical as seems generally preferred, or would you rather I didn't mess with it apart from the one small adjustment?

I would welcome a *separate* patch to sort things alphabetically, either as a preparation or as a follow-up patch.

But please don't combine that with this patch.



>>
>>>   
> <snip>
>>> +}
>>> +
>>> +static int dytc_perfmode_get(int *perfmode, int *funcmode)
>>> +{
>>> +    int output, err, cmd_err;
>>> +
>>> +    if (!dytc_profile_available)
>>> +        return -ENODEV;
>>> +
>>> +    err = dytc_command(DYTC_CMD_GET, &output);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    *funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
>>> +    if (*funcmode == DYTC_FUNCTION_CQL) {
>>> +        int dummy;
>>> +        /*
>>> +         * We can't get the mode when in CQL mode - so we disable CQL
>>> +         * mode retrieve the mode and then enable it again.
>>> +         * As disabling/enabling CQL triggers an event we set a flag to
>>> +         * ignore these events. This will be cleared by the event handler
>>> +         */
>>> +        dytc_ignore_next_event = true;
>>> +        err = dytc_command(DYTC_DISABLE_CQL, &dummy);
>>> +        if (err)
>>> +            return err;
>>> +
>>> +        cmd_err = dytc_command(DYTC_CMD_GET, &output);
>>> +        /* Check return condition after we've restored CQL state */
>>> +
>>> +        /* Again ignore this event */
>>> +        dytc_ignore_next_event = true;
>>
>> Are we sure the event-handler will have run before we do this second
>> setting of the ignore_next_event bool? Maybe make it an atomic integer
>> and increment / decrement the variable ?
>>
>> E.g.:
>>
>> Declaration:
>>
>> static atomic_t dytc_ignore_next_event = ATOMIC_INIT();
>>
>> Ignore next event:
>>         atomic_inc(&dytc_ignore_next_event);
>>        
>> Check if event should be ignored:
>>
>>         if (!atomic_add_unless(&dytc_ignore_next_event, -1, 0))
>>             dytc_profile_refresh();
>>
>> Note atomic_add_unless may needs some explanation, it adds -1 unless
>> the atomic_t already contains 0. And it returns true if the addition
>> was done. so if it returns true then dytc_ignore_next_event was not 0
>> (it might be zero afterwards).
>>
>> IOW if atomic_add_unless returns true then dytc_ignore_next_event was true,
>> so then we should NOT continue with the refresh.
>>
> In my testing the event handler always ran first, but the atomic approach is much nicer - thank you for the suggestion.
> I've played a bit with this and tried a few things over the last few days and it has been working nicely
> One thing I noticed is I think I need to add a mutex to protect so that a FN key press won't interfere with a user space access and vice-versa.

Ok, sounds good (adding a mutex if necessary is fine).

> 
>>
>>
>>
>>> +        err = dytc_command(DYTC_ENABLE_CQL, &dummy);
>>> +        if (err)
>>> +            return err;
>>> +        if (cmd_err)
>>> +            return cmd_err;
>>> +    }
>>> +    *perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
>>> +    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)
>>> +{
>>> +    int funcmode, perfmode;
>>> +    int err;
>>> +
>>> +    err = dytc_perfmode_get(&perfmode, &funcmode);
>>> +    if (err)
>>> +        return err;
>>
>> Can't we used a cached value here ? I presume we get an
>> event when this is changed by the hotkey ? Esp. with the
>> whole enable/disable CQL dance getting the value seems a
>> bit expensive, so using a cached value might be better?
> 
> Agreed - I'll implement.
>>
>>> +
>>> +    /* Convert Lenovo DYTC profile to platform_profile */
>>> +    err = convert_dytc_to_profile(perfmode, profile);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    dytc_current_profile = *profile;
>>> +    return 0;
>>> +}
>>> +
>>> +/*
>>> + * dytc_profile_set: Function to register with platform_profile
>>> + * handler. Sets current platform profile.
>>> + */
>>> +int dytc_profile_set(enum platform_profile_option profile)
>>> +{
>>> +    int cur_perfmode, cur_funcmode;
>>> +    int output;
>>> +    int err;
>>> +
>>> +    if (!dytc_profile_available)
>>> +        return -ENODEV;
>>> +
>>> +    if (profile == platform_profile_balance) {
>>> +        /* To get back to balance mode we just issue a reset command */
>>> +        err = dytc_command(DYTC_CMD_RESET, &output);
>>> +        if (err)
>>> +            return err;
>>> +    } else {
>>> +        int perfmode;
>>> +        int cmd_err;
>>> +
>>> +        err = convert_profile_to_dytc(profile, &perfmode);
>>> +        if (err)
>>> +            return err;
>>> +
>>> +        /* Determine if we are in CQL mode. This alters the commands we do */
>>> +        err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode);
>>> +        if (err)
>>> +            return err;
>>> +
>>> +        if (cur_funcmode == DYTC_FUNCTION_CQL) {
>>> +            /* To set the mode we need to disable CQL first*/
>>> +            dytc_ignore_next_event = true; /* Ignore event */
>>> +            err = dytc_command(DYTC_DISABLE_CQL, &output);
>>> +            if (err)
>>> +                return err;
>>> +        }
>>
>> This seems somewhat duplicated from the get() code-path. Also you already doing
>> a DYTC_DISABLE_CQL and DYTC_ENABLE_CQL in dytc_perfmode_get, which is not necessary
>> to just get the funcmode which is all you need here AFAICT.
>>
>> IOW it seems that when CQL is active you are now doing:
>>
>> 1. dytc_perfmode_get() calls DYTC_CMD_GET
>> 2. dytc_perfmode_get() calls DYTC_DISABLE_CQL
>> 3. dytc_perfmode_get() calls DYTC_CMD_GET again, result is ignored (not used by dytc_profile_set)
>> 4. dytc_perfmode_get() calls DYTC_ENABLE_CQL
>> 5. dytc_profile_set() calls DYTC_DISABLE_CQL
>> 6. dytc_profile_set() calls DYTC_SET_COMMAND
>> 7. dytc_profile_set() calls DYTC_ENABLE_CQL
>>
>> And you can really skip step 2-4 here.
>>
>> I think it would be good to add a bunch of helpers:
>>
>> 1. dytc_get_modes() -> DYTC_CMD_GET wrapper gets both modes, sets perfmode
>> to -1 when funcmode is CQL
>> 2. dytc_disable_cql_if_necessary() which takes funcmode as argument and is
>> a no-op when funcmode != CQL
>> 3. dytc_re_enable_cql_if_necessary() idem.
>>
>> And then the flow in dytc_perfmode_get could look something like this
>> (pseudo code minus error handling):
>>
>>     dytc_get_modes(&funcmode, &perfmode)
>>     if (funcmode != CQL) /* or alternatively check for perfmode != -1 */
>>         return success;
>>
>>     dytc_disable_cql_if_necessary(funcmode);
>>     dytc_get_modes(NULL, &perfmode);
>>     dytc_disable_cql_if_necessary(funcmode);
>>
>> And in the non-balanced path of dytc_profile_set:
>>
>>     dytc_get_modes(&funcmode, NULL)
>>
>>     dytc_disable_cql_if_necessary(funcmode);
>>     dytc_set_mode(...);
>>     dytc_disable_cql_if_necessary(funcmode);
>>
>> Note the NULL could be a dummy, but I find NULL a bit cleaner
>> (at the cost of having to check for it in dytc_get_modes).
>>
>> This is is just from a quick peek, when you implement this
>> it might turn out to be less then ideal, IOW this is just
>> a suggestion, feel free to deviate.
> 
> Agreed - and thank you for the suggestions. I did prototype a similar method and it has worked out nicely. I've got a bit more cleanup but the code is better than it was.
>>
>> ###
>>
>> Since this will require a bit of work, timing wise (wrt the 5.11 merge-window)
>> it might be best to just keep this patch as is for v5, and only change
>> patch 1 and 2 of the set, so that those will hopefully be ready for
>> merging in time for the 5.11 window. I plan to pick this one up
>> once 5.11-rc1 is out (and has the necessary ACPI bits) so we have some
>> more time to get this one in shape.
>>
>> For patch 1/2 the most important thing is to have a consumer of the
>> new internal APIs (almost) ready and this code fulfills that in
>> its current form.
>
> OK - I think that makes sense. Just curious though - will you then just accept the platform_profile pieces (1 & 2)? Would it make it easier if I just push the updated first two patches and drop thinkpad_acpi.c for now (it will follow shortly, but is going to be a couple more days) or would you rather have everything and just pick the bits you want?

Patches 1 & 2 should be merged by Rafael, who maintains drivers/acpi,
normally I would then ask Rafael for an immutable branch with those bits
and merge that into platform-drivers-x86.git/for-next and then merge the
3th patch there.

But given the timing it will be easier to just wait for 5.11-rc1, assuming
Rafael is still willing to take 1 and 2 as 5.11 material. The time window
for that is closing.

Rafael, would you be willing to take patches 1 and 2 of this series as
5.11 material assuming a new version addressing my review remarks get
posted soon and I then give my Reviewed-by ?

> I've got the v5 ready (I think) for the platform profile and am still working on thinkpad_acpi.c with the improvements from above. I think I'll be a couple more days there.

It would be best if you can send out v5 soon, even if the thinkpad_acpi
patch is not in perfect shape yet, it will still illustrate how the new
internal APIs from patch 2 will be used, which is very useful for
reviewing patch 2.

Regards,

Hans