Thanks Hans
On 2020-11-28 9:08 a.m., Hans de Goede wrote:
Hi,
On 11/26/20 5:51 PM, Mark Pearson wrote:
<snip>
diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
new file mode 100644
index 000000000000..678cb4596ada
--- /dev/null
+++ b/drivers/acpi/platform_profile.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* Platform profile sysfs interface */
+
+#include <linux/acpi.h>
+#include <linux/bits.h>
+#include <linux/init.h>
+#include <linux/mutex.h>
+#include <linux/platform_profile.h>
+#include <linux/sysfs.h>
+
+static const struct platform_profile_handler *cur_profile;
+static DEFINE_MUTEX(profile_lock);
+
+static const char * const profile_names[] = {
+ [platform_profile_low] = "low-power",
+ [platform_profile_cool] = "cool",
+ [platform_profile_quiet] = "quiet",
+ [platform_profile_balance] = "balance",
As Barnabás already pointed out the docs added in patch 1 say "balanced"
and here you use "balance" both in the enum value/label as we as in
the actual string. I have a slight preference for balanced, but the
most important thing here is being consistent everywhere.
Agreed - I'll fix, my mistake.
+ [platform_profile_perform] = "performance",
+};
+static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1);
It would be better to add an extra member/entry at the end of the enum
named platform_profile_no_profiles; and then use that instead of
platform_profile_perform+1. Also see below where I use this too.
Makes sense. Will do (and update the documentation too)
+
+static ssize_t platform_profile_choices_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ int len = 0;
+ int err, i;
+
+ err = mutex_lock_interruptible(&profile_lock);
+ if (err)
+ return err;
+
+ if (!cur_profile) {
+ mutex_unlock(&profile_lock);
+ return -ENODEV;
+ }
+
+ if (!cur_profile->choices) {
+ mutex_unlock(&profile_lock);
+ return sysfs_emit(buf, "\n");
+ }
If choices is empty, the for below will never print anything and
the end result is still just emitting "\n", so this whole if
block is unnecessary and can be removed.
Agreed - I'll remove
+
+ for (i = 0; i < ARRAY_SIZE(profile_names); i++) {
+ if (cur_profile->choices & BIT(i)) {
Please change the type of choices to an unsigned long array, like this:
unsigned long choices[BITS_TO_LONGS(platform_profile_no_profiles)];
And then replace the for + if with:
for_each_set_bit(i, cur_profile->choices, platform_profile_no_profiles) {
This has 2 advantages:
1. Using for_each_set_bit is nicer then open coding it
2. If we ever go over 32 profile choices this will keep working automatically
without needing any adjustment to the code.
Note this requires include/linux/bitops.h (in include/linux/platform_profile.h,
since you will be using BITS_TO_LONGS there).
Neat - I hadn't come across this before. I'll give it a go. Thank you.
+ 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 = platform_profile_balance;
+ int err;
+
+ err = mutex_lock_interruptible(&profile_lock);
+ if (err)
+ return err;
+
+ if (!cur_profile) {
+ mutex_unlock(&profile_lock);
+ return -ENODEV;
+ }
+
+ 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;
+
+ /* Check that profile is valid index */
+ if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))
+ return sysfs_emit(buf, "\n");
IMHO it would be better to do "return -EIO" here, since there
clearly is something wrong in this case.
Agreed. I'll correct this.
<snip>
+
+int platform_profile_register(const struct platform_profile_handler *pprof)
+{
+ int err;
Maybe sanity check the platform_profile_handler a bit here,
I think it would be ok to check that choices, profile_set and profile_get
are all not 0 / NULL here and otherwise just return -EINVAL; Doing so
allows making the code above a bit simpler, also removing some exit
paths which require an unlock before exiting.
Makes sense - will do.
+
+ err = mutex_lock_interruptible(&profile_lock);
+ if (err)
+ return err;
Please use a regular mutex_lock here, this is called during
driver probing, so no need to handle Ctrl+C and other signals.
Will do
+
+ /* We can only have one active profile */
+ if (cur_profile) {
+ mutex_unlock(&profile_lock);
+ return -EEXIST;
+ }
+
+ err = sysfs_create_group(acpi_kobj, &platform_profile_group);
+ if (err) {
+ mutex_unlock(&profile_lock);
+ return err;
+ }
+
+ cur_profile = pprof;
+ mutex_unlock(&profile_lock);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(platform_profile_register);
+
+int platform_profile_unregister(void)
+{
+ int err;
+
+ err = mutex_lock_interruptible(&profile_lock);
+ if (err)
+ return err;
Also please use a regular mutex_lock here.
Ack
+
+ if (!cur_profile) {
+ mutex_unlock(&profile_lock);
+ return -ENODEV;
+ }
+
+ 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);
AFAIK you do not need to provide a module_init function, since this
is a no-op you can just leave it out.
Ah - I never realised you could just not have init and exit functions.
I'll remove.
+static void __exit platform_profile_exit(void)
+{
+ /* Check if we have a registered profile, and clean up */
+ if (cur_profile) {
+ sysfs_remove_group(acpi_kobj, &platform_profile_group);
+ cur_profile = NULL;
+ }
+}
+module_exit(platform_profile_exit);
Any driver/module registering a platform_profile will depend on
symbols from this module, so this module can only be unloaded (aka exit)
if that other driver/module has already been unloaded. Unloading that
other module should have already unregistered the cur_profile provider,
otherwise things like cur_profile->profile_set will now point to no
longer loaded code which would be really really bad. So you can drop
this exit function entirely.
Will do
Sorry for not spotting some of these sooner. Still nothing really big,
so hopefully you will be able to post a v5 addressing these soonish and
then with some luck we can at least get patches 1 and 2 merged by Rafael
in time for 5.11 (assuming Rafael is happy with v5). And then I can merge
patch 3 once 5.11-rc1 is out.
Sounds good - I'll get working on those ASAP (and as an advance note -
I've read ahead to your other email and I'm on board with that plan)
Thanks for the review - really appreciated.
Mark
