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

[Mark Pearson <markpearson@lenovo.com>]

Thanks Barnabas

On 10/11/2020 05:15, Barnabás Pőcze wrote:
> 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.
Ack - I hadn't realised that was a requirement. I'll update
> 
> 
>> +
>> +struct platform_profile *cur_profile;
> 
> This should be `static`.
Agreed.

> 
> 
>> +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.
OK, fair enough. I debated about it and it's nice just to do
    echo 'L' > platform_profile
instead of typing in a whole string, but I appreciate it's lazy.

I'm OK with fixing based on your points.

> 
> 
>> +static char *profile_str[] = {
> 
> Why is it not `const`?
My mistake. I will fix
> 
> 
>> +	"Low-power",
>> +	"Cool",
>> +	"Quiet",
>> +	"Balance",
>> +	"Performance",
>> +	"Unknown"
> 
> "unknown" is not documented, yet it may be returned to userspace.
Ack - I'll look into if it's really needed, but it seemed sensible to 
have it whilst doing the implementation.
> 
> 
>> +};
> 
> The documentation has the names in all-lowercase, and in my opinion it'd be
> better to use lowercase names here as well.
Agreed - my bad. I'll fix.
> 
> 
>> +
>> +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?
Agreed, I'll fix.
> 
> 
>> +	}
>> +
>> +	for (i = profile_low; i < profile_unknown; i++) {
>> +		if (cur_profile->choices & (1 << i)) {
> 
> `BIT(i)`?
Yes, that would be better.

> 
> 
>> +			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.
OK - I hadn't come across that function. I'll update to use it.

> 
> 
>> +			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.
Interesting point, I think I agree.
I'll look at returning an empty string if an error occurs.
> 
> 
>> +			count += ret;
>> +		}
>> +	}
>> +	mutex_unlock(&profile_lock);
> 
> I think a newline character should be written at the end (possibly overwriting
> the last space).
OK.

> 
> 
>> +	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.
OK - I went through a few iterations here and it's a bit weak. I'll look 
at it again.

> 
> 
>> +	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.
Agreed - I'll fix this.
> 
> 
>> +}
>> +
>> +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?
OK.

> 
> 
>> +
>> +	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.
OK - I'd not come across that and will look at using it.
> 
> 
>> +
>> +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.
I guess I was just playing safe here, I think you're right.
I'll remove.
> 
> 
>> +	return 0;
>> +}
>> +
>> +static void platform_profile_exit(void)
> 
> This should be marked `__exit`.
Not sure how I missed that one.... Thanks.
> 
> 
>> +{
>> +	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.
I can add 'platform_'
I liked having profile_unknown as the last value as it makes scanning 
from 'low' to 'unknown' more future proof if other profiles get added 
(e.g in platform_profile_choices_show).
Is this something you feel strongly about?
> 
> 
>> +
>> +struct platform_profile {
> 
> Personally, I think a name like platform_profile_(handler|provider)
> would be a better fit.
OK - happy to update that.
> 
> 
>> +	unsigned int choices; /* bitmap of available choices */
> 
> Most comments are capitalized.
Ack.
> 
> 
>> +	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.
It seemed a logical place to keep it to me. I understand where you're 
coming from, and I can change it so it's just internal to 
thinkpad_acpi.c but it just seemed a nice place to keep it and I'm 
guessing other implementations would like to have it too.

I'd be interested to see what others have to say on this one.
> 
> 
>> +	int (*profile_get)(void);
>> +	int (*profile_set)(int profile);
> 
> Why does it take an `int` instead of `enum profile_option`?
Mostly for error conditions to be propagated if needs be, though I have 
some improvements to do based on the comments above :)
> 
> 
>> +};
>> +
>> +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.
OK.
> 
> 
>> +#endif  /*_PLATFORM_PROFILE_H_*/
>> --
>> 2.28.0
> 
> 
> Regards,
> Barnabás Pőcze
> 
Thank you for the detailed review - really appreciate the time you spent 
going through this.

Mark