[PATCH v3] ACPI: platform-profile: Add platform profile support

[PATCH v3] 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)

Big thank you to reviewers for all the suggestions.

Changes in v3:
 Add missed platform_profile.h file

 drivers/acpi/Kconfig             |  33 ++++++
 drivers/acpi/Makefile            |   1 +
 drivers/acpi/platform_profile.c  | 181 +++++++++++++++++++++++++++++++
 include/linux/platform_profile.h |  36 ++++++
 4 files changed, 251 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..73a99af5ec2c 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 ""
@@ -538,3 +552,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 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..e4bbee48c0f8
--- /dev/null
+++ b/drivers/acpi/platform_profile.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* Platform profile sysfs interface */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_profile.h>
+#include <linux/printk.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+
+static struct platform_profile_handler *cur_profile;
+static DEFINE_MUTEX(profile_lock);
+
+/* Ensure the first char of each profile is unique */
+static const char * const profile_names[] = {
+	[profile_low] = "low-power",
+	[profile_cool] = "cool",
+	[profile_quiet] = "quiet",
+	[profile_balance] = "balance",
+	[profile_perform] = "performance",
+};
+static_assert(ARRAY_SIZE(profile_names) == profile_perform+1);
+
+static ssize_t platform_profile_choices_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	int len = 0;
+	int i;
+
+	mutex_lock(&profile_lock);
+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -ENODEV;
+	}
+
+	if (!cur_profile->choices) {
+		mutex_unlock(&profile_lock);
+		return sysfs_emit(buf, "\n");
+	}
+
+	for (i = profile_low; i <= profile_perform; 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;
+	int err;
+
+	mutex_lock(&profile_lock);
+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -EOPNOTSUPP;
+	}
+
+	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;
+
+	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;
+
+	mutex_lock(&profile_lock);
+	if (!cur_profile) {
+		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;
+	}
+
+	if (!cur_profile->profile_set) {
+		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
+};
+
+int platform_profile_notify(void)
+{
+	if (!cur_profile)
+		return -EOPNOTSUPP;
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+	return 0;
+}
+EXPORT_SYMBOL_GPL(platform_profile_notify);
+
+int platform_profile_register(struct platform_profile_handler *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_group);
+}
+EXPORT_SYMBOL_GPL(platform_profile_register);
+
+int platform_profile_unregister(void)
+{
+	mutex_lock(&profile_lock);
+	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)
+{
+	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..f6592434c8ce
--- /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 {
+	profile_low,
+	profile_cool,
+	profile_quiet,
+	profile_balance,
+	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(struct platform_profile_handler *pprof);
+int platform_profile_unregister(void);
+int platform_profile_notify(void);
+
+#endif  /*_PLATFORM_PROFILE_H_*/
-- 
2.25.1

Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Barnabás Pőcze]

Hi

I believe it would have been easier for the maintainers
if you had resent the whole series.

Another thing is that `mutex_lock_interruptible()` seems to be preferred
instead of `mutex_lock()` [1].


2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta:

> [...]
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index edf1558c1105..73a99af5ec2c 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_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.
> +
> +

Am I missing something or is the ACPI_PLATFORM_PROFILE option in the Kconfig file
twice?


> [...]
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/* Platform profile sysfs interface */
> +
> +#include <linux/acpi.h>

linux/bits.h is missing, I believe the rule of thumb is that if you use
`X` and `X` is defined in `A.h`, then you should include `A.h` directly,
and not rely on the fact that another header file you use includes `A.h`.
An exception is ACPICA related things, I believe linux/acpi.h should be
preferred there.


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

I believe there are some unnecessary header files.
(maybe fs.h, kobject.h, ...)

> +
> +static struct platform_profile_handler *cur_profile;

I think this could be `const` as well.
(along with the argument of `platform_profile_register()`)


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

I think this comment is not needed anymore, no?


> [...]
> +static ssize_t platform_profile_choices_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	int len = 0;
> +	int i;
> +
> +	mutex_lock(&profile_lock);
> +	if (!cur_profile) {
> +		mutex_unlock(&profile_lock);
> +		return -ENODEV;
> +	}
> +
> +	if (!cur_profile->choices) {
> +		mutex_unlock(&profile_lock);
> +		return sysfs_emit(buf, "\n");
> +	}
> +
> +	for (i = profile_low; i <= profile_perform; 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]);
                                                                     ^^
I'm unsure why there are two spaces before `profile_names[i]` in both previous places.


> +		}
> +	}
> +	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;
> +	int err;
> +
> +	mutex_lock(&profile_lock);
> +	if (!cur_profile) {
> +		mutex_unlock(&profile_lock);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	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;
> +
> +	return sysfs_emit(buf, "%s\n", profile_names[profile]);

I believe `profile` should be initialized to some value, and the value after the
call to the handler it should be checked for validity, and maybe an warning should
be emitted if it's out of range - in my opinion.


> +}
> +
> +static ssize_t platform_profile_store(struct device *dev,
> +			    struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	int err, i;
> +
> +	mutex_lock(&profile_lock);
> +	if (!cur_profile) {
> +		mutex_unlock(&profile_lock);
> +		return -EOPNOTSUPP;

I'm not sure why you modified the errno. ENODEV seemed to me like a perfectly
fine error to return if `cur_profile` is not set. `platform_profile_choices_show()`
returns ENODEV, so there is a bit of inconsistency.
(same applies to `platform_profile_show()`)


> +	}
> +
> +	/* Scan for a matching profile */
> +	i = sysfs_match_string(profile_names, buf);
> +	if (i < 0) {
> +		mutex_unlock(&profile_lock);
> +		return -EINVAL;
> +	}
> +
> +	if (!cur_profile->profile_set) {
> +		mutex_unlock(&profile_lock);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	err = cur_profile->profile_set(i);
> +	mutex_unlock(&profile_lock);
> +	if (err)
> +		return err;
> +
> +	return count;
> +}
> [...]
> +int platform_profile_register(struct platform_profile_handler *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_group);

I believe the return value of `sysfs_create_group()` should be checked here,
and `cur_profile` should only be set if that succeeds.


> +}
> +EXPORT_SYMBOL_GPL(platform_profile_register);
> [...]
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> new file mode 100644
> index 000000000000..f6592434c8ce
> --- /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 {
> +	profile_low,
> +	profile_cool,
> +	profile_quiet,
> +	profile_balance,
> +	profile_perform
                       ^
I believe there's no reason to remove the comma from there, and in fact,
having a comma after the last entry in an array, enum, etc. seems to be
the preferred.

If you don't want to go the `platform_profile_last` route that I suggested previously,
then I think a comment should mention that the last profile should be used
in the static_assert() in platform-profile.c.

By the way, I still feel like the enum values are "too vague" and should be
prefixed with `platform_`. If you're not opposed to that change.


> +};
> +
> +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(struct platform_profile_handler *pprof);
> +int platform_profile_unregister(void);
> +int platform_profile_notify(void);

I don't think it's a big problem, but right now, I personally can't quite see the
purpose `platform_profile_notify()` and `platform_profile_unregister()`
returning any value.


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


[1]: https://www.kernel.org/doc/html/latest/kernel-hacking/locking.html#locking-only-in-user-context


Regards,
Barnabás Pőcze

Re: [External] Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Mark Pearson]

Thanks Barnabas

On 15/11/2020 13:26, Barnabás Pőcze wrote:
> Hi
> 
> I believe it would have been easier for the maintainers
> if you had resent the whole series.
> 
> Another thing is that `mutex_lock_interruptible()` seems to be preferred
> instead of `mutex_lock()` [1].
Thanks for the reference - I wasn't aware of that and will update.

> 
> 
> 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta:
> 
>> [...]
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index edf1558c1105..73a99af5ec2c 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_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.
>> +
>> + >
> Am I missing something or is the ACPI_PLATFORM_PROFILE option in the Kconfig file
> twice?
It is....and it shouldn't be. I'll remove. I looked over this patch so 
many times checking that I'd put in all the pieces of feedback from 
everyone that I'm not sure how I missed that :(
> 
> 
>> [...]
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +/* Platform profile sysfs interface */
>> +
>> +#include <linux/acpi.h>
> 
> linux/bits.h is missing, I believe the rule of thumb is that if you use
> `X` and `X` is defined in `A.h`, then you should include `A.h` directly,
> and not rely on the fact that another header file you use includes `A.h`.
> An exception is ACPICA related things, I believe linux/acpi.h should be
> preferred there.
Thanks - I didn't realise that was the case and I'll update
> 
> 
>> +#include <linux/device.h>
>> +#include <linux/fs.h>
>> +#include <linux/init.h>
>> +#include <linux/kobject.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_profile.h>
>> +#include <linux/printk.h>
>> +#include <linux/string.h>
>> +#include <linux/sysfs.h>
> 
> I believe there are some unnecessary header files.
> (maybe fs.h, kobject.h, ...)
Seems likely - I did go through a few iterations and you're right that 
these should be removed.
> 
>> +
>> +static struct platform_profile_handler *cur_profile;
> 
> I think this could be `const` as well.
> (along with the argument of `platform_profile_register()`)
ack

> 
> 
>> +static DEFINE_MUTEX(profile_lock);
>> +
>> +/* Ensure the first char of each profile is unique */
> 
> I think this comment is not needed anymore, no?
Agreed
> 
> 
>> [...]
>> +static ssize_t platform_profile_choices_show(struct device *dev,
>> +					struct device_attribute *attr,
>> +					char *buf)
>> +{
>> +	int len = 0;
>> +	int i;
>> +
>> +	mutex_lock(&profile_lock);
>> +	if (!cur_profile) {
>> +		mutex_unlock(&profile_lock);
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!cur_profile->choices) {
>> +		mutex_unlock(&profile_lock);
>> +		return sysfs_emit(buf, "\n");
>> +	}
>> +
>> +	for (i = profile_low; i <= profile_perform; 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]);
>                                                                       ^^
> I'm unsure why there are two spaces before `profile_names[i]` in both previous places.
I'm unsure how I introduced these too....I'll clean up
> 
> 
>> +		}
>> +	}
>> +	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;
>> +	int err;
>> +
>> +	mutex_lock(&profile_lock);
>> +	if (!cur_profile) {
>> +		mutex_unlock(&profile_lock);
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	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;
>> +
>> +	return sysfs_emit(buf, "%s\n", profile_names[profile]);
> 
> I believe `profile` should be initialized to some value, and the value after the
> call to the handler it should be checked for validity, and maybe an warning should
> be emitted if it's out of range - in my opinion.
Agreed - and a check would do no harm. I'll add.
> 
> 
>> +}
>> +
>> +static ssize_t platform_profile_store(struct device *dev,
>> +			    struct device_attribute *attr,
>> +			    const char *buf, size_t count)
>> +{
>> +	int err, i;
>> +
>> +	mutex_lock(&profile_lock);
>> +	if (!cur_profile) {
>> +		mutex_unlock(&profile_lock);
>> +		return -EOPNOTSUPP;
> 
> I'm not sure why you modified the errno. ENODEV seemed to me like a perfectly
> fine error to return if `cur_profile` is not set. `platform_profile_choices_show()`
> returns ENODEV, so there is a bit of inconsistency.
> (same applies to `platform_profile_show()`)
My thinking was it seemed a better message. I can't really see any 
conditions when you'd get here (a driver would have registered a driver 
and then not provided a profile?) but it seemed that if that was 
possible it was more because changing the settings wasn't supported.

I managed to convince myself it made more sense - but have no issue with 
changing it back.
> 
> 
>> +	}
>> +
>> +	/* Scan for a matching profile */
>> +	i = sysfs_match_string(profile_names, buf);
>> +	if (i < 0) {
>> +		mutex_unlock(&profile_lock);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!cur_profile->profile_set) {
>> +		mutex_unlock(&profile_lock);
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	err = cur_profile->profile_set(i);
>> +	mutex_unlock(&profile_lock);
>> +	if (err)
>> +		return err;
>> +
>> +	return count;
>> +}
>> [...]
>> +int platform_profile_register(struct platform_profile_handler *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_group);
> 
> I believe the return value of `sysfs_create_group()` should be checked here,
> and `cur_profile` should only be set if that succeeds.
agreed.
> 
> 
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_register);
>> [...]
>> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
>> new file mode 100644
>> index 000000000000..f6592434c8ce
>> --- /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 {
>> +	profile_low,
>> +	profile_cool,
>> +	profile_quiet,
>> +	profile_balance,
>> +	profile_perform
>                         ^
> I believe there's no reason to remove the comma from there, and in fact,
> having a comma after the last entry in an array, enum, etc. seems to be
> the preferred.
OK.
Have to be honest - I struggle to know when comma's are needed on the 
last entry and when they aren't (I've had similar corrections in other 
cases both ways :)). I do seem to have a knack of getting it 
consistently wrong....

> 
> If you don't want to go the `platform_profile_last` route that I suggested previously,
> then I think a comment should mention that the last profile should be used
> in the static_assert() in platform-profile.c.
ok.
> 
> By the way, I still feel like the enum values are "too vague" and should be
> prefixed with `platform_`. If you're not opposed to that change.
Sure - I can change that. For me it made the names long but I don't mind 
changing them.
> 
> 
>> +};
>> +
>> +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(struct platform_profile_handler *pprof);
>> +int platform_profile_unregister(void);
>> +int platform_profile_notify(void);
> 
> I don't think it's a big problem, but right now, I personally can't quite see the
> purpose `platform_profile_notify()` and `platform_profile_unregister()`
> returning any value.
OK - I can update
> 
> 
>> +
>> +#endif  /*_PLATFORM_PROFILE_H_*/
>> --
>> 2.25.1
> 
> 
> [1]: https://www.kernel.org/doc/html/latest/kernel-hacking/locking.html#locking-only-in-user-context
> 
> 
> Regards,
> Barnabás Pőcze
> 
Many thanks
Mark

Re: [External] Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Barnabás Pőcze]

Hi


2020. november 16., hétfő 0:04 keltezéssel, Mark Pearson írta:

> [...]
> >> +static ssize_t platform_profile_store(struct device *dev,
> >> +			    struct device_attribute *attr,
> >> +			    const char *buf, size_t count)
> >> +{
> >> +	int err, i;
> >> +
> >> +	mutex_lock(&profile_lock);
> >> +	if (!cur_profile) {
> >> +		mutex_unlock(&profile_lock);
> >> +		return -EOPNOTSUPP;
> >
> > I'm not sure why you modified the errno. ENODEV seemed to me like a perfectly
> > fine error to return if `cur_profile` is not set. `platform_profile_choices_show()`
> > returns ENODEV, so there is a bit of inconsistency.
> > (same applies to `platform_profile_show()`)
> My thinking was it seemed a better message. I can't really see any
> conditions when you'd get here (a driver would have registered a driver
> and then not provided a profile?) but it seemed that if that was
> possible it was more because changing the settings wasn't supported.
>
> I managed to convince myself it made more sense - but have no issue with
> changing it back.
> >
> >
> >> +	}
> >> +
> >> +	/* Scan for a matching profile */
> >> +	i = sysfs_match_string(profile_names, buf);
> >> +	if (i < 0) {
> >> +		mutex_unlock(&profile_lock);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	if (!cur_profile->profile_set) {
> >> +		mutex_unlock(&profile_lock);
> >> +		return -EOPNOTSUPP;
> >> +	}
> >> +

One thing I have just noticed is that I believe the selected profile should be
checked against `cur_profile->choices`, don't you think? I have assumed for
some reason that this check is done, and `profile_set` is only called with values
that the handler supports (they are in `choices`) up until now, but I see
that that's not what's happening.


> >> +	err = cur_profile->profile_set(i);
> >> +	mutex_unlock(&profile_lock);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	return count;
> >> +}
> [...]
> >> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> >> new file mode 100644
> >> index 000000000000..f6592434c8ce
> >> --- /dev/null
> >> +++ b/include/linux/platform_profile.h
> >> @@ -0,0 +1,36 @@
> [...]
> >
> > By the way, I still feel like the enum values are "too vague" and should be
> > prefixed with `platform_`. If you're not opposed to that change.
> Sure - I can change that. For me it made the names long but I don't mind
> changing them.

Short and succinct names a good, but I hope you can see why I'm saying what I'm
saying. I believe these names should be "properly namespaced" since they are in
a "kernel-global" header file.


> >
> >
> >> +};
> >> +
> >> [...]


Regards,
Barnabás Pőcze

Re: [External] Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Mark Pearson]

Hi Barnabás

On 16/11/2020 05:24, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2020. november 16., hétfő 0:04 keltezéssel, Mark Pearson írta:
> 
>> [...]
>>>> +static ssize_t platform_profile_store(struct device *dev,
>>>> +			    struct device_attribute *attr,
>>>> +			    const char *buf, size_t count)
>>>> +{
>>>> +	int err, i;
>>>> +
>>>> +	mutex_lock(&profile_lock);
>>>> +	if (!cur_profile) {
>>>> +		mutex_unlock(&profile_lock);
>>>> +		return -EOPNOTSUPP;
>>>
>>> I'm not sure why you modified the errno. ENODEV seemed to me like a perfectly
>>> fine error to return if `cur_profile` is not set. `platform_profile_choices_show()`
>>> returns ENODEV, so there is a bit of inconsistency.
>>> (same applies to `platform_profile_show()`)
>> My thinking was it seemed a better message. I can't really see any
>> conditions when you'd get here (a driver would have registered a driver
>> and then not provided a profile?) but it seemed that if that was
>> possible it was more because changing the settings wasn't supported.
>>
>> I managed to convince myself it made more sense - but have no issue with
>> changing it back.
>>>
>>>
>>>> +	}
>>>> +
>>>> +	/* Scan for a matching profile */
>>>> +	i = sysfs_match_string(profile_names, buf);
>>>> +	if (i < 0) {
>>>> +		mutex_unlock(&profile_lock);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	if (!cur_profile->profile_set) {
>>>> +		mutex_unlock(&profile_lock);
>>>> +		return -EOPNOTSUPP;
>>>> +	}
>>>> +
> 
> One thing I have just noticed is that I believe the selected profile should be
> checked against `cur_profile->choices`, don't you think? I have assumed for
> some reason that this check is done, and `profile_set` is only called with values
> that the handler supports (they are in `choices`) up until now, but I see
> that that's not what's happening.
True - I guess no harm adding the check in platform_profile.c too.

> 
> 
>>>> +	err = cur_profile->profile_set(i);
>>>> +	mutex_unlock(&profile_lock);
>>>> +	if (err)
>>>> +		return err;
>>>> +
>>>> +	return count;
>>>> +}
>> [...]
>>>> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
>>>> new file mode 100644
>>>> index 000000000000..f6592434c8ce
>>>> --- /dev/null
>>>> +++ b/include/linux/platform_profile.h
>>>> @@ -0,0 +1,36 @@
>> [...]
>>>
>>> By the way, I still feel like the enum values are "too vague" and should be
>>> prefixed with `platform_`. If you're not opposed to that change.
>> Sure - I can change that. For me it made the names long but I don't mind
>> changing them.
> 
> Short and succinct names a good, but I hope you can see why I'm saying what I'm
> saying. I believe these names should be "properly namespaced" since they are in
> a "kernel-global" header file.
I do and I will update :)

Thanks for all the pointers
Mark

Re: [External] Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Hans de Goede]

Hi,

On 11/16/20 12:04 AM, Mark Pearson wrote:

<snip>
>> I believe there's no reason to remove the comma from there, and in fact,
>> having a comma after the last entry in an array, enum, etc. seems to be
>> the preferred.
> OK.
> Have to be honest - I struggle to know when comma's are needed on the last entry and when they aren't (I've had similar corrections in other cases both ways :)). I do seem to have a knack of getting it consistently wrong....

Do the rule of thumb here is, if the last element is a terminating element,
e.g. NULL or {} or foo_number_of_foo_types in an enum foo declaration then
there should not be a comma after the last element. The reason for is is
that in case case new entries will be added one line above the last element.

If there is no terminating element (e.g. because ARRAY_SIZE is always used
on the array). Then the last element should end with a comma. The reason for
this is so that the unified diff of a patch adding a new element does not
have -++ lines, as would be necessary when the comma is missing (-+ to add
the comma, plus one more + for the new element).

I hope this helps explain.

I expect you will send out a v4 of the entire set addressing all current
remarks?

Regards,

Hans

Re: [External] Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Mark Pearson]

Thanks Hans

On 16/11/2020 09:33, Hans de Goede wrote:
> Hi,
> 
> On 11/16/20 12:04 AM, Mark Pearson wrote:
> 
> <snip>
>>> I believe there's no reason to remove the comma from there, and in fact,
>>> having a comma after the last entry in an array, enum, etc. seems to be
>>> the preferred.
>> OK.
>> Have to be honest - I struggle to know when comma's are needed on the last entry and when they aren't (I've had similar corrections in other cases both ways :)). I do seem to have a knack of getting it consistently wrong....
> 
> Do the rule of thumb here is, if the last element is a terminating element,
> e.g. NULL or {} or foo_number_of_foo_types in an enum foo declaration then
> there should not be a comma after the last element. The reason for is is
> that in case case new entries will be added one line above the last element.
> 
> If there is no terminating element (e.g. because ARRAY_SIZE is always used
> on the array). Then the last element should end with a comma. The reason for
> this is so that the unified diff of a patch adding a new element does not
> have -++ lines, as would be necessary when the comma is missing (-+ to add
> the comma, plus one more + for the new element).
> 
> I hope this helps explain.
It does - makes complete sense.
> 
> I expect you will send out a v4 of the entire set addressing all current
> remarks?

Absolutely. Hopefully will get that out soon but I'm going to take a bit 
longer on it as I was pretty disappointed with myself for some of the 
things that slipped into the last set. I'll aim to get a cleaner set out 
for v4.

> 
> Regards,
> 
> Hans
> 
Thanks
Mark

Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Barnabás Pőcze]

Hi


2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta:

> [...]
> +int platform_profile_register(struct platform_profile_handler *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_group);
> +}
> +EXPORT_SYMBOL_GPL(platform_profile_register);
> +
> +int platform_profile_unregister(void)
> +{
> +	mutex_lock(&profile_lock);
> +	sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +	cur_profile = NULL;
> +	mutex_unlock(&profile_lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(platform_profile_unregister);
> [...]


I just realized that the sysfs attributes are only created if a profile provider
is registered, and it is removed when the provide unregisters itself. I believe
it would be easier for system daemons if those attributes existed from module load
to module unload since they can just just open the file and watch it using poll,
select, etc. If it goes away when the provider unregisters itself, then I believe
a more complicated mechanism (like inotify) would need to be implemented in the
daemons to be notified when a new provider is registered. Thus my suggestion
for the next iteration is to create the sysfs attributes on module load,
and delete them on unload.

What do you think?


Regards,
Barnabás Pőcze

Re: [External] Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Mark Pearson]

On 20/11/2020 14:50, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta:
> 
>> [...]
>> +int platform_profile_register(struct platform_profile_handler *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_group);
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_register);
>> +
>> +int platform_profile_unregister(void)
>> +{
>> +	mutex_lock(&profile_lock);
>> +	sysfs_remove_group(acpi_kobj, &platform_profile_group);
>> +	cur_profile = NULL;
>> +	mutex_unlock(&profile_lock);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_unregister);
>> [...]
> 
> 
> I just realized that the sysfs attributes are only created if a profile provider
> is registered, and it is removed when the provide unregisters itself. I believe
> it would be easier for system daemons if those attributes existed from module load
> to module unload since they can just just open the file and watch it using poll,
> select, etc. If it goes away when the provider unregisters itself, then I believe
> a more complicated mechanism (like inotify) would need to be implemented in the
> daemons to be notified when a new provider is registered. Thus my suggestion
> for the next iteration is to create the sysfs attributes on module load,
> and delete them on unload.
> 
> What do you think?
> 
> 
> Regards,
> Barnabás Pőcze
> 
Good point. I'd like to have input from user space as to their 
preference - it seems like a simple enough change to make and I'm happy 
to go with what their preferences are. Bastien - would this make life 
easier for you?

Perhaps when the provider unregisters it sends a sysfs_notify and user 
space can look and detect/handle that condition (I think it will get 
EOPNOTSUPP on the next read). Is there something cleverer it can do?

Thanks
Mark

Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Hans de Goede]

Hi,

On 11/20/20 8:50 PM, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta:
> 
>> [...]
>> +int platform_profile_register(struct platform_profile_handler *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_group);
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_register);
>> +
>> +int platform_profile_unregister(void)
>> +{
>> +	mutex_lock(&profile_lock);
>> +	sysfs_remove_group(acpi_kobj, &platform_profile_group);
>> +	cur_profile = NULL;
>> +	mutex_unlock(&profile_lock);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(platform_profile_unregister);
>> [...]
> 
> 
> I just realized that the sysfs attributes are only created if a profile provider
> is registered, and it is removed when the provide unregisters itself. I believe
> it would be easier for system daemons if those attributes existed from module load
> to module unload since they can just just open the file and watch it using poll,
> select, etc. If it goes away when the provider unregisters itself, then I believe
> a more complicated mechanism (like inotify) would need to be implemented in the
> daemons to be notified when a new provider is registered. Thus my suggestion
> for the next iteration is to create the sysfs attributes on module load,
> and delete them on unload.
> 
> What do you think?

Actually I asked Mark to move this to the register / unregister time since
having a non functioning files in sysfs is a bit weird.

You make a good point about userspace having trouble figuring when this will
show up though (I'm not worried about removal that will normally never happen).

I would expect all modules to be loaded before any interested daemons load,
but that is an assumption and I must admit that that is a bit racy.

Bastien, what do you think about Barnabás' suggestion to always have the
files present and use poll (POLL_PRI) on it to see when it changes, listing
maybe "none" as available profiles when there is no provider?

Regards,

Hans

Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Barnabás Pőcze]

2020. november 21., szombat 15:27 keltezéssel, Hans de Goede írta:

> [...]
> > I just realized that the sysfs attributes are only created if a profile provider
> > is registered, and it is removed when the provide unregisters itself. I believe
> > it would be easier for system daemons if those attributes existed from module load
> > to module unload since they can just just open the file and watch it using poll,
> > select, etc. If it goes away when the provider unregisters itself, then I believe
> > a more complicated mechanism (like inotify) would need to be implemented in the
> > daemons to be notified when a new provider is registered. Thus my suggestion
> > for the next iteration is to create the sysfs attributes on module load,
> > and delete them on unload.
> >
> > What do you think?
>
> Actually I asked Mark to move this to the register / unregister time since
> having a non functioning files in sysfs is a bit weird.
> [...]

Ahh, I didn't know that, sorry. If a non-functioning sysfs attribute is a problem,
then there is another option: `platform_profile_choices` is always present;
if no provider is registered, it's empty. If a provider is (un)registered,
then `platform_profile_choices` is sysfs_notify()-ed. `platform_profile`
only exists while a provider is registered, so it is created on provider
registration and unregistered on provider unregistration.


Regards,
Barnabás Pőcze

Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Hans de Goede]

Hi,

On 11/21/20 10:18 PM, Barnabás Pőcze wrote:
> 2020. november 21., szombat 15:27 keltezéssel, Hans de Goede írta:
> 
>> [...]
>>> I just realized that the sysfs attributes are only created if a profile provider
>>> is registered, and it is removed when the provide unregisters itself. I believe
>>> it would be easier for system daemons if those attributes existed from module load
>>> to module unload since they can just just open the file and watch it using poll,
>>> select, etc. If it goes away when the provider unregisters itself, then I believe
>>> a more complicated mechanism (like inotify) would need to be implemented in the
>>> daemons to be notified when a new provider is registered. Thus my suggestion
>>> for the next iteration is to create the sysfs attributes on module load,
>>> and delete them on unload.
>>>
>>> What do you think?
>>
>> Actually I asked Mark to move this to the register / unregister time since
>> having a non functioning files in sysfs is a bit weird.
>> [...]
> 
> Ahh, I didn't know that, sorry. If a non-functioning sysfs attribute is a problem,
> then there is another option: `platform_profile_choices` is always present;
> if no provider is registered, it's empty. If a provider is (un)registered,
> then `platform_profile_choices` is sysfs_notify()-ed. `platform_profile`
> only exists while a provider is registered, so it is created on provider
> registration and unregistered on provider unregistration.

TBH, I don't like this suggestion. I would like to either want both of
them be present and report "none" (and -ENODEV on write in case of platform_profile),
or neither of them be present.

Note I do agree with you that userspace probably needs a way to find out when a
provider shows up. Which means we should probably go with always having both of
them present. But it would also be good to get some input from Bastien here,
as he is working on a userspace daemon using this API.

Regards,

Hans

Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Bastien Nocera]

On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/20/20 8:50 PM, Barnabás Pőcze wrote:
> > Hi
> > 
> > 
> > 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta:
> > 
> > > [...]
> > > +int platform_profile_register(struct platform_profile_handler
> > > *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_group);
> > > +}
> > > +EXPORT_SYMBOL_GPL(platform_profile_register);
> > > +
> > > +int platform_profile_unregister(void)
> > > +{
> > > +       mutex_lock(&profile_lock);
> > > +       sysfs_remove_group(acpi_kobj, &platform_profile_group);
> > > +       cur_profile = NULL;
> > > +       mutex_unlock(&profile_lock);
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(platform_profile_unregister);
> > > [...]
> > 
> > 
> > I just realized that the sysfs attributes are only created if a
> > profile provider
> > is registered, and it is removed when the provide unregisters
> > itself. I believe
> > it would be easier for system daemons if those attributes existed
> > from module load
> > to module unload since they can just just open the file and watch
> > it using poll,
> > select, etc. If it goes away when the provider unregisters itself,
> > then I believe
> > a more complicated mechanism (like inotify) would need to be
> > implemented in the
> > daemons to be notified when a new provider is registered. Thus my
> > suggestion
> > for the next iteration is to create the sysfs attributes on module
> > load,
> > and delete them on unload.
> > 
> > What do you think?
> 
> Actually I asked Mark to move this to the register / unregister time
> since
> having a non functioning files in sysfs is a bit weird.
> 
> You make a good point about userspace having trouble figuring when
> this will
> show up though (I'm not worried about removal that will normally
> never happen).
> 
> I would expect all modules to be loaded before any interested daemons
> load,
> but that is an assumption and I must admit that that is a bit racy.
> 
> Bastien, what do you think about Barnabás' suggestion to always have
> the
> files present and use poll (POLL_PRI) on it to see when it changes,
> listing
> maybe "none" as available profiles when there is no provider?

Whether the file exists or doesn't, we have ways to monitor it
appearing or disappearing. I can monitor the directory with inotify to
see the file appearing or disappearing, or I can monitor the file with
inotify to see whether it changes. But that doesn't solve the
challenges from user-space.

How do I know whether the computer will have this facility at any
point? Is "none" a placeholder, or a definite answer as to whether the
computer will have support for "platform profile"?

How am I supposed to handle fallbacks, eg. how can I offer support for,
say, changing the "energy performance preference" from the Intel p-
state driver if ACPI platform profile isn't available?

I'm fine with throwing more code at fixing that race, so whatever's
easier for you, it'll be a pain for user-space either way.

Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Hans de Goede]

Hi,

On 11/24/20 4:30 PM, Bastien Nocera wrote:
> On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/20/20 8:50 PM, Barnabás Pőcze wrote:
>>> Hi
>>>
>>>
>>> 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta:
>>>
>>>> [...]
>>>> +int platform_profile_register(struct platform_profile_handler
>>>> *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_group);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(platform_profile_register);
>>>> +
>>>> +int platform_profile_unregister(void)
>>>> +{
>>>> +       mutex_lock(&profile_lock);
>>>> +       sysfs_remove_group(acpi_kobj, &platform_profile_group);
>>>> +       cur_profile = NULL;
>>>> +       mutex_unlock(&profile_lock);
>>>> +       return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(platform_profile_unregister);
>>>> [...]
>>>
>>>
>>> I just realized that the sysfs attributes are only created if a
>>> profile provider
>>> is registered, and it is removed when the provide unregisters
>>> itself. I believe
>>> it would be easier for system daemons if those attributes existed
>>> from module load
>>> to module unload since they can just just open the file and watch
>>> it using poll,
>>> select, etc. If it goes away when the provider unregisters itself,
>>> then I believe
>>> a more complicated mechanism (like inotify) would need to be
>>> implemented in the
>>> daemons to be notified when a new provider is registered. Thus my
>>> suggestion
>>> for the next iteration is to create the sysfs attributes on module
>>> load,
>>> and delete them on unload.
>>>
>>> What do you think?
>>
>> Actually I asked Mark to move this to the register / unregister time
>> since
>> having a non functioning files in sysfs is a bit weird.
>>
>> You make a good point about userspace having trouble figuring when
>> this will
>> show up though (I'm not worried about removal that will normally
>> never happen).
>>
>> I would expect all modules to be loaded before any interested daemons
>> load,
>> but that is an assumption and I must admit that that is a bit racy.
>>
>> Bastien, what do you think about Barnabás' suggestion to always have
>> the
>> files present and use poll (POLL_PRI) on it to see when it changes,
>> listing
>> maybe "none" as available profiles when there is no provider?
> 
> Whether the file exists or doesn't, we have ways to monitor it
> appearing or disappearing. I can monitor the directory with inotify to
> see the file appearing or disappearing, or I can monitor the file with
> inotify to see whether it changes.

Ok, do you have a preference either way? I personally think having the
file only appear if its functional is a bit cleaner. So if it does not
matter much for userspace either way then I suggest we go that route.

> But that doesn't solve the challenges from user-space.
> 
> How do I know whether the computer will have this facility at any
> point? Is "none" a placeholder, or a definite answer as to whether the
> computer will have support for "platform profile"?
> 
> How am I supposed to handle fallbacks, eg. how can I offer support for,
> say, changing the "energy performance preference" from the Intel p-
> state driver if ACPI platform profile isn't available?
> 
> I'm fine with throwing more code at fixing that race, so whatever's
> easier for you, it'll be a pain for user-space either way.
 
Ugh, right this is a bit different from other hotplug cases, where
you just wait for a device to show up before using it, since in
this case you want to use the p-state driver as alternative mechanism
when there is no platform_profile provider.

I'm afraid that the answer here is that the kernel does not really
have anything to help you here. Since the advent of hotplug we had
this issue of determining when the initial hardware probing / driver
loading is done. This is basically an impossible problem since with
things like thunderbolt, etc. probing may be a bit slow and then if
there is a lot of USB devices connected to the thunderbolt dock,
those are slow to probe too, etc. See either we wait 5 minutes just to
be sure, or we cannot really tell when probing is done.

Time has taught us that determining when probing is done is an unsolvable
problem.

We had e.g. udev-settle for this. But that was both slow and not reliable,
so that is deprectated and we don't want to bring it back. In general whenever
someone wants something like udev-settle the answer is to make the
code wanting that more event driven/dynamic. So I'm afraid that that
is the answer here too.

I have the feeling that in most cases the platform_profile driver will have
loaded before the daemon starts. So you could use that as a base assumption
and then do something somewhat ugly like log a message + restart in case you
loose the race. Assuming that that is easy to implement, you could do that
for starters and then if loosing the race becomes a real problem, then
implement something smarter (and more complicated) later.

Sorry, I have no easy answers here.

Regards,

Hans

Re: [External] Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Mark Pearson]

On 25/11/2020 11:42, Hans de Goede wrote:
> Hi,
> 
> On 11/24/20 4:30 PM, Bastien Nocera wrote:
>> On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote:
>>> Hi,
>>> On 11/20/20 8:50 PM, Barnabás Pőcze wrote:
>>>> Hi
>>>> 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson
>>>> írta:
>>>> 
>>> Bastien, what do you think about Barnabás' suggestion to always
>>> have the files present and use poll (POLL_PRI) on it to see when
>>> it changes, listing maybe "none" as available profiles when there
>>> is no provider?
>> 
>> Whether the file exists or doesn't, we have ways to monitor it 
>> appearing or disappearing. I can monitor the directory with inotify
>> to see the file appearing or disappearing, or I can monitor the
>> file with inotify to see whether it changes.
> 
> Ok, do you have a preference either way? I personally think having
> the file only appear if its functional is a bit cleaner. So if it
> does not matter much for userspace either way then I suggest we go
> that route.
> 
My (limited) vote is having the file appear when the profile is loaded 
seems nicer (and to disappear if the profile is unloaded). No profile, 
no interface just seems elegant.

I pretty much have v4 of the patches ready to go (I wanted to rebase on 
the update for the palm sensor and finished that today). I'm happy to 
hold off on them until we're ready if that helps. Bastien - let me know 
if it would be useful to have a version to test against to see what will 
work best for you, or if you need something different. Definitely want 
to make sure user space gets the best option as my understanding is 
changing this later becomes a pain :)

Thanks
Mark

Re: [External] Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Bastien Nocera]

On Wed, 2020-11-25 at 14:41 -0500, Mark Pearson wrote:
> On 25/11/2020 11:42, Hans de Goede wrote:
> > Hi,
> > 
> > On 11/24/20 4:30 PM, Bastien Nocera wrote:
> > > On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote:
> > > > Hi,
> > > > On 11/20/20 8:50 PM, Barnabás Pőcze wrote:
> > > > > Hi
> > > > > 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson
> > > > > írta:
> > > > > 
> > > > Bastien, what do you think about Barnabás' suggestion to always
> > > > have the files present and use poll (POLL_PRI) on it to see
> > > > when
> > > > it changes, listing maybe "none" as available profiles when
> > > > there
> > > > is no provider?
> > > 
> > > Whether the file exists or doesn't, we have ways to monitor it 
> > > appearing or disappearing. I can monitor the directory with
> > > inotify
> > > to see the file appearing or disappearing, or I can monitor the
> > > file with inotify to see whether it changes.
> > 
> > Ok, do you have a preference either way? I personally think having
> > the file only appear if its functional is a bit cleaner. So if it
> > does not matter much for userspace either way then I suggest we go
> > that route.
> > 
> My (limited) vote is having the file appear when the profile is
> loaded 
> seems nicer (and to disappear if the profile is unloaded). No
> profile, 
> no interface just seems elegant.
> 
> I pretty much have v4 of the patches ready to go (I wanted to rebase
> on 
> the update for the palm sensor and finished that today). I'm happy to
> hold off on them until we're ready if that helps. Bastien - let me
> know 
> if it would be useful to have a version to test against to see what
> will 
> work best for you, or if you need something different. Definitely
> want 
> to make sure user space gets the best option as my understanding is 
> changing this later becomes a pain :)

I don't have the hardware. I agree with you that "no profile = no file"
seems cleaner, but whichever is easier for you.

Re: [External] Re: [PATCH v3] ACPI: platform-profile: Add platform profile support

[Hans de Goede]

Hi,

On 11/25/20 11:32 PM, Bastien Nocera wrote:
> On Wed, 2020-11-25 at 14:41 -0500, Mark Pearson wrote:
>> On 25/11/2020 11:42, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 11/24/20 4:30 PM, Bastien Nocera wrote:
>>>> On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote:
>>>>> Hi,
>>>>> On 11/20/20 8:50 PM, Barnabás Pőcze wrote:
>>>>>> Hi
>>>>>> 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson
>>>>>> írta:
>>>>>>
>>>>> Bastien, what do you think about Barnabás' suggestion to always
>>>>> have the files present and use poll (POLL_PRI) on it to see
>>>>> when
>>>>> it changes, listing maybe "none" as available profiles when
>>>>> there
>>>>> is no provider?
>>>>
>>>> Whether the file exists or doesn't, we have ways to monitor it 
>>>> appearing or disappearing. I can monitor the directory with
>>>> inotify
>>>> to see the file appearing or disappearing, or I can monitor the
>>>> file with inotify to see whether it changes.
>>>
>>> Ok, do you have a preference either way? I personally think having
>>> the file only appear if its functional is a bit cleaner. So if it
>>> does not matter much for userspace either way then I suggest we go
>>> that route.
>>>
>> My (limited) vote is having the file appear when the profile is
>> loaded 
>> seems nicer (and to disappear if the profile is unloaded). No
>> profile, 
>> no interface just seems elegant.
>>
>> I pretty much have v4 of the patches ready to go (I wanted to rebase
>> on 
>> the update for the palm sensor and finished that today). I'm happy to
>> hold off on them until we're ready if that helps. Bastien - let me
>> know 
>> if it would be useful to have a version to test against to see what
>> will 
>> work best for you, or if you need something different. Definitely
>> want 
>> to make sure user space gets the best option as my understanding is 
>> changing this later becomes a pain :)
> 
> I don't have the hardware. I agree with you that "no profile = no file"
> seems cleaner, but whichever is easier for you.

Ok, lets go with that then (so same as in v3).

Mark, if you can submit v4 upstream that would be great.

Regards,

Hans