| Message ID | 20201126165143.32776-2-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v4,1/3] Documentation: Add documentation for new platform_profile sysfs attribute | expand |
Hi 2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta: > [...] +static const char * const profile_names[] = { + [platform_profile_low] = "low-power", + [platform_profile_cool] = "cool", + [platform_profile_quiet] = "quiet", + [platform_profile_balance] = "balance", Documentation says "balanced". + [platform_profile_perform] = "performance", +}; > [...] > +static ssize_t platform_profile_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + enum platform_profile_option profile = platform_profile_balance; > + int err; > + > + err = mutex_lock_interruptible(&profile_lock); > + if (err) > + return err; > + > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -ENODEV; > + } > + > + 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; > + In `platform_profile_store()`, you do ``` err = cur_profile->profile_set(i); if (err) return err; ``` but here you do `if (err < 0)`, why? > + /* Check that profile is valid index */ > + if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names))) > + return sysfs_emit(buf, "\n"); > + I'd write `if (WARN_ON(profile < 0 ....))` since that is serious error in my opinion which should be logged. I am also not sure if > + return sysfs_emit(buf, "%s\n", profile_names[profile]); > +} > [...] > +int platform_profile_unregister(void) > +{ > + int err; > + > + err = mutex_lock_interruptible(&profile_lock); > + if (err) > + return err; > + I know it was me who said to prefer `mutex_lock_interruptible()`, but in this particular instance I believe `mutex_lock()` would be preferable to avoid the case where the module unloading is interrupted, and thus the profile handler is not unregistered properly. This could be handled in each module that uses this interface, however, I believe it'd be better to handle it here. > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -ENODEV; > + } > + > + sysfs_remove_group(acpi_kobj, &platform_profile_group); > + cur_profile = NULL; > + mutex_unlock(&profile_lock); > + return 0; > +} > [...] Regards, Barnabás Pőcze
Hi, On 11/26/20 5:51 PM, Mark Pearson 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> > --- > Changes in v2: > Address (hopefully) all recommendations from review including: > - reorder includes list alphabetically > - make globals statics and use const as required > - change profile name scanning to use full string > - clean up profile name lists to remove unwanted additions > - use sysfs_emit and sysfs_emit_at appropriately (much nicer!) > - improve error handling. Return errors to user in all cases and use > better error codes where appropriate (ENOOPSUPP) > - clean up sysfs output for better readability > - formatting fixes where needed > - improve structure and enum names to be clearer > - remove cur_profile field from structure. It is now local to the > actual platform driver file (patch 3 in series) > - improve checking so if future profile options are added profile_names > will be updated as well. > - move CONFIG option next to ACPI_THERMAL as it seemed slightly related > - removed MAINTAINERS update as not appropriate (note warning message > is seen when running checkpatch) > > Changes in v3: > - Add missed platform_profile.h file > > Changes in v4: > - Clean up duplicate entry in Kconfig file > - Add linux/bits.h to include list > - Remove unnecessary items from include list > - Make cur_profile const > - Clean up comments > - formatting clean-ups > - add checking of profile return value to show function > - add checking to store to see if it's a supported profile > - revert ENOTSUPP change in store function > - improved error checking in profile registration > - improved profile naming (now platform_profile_*) > > drivers/acpi/Kconfig | 14 ++ > drivers/acpi/Makefile | 1 + > drivers/acpi/platform_profile.c | 215 +++++++++++++++++++++++++++++++ > include/linux/platform_profile.h | 36 ++++++ > 4 files changed, 266 insertions(+) > create mode 100644 drivers/acpi/platform_profile.c > create mode 100644 include/linux/platform_profile.h > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index edf1558c1105..c1ca6255ff85 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_CUSTOM_DSDT_FILE > string "Custom DSDT Table file to include" > default "" > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index 44e412506317..c64a8af106c0 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -78,6 +78,7 @@ obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o > obj-$(CONFIG_ACPI_PROCESSOR) += processor.o > obj-$(CONFIG_ACPI) += container.o > obj-$(CONFIG_ACPI_THERMAL) += thermal.o > +obj-$(CONFIG_ACPI_PLATFORM_PROFILE) += platform_profile.o > obj-$(CONFIG_ACPI_NFIT) += nfit/ > obj-$(CONFIG_ACPI_NUMA) += numa/ > obj-$(CONFIG_ACPI) += acpi_memhotplug.o > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c > new file mode 100644 > index 000000000000..678cb4596ada > --- /dev/null > +++ b/drivers/acpi/platform_profile.c > @@ -0,0 +1,215 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +/* Platform profile sysfs interface */ > + > +#include <linux/acpi.h> > +#include <linux/bits.h> > +#include <linux/init.h> > +#include <linux/mutex.h> > +#include <linux/platform_profile.h> > +#include <linux/sysfs.h> > + > +static const struct platform_profile_handler *cur_profile; > +static DEFINE_MUTEX(profile_lock); > + > +static const char * const profile_names[] = { > + [platform_profile_low] = "low-power", > + [platform_profile_cool] = "cool", > + [platform_profile_quiet] = "quiet", > + [platform_profile_balance] = "balance", As Barnabás already pointed out the docs added in patch 1 say "balanced" and here you use "balance" both in the enum value/label as we as in the actual string. I have a slight preference for balanced, but the most important thing here is being consistent everywhere. > + [platform_profile_perform] = "performance", > +}; > +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1); It would be better to add an extra member/entry at the end of the enum named platform_profile_no_profiles; and then use that instead of platform_profile_perform+1. Also see below where I use this too. > + > +static ssize_t platform_profile_choices_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int len = 0; > + int err, i; > + > + err = mutex_lock_interruptible(&profile_lock); > + if (err) > + return err; > + > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -ENODEV; > + } > + > + if (!cur_profile->choices) { > + mutex_unlock(&profile_lock); > + return sysfs_emit(buf, "\n"); > + } If choices is empty, the for below will never print anything and the end result is still just emitting "\n", so this whole if block is unnecessary and can be removed. > + > + for (i = 0; i < ARRAY_SIZE(profile_names); i++) { > + if (cur_profile->choices & BIT(i)) { Please change the type of choices to an unsigned long array, like this: unsigned long choices[BITS_TO_LONGS(platform_profile_no_profiles)]; And then replace the for + if with: for_each_set_bit(i, cur_profile->choices, platform_profile_no_profiles) { This has 2 advantages: 1. Using for_each_set_bit is nicer then open coding it 2. If we ever go over 32 profile choices this will keep working automatically without needing any adjustment to the code. Note this requires include/linux/bitops.h (in include/linux/platform_profile.h, since you will be using BITS_TO_LONGS there). > + if (len == 0) > + len += sysfs_emit_at(buf, len, "%s", profile_names[i]); > + else > + len += sysfs_emit_at(buf, len, " %s", profile_names[i]); > + } > + } > + 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 = platform_profile_balance; > + int err; > + > + err = mutex_lock_interruptible(&profile_lock); > + if (err) > + return err; > + > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -ENODEV; > + } > + > + 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; > + > + /* Check that profile is valid index */ > + if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names))) > + return sysfs_emit(buf, "\n"); IMHO it would be better to do "return -EIO" here, since there clearly is something wrong in this case. > + > + return sysfs_emit(buf, "%s\n", profile_names[profile]); > +} > + > +static ssize_t platform_profile_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int err, i; > + > + err = mutex_lock_interruptible(&profile_lock); > + if (err) > + return err; > + > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -ENODEV; > + } > + > + if (!cur_profile->profile_set) { > + mutex_unlock(&profile_lock); > + return -EOPNOTSUPP; > + } > + > + /* Scan for a matching profile */ > + i = sysfs_match_string(profile_names, buf); > + if (i < 0) { > + mutex_unlock(&profile_lock); > + return -EINVAL; > + } > + > + /* Check that platform supports this profile choice */ > + if (!(cur_profile->choices & BIT(i))) { > + mutex_unlock(&profile_lock); > + return -EOPNOTSUPP; > + } > + > + err = cur_profile->profile_set(i); > + mutex_unlock(&profile_lock); > + if (err) > + return err; > + return count; > +} > + > +static DEVICE_ATTR_RO(platform_profile_choices); > +static DEVICE_ATTR_RW(platform_profile); > + > +static struct attribute *platform_profile_attrs[] = { > + &dev_attr_platform_profile_choices.attr, > + &dev_attr_platform_profile.attr, > + NULL > +}; > + > +static const struct attribute_group platform_profile_group = { > + .attrs = platform_profile_attrs > +}; > + > +void platform_profile_notify(void) > +{ > + if (!cur_profile) > + return; > + sysfs_notify(acpi_kobj, NULL, "platform_profile"); > +} > +EXPORT_SYMBOL_GPL(platform_profile_notify); > + > +int platform_profile_register(const struct platform_profile_handler *pprof) > +{ > + int err; Maybe sanity check the platform_profile_handler a bit here, I think it would be ok to check that choices, profile_set and profile_get are all not 0 / NULL here and otherwise just return -EINVAL; Doing so allows making the code above a bit simpler, also removing some exit paths which require an unlock before exiting. > + > + err = mutex_lock_interruptible(&profile_lock); > + if (err) > + return err; Please use a regular mutex_lock here, this is called during driver probing, so no need to handle Ctrl+C and other signals. > + > + /* We can only have one active profile */ > + if (cur_profile) { > + mutex_unlock(&profile_lock); > + return -EEXIST; > + } > + > + err = sysfs_create_group(acpi_kobj, &platform_profile_group); > + if (err) { > + mutex_unlock(&profile_lock); > + return err; > + } > + > + cur_profile = pprof; > + mutex_unlock(&profile_lock); > + return 0; > +} > +EXPORT_SYMBOL_GPL(platform_profile_register); > + > +int platform_profile_unregister(void) > +{ > + int err; > + > + err = mutex_lock_interruptible(&profile_lock); > + if (err) > + return err; Also please use a regular mutex_lock here. > + > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -ENODEV; > + } > + > + sysfs_remove_group(acpi_kobj, &platform_profile_group); > + cur_profile = NULL; > + mutex_unlock(&profile_lock); > + return 0; > +} > +EXPORT_SYMBOL_GPL(platform_profile_unregister); > + > +static int __init platform_profile_init(void) > +{ > + return 0; > +} > +module_init(platform_profile_init); AFAIK you do not need to provide a module_init function, since this is a no-op you can just leave it out. > +static void __exit platform_profile_exit(void) > +{ > + /* Check if we have a registered profile, and clean up */ > + if (cur_profile) { > + sysfs_remove_group(acpi_kobj, &platform_profile_group); > + cur_profile = NULL; > + } > +} > +module_exit(platform_profile_exit); Any driver/module registering a platform_profile will depend on symbols from this module, so this module can only be unloaded (aka exit) if that other driver/module has already been unloaded. Unloading that other module should have already unregistered the cur_profile provider, otherwise things like cur_profile->profile_set will now point to no longer loaded code which would be really really bad. So you can drop this exit function entirely. Sorry for not spotting some of these sooner. Still nothing really big, so hopefully you will be able to post a v5 addressing these soonish and then with some luck we can at least get patches 1 and 2 merged by Rafael in time for 5.11 (assuming Rafael is happy with v5). And then I can merge patch 3 once 5.11-rc1 is out. Regards, Hans > + > +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h > new file mode 100644 > index 000000000000..33ccd40bb9cf > --- /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 { > + platform_profile_low, > + platform_profile_cool, > + platform_profile_quiet, > + platform_profile_balance, > + platform_profile_perform, > +}; > + > +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(const struct platform_profile_handler *pprof); > +int platform_profile_unregister(void); > +void platform_profile_notify(void); > + > +#endif /*_PLATFORM_PROFILE_H_*/ >
Hi 2020. november 28., szombat 15:08 keltezéssel, Hans de Goede írta: > [...] > > +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1); > > It would be better to add an extra member/entry at the end of the enum > named platform_profile_no_profiles; and then use that instead of > platform_profile_perform+1. Also see below where I use this too. > I'm not sure if it's just me, but when I read "no_profiles", then "number of probiles" is not the first thing that comes to mind, maybe _end, _last, _max, etc. would be harder to mistake for something else? What do you think? > > + > > +static ssize_t platform_profile_choices_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + int len = 0; > > + int err, i; > > + > > + err = mutex_lock_interruptible(&profile_lock); > > + if (err) > > + return err; > > + > > + if (!cur_profile) { > > + mutex_unlock(&profile_lock); > > + return -ENODEV; > > + } > > + > > + if (!cur_profile->choices) { > > + mutex_unlock(&profile_lock); > > + return sysfs_emit(buf, "\n"); > > + } > > If choices is empty, the for below will never print anything and > the end result is still just emitting "\n", so this whole if > block is unnecessary and can be removed. > > > + > > + for (i = 0; i < ARRAY_SIZE(profile_names); i++) { > > + if (cur_profile->choices & BIT(i)) { > > Please change the type of choices to an unsigned long array, like this: > > unsigned long choices[BITS_TO_LONGS(platform_profile_no_profiles)]; > Sorry for the ignorant question, but does this play well with initialization? Can something like this be done easily? ``` struct platform_profile_handler pph = { .choices = BIT(...) | BIT(...) | etc. }; ``` Or do you need to use something like `__set_bit()` in a function? > [...] > > +int platform_profile_register(const struct platform_profile_handler *pprof) > > +{ > > + int err; > > Maybe sanity check the platform_profile_handler a bit here, > I think it would be ok to check that choices, profile_set and profile_get > are all not 0 / NULL here and otherwise just return -EINVAL; Doing so > allows making the code above a bit simpler, also removing some exit > paths which require an unlock before exiting. > > > + > > + err = mutex_lock_interruptible(&profile_lock); > > + if (err) > > + return err; > > Please use a regular mutex_lock here, this is called during > driver probing, so no need to handle Ctrl+C and other signals. > [...] I believe insmod/modprobe can be interrupted, but I agree that's not really of any concern here. Regards, Barnabás Pőcze
Thanks Barnabas, On 2020-11-27 2:14 p.m., Barnabás Pőcze wrote: > Hi > > > 2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta: > >> [...] > +static const char * const profile_names[] = { > + [platform_profile_low] = "low-power", > + [platform_profile_cool] = "cool", > + [platform_profile_quiet] = "quiet", > + [platform_profile_balance] = "balance", > > Documentation says "balanced". Ah, my bad. I will correct. > > > + [platform_profile_perform] = "performance", > +}; >> [...] >> +static ssize_t platform_profile_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + enum platform_profile_option profile = platform_profile_balance; >> + int err; >> + >> + err = mutex_lock_interruptible(&profile_lock); >> + if (err) >> + return err; >> + >> + if (!cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -ENODEV; >> + } >> + >> + 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; >> + > > In `platform_profile_store()`, you do > ``` > err = cur_profile->profile_set(i); > if (err) > return err; > ``` > but here you do `if (err < 0)`, why? At one point I had this function returning the profile, but then it ended up getting messy for handling errors. I changed it but didn't change the error handling - I'll tidy this up. Thanks! > > >> + /* Check that profile is valid index */ >> + if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names))) >> + return sysfs_emit(buf, "\n"); >> + > > I'd write `if (WARN_ON(profile < 0 ....))` since that is serious error in my > opinion which should be logged. I am also not sure if Will do. I've not had any experience with the WARN_ON macro before - thanks for the suggestion. > > >> + return sysfs_emit(buf, "%s\n", profile_names[profile]); >> +} >> [...] >> +int platform_profile_unregister(void) >> +{ >> + int err; >> + >> + err = mutex_lock_interruptible(&profile_lock); >> + if (err) >> + return err; >> + > > I know it was me who said to prefer `mutex_lock_interruptible()`, but in this > particular instance I believe `mutex_lock()` would be preferable to avoid the case > where the module unloading is interrupted, and thus the profile handler is not > unregistered properly. This could be handled in each module that uses this > interface, however, I believe it'd be better to handle it here. Agreed (and I know Hans makes the same point in his email). Your suggestion makes sense, I'll switch back to mutex_lock for the register and unregister functions. > > >> + if (!cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -ENODEV; >> + } >> + >> + sysfs_remove_group(acpi_kobj, &platform_profile_group); >> + cur_profile = NULL; >> + mutex_unlock(&profile_lock); >> + return 0; >> +} >> [...] > > > Regards, > Barnabás Pőcze > Thank you for the review! Mark
Thanks Hans On 2020-11-28 9:08 a.m., Hans de Goede wrote: > Hi, > > On 11/26/20 5:51 PM, Mark Pearson wrote: <snip> >> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c >> new file mode 100644 >> index 000000000000..678cb4596ada >> --- /dev/null >> +++ b/drivers/acpi/platform_profile.c >> @@ -0,0 +1,215 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> + >> +/* Platform profile sysfs interface */ >> + >> +#include <linux/acpi.h> >> +#include <linux/bits.h> >> +#include <linux/init.h> >> +#include <linux/mutex.h> >> +#include <linux/platform_profile.h> >> +#include <linux/sysfs.h> >> + >> +static const struct platform_profile_handler *cur_profile; >> +static DEFINE_MUTEX(profile_lock); >> + >> +static const char * const profile_names[] = { >> + [platform_profile_low] = "low-power", >> + [platform_profile_cool] = "cool", >> + [platform_profile_quiet] = "quiet", >> + [platform_profile_balance] = "balance", > > As Barnabás already pointed out the docs added in patch 1 say "balanced" > and here you use "balance" both in the enum value/label as we as in > the actual string. I have a slight preference for balanced, but the > most important thing here is being consistent everywhere. Agreed - I'll fix, my mistake. > >> + [platform_profile_perform] = "performance", >> +}; >> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1); > > It would be better to add an extra member/entry at the end of the enum > named platform_profile_no_profiles; and then use that instead of > platform_profile_perform+1. Also see below where I use this too. Makes sense. Will do (and update the documentation too) > >> + >> +static ssize_t platform_profile_choices_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + int len = 0; >> + int err, i; >> + >> + err = mutex_lock_interruptible(&profile_lock); >> + if (err) >> + return err; >> + >> + if (!cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -ENODEV; >> + } >> + >> + if (!cur_profile->choices) { >> + mutex_unlock(&profile_lock); >> + return sysfs_emit(buf, "\n"); >> + } > > If choices is empty, the for below will never print anything and > the end result is still just emitting "\n", so this whole if > block is unnecessary and can be removed. Agreed - I'll remove > >> + >> + for (i = 0; i < ARRAY_SIZE(profile_names); i++) { >> + if (cur_profile->choices & BIT(i)) { > > Please change the type of choices to an unsigned long array, like this: > > unsigned long choices[BITS_TO_LONGS(platform_profile_no_profiles)]; > > And then replace the for + if with: > > for_each_set_bit(i, cur_profile->choices, platform_profile_no_profiles) { > > This has 2 advantages: > 1. Using for_each_set_bit is nicer then open coding it > 2. If we ever go over 32 profile choices this will keep working automatically > without needing any adjustment to the code. > > Note this requires include/linux/bitops.h (in include/linux/platform_profile.h, > since you will be using BITS_TO_LONGS there). Neat - I hadn't come across this before. I'll give it a go. Thank you. > >> + if (len == 0) >> + len += sysfs_emit_at(buf, len, "%s", profile_names[i]); >> + else >> + len += sysfs_emit_at(buf, len, " %s", profile_names[i]); >> + } >> + } >> + 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 = platform_profile_balance; >> + int err; >> + >> + err = mutex_lock_interruptible(&profile_lock); >> + if (err) >> + return err; >> + >> + if (!cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -ENODEV; >> + } >> + >> + 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; >> + >> + /* Check that profile is valid index */ >> + if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names))) >> + return sysfs_emit(buf, "\n"); > > IMHO it would be better to do "return -EIO" here, since there > clearly is something wrong in this case. Agreed. I'll correct this. > <snip> >> + >> +int platform_profile_register(const struct platform_profile_handler *pprof) >> +{ >> + int err; > > Maybe sanity check the platform_profile_handler a bit here, > I think it would be ok to check that choices, profile_set and profile_get > are all not 0 / NULL here and otherwise just return -EINVAL; Doing so > allows making the code above a bit simpler, also removing some exit > paths which require an unlock before exiting. Makes sense - will do. > >> + >> + err = mutex_lock_interruptible(&profile_lock); >> + if (err) >> + return err; > > Please use a regular mutex_lock here, this is called during > driver probing, so no need to handle Ctrl+C and other signals. Will do > >> + >> + /* We can only have one active profile */ >> + if (cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -EEXIST; >> + } >> + >> + err = sysfs_create_group(acpi_kobj, &platform_profile_group); >> + if (err) { >> + mutex_unlock(&profile_lock); >> + return err; >> + } >> + >> + cur_profile = pprof; >> + mutex_unlock(&profile_lock); >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(platform_profile_register); >> + >> +int platform_profile_unregister(void) >> +{ >> + int err; >> + >> + err = mutex_lock_interruptible(&profile_lock); >> + if (err) >> + return err; > > Also please use a regular mutex_lock here. Ack > >> + >> + if (!cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -ENODEV; >> + } >> + >> + sysfs_remove_group(acpi_kobj, &platform_profile_group); >> + cur_profile = NULL; >> + mutex_unlock(&profile_lock); >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(platform_profile_unregister); >> + >> +static int __init platform_profile_init(void) >> +{ >> + return 0; >> +} >> +module_init(platform_profile_init); > > AFAIK you do not need to provide a module_init function, since this > is a no-op you can just leave it out. Ah - I never realised you could just not have init and exit functions. I'll remove. > >> +static void __exit platform_profile_exit(void) >> +{ >> + /* Check if we have a registered profile, and clean up */ >> + if (cur_profile) { >> + sysfs_remove_group(acpi_kobj, &platform_profile_group); >> + cur_profile = NULL; >> + } >> +} >> +module_exit(platform_profile_exit); > > Any driver/module registering a platform_profile will depend on > symbols from this module, so this module can only be unloaded (aka exit) > if that other driver/module has already been unloaded. Unloading that > other module should have already unregistered the cur_profile provider, > otherwise things like cur_profile->profile_set will now point to no > longer loaded code which would be really really bad. So you can drop > this exit function entirely. Will do > > Sorry for not spotting some of these sooner. Still nothing really big, > so hopefully you will be able to post a v5 addressing these soonish and > then with some luck we can at least get patches 1 and 2 merged by Rafael > in time for 5.11 (assuming Rafael is happy with v5). And then I can merge > patch 3 once 5.11-rc1 is out. > Sounds good - I'll get working on those ASAP (and as an advance note - I've read ahead to your other email and I'm on board with that plan) Thanks for the review - really appreciated. Mark
On 2020-11-28 10:37 a.m., Barnabás Pőcze wrote: > Hi > > > 2020. november 28., szombat 15:08 keltezéssel, Hans de Goede írta: > >> [...] >>> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1); >> >> It would be better to add an extra member/entry at the end of the enum >> named platform_profile_no_profiles; and then use that instead of >> platform_profile_perform+1. Also see below where I use this too. >> > > I'm not sure if it's just me, but when I read "no_profiles", then "number of probiles" > is not the first thing that comes to mind, maybe _end, _last, _max, etc. > would be harder to mistake for something else? What do you think? > FWIW - my vote would be platform_profile_last, it just seems to fit well when reading the code. Mark
Hi, On 11/29/20 2:19 AM, Mark Pearson wrote: > > > On 2020-11-28 10:37 a.m., Barnabás Pőcze wrote: >> Hi >> >> >> 2020. november 28., szombat 15:08 keltezéssel, Hans de Goede írta: >> >>> [...] >>>> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1); >>> >>> It would be better to add an extra member/entry at the end of the enum >>> named platform_profile_no_profiles; and then use that instead of >>> platform_profile_perform+1. Also see below where I use this too. >>> >> >> I'm not sure if it's just me, but when I read "no_profiles", then "number of probiles" >> is not the first thing that comes to mind, maybe _end, _last, _max, etc. >> would be harder to mistake for something else? What do you think? >> > FWIW - my vote would be platform_profile_last, it just seems to fit well when reading the code. I'm fine with platform_profile_last, or iow please feel free to paint this bikeshed in any color you like :) Regards, Hans
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index edf1558c1105..c1ca6255ff85 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_CUSTOM_DSDT_FILE string "Custom DSDT Table file to include" default "" diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 44e412506317..c64a8af106c0 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -78,6 +78,7 @@ obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o obj-$(CONFIG_ACPI_PROCESSOR) += processor.o obj-$(CONFIG_ACPI) += container.o obj-$(CONFIG_ACPI_THERMAL) += thermal.o +obj-$(CONFIG_ACPI_PLATFORM_PROFILE) += platform_profile.o obj-$(CONFIG_ACPI_NFIT) += nfit/ obj-$(CONFIG_ACPI_NUMA) += numa/ obj-$(CONFIG_ACPI) += acpi_memhotplug.o diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c new file mode 100644 index 000000000000..678cb4596ada --- /dev/null +++ b/drivers/acpi/platform_profile.c @@ -0,0 +1,215 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* Platform profile sysfs interface */ + +#include <linux/acpi.h> +#include <linux/bits.h> +#include <linux/init.h> +#include <linux/mutex.h> +#include <linux/platform_profile.h> +#include <linux/sysfs.h> + +static const struct platform_profile_handler *cur_profile; +static DEFINE_MUTEX(profile_lock); + +static const char * const profile_names[] = { + [platform_profile_low] = "low-power", + [platform_profile_cool] = "cool", + [platform_profile_quiet] = "quiet", + [platform_profile_balance] = "balance", + [platform_profile_perform] = "performance", +}; +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1); + +static ssize_t platform_profile_choices_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int len = 0; + int err, i; + + err = mutex_lock_interruptible(&profile_lock); + if (err) + return err; + + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -ENODEV; + } + + if (!cur_profile->choices) { + mutex_unlock(&profile_lock); + return sysfs_emit(buf, "\n"); + } + + for (i = 0; i < ARRAY_SIZE(profile_names); 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]); + } + } + 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 = platform_profile_balance; + int err; + + err = mutex_lock_interruptible(&profile_lock); + if (err) + return err; + + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -ENODEV; + } + + 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; + + /* Check that profile is valid index */ + if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names))) + return sysfs_emit(buf, "\n"); + + return sysfs_emit(buf, "%s\n", profile_names[profile]); +} + +static ssize_t platform_profile_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int err, i; + + err = mutex_lock_interruptible(&profile_lock); + if (err) + return err; + + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -ENODEV; + } + + if (!cur_profile->profile_set) { + mutex_unlock(&profile_lock); + return -EOPNOTSUPP; + } + + /* Scan for a matching profile */ + i = sysfs_match_string(profile_names, buf); + if (i < 0) { + mutex_unlock(&profile_lock); + return -EINVAL; + } + + /* Check that platform supports this profile choice */ + if (!(cur_profile->choices & BIT(i))) { + mutex_unlock(&profile_lock); + return -EOPNOTSUPP; + } + + err = cur_profile->profile_set(i); + mutex_unlock(&profile_lock); + if (err) + return err; + return count; +} + +static DEVICE_ATTR_RO(platform_profile_choices); +static DEVICE_ATTR_RW(platform_profile); + +static struct attribute *platform_profile_attrs[] = { + &dev_attr_platform_profile_choices.attr, + &dev_attr_platform_profile.attr, + NULL +}; + +static const struct attribute_group platform_profile_group = { + .attrs = platform_profile_attrs +}; + +void platform_profile_notify(void) +{ + if (!cur_profile) + return; + sysfs_notify(acpi_kobj, NULL, "platform_profile"); +} +EXPORT_SYMBOL_GPL(platform_profile_notify); + +int platform_profile_register(const struct platform_profile_handler *pprof) +{ + int err; + + err = mutex_lock_interruptible(&profile_lock); + if (err) + return err; + + /* We can only have one active profile */ + if (cur_profile) { + mutex_unlock(&profile_lock); + return -EEXIST; + } + + err = sysfs_create_group(acpi_kobj, &platform_profile_group); + if (err) { + mutex_unlock(&profile_lock); + return err; + } + + cur_profile = pprof; + mutex_unlock(&profile_lock); + return 0; +} +EXPORT_SYMBOL_GPL(platform_profile_register); + +int platform_profile_unregister(void) +{ + int err; + + err = mutex_lock_interruptible(&profile_lock); + if (err) + return err; + + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -ENODEV; + } + + sysfs_remove_group(acpi_kobj, &platform_profile_group); + cur_profile = NULL; + mutex_unlock(&profile_lock); + return 0; +} +EXPORT_SYMBOL_GPL(platform_profile_unregister); + +static int __init platform_profile_init(void) +{ + return 0; +} +module_init(platform_profile_init); + +static void __exit platform_profile_exit(void) +{ + /* Check if we have a registered profile, and clean up */ + if (cur_profile) { + sysfs_remove_group(acpi_kobj, &platform_profile_group); + cur_profile = NULL; + } +} +module_exit(platform_profile_exit); + +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h new file mode 100644 index 000000000000..33ccd40bb9cf --- /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 { + platform_profile_low, + platform_profile_cool, + platform_profile_quiet, + platform_profile_balance, + platform_profile_perform, +}; + +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(const struct platform_profile_handler *pprof); +int platform_profile_unregister(void); +void 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> --- Changes in v2: Address (hopefully) all recommendations from review including: - reorder includes list alphabetically - make globals statics and use const as required - change profile name scanning to use full string - clean up profile name lists to remove unwanted additions - use sysfs_emit and sysfs_emit_at appropriately (much nicer!) - improve error handling. Return errors to user in all cases and use better error codes where appropriate (ENOOPSUPP) - clean up sysfs output for better readability - formatting fixes where needed - improve structure and enum names to be clearer - remove cur_profile field from structure. It is now local to the actual platform driver file (patch 3 in series) - improve checking so if future profile options are added profile_names will be updated as well. - move CONFIG option next to ACPI_THERMAL as it seemed slightly related - removed MAINTAINERS update as not appropriate (note warning message is seen when running checkpatch) Changes in v3: - Add missed platform_profile.h file Changes in v4: - Clean up duplicate entry in Kconfig file - Add linux/bits.h to include list - Remove unnecessary items from include list - Make cur_profile const - Clean up comments - formatting clean-ups - add checking of profile return value to show function - add checking to store to see if it's a supported profile - revert ENOTSUPP change in store function - improved error checking in profile registration - improved profile naming (now platform_profile_*) drivers/acpi/Kconfig | 14 ++ drivers/acpi/Makefile | 1 + drivers/acpi/platform_profile.c | 215 +++++++++++++++++++++++++++++++ include/linux/platform_profile.h | 36 ++++++ 4 files changed, 266 insertions(+) create mode 100644 drivers/acpi/platform_profile.c create mode 100644 include/linux/platform_profile.h