Re: [PATCH 2/3] ACPI: platform-profile: Add platform profile support

From: "Barnabás Pőcze" <pobrn@protonmail.com>
To: Mark Pearson <markpearson@lenovo.com>
Cc: "rjw@rjwysocki.net" <rjw@rjwysocki.net>,
	"hdegoede@redhat.com" <hdegoede@redhat.com>,
	"mgross@linux.intel.com" <mgross@linux.intel.com>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"mario.limonciello@dell.com" <mario.limonciello@dell.com>,
	"eliadevito@gmail.com" <eliadevito@gmail.com>,
	"hadess@hadess.net" <hadess@hadess.net>,
	"bberg@redhat.com" <bberg@redhat.com>,
	"platform-driver-x86@vger.kernel.org" 
	<platform-driver-x86@vger.kernel.org>,
	"dvhart@infradead.org" <dvhart@infradead.org>
Subject: Re: [PATCH 2/3] ACPI: platform-profile: Add platform profile support
Date: Tue, 10 Nov 2020 10:15:35 +0000	[thread overview]
Message-ID: <2gY5rkKaKLKayk0DYW0lvZ_aIAs8vSf9FOy2obdGvph_7XcpyHlkafBTpW8RHKC5nEcEz_eY-s4pJtuR2ebltW2Fu10GRssTmMxKMuS4PU8=@protonmail.com> (raw)
In-Reply-To: <20201110033124.3211-3-markpearson@lenovo.com>

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@lenovo.com>");
> +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