[PATCH 0/3] Platform Profile support

[PATCH 0/3] Platform Profile support

[Mark Pearson]

This patch series is for the implementation of the platform-profile
feature - the ability to determine which mode the platform is in and
to change the mode using a sysfs entry.

The first patch is an update of the document I've been working on with
review and help from the kernel community. Thank you to everybody for
their input.

The second patch implements the platform-profile sysfs and API's needed.

The third patch has Lenovo specific changes in thinkpad_acpi.c to use 
the new platform-profile implementation and be able to switch between
low, medium and high power modes.

Thanks
Mark


Mark Pearson (3):
  Documentation: Add documentation for new platform_profile sysfs
    attribute
  ACPI: platform-profile: Add platform profile support
  platform/x86: thinkpad_acpi: Add platform profile support

 .../ABI/testing/sysfs-platform_profile        |  66 +++++
 MAINTAINERS                                   |   8 +
 drivers/acpi/Kconfig                          |  19 ++
 drivers/acpi/Makefile                         |   1 +
 drivers/acpi/platform_profile.c               | 171 ++++++++++++
 drivers/platform/x86/thinkpad_acpi.c          | 261 +++++++++++++++++-
 include/linux/platform_profile.h              |  36 +++
 7 files changed, 550 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform_profile
 create mode 100644 drivers/acpi/platform_profile.c
 create mode 100644 include/linux/platform_profile.h

-- 
2.28.0

[PATCH 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 somei
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>
---
 .../ABI/testing/sysfs-platform_profile        | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform_profile

diff --git a/Documentation/ABI/testing/sysfs-platform_profile b/Documentation/ABI/testing/sysfs-platform_profile
new file mode 100644
index 000000000000..abf900dafbfb
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform_profile
@@ -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

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

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 5:35 AM Mark Pearson <markpearson@lenovo.com> wrote:
>
> On modern systems the platform performance, temperature, fan and other
> hardware related characteristics are often dynamically configurable. The
> profile is often automatically adjusted to the load by somei
> 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.

>  .../ABI/testing/sysfs-platform_profile        | 66 +++++++++++++++++++

Is this in reST format?
Mauro have been converting ABI to be reST compatible (at least).

-- 
With Best Regards,
Andy Shevchenko

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

[Mark Pearson]

Thanks Andy

On 10/11/2020 05:03, Andy Shevchenko wrote:
> On Tue, Nov 10, 2020 at 5:35 AM Mark Pearson <markpearson@lenovo.com> wrote:
>>
>> On modern systems the platform performance, temperature, fan and other
>> hardware related characteristics are often dynamically configurable. The
>> profile is often automatically adjusted to the load by somei
>> 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.
> 
>>   .../ABI/testing/sysfs-platform_profile        | 66 +++++++++++++++++++
> 
> Is this in reST format?
> Mauro have been converting ABI to be reST compatible (at least).
> 
I'm assuming reST is "reStructured Text"? (it's a new one for me)

I'll have a look and check it's compatible and see if I can fix up 
anything missing. If you have any pointers to a document that has 
already been updated and is a good reference point that would be appreciated

Thanks
Mark

[PATCH 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>
---
 MAINTAINERS                      |   8 ++
 drivers/acpi/Kconfig             |  19 ++++
 drivers/acpi/Makefile            |   1 +
 drivers/acpi/platform_profile.c  | 172 +++++++++++++++++++++++++++++++
 include/linux/platform_profile.h |  36 +++++++
 5 files changed, 236 insertions(+)
 create mode 100644 drivers/acpi/platform_profile.c
 create mode 100644 include/linux/platform_profile.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 9a54806ebf02..e731ac1c4447 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -436,6 +436,14 @@ S:	Orphan
 F:	drivers/platform/x86/wmi.c
 F:	include/uapi/linux/wmi.h
 
+ACPI PLATFORM PROFILE DRIVER
+M:	Mark Pearson <markpearons@lenovo.com>
+L:	linux-acpi@vger.kernel.org
+S:	Supported
+W:	https://01.org/linux-acpi
+B:	https://bugzilla.kernel.org
+F:	drivers/acpi/platform_profile.c
+
 AD1889 ALSA SOUND DRIVER
 L:	linux-parisc@vger.kernel.org
 S:	Maintained
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 7540a5179a47..b10a8e0863cf 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -601,3 +601,22 @@ config X86_PM_TIMER
 
 	  You should nearly always say Y here because many modern
 	  systems require this timer.
+
+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.
+
+
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 9a957544e357..82dbdc0300ed 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -93,6 +93,7 @@ obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
 obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
 obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
 obj-$(CONFIG_ACPI_PPTT) 	+= pptt.o
+obj-$(CONFIG_ACPI_PLATFORM_PROFILE) 	+= platform_profile.o
 
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
new file mode 100644
index 000000000000..3c460c0a3857
--- /dev/null
+++ b/drivers/acpi/platform_profile.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ *  platform_profile.c - Platform profile sysfs interface
+ */
+
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+#include <linux/init.h>
+#include <linux/fs.h>
+#include <linux/string.h>
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include <linux/mutex.h>
+#include <acpi/acpi_bus.h>
+#include <linux/platform_profile.h>
+
+struct platform_profile *cur_profile;
+DEFINE_MUTEX(profile_lock);
+
+/* Ensure the first char of each profile is unique */
+static char *profile_str[] = {
+	"Low-power",
+	"Cool",
+	"Quiet",
+	"Balance",
+	"Performance",
+	"Unknown"
+};
+
+static ssize_t platform_profile_choices_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	int i;
+	int ret, count = 0;
+
+	mutex_lock(&profile_lock);
+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -ENODEV;
+	}
+
+	if (!cur_profile->choices) {
+		mutex_unlock(&profile_lock);
+		return snprintf(buf, PAGE_SIZE, "None");
+	}
+
+	for (i = profile_low; i < profile_unknown; i++) {
+		if (cur_profile->choices & (1 << i)) {
+			ret = snprintf(buf+count, PAGE_SIZE, "%s ", profile_str[i]);
+			if (ret < 0)
+				break;
+			count += ret;
+		}
+	}
+	mutex_unlock(&profile_lock);
+	return count;
+}
+
+static ssize_t platform_profile_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	enum profile_option profile = profile_unknown;
+
+	mutex_lock(&profile_lock);
+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -ENODEV;
+	}
+	if (cur_profile->profile_get)
+		profile = cur_profile->profile_get();
+	mutex_unlock(&profile_lock);
+
+	return snprintf(buf, PAGE_SIZE, "%s", profile_str[profile]);
+}
+
+static ssize_t platform_profile_store(struct device *dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	enum profile_option profile;
+
+	mutex_lock(&profile_lock);
+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -ENODEV;
+	}
+
+	/* Scan for a matching profile */
+	for (profile = profile_low; profile < profile_unknown; profile++) {
+		if (toupper(buf[0]) == profile_str[profile][0])
+			break;
+	}
+	if (profile == profile_unknown) {
+		mutex_unlock(&profile_lock);
+		return -EINVAL;
+	}
+
+	if (cur_profile->profile_set)
+		cur_profile->profile_set(profile);
+
+	mutex_unlock(&profile_lock);
+	return count;
+}
+
+static DEVICE_ATTR_RO(platform_profile_choices);
+static DEVICE_ATTR_RW(platform_profile);
+
+static struct attribute *platform_profile_attributes[] = {
+	&dev_attr_platform_profile_choices.attr,
+	&dev_attr_platform_profile.attr,
+	NULL,
+};
+
+static const struct attribute_group platform_profile_attr_group = {
+	.attrs = platform_profile_attributes,
+};
+
+int platform_profile_notify(void)
+{
+	if (!cur_profile)
+		return -ENODEV;
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+	return 0;
+}
+EXPORT_SYMBOL_GPL(platform_profile_notify);
+
+int platform_profile_register(struct platform_profile *pprof)
+{
+	mutex_lock(&profile_lock);
+	/* We can only have one active profile */
+	if (cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -EEXIST;
+	}
+	cur_profile = pprof;
+	mutex_unlock(&profile_lock);
+	return sysfs_create_group(acpi_kobj, &platform_profile_attr_group);
+}
+EXPORT_SYMBOL_GPL(platform_profile_register);
+
+int platform_profile_unregister(void)
+{
+	mutex_lock(&profile_lock);
+	sysfs_remove_group(acpi_kobj, &platform_profile_attr_group);
+	cur_profile = NULL;
+	mutex_unlock(&profile_lock);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(platform_profile_unregister);
+
+static int __init platform_profile_init(void)
+{
+	cur_profile = NULL;
+	return 0;
+}
+
+static void platform_profile_exit(void)
+{
+	sysfs_remove_group(acpi_kobj, &platform_profile_attr_group);
+	cur_profile = NULL;
+}
+
+MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
+MODULE_LICENSE("GPL");
+
+module_init(platform_profile_init);
+module_exit(platform_profile_exit);
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
new file mode 100644
index 000000000000..347a12172c09
--- /dev/null
+++ b/include/linux/platform_profile.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * platform_profile.h - platform profile sysfs interface
+ *
+ * See Documentation/ABI/testing/sysfs-platform_profile for more information.
+ */
+
+#ifndef _PLATFORM_PROFILE_H_
+#define _PLATFORM_PROFILE_H_
+
+/*
+ * If more options are added please update profile_str
+ * array in platform-profile.c
+ */
+
+enum profile_option {
+	profile_low,
+	profile_cool,
+	profile_quiet,
+	profile_balance,
+	profile_perform,
+	profile_unknown /* Must always be last */
+};
+
+struct platform_profile {
+	unsigned int choices; /* bitmap of available choices */
+	int cur_profile;      /* Current active profile */
+	int (*profile_get)(void);
+	int (*profile_set)(int profile);
+};
+
+extern int platform_profile_register(struct platform_profile *pprof);
+extern int platform_profile_unregister(void);
+extern int platform_profile_notify(void);
+
+#endif  /*_PLATFORM_PROFILE_H_*/
-- 
2.28.0

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

[Barnabás Pőcze]

Hi

I've added some questions and comments inline.



2020. november 10., kedd 4:31 keltezéssel, Mark Pearson írta:

> [...]
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> new file mode 100644
> index 000000000000..3c460c0a3857
> --- /dev/null
> +++ b/drivers/acpi/platform_profile.c
> @@ -0,0 +1,172 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + *  platform_profile.c - Platform profile sysfs interface
> + */
> +
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/fs.h>
> +#include <linux/string.h>
> +#include <linux/device.h>
> +#include <linux/acpi.h>
> +#include <linux/mutex.h>
> +#include <acpi/acpi_bus.h>
> +#include <linux/platform_profile.h>

This should preferably be alphabetically sorted.


> +
> +struct platform_profile *cur_profile;

This should be `static`.


> +DEFINE_MUTEX(profile_lock);
> +
> +/* Ensure the first char of each profile is unique */

I wholeheartedly disagree that only the first character should be considered.
It is not future-proof, potentially subverts user expectation, and even worse,
someone could rely on this (undocumented) behaviour.


> +static char *profile_str[] = {

Why is it not `const`?


> +	"Low-power",
> +	"Cool",
> +	"Quiet",
> +	"Balance",
> +	"Performance",
> +	"Unknown"

"unknown" is not documented, yet it may be returned to userspace.


> +};

The documentation has the names in all-lowercase, and in my opinion it'd be
better to use lowercase names here as well.


> +
> +static ssize_t platform_profile_choices_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	int i;
> +	int ret, count = 0;
> +
> +	mutex_lock(&profile_lock);
> +	if (!cur_profile) {
> +		mutex_unlock(&profile_lock);
> +		return -ENODEV;
> +	}
> +
> +	if (!cur_profile->choices) {
> +		mutex_unlock(&profile_lock);
> +		return snprintf(buf, PAGE_SIZE, "None");

"None" is not documented anywhere as far as I can see, maybe an empty line
would be better in this case?


> +	}
> +
> +	for (i = profile_low; i < profile_unknown; i++) {
> +		if (cur_profile->choices & (1 << i)) {

`BIT(i)`?


> +			ret = snprintf(buf+count, PAGE_SIZE, "%s ", profile_str[i]);

You could use `sysfs_emit_at()`. `ret` is only used in this block, so it could be
defined here.


> +			if (ret < 0)
> +				break;

However unlikely this case is, I'm not sure if providing partial values is
better than not providing any data at all.


> +			count += ret;
> +		}
> +	}
> +	mutex_unlock(&profile_lock);

I think a newline character should be written at the end (possibly overwriting
the last space).


> +	return count;
> +}
> +
> +static ssize_t platform_profile_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	enum profile_option profile = profile_unknown;
> +
> +	mutex_lock(&profile_lock);
> +	if (!cur_profile) {
> +		mutex_unlock(&profile_lock);
> +		return -ENODEV;
> +	}
> +	if (cur_profile->profile_get)
> +		profile = cur_profile->profile_get();

I'd assume that `profile_get()` can return any arbitrary errno, which is then
propagated to the "reader", but it seems that's not the case?
I think returning `-EOPNOTSUPP` would be better if `profile_get` is NULL.


> +	mutex_unlock(&profile_lock);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s", profile_str[profile]);

There is `sysfs_emit()`, as far as I know it is supposed to replace this exact
snprintf(...) idiom. Directly indexing the `profile_str` with an unchecked
value here is rather unsafe in my opinion.


> +}
> +
> +static ssize_t platform_profile_store(struct device *dev,
> +			    struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	enum profile_option profile;
> +
> +	mutex_lock(&profile_lock);
> +	if (!cur_profile) {
> +		mutex_unlock(&profile_lock);
> +		return -ENODEV;
> +	}
> +
> +	/* Scan for a matching profile */
> +	for (profile = profile_low; profile < profile_unknown; profile++) {
> +		if (toupper(buf[0]) == profile_str[profile][0])
> +			break;
> +	}
> +	if (profile == profile_unknown) {
> +		mutex_unlock(&profile_lock);
> +		return -EINVAL;
> +	}
> +
> +	if (cur_profile->profile_set)
> +		cur_profile->profile_set(profile);

The return value is entirely discarded? I'd assume it's returned to the "writer".
I'm also not sure if ignoring if `profile_set` is NULL is the best course of
action. Maybe returning `-EOPNOTSUPP` would be better?


> +
> +	mutex_unlock(&profile_lock);
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RO(platform_profile_choices);
> +static DEVICE_ATTR_RW(platform_profile);
> +
> +static struct attribute *platform_profile_attributes[] = {
> +	&dev_attr_platform_profile_choices.attr,
> +	&dev_attr_platform_profile.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group platform_profile_attr_group = {
> +	.attrs = platform_profile_attributes,
> +};

It's a minor thing, but there is an `ATTRIBUTE_GROUPS()` macro which could possibly
simplify the above part.


> +
> +int platform_profile_notify(void)
> +{
> +	if (!cur_profile)
> +		return -ENODEV;
> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(platform_profile_notify);
> +
> +int platform_profile_register(struct platform_profile *pprof)
> +{
> +	mutex_lock(&profile_lock);
> +	/* We can only have one active profile */
> +	if (cur_profile) {
> +		mutex_unlock(&profile_lock);
> +		return -EEXIST;
> +	}
> +	cur_profile = pprof;
> +	mutex_unlock(&profile_lock);
> +	return sysfs_create_group(acpi_kobj, &platform_profile_attr_group);
> +}
> +EXPORT_SYMBOL_GPL(platform_profile_register);
> +
> +int platform_profile_unregister(void)
> +{
> +	mutex_lock(&profile_lock);
> +	sysfs_remove_group(acpi_kobj, &platform_profile_attr_group);
> +	cur_profile = NULL;
> +	mutex_unlock(&profile_lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(platform_profile_unregister);
> +
> +static int __init platform_profile_init(void)
> +{
> +	cur_profile = NULL;

If I'm not missing anything, `cur_profile` will be initialized to NULL, thus
this is not needed.


> +	return 0;
> +}
> +
> +static void platform_profile_exit(void)

This should be marked `__exit`.


> +{
> +	sysfs_remove_group(acpi_kobj, &platform_profile_attr_group);
> +	cur_profile = NULL;
> +}
> +
> +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
> +MODULE_LICENSE("GPL");
> +
> +module_init(platform_profile_init);
> +module_exit(platform_profile_exit);
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> new file mode 100644
> index 000000000000..347a12172c09
> --- /dev/null
> +++ b/include/linux/platform_profile.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * platform_profile.h - platform profile sysfs interface
> + *
> + * See Documentation/ABI/testing/sysfs-platform_profile for more information.
> + */
> +
> +#ifndef _PLATFORM_PROFILE_H_
> +#define _PLATFORM_PROFILE_H_
> +
> +/*
> + * If more options are added please update profile_str
> + * array in platform-profile.c
> + */
> +
> +enum profile_option {
> +	profile_low,
> +	profile_cool,
> +	profile_quiet,
> +	profile_balance,
> +	profile_perform,
> +	profile_unknown /* Must always be last */
> +};

Shouldn't these be prefixed by `platform_`? And I think it'd be better to have
`profile_unknown` as the first value in the enumeration.


> +
> +struct platform_profile {

Personally, I think a name like platform_profile_(handler|provider)
would be a better fit.


> +	unsigned int choices; /* bitmap of available choices */

Most comments are capitalized.


> +	int cur_profile;      /* Current active profile */

`cur_profile` field doesn't seem to be used here. I see that it's utilized in the
thinkpad_acpi driver, but I feel like this does not "belong" here.


> +	int (*profile_get)(void);
> +	int (*profile_set)(int profile);

Why does it take an `int` instead of `enum profile_option`?


> +};
> +
> +extern int platform_profile_register(struct platform_profile *pprof);
> +extern int platform_profile_unregister(void);
> +extern int platform_profile_notify(void);
> +

`extern` could be omitted from here. Although it seems rather "unregulated"
whether `extern` is to be present in header files or not.


> +#endif  /*_PLATFORM_PROFILE_H_*/
> --
> 2.28.0


Regards,
Barnabás Pőcze

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

[Mark Pearson]

Thanks Barnabas

On 10/11/2020 05:15, Barnabás Pőcze wrote:
> Hi
> 
> I've added some questions and comments inline.
> 
> 
> 
> 2020. november 10., kedd 4:31 keltezéssel, Mark Pearson írta:
> 
>> [...]
>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>> new file mode 100644
>> index 000000000000..3c460c0a3857
>> --- /dev/null
>> +++ b/drivers/acpi/platform_profile.c
>> @@ -0,0 +1,172 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +/*
>> + *  platform_profile.c - Platform profile sysfs interface
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/printk.h>
>> +#include <linux/kobject.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/init.h>
>> +#include <linux/fs.h>
>> +#include <linux/string.h>
>> +#include <linux/device.h>
>> +#include <linux/acpi.h>
>> +#include <linux/mutex.h>
>> +#include <acpi/acpi_bus.h>
>> +#include <linux/platform_profile.h>
> 
> This should preferably be alphabetically sorted.
Ack - I hadn't realised that was a requirement. I'll update
> 
> 
>> +
>> +struct platform_profile *cur_profile;
> 
> This should be `static`.
Agreed.

> 
> 
>> +DEFINE_MUTEX(profile_lock);
>> +
>> +/* Ensure the first char of each profile is unique */
> 
> I wholeheartedly disagree that only the first character should be considered.
> It is not future-proof, potentially subverts user expectation, and even worse,
> someone could rely on this (undocumented) behaviour.
OK, fair enough. I debated about it and it's nice just to do
    echo 'L' > platform_profile
instead of typing in a whole string, but I appreciate it's lazy.

I'm OK with fixing based on your points.

> 
> 
>> +static char *profile_str[] = {
> 
> Why is it not `const`?
My mistake. I will fix
> 
> 
>> +	"Low-power",
>> +	"Cool",
>> +	"Quiet",
>> +	"Balance",
>> +	"Performance",
>> +	"Unknown"
> 
> "unknown" is not documented, yet it may be returned to userspace.
Ack - I'll look into if it's really needed, but it seemed sensible to 
have it whilst doing the implementation.
> 
> 
>> +};
> 
> The documentation has the names in all-lowercase, and in my opinion it'd be
> better to use lowercase names here as well.
Agreed - my bad. I'll fix.
> 
> 
>> +
>> +static ssize_t platform_profile_choices_show(struct device *dev,
>> +					struct device_attribute *attr,
>> +					char *buf)
>> +{
>> +	int i;
>> +	int ret, count = 0;
>> +
>> +	mutex_lock(&profile_lock);
>> +	if (!cur_profile) {
>> +		mutex_unlock(&profile_lock);
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!cur_profile->choices) {
>> +		mutex_unlock(&profile_lock);
>> +		return snprintf(buf, PAGE_SIZE, "None");
> 
> "None" is not documented anywhere as far as I can see, maybe an empty line
> would be better in this case?
Agreed, I'll fix.
> 
> 
>> +	}
>> +
>> +	for (i = profile_low; i < profile_unknown; i++) {
>> +		if (cur_profile->choices & (1 << i)) {
> 
> `BIT(i)`?
Yes, that would be better.

> 
> 
>> +			ret = snprintf(buf+count, PAGE_SIZE, "%s ", profile_str[i]);
> 
> You could use `sysfs_emit_at()`. `ret` is only used in this block, so it could be
> defined here.
OK - I hadn't come across that function. I'll update to use it.

> 
> 
>> +			if (ret < 0)
>> +				break;
> 
> However unlikely this case is, I'm not sure if providing partial values is
> better than not providing any data at all.
Interesting point, I think I agree.
I'll look at returning an empty string if an error occurs.
> 
> 
>> +			count += ret;
>> +		}
>> +	}
>> +	mutex_unlock(&profile_lock);
> 
> I think a newline character should be written at the end (possibly overwriting
> the last space).
OK.

> 
> 
>> +	return count;
>> +}
>> +
>> +static ssize_t platform_profile_show(struct device *dev,
>> +					struct device_attribute *attr,
>> +					char *buf)
>> +{
>> +	enum profile_option profile = profile_unknown;
>> +
>> +	mutex_lock(&profile_lock);
>> +	if (!cur_profile) {
>> +		mutex_unlock(&profile_lock);
>> +		return -ENODEV;
>> +	}
>> +	if (cur_profile->profile_get)
>> +		profile = cur_profile->profile_get();
> 
> I'd assume that `profile_get()` can return any arbitrary errno, which is then
> propagated to the "reader", but it seems that's not the case?
> I think returning `-EOPNOTSUPP` would be better if `profile_get` is NULL.
OK - I went through a few iterations here and it's a bit weak. I'll look 
at it again.

> 
> 
>> +	mutex_unlock(&profile_lock);
>> +
>> +	return snprintf(buf, PAGE_SIZE, "%s", profile_str[profile]);
> 
> There is `sysfs_emit()`, as far as I know it is supposed to replace this exact
> snprintf(...) idiom. Directly indexing the `profile_str` with an unchecked
> value here is rather unsafe in my opinion.
Agreed - I'll fix this.
> 
> 
>> +}
>> +
>> +static ssize_t platform_profile_store(struct device *dev,
>> +			    struct device_attribute *attr,
>> +			    const char *buf, size_t count)
>> +{
>> +	enum profile_option profile;
>> +
>> +	mutex_lock(&profile_lock);
>> +	if (!cur_profile) {
>> +		mutex_unlock(&profile_lock);
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* Scan for a matching profile */
>> +	for (profile = profile_low; profile < profile_unknown; profile++) {
>> +		if (toupper(buf[0]) == profile_str[profile][0])
>> +			break;
>> +	}
>> +	if (profile == profile_unknown) {
>> +		mutex_unlock(&profile_lock);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (cur_profile->profile_set)
>> +		cur_profile->profile_set(profile);
> 
> The return value is entirely discarded? I'd assume it's returned to the "writer".
> I'm also not sure if ignoring if `profile_set` is NULL is the best course of
> action. Maybe returning `-EOPNOTSUPP` would be better?
OK.

> 
> 
>> +
>> +	mutex_unlock(&profile_lock);
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR_RO(platform_profile_choices);
>> +static DEVICE_ATTR_RW(platform_profile);
>> +
>> +static struct attribute *platform_profile_attributes[] = {
>> +	&dev_attr_platform_profile_choices.attr,
>> +	&dev_attr_platform_profile.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group platform_profile_attr_group = {
>> +	.attrs = platform_profile_attributes,
>> +};
> 
> It's a minor thing, but there is an `ATTRIBUTE_GROUPS()` macro which could possibly
> simplify the above part.
OK - I'd not come across that and will look at using it.
> 
> 
>> +
>> +int platform_profile_notify(void)
>> +{
>> +	if (!cur_profile)
>> +		return -ENODEV;
>> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_notify);
>> +
>> +int platform_profile_register(struct platform_profile *pprof)
>> +{
>> +	mutex_lock(&profile_lock);
>> +	/* We can only have one active profile */
>> +	if (cur_profile) {
>> +		mutex_unlock(&profile_lock);
>> +		return -EEXIST;
>> +	}
>> +	cur_profile = pprof;
>> +	mutex_unlock(&profile_lock);
>> +	return sysfs_create_group(acpi_kobj, &platform_profile_attr_group);
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_register);
>> +
>> +int platform_profile_unregister(void)
>> +{
>> +	mutex_lock(&profile_lock);
>> +	sysfs_remove_group(acpi_kobj, &platform_profile_attr_group);
>> +	cur_profile = NULL;
>> +	mutex_unlock(&profile_lock);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_unregister);
>> +
>> +static int __init platform_profile_init(void)
>> +{
>> +	cur_profile = NULL;
> 
> If I'm not missing anything, `cur_profile` will be initialized to NULL, thus
> this is not needed.
I guess I was just playing safe here, I think you're right.
I'll remove.
> 
> 
>> +	return 0;
>> +}
>> +
>> +static void platform_profile_exit(void)
> 
> This should be marked `__exit`.
Not sure how I missed that one.... Thanks.
> 
> 
>> +{
>> +	sysfs_remove_group(acpi_kobj, &platform_profile_attr_group);
>> +	cur_profile = NULL;
>> +}
>> +
>> +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
>> +MODULE_LICENSE("GPL");
>> +
>> +module_init(platform_profile_init);
>> +module_exit(platform_profile_exit);
>> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
>> new file mode 100644
>> index 000000000000..347a12172c09
>> --- /dev/null
>> +++ b/include/linux/platform_profile.h
>> @@ -0,0 +1,36 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * platform_profile.h - platform profile sysfs interface
>> + *
>> + * See Documentation/ABI/testing/sysfs-platform_profile for more information.
>> + */
>> +
>> +#ifndef _PLATFORM_PROFILE_H_
>> +#define _PLATFORM_PROFILE_H_
>> +
>> +/*
>> + * If more options are added please update profile_str
>> + * array in platform-profile.c
>> + */
>> +
>> +enum profile_option {
>> +	profile_low,
>> +	profile_cool,
>> +	profile_quiet,
>> +	profile_balance,
>> +	profile_perform,
>> +	profile_unknown /* Must always be last */
>> +};
> 
> Shouldn't these be prefixed by `platform_`? And I think it'd be better to have
> `profile_unknown` as the first value in the enumeration.
I can add 'platform_'
I liked having profile_unknown as the last value as it makes scanning 
from 'low' to 'unknown' more future proof if other profiles get added 
(e.g in platform_profile_choices_show).
Is this something you feel strongly about?
> 
> 
>> +
>> +struct platform_profile {
> 
> Personally, I think a name like platform_profile_(handler|provider)
> would be a better fit.
OK - happy to update that.
> 
> 
>> +	unsigned int choices; /* bitmap of available choices */
> 
> Most comments are capitalized.
Ack.
> 
> 
>> +	int cur_profile;      /* Current active profile */
> 
> `cur_profile` field doesn't seem to be used here. I see that it's utilized in the
> thinkpad_acpi driver, but I feel like this does not "belong" here.
It seemed a logical place to keep it to me. I understand where you're 
coming from, and I can change it so it's just internal to 
thinkpad_acpi.c but it just seemed a nice place to keep it and I'm 
guessing other implementations would like to have it too.

I'd be interested to see what others have to say on this one.
> 
> 
>> +	int (*profile_get)(void);
>> +	int (*profile_set)(int profile);
> 
> Why does it take an `int` instead of `enum profile_option`?
Mostly for error conditions to be propagated if needs be, though I have 
some improvements to do based on the comments above :)
> 
> 
>> +};
>> +
>> +extern int platform_profile_register(struct platform_profile *pprof);
>> +extern int platform_profile_unregister(void);
>> +extern int platform_profile_notify(void);
>> +
> 
> `extern` could be omitted from here. Although it seems rather "unregulated"
> whether `extern` is to be present in header files or not.
OK.
> 
> 
>> +#endif  /*_PLATFORM_PROFILE_H_*/
>> --
>> 2.28.0
> 
> 
> Regards,
> Barnabás Pőcze
> 
Thank you for the detailed review - really appreciate the time you spent 
going through this.

Mark

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

[Barnabás Pőcze]

Hi


> [...]
> >> +static char *profile_str[] = {
> >
> > Why is it not `const`?
> My mistake. I will fix
> >
> >
> >> +	"Low-power",
> >> +	"Cool",
> >> +	"Quiet",
> >> +	"Balance",
> >> +	"Performance",
> >> +	"Unknown"
> >
> > "unknown" is not documented, yet it may be returned to userspace.
> Ack - I'll look into if it's really needed, but it seemed sensible to
> have it whilst doing the implementation.

I don't advocate for its removal, just that it be documented if it may be
returned to userspace.


> >
> >
> >> +};
> [...]
> >> +#ifndef _PLATFORM_PROFILE_H_
> >> +#define _PLATFORM_PROFILE_H_
> >> +
> >> +/*
> >> + * If more options are added please update profile_str
> >> + * array in platform-profile.c
> >> + */
> >> +
> >> +enum profile_option {
> >> +	profile_low,
> >> +	profile_cool,
> >> +	profile_quiet,
> >> +	profile_balance,
> >> +	profile_perform,
> >> +	profile_unknown /* Must always be last */
> >> +};
> >
> > Shouldn't these be prefixed by `platform_`? And I think it'd be better to have
> > `profile_unknown` as the first value in the enumeration.
> I can add 'platform_'
> I liked having profile_unknown as the last value as it makes scanning
> from 'low' to 'unknown' more future proof if other profiles get added
> (e.g in platform_profile_choices_show).
> Is this something you feel strongly about?

I don't feel strongly about it, just that right now, for all practical purposes
`profile_unknown` feels like a first-class profile option in the current form of
the patch, and it didn't seem right that it can just change. I'd do something like
```
enum performance_profile_option {
  performance_profile_unknown,
  performance_profile_low,
  ...
  performance_profile_max, /* must be last */
};
```

But I don't have a strong preference for either one of them. Maybe someone
could chime in and tell us which one is more prevalent/preferred.

And as a side note, I think you could put something like
`static_assert(ARRAY_SIZE(profile_str) == performance_profile_max);`
in the code somewhere to make sure there are as many strings in the array as
profile options.

You might actually do the following as well:
```
static const char *profile_str[] = {
  [performance_profile_unknown] = "unknown",
  [performance_profile_low]     = "low",
  ...
};
```

I realize I might be a bit too paranoid here. :-) But if you do these three things,
or something similar, then the chances of the enum and the array being out of
sync (by accident) will be very slim.


> [...]

Regards,
Barnabás Pőcze

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

[Hans de Goede]

Hi,

On 11/10/20 3:32 PM, Mark Pearson wrote:
> Thanks Barnabas
> 
> On 10/11/2020 05:15, Barnabás Pőcze wrote:
>> Hi
>>
>> I've added some questions and comments inline.
>>
>>
>>
>> 2020. november 10., kedd 4:31 keltezéssel, Mark Pearson írta:

<snip>

>>> +    "Low-power",
>>> +    "Cool",
>>> +    "Quiet",
>>> +    "Balance",
>>> +    "Performance",
>>> +    "Unknown"
>>
>> "unknown" is not documented, yet it may be returned to userspace.
> Ack - I'll look into if it's really needed, but it seemed sensible to have it whilst doing the implementation.


AFAIK with the currently agreed upon version of the API we do not need this,
please remove it. We can always add it later if necessary, but for now I would
like to avoid drivers being tempted to use this.

Regards,

Hans

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

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 5:35 AM Mark Pearson <markpearson@lenovo.com> wrote:
>
> This is the initial implementation of the platform-profile feature.
> It provides the details discussed and outlined in the
> sysfs-platform_profile document.
>
> Many modern systems have the ability to modify the operating profile to
> control aspects like fan speed, temperature and power levels. This
> module provides a common sysfs interface that platform modules can register
> against to control their individual profile options.

...

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

> +
> +

None of the blank lines is enough. But can you consider to find
perhaps better place (I imply some logical group of options in the
file).

...

>  obj-$(CONFIG_ACPI_SPCR_TABLE)  += spcr.o
>  obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
>  obj-$(CONFIG_ACPI_PPTT)        += pptt.o
> +obj-$(CONFIG_ACPI_PLATFORM_PROFILE)    += platform_profile.o

...yes, and this becomes consistent with the above.

...

> +/*
> + *  platform_profile.c - Platform profile sysfs interface
> + */

One line. PLease, don't put the file name into the file. If we want to
rename it, it will give additional churn and as shown in practice
people often forget this change to follow.

...

> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/fs.h>
> +#include <linux/string.h>
> +#include <linux/device.h>
> +#include <linux/acpi.h>
> +#include <linux/mutex.h>
> +#include <acpi/acpi_bus.h>
> +#include <linux/platform_profile.h>

Perhaps sorted?
Why do you need a specific acpi_bus.h? I thought acpi.h includes it already, no?

...

> +struct platform_profile *cur_profile;

Better naming since it's a global variable.
Is it supposed to be exported to modules?

...

> +DEFINE_MUTEX(profile_lock);

No static?

...

> +/* Ensure the first char of each profile is unique */
> +static char *profile_str[] = {

static const char * const profile_names[]

Also naming (perhaps like I proposed above?).

> +       "Low-power",
> +       "Cool",
> +       "Quiet",
> +       "Balance",
> +       "Performance",

> +       "Unknown"

Leave the comma here.

> +};

...

> +       int i;
> +       int ret, count = 0;

count AFAICS should be size_t (or ssize_t).
Can you make them in reversed xmas tree order?

...

> +       return snprintf(buf, PAGE_SIZE, "%s", profile_str[profile]);

Nowadays we have sysfs_emit(), use it.

...

> +       /* Scan for a matching profile */
> +       for (profile = profile_low; profile < profile_unknown; profile++) {
> +               if (toupper(buf[0]) == profile_str[profile][0])
> +                       break;
> +       }

match_string() / sysfs_match_string() ?

...

> +static struct attribute *platform_profile_attributes[] = {
> +       &dev_attr_platform_profile_choices.attr,
> +       &dev_attr_platform_profile.attr,

> +       NULL,

Drop comma in terminator line.

> +};

...

> +module_init(platform_profile_init);
> +module_exit(platform_profile_exit);

Attach them to respective functions.

...

> +/*
> + * platform_profile.h - platform profile sysfs interface

No file name.

> + *
> + * See Documentation/ABI/testing/sysfs-platform_profile for more information.
> + */

...

> +/*
> + * If more options are added please update profile_str
> + * array in platform-profile.c
> + */

Kernel doc?

> +enum profile_option {
> +       profile_low,
> +       profile_cool,
> +       profile_quiet,
> +       profile_balance,
> +       profile_perform,

> +       profile_unknown /* Must always be last */

Comment is semi-useless. Comma at the end (or its absence) is usually
enough to give a clue, but okay, comment makes this explicit.

...

> +struct platform_profile {
> +       unsigned int choices; /* bitmap of available choices */
> +       int cur_profile;      /* Current active profile */

Kernel doc?

> +       int (*profile_get)(void);
> +       int (*profile_set)(int profile);
> +};

...

> +extern int platform_profile_register(struct platform_profile *pprof);
> +extern int platform_profile_unregister(void);
> +extern int platform_profile_notify(void);

extern is not needed.

-- 
With Best Regards,
Andy Shevchenko

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

[Mark Pearson]

Hi Andy,

On 10/11/2020 05:26, Andy Shevchenko wrote:
> On Tue, Nov 10, 2020 at 5:35 AM Mark Pearson <markpearson@lenovo.com> wrote:
>>
>> This is the initial implementation of the platform-profile feature.
>> It provides the details discussed and outlined in the
>> sysfs-platform_profile document.
>>
>> Many modern systems have the ability to modify the operating profile to
>> control aspects like fan speed, temperature and power levels. This
>> module provides a common sysfs interface that platform modules can register
>> against to control their individual profile options.
> 
> ...
> 
>> +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.
> 
>> +
>> +
> 
> None of the blank lines is enough. But can you consider to find
> perhaps better place (I imply some logical group of options in the
> file).
Will do.

> 
> ...
> 
>>   obj-$(CONFIG_ACPI_SPCR_TABLE)  += spcr.o
>>   obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
>>   obj-$(CONFIG_ACPI_PPTT)        += pptt.o
>> +obj-$(CONFIG_ACPI_PLATFORM_PROFILE)    += platform_profile.o
> 
> ...yes, and this becomes consistent with the above.
> 
> ...
> 
>> +/*
>> + *  platform_profile.c - Platform profile sysfs interface
>> + */
> 
> One line. PLease, don't put the file name into the file. If we want to
> rename it, it will give additional churn and as shown in practice
> people often forget this change to follow.
Ack.

> 
> ...
> 
>> +#include <linux/module.h>
>> +#include <linux/printk.h>
>> +#include <linux/kobject.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/init.h>
>> +#include <linux/fs.h>
>> +#include <linux/string.h>
>> +#include <linux/device.h>
>> +#include <linux/acpi.h>
>> +#include <linux/mutex.h>
>> +#include <acpi/acpi_bus.h>
>> +#include <linux/platform_profile.h>
> 
> Perhaps sorted?
> Why do you need a specific acpi_bus.h? I thought acpi.h includes it already, no?
Sure - I hadn't realised sorted was a requirement :)

I'll check on acpi_bus.h - I think I initially added acpi_bus.h thinking 
it was all I needed and then added acpi.h later. I'll clean up if I can.
> 
> ...
> 
>> +struct platform_profile *cur_profile;
> 
> Better naming since it's a global variable.
> Is it supposed to be exported to modules?
It's internal only, but agreed on improving the naming.
(Barabas pointed out that it should be a static too)
> 
> ...
> 
>> +DEFINE_MUTEX(profile_lock);
> 
> No static?
Yes, it should be.
> 
> ...
> 
>> +/* Ensure the first char of each profile is unique */
>> +static char *profile_str[] = {
> 
> static const char * const profile_names[]
> 
> Also naming (perhaps like I proposed above?).
Ack.
> 
>> +       "Low-power",
>> +       "Cool",
>> +       "Quiet",
>> +       "Balance",
>> +       "Performance",
> 
>> +       "Unknown"
> 
> Leave the comma here.
Ack.
> 
>> +};
> 
> ...
> 
>> +       int i;
>> +       int ret, count = 0;
> 
> count AFAICS should be size_t (or ssize_t).
> Can you make them in reversed xmas tree order?
Sure - will fix.

> 
> ...
> 
>> +       return snprintf(buf, PAGE_SIZE, "%s", profile_str[profile]);
> 
> Nowadays we have sysfs_emit(), use it.
OK - that was a new one to me. Will update.
> 
> ...
> 
>> +       /* Scan for a matching profile */
>> +       for (profile = profile_low; profile < profile_unknown; profile++) {
>> +               if (toupper(buf[0]) == profile_str[profile][0])
>> +                       break;
>> +       }
> 
> match_string() / sysfs_match_string() ?
Yes, Barnabas picked up on this too. I'll correct.

> 
> ...
> 
>> +static struct attribute *platform_profile_attributes[] = {
>> +       &dev_attr_platform_profile_choices.attr,
>> +       &dev_attr_platform_profile.attr,
> 
>> +       NULL,
> 
> Drop comma in terminator line.
Ack.
> 
>> +};
> 
> ...
> 
>> +module_init(platform_profile_init);
>> +module_exit(platform_profile_exit);
> 
> Attach them to respective functions.
OK.

> 
> ...
> 
>> +/*
>> + * platform_profile.h - platform profile sysfs interface
> 
> No file name.
Ack.
> 
>> + *
>> + * See Documentation/ABI/testing/sysfs-platform_profile for more information.
>> + */
> 
> ...
> 
>> +/*
>> + * If more options are added please update profile_str
>> + * array in platform-profile.c
>> + */
> 
> Kernel doc?
OK.
Just in case - I assume this should go int the sysfs-platform-profile 
doc (patch 1 of the series). Let me know if that's not the expectation.

> 
>> +enum profile_option {
>> +       profile_low,
>> +       profile_cool,
>> +       profile_quiet,
>> +       profile_balance,
>> +       profile_perform,
> 
>> +       profile_unknown /* Must always be last */
> 
> Comment is semi-useless. Comma at the end (or its absence) is usually
> enough to give a clue, but okay, comment makes this explicit.
I prefer explicit but let me know if you feel strongly about this one.

> 
> ...
> 
>> +struct platform_profile {
>> +       unsigned int choices; /* bitmap of available choices */
>> +       int cur_profile;      /* Current active profile */
> 
> Kernel doc?
Sure
> 
>> +       int (*profile_get)(void);
>> +       int (*profile_set)(int profile);
>> +};
> 
> ...
> 
>> +extern int platform_profile_register(struct platform_profile *pprof);
>> +extern int platform_profile_unregister(void);
>> +extern int platform_profile_notify(void);
> 
> extern is not needed.
> 
I will remove.

Thanks for the detailed review - very much appreciated.
Mark

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

[Rafael J. Wysocki]

On Tue, Nov 10, 2020 at 4:32 AM Mark Pearson <markpearson@lenovo.com> wrote:
>
> This is the initial implementation of the platform-profile feature.
> It provides the details discussed and outlined in the
> sysfs-platform_profile document.
>
> Many modern systems have the ability to modify the operating profile to
> control aspects like fan speed, temperature and power levels. This
> module provides a common sysfs interface that platform modules can register
> against to control their individual profile options.
>
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
> ---
>  MAINTAINERS                      |   8 ++
>  drivers/acpi/Kconfig             |  19 ++++
>  drivers/acpi/Makefile            |   1 +
>  drivers/acpi/platform_profile.c  | 172 +++++++++++++++++++++++++++++++
>  include/linux/platform_profile.h |  36 +++++++
>  5 files changed, 236 insertions(+)
>  create mode 100644 drivers/acpi/platform_profile.c
>  create mode 100644 include/linux/platform_profile.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9a54806ebf02..e731ac1c4447 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -436,6 +436,14 @@ S: Orphan
>  F:     drivers/platform/x86/wmi.c
>  F:     include/uapi/linux/wmi.h
>
> +ACPI PLATFORM PROFILE DRIVER
> +M:     Mark Pearson <markpearons@lenovo.com>
> +L:     linux-acpi@vger.kernel.org
> +S:     Supported
> +W:     https://01.org/linux-acpi
> +B:     https://bugzilla.kernel.org
> +F:     drivers/acpi/platform_profile.c

IMO it is premature to add a MAINTAINERS entry for this until it turns
out to be really necessary.

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

[Mark Pearson]

Hi Rafael

On 10/11/2020 07:53, Rafael J. Wysocki wrote:
> On Tue, Nov 10, 2020 at 4:32 AM Mark Pearson <markpearson@lenovo.com> wrote:
>>
>> This is the initial implementation of the platform-profile feature.
>> It provides the details discussed and outlined in the
>> sysfs-platform_profile document.
>>
>> Many modern systems have the ability to modify the operating profile to
>> control aspects like fan speed, temperature and power levels. This
>> module provides a common sysfs interface that platform modules can register
>> against to control their individual profile options.
>>
>> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
>> ---
>>   MAINTAINERS                      |   8 ++
>>   drivers/acpi/Kconfig             |  19 ++++
>>   drivers/acpi/Makefile            |   1 +
>>   drivers/acpi/platform_profile.c  | 172 +++++++++++++++++++++++++++++++
>>   include/linux/platform_profile.h |  36 +++++++
>>   5 files changed, 236 insertions(+)
>>   create mode 100644 drivers/acpi/platform_profile.c
>>   create mode 100644 include/linux/platform_profile.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9a54806ebf02..e731ac1c4447 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -436,6 +436,14 @@ S: Orphan
>>   F:     drivers/platform/x86/wmi.c
>>   F:     include/uapi/linux/wmi.h
>>
>> +ACPI PLATFORM PROFILE DRIVER
>> +M:     Mark Pearson <markpearons@lenovo.com>
>> +L:     linux-acpi@vger.kernel.org
>> +S:     Supported
>> +W:     https://01.org/linux-acpi
>> +B:     https://bugzilla.kernel.org
>> +F:     drivers/acpi/platform_profile.c
> 
> IMO it is premature to add a MAINTAINERS entry for this until it turns
> out to be really necessary.
> 
Not a problem - I'll remove this. I wasn't particularly sure about the 
etiquette here and I ran checkpatch and it suggested it.

I'm obviously happy to help with fixes etc. My plan is not to dump and 
run :)

Mark

[PATCH 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>
---
 drivers/platform/x86/thinkpad_acpi.c | 263 +++++++++++++++++++++++++--
 1 file changed, 246 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index fe438a5e1dbe..d70181b5772b 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
@@ -9824,10 +9825,40 @@ static struct ibm_struct lcdshadow_driver_data = {
  * DYTC subdriver, for the Lenovo lapmode feature
  */
 
+#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_GET          2 /* To get current IC function and mode */
 #define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
+#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 revisision */
+#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_GET_LAPMODE_BIT  17 /* Bit 17 - lapmode. Set when on lap */
+
+#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - funct 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_DISABLE_CQL ((DYTC_MODE_BALANCE << DYTC_SET_MODE_BIT) | \
+		(DYTC_FUNCTION_CQL << DYTC_SET_FUNCTION_BIT) | \
+		DYTC_CMD_SET)
+#define DYTC_ENABLE_CQL (DYTC_DISABLE_CQL | (1 << DYTC_SET_VALID_BIT))
 
 static bool dytc_lapmode;
+static bool dytc_available;
+static bool dytc_ignore_next_event;
 
 static void dytc_lapmode_notify_change(void)
 {
@@ -9881,22 +9912,172 @@ static ssize_t dytc_lapmode_show(struct device *dev,
 
 static DEVICE_ATTR_RO(dytc_lapmode);
 
-static struct attribute *dytc_attributes[] = {
-	&dev_attr_dytc_lapmode.attr,
-	NULL,
-};
+#if defined(CONFIG_ACPI_PLATFORM_PROFILE)
+static struct platform_profile dytc_profile;
 
-static const struct attribute_group dytc_attr_group = {
-	.attrs = dytc_attributes,
-};
+static int convert_dytc_to_profile(int dytcmode)
+{
+	switch (dytcmode) {
+	case DYTC_MODE_QUIET:
+		return profile_low;
+	case DYTC_MODE_BALANCE:
+		return profile_balance;
+	case DYTC_MODE_PERFORM:
+		return profile_perform;
+	default: /* Unknown mode */
+		return profile_unknown;
+	}
+}
 
-static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
+static int convert_profile_to_dytc(int profile)
+{
+	switch (profile) {
+	case profile_low:
+		return DYTC_MODE_QUIET;
+	case profile_balance:
+		return DYTC_MODE_BALANCE;
+	case profile_perform:
+		return DYTC_MODE_PERFORM;
+	default: /* Unknown profile */
+		return -EINVAL;
+	}
+}
+
+static int dytc_perfmode_get(int *perfmode, int *funcmode)
+{
+	int output, err;
+
+	if (!dytc_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;
+		err = dytc_command(DYTC_CMD_GET, &output);
+		if (err)
+			return err;
+		/* Again ignore this event */
+		dytc_ignore_next_event = true;
+		err = dytc_command(DYTC_ENABLE_CQL, &dummy);
+		if (err)
+			return 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(void)
 {
 	int err;
+	int funcmode, perfmode;
+
+	err = dytc_perfmode_get(&perfmode, &funcmode);
+	if (err)
+		return err;
 
-	err = dytc_lapmode_get(&dytc_lapmode);
-	/* If support isn't available (ENODEV) then don't return an error
-	 * but just don't create the sysfs group
+	/* Convert Lenovo DYTC profile to platform_profile */
+	dytc_profile.cur_profile = convert_dytc_to_profile(perfmode);
+	return dytc_profile.cur_profile;
+}
+
+/*
+ * dytc_profile_set: Function to register with platform_profile
+ * handler. Sets current platform profile.
+ */
+int dytc_profile_set(int profile)
+{
+	int err, dytc_set;
+	int output;
+	int cur_perfmode, cur_funcmode;
+
+	if (!dytc_available)
+		return -ENODEV;
+
+	if (profile == 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 = convert_profile_to_dytc(profile);
+
+		if (perfmode < 0)
+			return perfmode;
+
+		/* 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;
+		}
+		dytc_set = (1 << DYTC_SET_VALID_BIT) |
+			(DYTC_FUNCTION_MMC << DYTC_SET_FUNCTION_BIT) |
+			(perfmode << DYTC_SET_MODE_BIT) |
+			DYTC_CMD_SET;
+		err = dytc_command(dytc_set, &output);
+		if (err)
+			return err;
+		if (cur_funcmode == DYTC_FUNCTION_CQL) {
+			dytc_ignore_next_event = true; /*ignore event*/
+			err = dytc_command(DYTC_ENABLE_CQL, &output);
+			if (err)
+				return err;
+		}
+	}
+	/* Success - update current profile */
+	dytc_profile.cur_profile = profile;
+	return 0;
+}
+
+static void dytc_profile_refresh(void)
+{
+	int perfmode, funcmode, profile;
+	int err;
+
+	err = dytc_perfmode_get(&perfmode, &funcmode);
+	if (err)
+		return;
+
+	profile = convert_dytc_to_profile(perfmode);
+	if (profile != dytc_profile.cur_profile) {
+		dytc_profile.cur_profile = profile;
+		platform_profile_notify();
+	}
+}
+#endif
+
+static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
+{
+	int err, output;
+
+	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;
@@ -9904,14 +10085,55 @@ static int tpacpi_dytc_init(struct ibm_init_struct *iibm)
 	if (err)
 		return err;
 
-	/* Platform supports this feature - create the group */
-	err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
-	return err;
+	/* Check DYTC is enabled and supports mode setting */
+	dytc_available = false;
+	dytc_ignore_next_event = false;
+
+	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);
+			dytc_available = true;
+#if defined(CONFIG_ACPI_PLATFORM_PROFILE)
+			/* Create platform_profile structure and register */
+			dytc_profile.choices = (1 << profile_low) |
+				(1 << profile_balance) |
+				(1 << profile_perform);
+			dytc_profile.profile_get = dytc_profile_get;
+			dytc_profile.profile_set = dytc_profile_set;
+			dytc_profile.cur_profile = profile_unknown;
+			err = platform_profile_register(&dytc_profile);
+			/*
+			 * If for some reason platform_profiles aren't enabled
+			 * don't quit terminally.
+			 */
+			if (err)
+				return 0;
+#endif
+			/*
+			 * Note - this has been deprecated by the input sensor implementation,
+			 * but can't be removed until we confirm user space is no longer using
+			 */
+			dytc_lapmode_get(&dytc_lapmode);
+			return device_create_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode);
+		}
+	}
+	return 0;
 }
 
 static void dytc_exit(void)
 {
-	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group);
+	if (dytc_available) {
+		device_remove_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode);
+#if defined(CONFIG_ACPI_PLATFORM_PROFILE)
+		platform_profile_unregister();
+#endif
+		dytc_available = false;
+	}
 }
 
 static struct ibm_struct dytc_driver_data = {
@@ -10095,8 +10317,15 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
 	}
 
 	if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) {
-		dytc_lapmode_refresh();
-		lapsensor_refresh();
+		if (dytc_ignore_next_event)
+			dytc_ignore_next_event = false; /*clear setting*/
+		else {
+			dytc_lapmode_refresh();
+#if defined(CONFIG_ACPI_PLATFORM_PROFILE)
+			dytc_profile_refresh();
+#endif
+			lapsensor_refresh();
+		}
 	}
 
 }
-- 
2.28.0

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

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 5:34 AM Mark Pearson <markpearson@lenovo.com> 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.

...

> +#if defined(CONFIG_ACPI_PLATFORM_PROFILE)
> +               platform_profile_unregister();
> +#endif
> +               dytc_available = false;

> +#if defined(CONFIG_ACPI_PLATFORM_PROFILE)
> +                       dytc_profile_refresh();
> +#endif

Better to use (e.g. test coverage) if (IS_BUILTIN()) / if (IS_ENABLE()) / etc.

-- 
With Best Regards,
Andy Shevchenko

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

[Mark Pearson]

Hi Andy,

On 10/11/2020 05:06, Andy Shevchenko wrote:
> On Tue, Nov 10, 2020 at 5:34 AM Mark Pearson <markpearson@lenovo.com> 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.
> 
> ...
> 
>> +#if defined(CONFIG_ACPI_PLATFORM_PROFILE)
>> +               platform_profile_unregister();
>> +#endif
>> +               dytc_available = false;
> 
>> +#if defined(CONFIG_ACPI_PLATFORM_PROFILE)
>> +                       dytc_profile_refresh();
>> +#endif
> 
> Better to use (e.g. test coverage) if (IS_BUILTIN()) / if (IS_ENABLE()) / etc.
> 
Thanks - I wasn't aware of those. I'll update

Mark

Re: [PATCH 0/3] Platform Profile support

[Bastien Nocera]

On Mon, 2020-11-09 at 22:31 -0500, Mark Pearson wrote:
> This patch series is for the implementation of the platform-profile
> feature - the ability to determine which mode the platform is in and
> to change the mode using a sysfs entry.
> 
> The first patch is an update of the document I've been working on
> with
> review and help from the kernel community. Thank you to everybody for
> their input.
> 
> The second patch implements the platform-profile sysfs and API's
> needed.
> 
> The third patch has Lenovo specific changes in thinkpad_acpi.c to use
> the new platform-profile implementation and be able to switch between
> low, medium and high power modes.

I've implemented this in power-profiles-daemon:
https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/merge_requests/41

Was pretty straight forward to implement, so good job :)