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].
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?
> [...]
> +// 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.
> +#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, ...)
> +
> +static struct platform_profile_handler *cur_profile;
I think this could be `const` as well.
(along with the argument of `platform_profile_register()`)
> +static DEFINE_MUTEX(profile_lock);
> +
> +/* Ensure the first char of each profile is unique */
I think this comment is not needed anymore, no?
> [...]
> +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.
> + }
> + }
> + 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.
> +}
> +
> +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()`)
> + }
> +
> + /* 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.
> +}
> +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.
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.
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.
> +};
> +
> +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.
> +
> +#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
