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
