| Message ID | 20201110033124.3211-3-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Platform Profile support | expand |
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
On Tue, Nov 10, 2020 at 5:35 AM Mark Pearson <markpearson@lenovo.com> wrote: > > This is the initial implementation of the platform-profile feature. > It provides the details discussed and outlined in the > sysfs-platform_profile document. > > Many modern systems have the ability to modify the operating profile to > control aspects like fan speed, temperature and power levels. This > module provides a common sysfs interface that platform modules can register > against to control their individual profile options. ... > +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. > + > + None of the blank lines is enough. But can you consider to find perhaps better place (I imply some logical group of options in the file). ... > obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o > obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o > obj-$(CONFIG_ACPI_PPTT) += pptt.o > +obj-$(CONFIG_ACPI_PLATFORM_PROFILE) += platform_profile.o ...yes, and this becomes consistent with the above. ... > +/* > + * platform_profile.c - Platform profile sysfs interface > + */ One line. PLease, don't put the file name into the file. If we want to rename it, it will give additional churn and as shown in practice people often forget this change to follow. ... > +#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> Perhaps sorted? Why do you need a specific acpi_bus.h? I thought acpi.h includes it already, no? ... > +struct platform_profile *cur_profile; Better naming since it's a global variable. Is it supposed to be exported to modules? ... > +DEFINE_MUTEX(profile_lock); No static? ... > +/* Ensure the first char of each profile is unique */ > +static char *profile_str[] = { static const char * const profile_names[] Also naming (perhaps like I proposed above?). > + "Low-power", > + "Cool", > + "Quiet", > + "Balance", > + "Performance", > + "Unknown" Leave the comma here. > +}; ... > + int i; > + int ret, count = 0; count AFAICS should be size_t (or ssize_t). Can you make them in reversed xmas tree order? ... > + return snprintf(buf, PAGE_SIZE, "%s", profile_str[profile]); Nowadays we have sysfs_emit(), use it. ... > + /* Scan for a matching profile */ > + for (profile = profile_low; profile < profile_unknown; profile++) { > + if (toupper(buf[0]) == profile_str[profile][0]) > + break; > + } match_string() / sysfs_match_string() ? ... > +static struct attribute *platform_profile_attributes[] = { > + &dev_attr_platform_profile_choices.attr, > + &dev_attr_platform_profile.attr, > + NULL, Drop comma in terminator line. > +}; ... > +module_init(platform_profile_init); > +module_exit(platform_profile_exit); Attach them to respective functions. ... > +/* > + * platform_profile.h - platform profile sysfs interface No file name. > + * > + * See Documentation/ABI/testing/sysfs-platform_profile for more information. > + */ ... > +/* > + * If more options are added please update profile_str > + * array in platform-profile.c > + */ Kernel doc? > +enum profile_option { > + profile_low, > + profile_cool, > + profile_quiet, > + profile_balance, > + profile_perform, > + profile_unknown /* Must always be last */ Comment is semi-useless. Comma at the end (or its absence) is usually enough to give a clue, but okay, comment makes this explicit. ... > +struct platform_profile { > + unsigned int choices; /* bitmap of available choices */ > + int cur_profile; /* Current active profile */ Kernel doc? > + int (*profile_get)(void); > + int (*profile_set)(int profile); > +}; ... > +extern int platform_profile_register(struct platform_profile *pprof); > +extern int platform_profile_unregister(void); > +extern int platform_profile_notify(void); extern is not needed.
On Tue, Nov 10, 2020 at 4:32 AM Mark Pearson <markpearson@lenovo.com> wrote: > > This is the initial implementation of the platform-profile feature. > It provides the details discussed and outlined in the > sysfs-platform_profile document. > > Many modern systems have the ability to modify the operating profile to > control aspects like fan speed, temperature and power levels. This > module provides a common sysfs interface that platform modules can register > against to control their individual profile options. > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> > --- > MAINTAINERS | 8 ++ > drivers/acpi/Kconfig | 19 ++++ > drivers/acpi/Makefile | 1 + > drivers/acpi/platform_profile.c | 172 +++++++++++++++++++++++++++++++ > include/linux/platform_profile.h | 36 +++++++ > 5 files changed, 236 insertions(+) > create mode 100644 drivers/acpi/platform_profile.c > create mode 100644 include/linux/platform_profile.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9a54806ebf02..e731ac1c4447 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -436,6 +436,14 @@ S: Orphan > F: drivers/platform/x86/wmi.c > F: include/uapi/linux/wmi.h > > +ACPI PLATFORM PROFILE DRIVER > +M: Mark Pearson <markpearons@lenovo.com> > +L: linux-acpi@vger.kernel.org > +S: Supported > +W: https://01.org/linux-acpi > +B: https://bugzilla.kernel.org > +F: drivers/acpi/platform_profile.c IMO it is premature to add a MAINTAINERS entry for this until it turns out to be really necessary.
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
Hi Andy, On 10/11/2020 05:26, Andy Shevchenko wrote: > On Tue, Nov 10, 2020 at 5:35 AM Mark Pearson <markpearson@lenovo.com> wrote: >> >> This is the initial implementation of the platform-profile feature. >> It provides the details discussed and outlined in the >> sysfs-platform_profile document. >> >> Many modern systems have the ability to modify the operating profile to >> control aspects like fan speed, temperature and power levels. This >> module provides a common sysfs interface that platform modules can register >> against to control their individual profile options. > > ... > >> +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. > >> + >> + > > None of the blank lines is enough. But can you consider to find > perhaps better place (I imply some logical group of options in the > file). Will do. > > ... > >> obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o >> obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o >> obj-$(CONFIG_ACPI_PPTT) += pptt.o >> +obj-$(CONFIG_ACPI_PLATFORM_PROFILE) += platform_profile.o > > ...yes, and this becomes consistent with the above. > > ... > >> +/* >> + * platform_profile.c - Platform profile sysfs interface >> + */ > > One line. PLease, don't put the file name into the file. If we want to > rename it, it will give additional churn and as shown in practice > people often forget this change to follow. Ack. > > ... > >> +#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> > > Perhaps sorted? > Why do you need a specific acpi_bus.h? I thought acpi.h includes it already, no? Sure - I hadn't realised sorted was a requirement :) I'll check on acpi_bus.h - I think I initially added acpi_bus.h thinking it was all I needed and then added acpi.h later. I'll clean up if I can. > > ... > >> +struct platform_profile *cur_profile; > > Better naming since it's a global variable. > Is it supposed to be exported to modules? It's internal only, but agreed on improving the naming. (Barabas pointed out that it should be a static too) > > ... > >> +DEFINE_MUTEX(profile_lock); > > No static? Yes, it should be. > > ... > >> +/* Ensure the first char of each profile is unique */ >> +static char *profile_str[] = { > > static const char * const profile_names[] > > Also naming (perhaps like I proposed above?). Ack. > >> + "Low-power", >> + "Cool", >> + "Quiet", >> + "Balance", >> + "Performance", > >> + "Unknown" > > Leave the comma here. Ack. > >> +}; > > ... > >> + int i; >> + int ret, count = 0; > > count AFAICS should be size_t (or ssize_t). > Can you make them in reversed xmas tree order? Sure - will fix. > > ... > >> + return snprintf(buf, PAGE_SIZE, "%s", profile_str[profile]); > > Nowadays we have sysfs_emit(), use it. OK - that was a new one to me. Will update. > > ... > >> + /* Scan for a matching profile */ >> + for (profile = profile_low; profile < profile_unknown; profile++) { >> + if (toupper(buf[0]) == profile_str[profile][0]) >> + break; >> + } > > match_string() / sysfs_match_string() ? Yes, Barnabas picked up on this too. I'll correct. > > ... > >> +static struct attribute *platform_profile_attributes[] = { >> + &dev_attr_platform_profile_choices.attr, >> + &dev_attr_platform_profile.attr, > >> + NULL, > > Drop comma in terminator line. Ack. > >> +}; > > ... > >> +module_init(platform_profile_init); >> +module_exit(platform_profile_exit); > > Attach them to respective functions. OK. > > ... > >> +/* >> + * platform_profile.h - platform profile sysfs interface > > No file name. Ack. > >> + * >> + * See Documentation/ABI/testing/sysfs-platform_profile for more information. >> + */ > > ... > >> +/* >> + * If more options are added please update profile_str >> + * array in platform-profile.c >> + */ > > Kernel doc? OK. Just in case - I assume this should go int the sysfs-platform-profile doc (patch 1 of the series). Let me know if that's not the expectation. > >> +enum profile_option { >> + profile_low, >> + profile_cool, >> + profile_quiet, >> + profile_balance, >> + profile_perform, > >> + profile_unknown /* Must always be last */ > > Comment is semi-useless. Comma at the end (or its absence) is usually > enough to give a clue, but okay, comment makes this explicit. I prefer explicit but let me know if you feel strongly about this one. > > ... > >> +struct platform_profile { >> + unsigned int choices; /* bitmap of available choices */ >> + int cur_profile; /* Current active profile */ > > Kernel doc? Sure > >> + int (*profile_get)(void); >> + int (*profile_set)(int profile); >> +}; > > ... > >> +extern int platform_profile_register(struct platform_profile *pprof); >> +extern int platform_profile_unregister(void); >> +extern int platform_profile_notify(void); > > extern is not needed. > I will remove. Thanks for the detailed review - very much appreciated. Mark
Hi Rafael On 10/11/2020 07:53, Rafael J. Wysocki wrote: > On Tue, Nov 10, 2020 at 4:32 AM Mark Pearson <markpearson@lenovo.com> wrote: >> >> This is the initial implementation of the platform-profile feature. >> It provides the details discussed and outlined in the >> sysfs-platform_profile document. >> >> Many modern systems have the ability to modify the operating profile to >> control aspects like fan speed, temperature and power levels. This >> module provides a common sysfs interface that platform modules can register >> against to control their individual profile options. >> >> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >> --- >> MAINTAINERS | 8 ++ >> drivers/acpi/Kconfig | 19 ++++ >> drivers/acpi/Makefile | 1 + >> drivers/acpi/platform_profile.c | 172 +++++++++++++++++++++++++++++++ >> include/linux/platform_profile.h | 36 +++++++ >> 5 files changed, 236 insertions(+) >> create mode 100644 drivers/acpi/platform_profile.c >> create mode 100644 include/linux/platform_profile.h >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 9a54806ebf02..e731ac1c4447 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -436,6 +436,14 @@ S: Orphan >> F: drivers/platform/x86/wmi.c >> F: include/uapi/linux/wmi.h >> >> +ACPI PLATFORM PROFILE DRIVER >> +M: Mark Pearson <markpearons@lenovo.com> >> +L: linux-acpi@vger.kernel.org >> +S: Supported >> +W: https://01.org/linux-acpi >> +B: https://bugzilla.kernel.org >> +F: drivers/acpi/platform_profile.c > > IMO it is premature to add a MAINTAINERS entry for this until it turns > out to be really necessary. > Not a problem - I'll remove this. I wasn't particularly sure about the etiquette here and I ran checkpatch and it suggested it. I'm obviously happy to help with fixes etc. My plan is not to dump and run :) Mark
Hi > [...] > >> +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. I don't advocate for its removal, just that it be documented if it may be returned to userspace. > > > > > >> +}; > [...] > >> +#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? I don't feel strongly about it, just that right now, for all practical purposes `profile_unknown` feels like a first-class profile option in the current form of the patch, and it didn't seem right that it can just change. I'd do something like ``` enum performance_profile_option { performance_profile_unknown, performance_profile_low, ... performance_profile_max, /* must be last */ }; ``` But I don't have a strong preference for either one of them. Maybe someone could chime in and tell us which one is more prevalent/preferred. And as a side note, I think you could put something like `static_assert(ARRAY_SIZE(profile_str) == performance_profile_max);` in the code somewhere to make sure there are as many strings in the array as profile options. You might actually do the following as well: ``` static const char *profile_str[] = { [performance_profile_unknown] = "unknown", [performance_profile_low] = "low", ... }; ``` I realize I might be a bit too paranoid here. :-) But if you do these three things, or something similar, then the chances of the enum and the array being out of sync (by accident) will be very slim. > [...] Regards, Barnabás Pőcze
Hi, On 11/10/20 3:32 PM, Mark Pearson wrote: > 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: <snip> >>> + "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. AFAIK with the currently agreed upon version of the API we do not need this, please remove it. We can always add it later if necessary, but for now I would like to avoid drivers being tempted to use this. Regards, Hans
diff --git a/MAINTAINERS b/MAINTAINERS index 9a54806ebf02..e731ac1c4447 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -436,6 +436,14 @@ S: Orphan F: drivers/platform/x86/wmi.c F: include/uapi/linux/wmi.h +ACPI PLATFORM PROFILE DRIVER +M: Mark Pearson <markpearons@lenovo.com> +L: linux-acpi@vger.kernel.org +S: Supported +W: https://01.org/linux-acpi +B: https://bugzilla.kernel.org +F: drivers/acpi/platform_profile.c + AD1889 ALSA SOUND DRIVER L: linux-parisc@vger.kernel.org S: Maintained diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 7540a5179a47..b10a8e0863cf 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -601,3 +601,22 @@ config X86_PM_TIMER You should nearly always say Y here because many modern systems require this timer. + +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. + + diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 9a957544e357..82dbdc0300ed 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -93,6 +93,7 @@ obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o obj-$(CONFIG_ACPI_PPTT) += pptt.o +obj-$(CONFIG_ACPI_PLATFORM_PROFILE) += platform_profile.o # processor has its own "processor." module_param namespace processor-y := processor_driver.o 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> + +struct platform_profile *cur_profile; +DEFINE_MUTEX(profile_lock); + +/* Ensure the first char of each profile is unique */ +static char *profile_str[] = { + "Low-power", + "Cool", + "Quiet", + "Balance", + "Performance", + "Unknown" +}; + +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"); + } + + for (i = profile_low; i < profile_unknown; i++) { + if (cur_profile->choices & (1 << i)) { + ret = snprintf(buf+count, PAGE_SIZE, "%s ", profile_str[i]); + if (ret < 0) + break; + count += ret; + } + } + mutex_unlock(&profile_lock); + 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(); + mutex_unlock(&profile_lock); + + return snprintf(buf, PAGE_SIZE, "%s", profile_str[profile]); +} + +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); + + 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, +}; + +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; + return 0; +} + +static void platform_profile_exit(void) +{ + 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 */ +}; + +struct platform_profile { + unsigned int choices; /* bitmap of available choices */ + int cur_profile; /* Current active profile */ + int (*profile_get)(void); + int (*profile_set)(int profile); +}; + +extern int platform_profile_register(struct platform_profile *pprof); +extern int platform_profile_unregister(void); +extern int platform_profile_notify(void); + +#endif /*_PLATFORM_PROFILE_H_*/
This is the initial implementation of the platform-profile feature. It provides the details discussed and outlined in the sysfs-platform_profile document. Many modern systems have the ability to modify the operating profile to control aspects like fan speed, temperature and power levels. This module provides a common sysfs interface that platform modules can register against to control their individual profile options. Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- MAINTAINERS | 8 ++ drivers/acpi/Kconfig | 19 ++++ drivers/acpi/Makefile | 1 + drivers/acpi/platform_profile.c | 172 +++++++++++++++++++++++++++++++ include/linux/platform_profile.h | 36 +++++++ 5 files changed, 236 insertions(+) create mode 100644 drivers/acpi/platform_profile.c create mode 100644 include/linux/platform_profile.h