| Message ID | 20201202171120.65269-2-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [v2] platform/x86: thinkpad_acpi: lap or desk mode interface | expand |
On Wed, Dec 2, 2020 at 6:16 PM 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> > --- > 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_*) > > Changes in v5: > - correct 'balance' to 'balanced' to be consistent with documentation > - add WARN_ON when checking profile index in show function > - switch mutex_lock_interruptible back to mutex_lock where appropriate > - add 'platform_profile_last' as final entry in profile entry. Update > implementation to use this appropriately > - Use BITS_TO_LONG and appropriate access functions for choices field > - Correct error handling as recommended > - Sanity check profile fields on registration > - Remove unnecessary init and exit functions > > drivers/acpi/Kconfig | 14 +++ > drivers/acpi/Makefile | 1 + > drivers/acpi/platform_profile.c | 181 +++++++++++++++++++++++++++++++ > include/linux/platform_profile.h | 39 +++++++ > 4 files changed, 235 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 default m > + help > + This driver adds support for platform-profiles on platforms that > + support it. Empty line here, please. > + 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. And here. > + This driver provides the sysfs interface and is used as the registration > + point for platform specific drivers. And here. > + 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..1bc092359e35 > --- /dev/null > +++ b/drivers/acpi/platform_profile.c > @@ -0,0 +1,181 @@ > +// 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_balanced] = "balanced", > + [platform_profile_perform] = "performance", The enum values in upper case, please. > +}; > +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last); > + > +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); Why interruptible? And why is the lock needed in the first place? > + if (err) > + return err; > + > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -ENODEV; > + } > + > + for_each_set_bit(i, cur_profile->choices, platform_profile_last) { > + 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_balanced; > + int err; > + > + err = mutex_lock_interruptible(&profile_lock); > + if (err) > + return err; > + > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -ENODEV; > + } > + > + err = cur_profile->profile_get(&profile); In which cases this can fail? > + mutex_unlock(&profile_lock); > + if (err) > + return err; > + > + /* Check that profile is valid index */ > + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) > + return -EIO; > + > + 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; > + } > + > + /* 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 (!test_bit(i, cur_profile->choices)) { > + mutex_unlock(&profile_lock); > + return -EOPNOTSUPP; > + } > + > + err = cur_profile->profile_set(i); What if this gets a signal in the middle of the ->profile_set() execution? Is this always guaranteed to work? > + 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; > + > + mutex_lock(&profile_lock); > + /* We can only have one active profile */ > + if (cur_profile) { > + mutex_unlock(&profile_lock); > + return -EEXIST; > + } > + > + /* Sanity check the profile handler field are set */ > + if (!pprof || !pprof->choices || !pprof->profile_set || > + !pprof->profile_get) { > + mutex_unlock(&profile_lock); > + return -EINVAL; > + } > + > + 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) "Unregister" functions typically take an argument pointing to the target object, so something like platform_profile_remove() may be a better choice here. > +{ > + mutex_lock(&profile_lock); > + 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); > + > +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..f2e1b1c90482 > --- /dev/null > +++ b/include/linux/platform_profile.h > @@ -0,0 +1,39 @@ > +/* 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_ > + > +#include <linux/bitops.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_balanced, > + platform_profile_perform, > + platform_profile_last, /*must always be last */ > +}; > + > +struct platform_profile_handler { > + unsigned long choices[BITS_TO_LONGS(platform_profile_last)]; > + 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_*/ > --
On Tue, Dec 8, 2020 at 7:26 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@lenovo.com> wrote: [cut] > > + > > +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; > > + } > > + > > + /* 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 (!test_bit(i, cur_profile->choices)) { > > + mutex_unlock(&profile_lock); > > + return -EOPNOTSUPP; > > + } > > + > > + err = cur_profile->profile_set(i); > > What if this gets a signal in the middle of the ->profile_set() > execution? Is this always guaranteed to work? I got this backwards, sorry. The "interruptible" variant is used to allow the waiters to be interrupted, so I guess the concern is that ->profile_set() may get stuck or just take too much time? > > + mutex_unlock(&profile_lock); > > + if (err) > > + return err; > > + return count; > > +} > > +
Hi Rafael, Thanks for the review - a couple of questions (and a bunch of acks) below On 08/12/2020 13:26, Rafael J. Wysocki wrote: > On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@lenovo.com> wrote: >> <snip> >> >> drivers/acpi/Kconfig | 14 +++ >> drivers/acpi/Makefile | 1 + >> drivers/acpi/platform_profile.c | 181 +++++++++++++++++++++++++++++++ >> include/linux/platform_profile.h | 39 +++++++ >> 4 files changed, 235 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 > > default m OK > >> + help >> + This driver adds support for platform-profiles on platforms that >> + support it. > > Empty line here, please. Ack > >> + 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. > > And here. Ack > >> + This driver provides the sysfs interface and is used as the registration >> + point for platform specific drivers. > > And here. Ack > >> + 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..1bc092359e35 >> --- /dev/null >> +++ b/drivers/acpi/platform_profile.c >> @@ -0,0 +1,181 @@ >> +// 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_balanced] = "balanced", >> + [platform_profile_perform] = "performance", > > The enum values in upper case, please. Sorry, I'm a bit confused here - do you mean change to "Low-power" or something else (maybe PLATFORM_PROFILE_LOW?) Just want to make sure I'm getting it correct. If I change the strings it will impact patch1 in the series which is integrated into your bleeding-edge branch. > >> +}; >> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last); >> + >> +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); > > Why interruptible? > > And why is the lock needed in the first place? My thinking was that I don't know what happens when I hand over to thhe platform driver who does the get/set, so having a lock to prevent a get whilst a set is in operation seemed like a good idea. It was interruptible as a suggestion in an earlier reivew as the preferred way of doing these things for functions that could be called by user space. Do you think the lock is a problem? > >> + if (err) >> + return err; >> + >> + if (!cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -ENODEV; >> + } >> + >> + for_each_set_bit(i, cur_profile->choices, platform_profile_last) { >> + 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_balanced; >> + int err; >> + >> + err = mutex_lock_interruptible(&profile_lock); >> + if (err) >> + return err; >> + >> + if (!cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -ENODEV; >> + } >> + >> + err = cur_profile->profile_get(&profile); > > In which cases this can fail? I'm not sure - but as this is supposed to be vendor agnostic I can't foresee what might be wanted or could happen on various hardware. I agree a failure is probably unlikely in the Lenovo case where we're doing an ACPI call, but is there any issue in handling error codes? It doesn't seem to gain much by removing it and may have future impacts. > >> + mutex_unlock(&profile_lock); >> + if (err) >> + return err; >> + >> + /* Check that profile is valid index */ >> + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) >> + return -EIO; >> + >> + 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; >> + } >> + >> + /* 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 (!test_bit(i, cur_profile->choices)) { >> + mutex_unlock(&profile_lock); >> + return -EOPNOTSUPP; >> + } >> + >> + err = cur_profile->profile_set(i); > > What if this gets a signal in the middle of the ->profile_set() > execution? Is this always guaranteed to work? I'm afraid I don't know the answer to this one. What would be the recommendation to cover this event? > >> + 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; >> + >> + mutex_lock(&profile_lock); >> + /* We can only have one active profile */ >> + if (cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -EEXIST; >> + } >> + >> + /* Sanity check the profile handler field are set */ >> + if (!pprof || !pprof->choices || !pprof->profile_set || >> + !pprof->profile_get) { >> + mutex_unlock(&profile_lock); >> + return -EINVAL; >> + } >> + >> + 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) > > "Unregister" functions typically take an argument pointing to the > target object, so something like platform_profile_remove() may be a > better choice here. Sure - happy to change that > >> +{ >> + mutex_lock(&profile_lock); >> + 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); >> + >> +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..f2e1b1c90482 >> --- /dev/null >> +++ b/include/linux/platform_profile.h >> @@ -0,0 +1,39 @@ >> +/* 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_ >> + >> +#include <linux/bitops.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_balanced, >> + platform_profile_perform, >> + platform_profile_last, /*must always be last */ >> +}; >> + >> +struct platform_profile_handler { >> + unsigned long choices[BITS_TO_LONGS(platform_profile_last)]; >> + 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_*/ >> -- Thanks Mark
On Tue, Dec 8, 2020 at 7:54 PM Mark Pearson <markpearson@lenovo.com> wrote: > > Hi Rafael, > > Thanks for the review - a couple of questions (and a bunch of acks) below > > On 08/12/2020 13:26, Rafael J. Wysocki wrote: > > On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@lenovo.com> wrote: [cut] > >> +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_balanced] = "balanced", > >> + [platform_profile_perform] = "performance", > > > > The enum values in upper case, please. > Sorry, I'm a bit confused here - do you mean change to "Low-power" or > something else (maybe PLATFORM_PROFILE_LOW?) platform_profile_low -> PLATFORM_PROFILE_LOW platform_profile_cool -> PLATFORM_PROFILE_COOL etc. > Just want to make sure I'm getting it correct. If I change the strings > it will impact patch1 in the series which is integrated into your > bleeding-edge branch. > > > > >> +}; > >> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last); > >> + > >> +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); > > > > Why interruptible? > > > > And why is the lock needed in the first place? > > My thinking was that I don't know what happens when I hand over to thhe > platform driver who does the get/set, so having a lock to prevent a get > whilst a set is in operation seemed like a good idea. Taking it over get/set probably is (and you need to protect the cur_profile pointer from concurrent updates). And here you need to ensure that the cur_profile object doesn't go away while this is running. So that's why. > It was interruptible as a suggestion in an earlier reivew as the > preferred way of doing these things for functions that could be called > by user space. Well, it is not used consistently this way at least. But OK. > Do you think the lock is a problem? No, it isn't in principle. > > > >> + if (err) > >> + return err; > >> + > >> + if (!cur_profile) { > >> + mutex_unlock(&profile_lock); > >> + return -ENODEV; > >> + } > >> + > >> + for_each_set_bit(i, cur_profile->choices, platform_profile_last) { > >> + 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_balanced; > >> + int err; > >> + > >> + err = mutex_lock_interruptible(&profile_lock); > >> + if (err) > >> + return err; > >> + > >> + if (!cur_profile) { > >> + mutex_unlock(&profile_lock); > >> + return -ENODEV; > >> + } > >> + > >> + err = cur_profile->profile_get(&profile); > > > > In which cases this can fail? > I'm not sure - but as this is supposed to be vendor agnostic I can't > foresee what might be wanted or could happen on various hardware. It returns the index of the current profile AFAICS, so I don't really see a reason for it to fail. Moreover, the index could be maintained by the common code along with the cur_profile pointer, couldn't it? > I agree a failure is probably unlikely in the Lenovo case where we're > doing an ACPI call, but is there any issue in handling error codes? > It doesn't seem to gain much by removing it and may have future impacts. > > > >> + mutex_unlock(&profile_lock); > >> + if (err) > >> + return err; > >> + > >> + /* Check that profile is valid index */ > >> + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) > >> + return -EIO; > >> + > >> + 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; > >> + } > >> + > >> + /* 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 (!test_bit(i, cur_profile->choices)) { > >> + mutex_unlock(&profile_lock); > >> + return -EOPNOTSUPP; > >> + } > >> + > >> + err = cur_profile->profile_set(i); > > > > What if this gets a signal in the middle of the ->profile_set() > > execution? Is this always guaranteed to work? > I'm afraid I don't know the answer to this one. What would be the > recommendation to cover this event? Never mind, this was a mistake of mine. > > > >> + 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; > >> + > >> + mutex_lock(&profile_lock); > >> + /* We can only have one active profile */ > >> + if (cur_profile) { > >> + mutex_unlock(&profile_lock); > >> + return -EEXIST; > >> + } > >> + > >> + /* Sanity check the profile handler field are set */ > >> + if (!pprof || !pprof->choices || !pprof->profile_set || > >> + !pprof->profile_get) { > >> + mutex_unlock(&profile_lock); > >> + return -EINVAL; > >> + } > >> + > >> + 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) > > > > "Unregister" functions typically take an argument pointing to the > > target object, so something like platform_profile_remove() may be a > > better choice here. > Sure - happy to change that > > > > >> +{ > >> + mutex_lock(&profile_lock); > >> + 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); > >> + > >> +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..f2e1b1c90482 > >> --- /dev/null > >> +++ b/include/linux/platform_profile.h > >> @@ -0,0 +1,39 @@ > >> +/* 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_ > >> + > >> +#include <linux/bitops.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_balanced, > >> + platform_profile_perform, > >> + platform_profile_last, /*must always be last */ So please use upper-case names in this list. > >> +}; > >> + > >> +struct platform_profile_handler { > >> + unsigned long choices[BITS_TO_LONGS(platform_profile_last)]; > >> + 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_*/ > >> --
On 08/12/2020 14:18, Rafael J. Wysocki wrote: > On Tue, Dec 8, 2020 at 7:54 PM Mark Pearson <markpearson@lenovo.com> wrote: >> >> Hi Rafael, >> >> Thanks for the review - a couple of questions (and a bunch of acks) below >> >> On 08/12/2020 13:26, Rafael J. Wysocki wrote: >>> On Wed, Dec 2, 2020 at 6:16 PM Mark Pearson <markpearson@lenovo.com> wrote: > > [cut] > >>>> +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_balanced] = "balanced", >>>> + [platform_profile_perform] = "performance", >>> >>> The enum values in upper case, please. >> Sorry, I'm a bit confused here - do you mean change to "Low-power" or >> something else (maybe PLATFORM_PROFILE_LOW?) > > platform_profile_low -> PLATFORM_PROFILE_LOW > platform_profile_cool -> PLATFORM_PROFILE_COOL > > etc. > Got it - I'll update. Thanks for the clarification >> Just want to make sure I'm getting it correct. If I change the strings >> it will impact patch1 in the series which is integrated into your >> bleeding-edge branch. >> >>> >>>> +}; >>>> +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last); >>>> + >>>> +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); >>> >>> Why interruptible? >>> >>> And why is the lock needed in the first place? >> >> My thinking was that I don't know what happens when I hand over to thhe >> platform driver who does the get/set, so having a lock to prevent a get >> whilst a set is in operation seemed like a good idea. > > Taking it over get/set probably is (and you need to protect the > cur_profile pointer from concurrent updates). > > And here you need to ensure that the cur_profile object doesn't go > away while this is running. So that's why. > >> It was interruptible as a suggestion in an earlier reivew as the >> preferred way of doing these things for functions that could be called >> by user space. > > Well, it is not used consistently this way at least. But OK. That was based on review comments - interruptible used where it could be accessed from user space and not where it couldn't. Hans made some good notes about it previously. > >> Do you think the lock is a problem? > > No, it isn't in principle. > >>> >>>> + if (err) >>>> + return err; >>>> + >>>> + if (!cur_profile) { >>>> + mutex_unlock(&profile_lock); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + for_each_set_bit(i, cur_profile->choices, platform_profile_last) { >>>> + 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_balanced; >>>> + int err; >>>> + >>>> + err = mutex_lock_interruptible(&profile_lock); >>>> + if (err) >>>> + return err; >>>> + >>>> + if (!cur_profile) { >>>> + mutex_unlock(&profile_lock); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + err = cur_profile->profile_get(&profile); >>> >>> In which cases this can fail? >> I'm not sure - but as this is supposed to be vendor agnostic I can't >> foresee what might be wanted or could happen on various hardware. > > It returns the index of the current profile AFAICS, so I don't really > see a reason for it to fail. > > Moreover, the index could be maintained by the common code along with > the cur_profile pointer, couldn't it? OK - I see your point. I think that it's good to check for an error from the driver and to not display a potentially incorrect value in the case of an error. I double checked and in the Lenovo case (patch 3) all of this works exactly as you describe (so my previous comment was incorrect). The value is cached in thinkpad_acpi and there can't be an error returned. But it seems wrong to make assumptions on others wanting to do the same in the future as their implementation might have different constraints. Let me know if you feel strongly about this and I can update, from my point of view I lean towards leaving it as it is but it doesn't impact the Lenovo implementation. > <snip>>>>> + >>>> +enum platform_profile_option { >>>> + platform_profile_low, >>>> + platform_profile_cool, >>>> + platform_profile_quiet, >>>> + platform_profile_balanced, >>>> + platform_profile_perform, >>>> + platform_profile_last, /*must always be last */ > > So please use upper-case names in this list. > Ack Thanks for the clarifications Mark
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..1bc092359e35 --- /dev/null +++ b/drivers/acpi/platform_profile.c @@ -0,0 +1,181 @@ +// 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_balanced] = "balanced", + [platform_profile_perform] = "performance", +}; +static_assert(ARRAY_SIZE(profile_names) == platform_profile_last); + +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; + } + + for_each_set_bit(i, cur_profile->choices, platform_profile_last) { + 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_balanced; + int err; + + err = mutex_lock_interruptible(&profile_lock); + if (err) + return err; + + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -ENODEV; + } + + err = cur_profile->profile_get(&profile); + mutex_unlock(&profile_lock); + if (err) + return err; + + /* Check that profile is valid index */ + if (WARN_ON((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))) + return -EIO; + + 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; + } + + /* 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 (!test_bit(i, cur_profile->choices)) { + 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; + + mutex_lock(&profile_lock); + /* We can only have one active profile */ + if (cur_profile) { + mutex_unlock(&profile_lock); + return -EEXIST; + } + + /* Sanity check the profile handler field are set */ + if (!pprof || !pprof->choices || !pprof->profile_set || + !pprof->profile_get) { + mutex_unlock(&profile_lock); + return -EINVAL; + } + + 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) +{ + mutex_lock(&profile_lock); + 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); + +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..f2e1b1c90482 --- /dev/null +++ b/include/linux/platform_profile.h @@ -0,0 +1,39 @@ +/* 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_ + +#include <linux/bitops.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_balanced, + platform_profile_perform, + platform_profile_last, /*must always be last */ +}; + +struct platform_profile_handler { + unsigned long choices[BITS_TO_LONGS(platform_profile_last)]; + 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_*) Changes in v5: - correct 'balance' to 'balanced' to be consistent with documentation - add WARN_ON when checking profile index in show function - switch mutex_lock_interruptible back to mutex_lock where appropriate - add 'platform_profile_last' as final entry in profile entry. Update implementation to use this appropriately - Use BITS_TO_LONG and appropriate access functions for choices field - Correct error handling as recommended - Sanity check profile fields on registration - Remove unnecessary init and exit functions drivers/acpi/Kconfig | 14 +++ drivers/acpi/Makefile | 1 + drivers/acpi/platform_profile.c | 181 +++++++++++++++++++++++++++++++ include/linux/platform_profile.h | 39 +++++++ 4 files changed, 235 insertions(+) create mode 100644 drivers/acpi/platform_profile.c create mode 100644 include/linux/platform_profile.h