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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>");
+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



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux