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.
> +
> +struct platform_profile *cur_profile;
This should be `static`.
> +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.
> +static char *profile_str[] = {
Why is it not `const`?
> + "Low-power",
> + "Cool",
> + "Quiet",
> + "Balance",
> + "Performance",
> + "Unknown"
"unknown" is not documented, yet it may be returned to userspace.
> +};
The documentation has the names in all-lowercase, and in my opinion it'd be
better to use lowercase names here as well.
> +
> +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?
> + }
> +
> + for (i = profile_low; i < profile_unknown; i++) {
> + if (cur_profile->choices & (1 << i)) {
`BIT(i)`?
> + 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.
> + 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.
> + count += ret;
> + }
> + }
> + mutex_unlock(&profile_lock);
I think a newline character should be written at the end (possibly overwriting
the last space).
> + 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.
> + 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.
> +}
> +
> +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?
> +
> + 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.
> +
> +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.
> + return 0;
> +}
> +
> +static void platform_profile_exit(void)
This should be marked `__exit`.
> +{
> + 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.
> +
> +struct platform_profile {
Personally, I think a name like platform_profile_(handler|provider)
would be a better fit.
> + unsigned int choices; /* bitmap of available choices */
Most comments are capitalized.
> + 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.
> + int (*profile_get)(void);
> + int (*profile_set)(int profile);
Why does it take an `int` instead of `enum profile_option`?
> +};
> +
> +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.
> +#endif /*_PLATFORM_PROFILE_H_*/
> --
> 2.28.0
Regards,
Barnabás Pőcze
