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
