| Message ID | 20201115004402.342838-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3] ACPI: platform-profile: Add platform profile support | expand |
Hi I believe it would have been easier for the maintainers if you had resent the whole series. Another thing is that `mutex_lock_interruptible()` seems to be preferred instead of `mutex_lock()` [1]. 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta: > [...] > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index edf1558c1105..73a99af5ec2c 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_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. > + > + Am I missing something or is the ACPI_PLATFORM_PROFILE option in the Kconfig file twice? > [...] > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +/* Platform profile sysfs interface */ > + > +#include <linux/acpi.h> linux/bits.h is missing, I believe the rule of thumb is that if you use `X` and `X` is defined in `A.h`, then you should include `A.h` directly, and not rely on the fact that another header file you use includes `A.h`. An exception is ACPICA related things, I believe linux/acpi.h should be preferred there. > +#include <linux/device.h> > +#include <linux/fs.h> > +#include <linux/init.h> > +#include <linux/kobject.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/platform_profile.h> > +#include <linux/printk.h> > +#include <linux/string.h> > +#include <linux/sysfs.h> I believe there are some unnecessary header files. (maybe fs.h, kobject.h, ...) > + > +static struct platform_profile_handler *cur_profile; I think this could be `const` as well. (along with the argument of `platform_profile_register()`) > +static DEFINE_MUTEX(profile_lock); > + > +/* Ensure the first char of each profile is unique */ I think this comment is not needed anymore, no? > [...] > +static ssize_t platform_profile_choices_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int len = 0; > + int i; > + > + mutex_lock(&profile_lock); > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -ENODEV; > + } > + > + if (!cur_profile->choices) { > + mutex_unlock(&profile_lock); > + return sysfs_emit(buf, "\n"); > + } > + > + for (i = profile_low; i <= profile_perform; 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]); ^^ I'm unsure why there are two spaces before `profile_names[i]` in both previous places. > + } > + } > + 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; > + int err; > + > + mutex_lock(&profile_lock); > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -EOPNOTSUPP; > + } > + > + 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; > + > + return sysfs_emit(buf, "%s\n", profile_names[profile]); I believe `profile` should be initialized to some value, and the value after the call to the handler it should be checked for validity, and maybe an warning should be emitted if it's out of range - in my opinion. > +} > + > +static ssize_t platform_profile_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int err, i; > + > + mutex_lock(&profile_lock); > + if (!cur_profile) { > + mutex_unlock(&profile_lock); > + return -EOPNOTSUPP; I'm not sure why you modified the errno. ENODEV seemed to me like a perfectly fine error to return if `cur_profile` is not set. `platform_profile_choices_show()` returns ENODEV, so there is a bit of inconsistency. (same applies to `platform_profile_show()`) > + } > + > + /* Scan for a matching profile */ > + i = sysfs_match_string(profile_names, buf); > + if (i < 0) { > + mutex_unlock(&profile_lock); > + return -EINVAL; > + } > + > + if (!cur_profile->profile_set) { > + mutex_unlock(&profile_lock); > + return -EOPNOTSUPP; > + } > + > + err = cur_profile->profile_set(i); > + mutex_unlock(&profile_lock); > + if (err) > + return err; > + > + return count; > +} > [...] > +int platform_profile_register(struct platform_profile_handler *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_group); I believe the return value of `sysfs_create_group()` should be checked here, and `cur_profile` should only be set if that succeeds. > +} > +EXPORT_SYMBOL_GPL(platform_profile_register); > [...] > diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h > new file mode 100644 > index 000000000000..f6592434c8ce > --- /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 { > + profile_low, > + profile_cool, > + profile_quiet, > + profile_balance, > + profile_perform ^ I believe there's no reason to remove the comma from there, and in fact, having a comma after the last entry in an array, enum, etc. seems to be the preferred. If you don't want to go the `platform_profile_last` route that I suggested previously, then I think a comment should mention that the last profile should be used in the static_assert() in platform-profile.c. By the way, I still feel like the enum values are "too vague" and should be prefixed with `platform_`. If you're not opposed to that change. > +}; > + > +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(struct platform_profile_handler *pprof); > +int platform_profile_unregister(void); > +int platform_profile_notify(void); I don't think it's a big problem, but right now, I personally can't quite see the purpose `platform_profile_notify()` and `platform_profile_unregister()` returning any value. > + > +#endif /*_PLATFORM_PROFILE_H_*/ > -- > 2.25.1 [1]: https://www.kernel.org/doc/html/latest/kernel-hacking/locking.html#locking-only-in-user-context Regards, Barnabás Pőcze
Thanks Barnabas On 15/11/2020 13:26, Barnabás Pőcze wrote: > Hi > > I believe it would have been easier for the maintainers > if you had resent the whole series. > > Another thing is that `mutex_lock_interruptible()` seems to be preferred > instead of `mutex_lock()` [1]. Thanks for the reference - I wasn't aware of that and will update. > > > 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta: > >> [...] >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index edf1558c1105..73a99af5ec2c 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_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. >> + >> + > > Am I missing something or is the ACPI_PLATFORM_PROFILE option in the Kconfig file > twice? It is....and it shouldn't be. I'll remove. I looked over this patch so many times checking that I'd put in all the pieces of feedback from everyone that I'm not sure how I missed that :( > > >> [...] >> +// SPDX-License-Identifier: GPL-2.0-or-later >> + >> +/* Platform profile sysfs interface */ >> + >> +#include <linux/acpi.h> > > linux/bits.h is missing, I believe the rule of thumb is that if you use > `X` and `X` is defined in `A.h`, then you should include `A.h` directly, > and not rely on the fact that another header file you use includes `A.h`. > An exception is ACPICA related things, I believe linux/acpi.h should be > preferred there. Thanks - I didn't realise that was the case and I'll update > > >> +#include <linux/device.h> >> +#include <linux/fs.h> >> +#include <linux/init.h> >> +#include <linux/kobject.h> >> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/platform_profile.h> >> +#include <linux/printk.h> >> +#include <linux/string.h> >> +#include <linux/sysfs.h> > > I believe there are some unnecessary header files. > (maybe fs.h, kobject.h, ...) Seems likely - I did go through a few iterations and you're right that these should be removed. > >> + >> +static struct platform_profile_handler *cur_profile; > > I think this could be `const` as well. > (along with the argument of `platform_profile_register()`) ack > > >> +static DEFINE_MUTEX(profile_lock); >> + >> +/* Ensure the first char of each profile is unique */ > > I think this comment is not needed anymore, no? Agreed > > >> [...] >> +static ssize_t platform_profile_choices_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + int len = 0; >> + int i; >> + >> + mutex_lock(&profile_lock); >> + if (!cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -ENODEV; >> + } >> + >> + if (!cur_profile->choices) { >> + mutex_unlock(&profile_lock); >> + return sysfs_emit(buf, "\n"); >> + } >> + >> + for (i = profile_low; i <= profile_perform; 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]); > ^^ > I'm unsure why there are two spaces before `profile_names[i]` in both previous places. I'm unsure how I introduced these too....I'll clean up > > >> + } >> + } >> + 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; >> + int err; >> + >> + mutex_lock(&profile_lock); >> + if (!cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -EOPNOTSUPP; >> + } >> + >> + 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; >> + >> + return sysfs_emit(buf, "%s\n", profile_names[profile]); > > I believe `profile` should be initialized to some value, and the value after the > call to the handler it should be checked for validity, and maybe an warning should > be emitted if it's out of range - in my opinion. Agreed - and a check would do no harm. I'll add. > > >> +} >> + >> +static ssize_t platform_profile_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + int err, i; >> + >> + mutex_lock(&profile_lock); >> + if (!cur_profile) { >> + mutex_unlock(&profile_lock); >> + return -EOPNOTSUPP; > > I'm not sure why you modified the errno. ENODEV seemed to me like a perfectly > fine error to return if `cur_profile` is not set. `platform_profile_choices_show()` > returns ENODEV, so there is a bit of inconsistency. > (same applies to `platform_profile_show()`) My thinking was it seemed a better message. I can't really see any conditions when you'd get here (a driver would have registered a driver and then not provided a profile?) but it seemed that if that was possible it was more because changing the settings wasn't supported. I managed to convince myself it made more sense - but have no issue with changing it back. > > >> + } >> + >> + /* Scan for a matching profile */ >> + i = sysfs_match_string(profile_names, buf); >> + if (i < 0) { >> + mutex_unlock(&profile_lock); >> + return -EINVAL; >> + } >> + >> + if (!cur_profile->profile_set) { >> + mutex_unlock(&profile_lock); >> + return -EOPNOTSUPP; >> + } >> + >> + err = cur_profile->profile_set(i); >> + mutex_unlock(&profile_lock); >> + if (err) >> + return err; >> + >> + return count; >> +} >> [...] >> +int platform_profile_register(struct platform_profile_handler *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_group); > > I believe the return value of `sysfs_create_group()` should be checked here, > and `cur_profile` should only be set if that succeeds. agreed. > > >> +} >> +EXPORT_SYMBOL_GPL(platform_profile_register); >> [...] >> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h >> new file mode 100644 >> index 000000000000..f6592434c8ce >> --- /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 { >> + profile_low, >> + profile_cool, >> + profile_quiet, >> + profile_balance, >> + profile_perform > ^ > I believe there's no reason to remove the comma from there, and in fact, > having a comma after the last entry in an array, enum, etc. seems to be > the preferred. OK. Have to be honest - I struggle to know when comma's are needed on the last entry and when they aren't (I've had similar corrections in other cases both ways :)). I do seem to have a knack of getting it consistently wrong.... > > If you don't want to go the `platform_profile_last` route that I suggested previously, > then I think a comment should mention that the last profile should be used > in the static_assert() in platform-profile.c. ok. > > By the way, I still feel like the enum values are "too vague" and should be > prefixed with `platform_`. If you're not opposed to that change. Sure - I can change that. For me it made the names long but I don't mind changing them. > > >> +}; >> + >> +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(struct platform_profile_handler *pprof); >> +int platform_profile_unregister(void); >> +int platform_profile_notify(void); > > I don't think it's a big problem, but right now, I personally can't quite see the > purpose `platform_profile_notify()` and `platform_profile_unregister()` > returning any value. OK - I can update > > >> + >> +#endif /*_PLATFORM_PROFILE_H_*/ >> -- >> 2.25.1 > > > [1]: https://www.kernel.org/doc/html/latest/kernel-hacking/locking.html#locking-only-in-user-context > > > Regards, > Barnabás Pőcze > Many thanks Mark
Hi 2020. november 16., hétfő 0:04 keltezéssel, Mark Pearson írta: > [...] > >> +static ssize_t platform_profile_store(struct device *dev, > >> + struct device_attribute *attr, > >> + const char *buf, size_t count) > >> +{ > >> + int err, i; > >> + > >> + mutex_lock(&profile_lock); > >> + if (!cur_profile) { > >> + mutex_unlock(&profile_lock); > >> + return -EOPNOTSUPP; > > > > I'm not sure why you modified the errno. ENODEV seemed to me like a perfectly > > fine error to return if `cur_profile` is not set. `platform_profile_choices_show()` > > returns ENODEV, so there is a bit of inconsistency. > > (same applies to `platform_profile_show()`) > My thinking was it seemed a better message. I can't really see any > conditions when you'd get here (a driver would have registered a driver > and then not provided a profile?) but it seemed that if that was > possible it was more because changing the settings wasn't supported. > > I managed to convince myself it made more sense - but have no issue with > changing it back. > > > > > >> + } > >> + > >> + /* Scan for a matching profile */ > >> + i = sysfs_match_string(profile_names, buf); > >> + if (i < 0) { > >> + mutex_unlock(&profile_lock); > >> + return -EINVAL; > >> + } > >> + > >> + if (!cur_profile->profile_set) { > >> + mutex_unlock(&profile_lock); > >> + return -EOPNOTSUPP; > >> + } > >> + One thing I have just noticed is that I believe the selected profile should be checked against `cur_profile->choices`, don't you think? I have assumed for some reason that this check is done, and `profile_set` is only called with values that the handler supports (they are in `choices`) up until now, but I see that that's not what's happening. > >> + err = cur_profile->profile_set(i); > >> + mutex_unlock(&profile_lock); > >> + if (err) > >> + return err; > >> + > >> + return count; > >> +} > [...] > >> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h > >> new file mode 100644 > >> index 000000000000..f6592434c8ce > >> --- /dev/null > >> +++ b/include/linux/platform_profile.h > >> @@ -0,0 +1,36 @@ > [...] > > > > By the way, I still feel like the enum values are "too vague" and should be > > prefixed with `platform_`. If you're not opposed to that change. > Sure - I can change that. For me it made the names long but I don't mind > changing them. Short and succinct names a good, but I hope you can see why I'm saying what I'm saying. I believe these names should be "properly namespaced" since they are in a "kernel-global" header file. > > > > > >> +}; > >> + > >> [...] Regards, Barnabás Pőcze
Hi, On 11/16/20 12:04 AM, Mark Pearson wrote: <snip> >> I believe there's no reason to remove the comma from there, and in fact, >> having a comma after the last entry in an array, enum, etc. seems to be >> the preferred. > OK. > Have to be honest - I struggle to know when comma's are needed on the last entry and when they aren't (I've had similar corrections in other cases both ways :)). I do seem to have a knack of getting it consistently wrong.... Do the rule of thumb here is, if the last element is a terminating element, e.g. NULL or {} or foo_number_of_foo_types in an enum foo declaration then there should not be a comma after the last element. The reason for is is that in case case new entries will be added one line above the last element. If there is no terminating element (e.g. because ARRAY_SIZE is always used on the array). Then the last element should end with a comma. The reason for this is so that the unified diff of a patch adding a new element does not have -++ lines, as would be necessary when the comma is missing (-+ to add the comma, plus one more + for the new element). I hope this helps explain. I expect you will send out a v4 of the entire set addressing all current remarks? Regards, Hans
Thanks Hans On 16/11/2020 09:33, Hans de Goede wrote: > Hi, > > On 11/16/20 12:04 AM, Mark Pearson wrote: > > <snip> >>> I believe there's no reason to remove the comma from there, and in fact, >>> having a comma after the last entry in an array, enum, etc. seems to be >>> the preferred. >> OK. >> Have to be honest - I struggle to know when comma's are needed on the last entry and when they aren't (I've had similar corrections in other cases both ways :)). I do seem to have a knack of getting it consistently wrong.... > > Do the rule of thumb here is, if the last element is a terminating element, > e.g. NULL or {} or foo_number_of_foo_types in an enum foo declaration then > there should not be a comma after the last element. The reason for is is > that in case case new entries will be added one line above the last element. > > If there is no terminating element (e.g. because ARRAY_SIZE is always used > on the array). Then the last element should end with a comma. The reason for > this is so that the unified diff of a patch adding a new element does not > have -++ lines, as would be necessary when the comma is missing (-+ to add > the comma, plus one more + for the new element). > > I hope this helps explain. It does - makes complete sense. > > I expect you will send out a v4 of the entire set addressing all current > remarks? Absolutely. Hopefully will get that out soon but I'm going to take a bit longer on it as I was pretty disappointed with myself for some of the things that slipped into the last set. I'll aim to get a cleaner set out for v4. > > Regards, > > Hans > Thanks Mark
Hi Barnabás On 16/11/2020 05:24, Barnabás Pőcze wrote: > Hi > > > 2020. november 16., hétfő 0:04 keltezéssel, Mark Pearson írta: > >> [...] >>>> +static ssize_t platform_profile_store(struct device *dev, >>>> + struct device_attribute *attr, >>>> + const char *buf, size_t count) >>>> +{ >>>> + int err, i; >>>> + >>>> + mutex_lock(&profile_lock); >>>> + if (!cur_profile) { >>>> + mutex_unlock(&profile_lock); >>>> + return -EOPNOTSUPP; >>> >>> I'm not sure why you modified the errno. ENODEV seemed to me like a perfectly >>> fine error to return if `cur_profile` is not set. `platform_profile_choices_show()` >>> returns ENODEV, so there is a bit of inconsistency. >>> (same applies to `platform_profile_show()`) >> My thinking was it seemed a better message. I can't really see any >> conditions when you'd get here (a driver would have registered a driver >> and then not provided a profile?) but it seemed that if that was >> possible it was more because changing the settings wasn't supported. >> >> I managed to convince myself it made more sense - but have no issue with >> changing it back. >>> >>> >>>> + } >>>> + >>>> + /* Scan for a matching profile */ >>>> + i = sysfs_match_string(profile_names, buf); >>>> + if (i < 0) { >>>> + mutex_unlock(&profile_lock); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if (!cur_profile->profile_set) { >>>> + mutex_unlock(&profile_lock); >>>> + return -EOPNOTSUPP; >>>> + } >>>> + > > One thing I have just noticed is that I believe the selected profile should be > checked against `cur_profile->choices`, don't you think? I have assumed for > some reason that this check is done, and `profile_set` is only called with values > that the handler supports (they are in `choices`) up until now, but I see > that that's not what's happening. True - I guess no harm adding the check in platform_profile.c too. > > >>>> + err = cur_profile->profile_set(i); >>>> + mutex_unlock(&profile_lock); >>>> + if (err) >>>> + return err; >>>> + >>>> + return count; >>>> +} >> [...] >>>> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h >>>> new file mode 100644 >>>> index 000000000000..f6592434c8ce >>>> --- /dev/null >>>> +++ b/include/linux/platform_profile.h >>>> @@ -0,0 +1,36 @@ >> [...] >>> >>> By the way, I still feel like the enum values are "too vague" and should be >>> prefixed with `platform_`. If you're not opposed to that change. >> Sure - I can change that. For me it made the names long but I don't mind >> changing them. > > Short and succinct names a good, but I hope you can see why I'm saying what I'm > saying. I believe these names should be "properly namespaced" since they are in > a "kernel-global" header file. I do and I will update :) Thanks for all the pointers Mark
Hi 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta: > [...] > +int platform_profile_register(struct platform_profile_handler *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_group); > +} > +EXPORT_SYMBOL_GPL(platform_profile_register); > + > +int platform_profile_unregister(void) > +{ > + mutex_lock(&profile_lock); > + sysfs_remove_group(acpi_kobj, &platform_profile_group); > + cur_profile = NULL; > + mutex_unlock(&profile_lock); > + return 0; > +} > +EXPORT_SYMBOL_GPL(platform_profile_unregister); > [...] I just realized that the sysfs attributes are only created if a profile provider is registered, and it is removed when the provide unregisters itself. I believe it would be easier for system daemons if those attributes existed from module load to module unload since they can just just open the file and watch it using poll, select, etc. If it goes away when the provider unregisters itself, then I believe a more complicated mechanism (like inotify) would need to be implemented in the daemons to be notified when a new provider is registered. Thus my suggestion for the next iteration is to create the sysfs attributes on module load, and delete them on unload. What do you think? Regards, Barnabás Pőcze
On 20/11/2020 14:50, Barnabás Pőcze wrote: > Hi > > > 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta: > >> [...] >> +int platform_profile_register(struct platform_profile_handler *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_group); >> +} >> +EXPORT_SYMBOL_GPL(platform_profile_register); >> + >> +int platform_profile_unregister(void) >> +{ >> + mutex_lock(&profile_lock); >> + sysfs_remove_group(acpi_kobj, &platform_profile_group); >> + cur_profile = NULL; >> + mutex_unlock(&profile_lock); >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(platform_profile_unregister); >> [...] > > > I just realized that the sysfs attributes are only created if a profile provider > is registered, and it is removed when the provide unregisters itself. I believe > it would be easier for system daemons if those attributes existed from module load > to module unload since they can just just open the file and watch it using poll, > select, etc. If it goes away when the provider unregisters itself, then I believe > a more complicated mechanism (like inotify) would need to be implemented in the > daemons to be notified when a new provider is registered. Thus my suggestion > for the next iteration is to create the sysfs attributes on module load, > and delete them on unload. > > What do you think? > > > Regards, > Barnabás Pőcze > Good point. I'd like to have input from user space as to their preference - it seems like a simple enough change to make and I'm happy to go with what their preferences are. Bastien - would this make life easier for you? Perhaps when the provider unregisters it sends a sysfs_notify and user space can look and detect/handle that condition (I think it will get EOPNOTSUPP on the next read). Is there something cleverer it can do? Thanks Mark
Hi, On 11/20/20 8:50 PM, Barnabás Pőcze wrote: > Hi > > > 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta: > >> [...] >> +int platform_profile_register(struct platform_profile_handler *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_group); >> +} >> +EXPORT_SYMBOL_GPL(platform_profile_register); >> + >> +int platform_profile_unregister(void) >> +{ >> + mutex_lock(&profile_lock); >> + sysfs_remove_group(acpi_kobj, &platform_profile_group); >> + cur_profile = NULL; >> + mutex_unlock(&profile_lock); >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(platform_profile_unregister); >> [...] > > > I just realized that the sysfs attributes are only created if a profile provider > is registered, and it is removed when the provide unregisters itself. I believe > it would be easier for system daemons if those attributes existed from module load > to module unload since they can just just open the file and watch it using poll, > select, etc. If it goes away when the provider unregisters itself, then I believe > a more complicated mechanism (like inotify) would need to be implemented in the > daemons to be notified when a new provider is registered. Thus my suggestion > for the next iteration is to create the sysfs attributes on module load, > and delete them on unload. > > What do you think? Actually I asked Mark to move this to the register / unregister time since having a non functioning files in sysfs is a bit weird. You make a good point about userspace having trouble figuring when this will show up though (I'm not worried about removal that will normally never happen). I would expect all modules to be loaded before any interested daemons load, but that is an assumption and I must admit that that is a bit racy. Bastien, what do you think about Barnabás' suggestion to always have the files present and use poll (POLL_PRI) on it to see when it changes, listing maybe "none" as available profiles when there is no provider? Regards, Hans
2020. november 21., szombat 15:27 keltezéssel, Hans de Goede írta: > [...] > > I just realized that the sysfs attributes are only created if a profile provider > > is registered, and it is removed when the provide unregisters itself. I believe > > it would be easier for system daemons if those attributes existed from module load > > to module unload since they can just just open the file and watch it using poll, > > select, etc. If it goes away when the provider unregisters itself, then I believe > > a more complicated mechanism (like inotify) would need to be implemented in the > > daemons to be notified when a new provider is registered. Thus my suggestion > > for the next iteration is to create the sysfs attributes on module load, > > and delete them on unload. > > > > What do you think? > > Actually I asked Mark to move this to the register / unregister time since > having a non functioning files in sysfs is a bit weird. > [...] Ahh, I didn't know that, sorry. If a non-functioning sysfs attribute is a problem, then there is another option: `platform_profile_choices` is always present; if no provider is registered, it's empty. If a provider is (un)registered, then `platform_profile_choices` is sysfs_notify()-ed. `platform_profile` only exists while a provider is registered, so it is created on provider registration and unregistered on provider unregistration. Regards, Barnabás Pőcze
Hi, On 11/21/20 10:18 PM, Barnabás Pőcze wrote: > 2020. november 21., szombat 15:27 keltezéssel, Hans de Goede írta: > >> [...] >>> I just realized that the sysfs attributes are only created if a profile provider >>> is registered, and it is removed when the provide unregisters itself. I believe >>> it would be easier for system daemons if those attributes existed from module load >>> to module unload since they can just just open the file and watch it using poll, >>> select, etc. If it goes away when the provider unregisters itself, then I believe >>> a more complicated mechanism (like inotify) would need to be implemented in the >>> daemons to be notified when a new provider is registered. Thus my suggestion >>> for the next iteration is to create the sysfs attributes on module load, >>> and delete them on unload. >>> >>> What do you think? >> >> Actually I asked Mark to move this to the register / unregister time since >> having a non functioning files in sysfs is a bit weird. >> [...] > > Ahh, I didn't know that, sorry. If a non-functioning sysfs attribute is a problem, > then there is another option: `platform_profile_choices` is always present; > if no provider is registered, it's empty. If a provider is (un)registered, > then `platform_profile_choices` is sysfs_notify()-ed. `platform_profile` > only exists while a provider is registered, so it is created on provider > registration and unregistered on provider unregistration. TBH, I don't like this suggestion. I would like to either want both of them be present and report "none" (and -ENODEV on write in case of platform_profile), or neither of them be present. Note I do agree with you that userspace probably needs a way to find out when a provider shows up. Which means we should probably go with always having both of them present. But it would also be good to get some input from Bastien here, as he is working on a userspace daemon using this API. Regards, Hans
On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote: > Hi, > > On 11/20/20 8:50 PM, Barnabás Pőcze wrote: > > Hi > > > > > > 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta: > > > > > [...] > > > +int platform_profile_register(struct platform_profile_handler > > > *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_group); > > > +} > > > +EXPORT_SYMBOL_GPL(platform_profile_register); > > > + > > > +int platform_profile_unregister(void) > > > +{ > > > + mutex_lock(&profile_lock); > > > + sysfs_remove_group(acpi_kobj, &platform_profile_group); > > > + cur_profile = NULL; > > > + mutex_unlock(&profile_lock); > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(platform_profile_unregister); > > > [...] > > > > > > I just realized that the sysfs attributes are only created if a > > profile provider > > is registered, and it is removed when the provide unregisters > > itself. I believe > > it would be easier for system daemons if those attributes existed > > from module load > > to module unload since they can just just open the file and watch > > it using poll, > > select, etc. If it goes away when the provider unregisters itself, > > then I believe > > a more complicated mechanism (like inotify) would need to be > > implemented in the > > daemons to be notified when a new provider is registered. Thus my > > suggestion > > for the next iteration is to create the sysfs attributes on module > > load, > > and delete them on unload. > > > > What do you think? > > Actually I asked Mark to move this to the register / unregister time > since > having a non functioning files in sysfs is a bit weird. > > You make a good point about userspace having trouble figuring when > this will > show up though (I'm not worried about removal that will normally > never happen). > > I would expect all modules to be loaded before any interested daemons > load, > but that is an assumption and I must admit that that is a bit racy. > > Bastien, what do you think about Barnabás' suggestion to always have > the > files present and use poll (POLL_PRI) on it to see when it changes, > listing > maybe "none" as available profiles when there is no provider? Whether the file exists or doesn't, we have ways to monitor it appearing or disappearing. I can monitor the directory with inotify to see the file appearing or disappearing, or I can monitor the file with inotify to see whether it changes. But that doesn't solve the challenges from user-space. How do I know whether the computer will have this facility at any point? Is "none" a placeholder, or a definite answer as to whether the computer will have support for "platform profile"? How am I supposed to handle fallbacks, eg. how can I offer support for, say, changing the "energy performance preference" from the Intel p- state driver if ACPI platform profile isn't available? I'm fine with throwing more code at fixing that race, so whatever's easier for you, it'll be a pain for user-space either way.
Hi, On 11/24/20 4:30 PM, Bastien Nocera wrote: > On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote: >> Hi, >> >> On 11/20/20 8:50 PM, Barnabás Pőcze wrote: >>> Hi >>> >>> >>> 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson írta: >>> >>>> [...] >>>> +int platform_profile_register(struct platform_profile_handler >>>> *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_group); >>>> +} >>>> +EXPORT_SYMBOL_GPL(platform_profile_register); >>>> + >>>> +int platform_profile_unregister(void) >>>> +{ >>>> + mutex_lock(&profile_lock); >>>> + sysfs_remove_group(acpi_kobj, &platform_profile_group); >>>> + cur_profile = NULL; >>>> + mutex_unlock(&profile_lock); >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL_GPL(platform_profile_unregister); >>>> [...] >>> >>> >>> I just realized that the sysfs attributes are only created if a >>> profile provider >>> is registered, and it is removed when the provide unregisters >>> itself. I believe >>> it would be easier for system daemons if those attributes existed >>> from module load >>> to module unload since they can just just open the file and watch >>> it using poll, >>> select, etc. If it goes away when the provider unregisters itself, >>> then I believe >>> a more complicated mechanism (like inotify) would need to be >>> implemented in the >>> daemons to be notified when a new provider is registered. Thus my >>> suggestion >>> for the next iteration is to create the sysfs attributes on module >>> load, >>> and delete them on unload. >>> >>> What do you think? >> >> Actually I asked Mark to move this to the register / unregister time >> since >> having a non functioning files in sysfs is a bit weird. >> >> You make a good point about userspace having trouble figuring when >> this will >> show up though (I'm not worried about removal that will normally >> never happen). >> >> I would expect all modules to be loaded before any interested daemons >> load, >> but that is an assumption and I must admit that that is a bit racy. >> >> Bastien, what do you think about Barnabás' suggestion to always have >> the >> files present and use poll (POLL_PRI) on it to see when it changes, >> listing >> maybe "none" as available profiles when there is no provider? > > Whether the file exists or doesn't, we have ways to monitor it > appearing or disappearing. I can monitor the directory with inotify to > see the file appearing or disappearing, or I can monitor the file with > inotify to see whether it changes. Ok, do you have a preference either way? I personally think having the file only appear if its functional is a bit cleaner. So if it does not matter much for userspace either way then I suggest we go that route. > But that doesn't solve the challenges from user-space. > > How do I know whether the computer will have this facility at any > point? Is "none" a placeholder, or a definite answer as to whether the > computer will have support for "platform profile"? > > How am I supposed to handle fallbacks, eg. how can I offer support for, > say, changing the "energy performance preference" from the Intel p- > state driver if ACPI platform profile isn't available? > > I'm fine with throwing more code at fixing that race, so whatever's > easier for you, it'll be a pain for user-space either way. Ugh, right this is a bit different from other hotplug cases, where you just wait for a device to show up before using it, since in this case you want to use the p-state driver as alternative mechanism when there is no platform_profile provider. I'm afraid that the answer here is that the kernel does not really have anything to help you here. Since the advent of hotplug we had this issue of determining when the initial hardware probing / driver loading is done. This is basically an impossible problem since with things like thunderbolt, etc. probing may be a bit slow and then if there is a lot of USB devices connected to the thunderbolt dock, those are slow to probe too, etc. See either we wait 5 minutes just to be sure, or we cannot really tell when probing is done. Time has taught us that determining when probing is done is an unsolvable problem. We had e.g. udev-settle for this. But that was both slow and not reliable, so that is deprectated and we don't want to bring it back. In general whenever someone wants something like udev-settle the answer is to make the code wanting that more event driven/dynamic. So I'm afraid that that is the answer here too. I have the feeling that in most cases the platform_profile driver will have loaded before the daemon starts. So you could use that as a base assumption and then do something somewhat ugly like log a message + restart in case you loose the race. Assuming that that is easy to implement, you could do that for starters and then if loosing the race becomes a real problem, then implement something smarter (and more complicated) later. Sorry, I have no easy answers here. Regards, Hans
On 25/11/2020 11:42, Hans de Goede wrote: > Hi, > > On 11/24/20 4:30 PM, Bastien Nocera wrote: >> On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote: >>> Hi, >>> On 11/20/20 8:50 PM, Barnabás Pőcze wrote: >>>> Hi >>>> 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson >>>> írta: >>>> >>> Bastien, what do you think about Barnabás' suggestion to always >>> have the files present and use poll (POLL_PRI) on it to see when >>> it changes, listing maybe "none" as available profiles when there >>> is no provider? >> >> Whether the file exists or doesn't, we have ways to monitor it >> appearing or disappearing. I can monitor the directory with inotify >> to see the file appearing or disappearing, or I can monitor the >> file with inotify to see whether it changes. > > Ok, do you have a preference either way? I personally think having > the file only appear if its functional is a bit cleaner. So if it > does not matter much for userspace either way then I suggest we go > that route. > My (limited) vote is having the file appear when the profile is loaded seems nicer (and to disappear if the profile is unloaded). No profile, no interface just seems elegant. I pretty much have v4 of the patches ready to go (I wanted to rebase on the update for the palm sensor and finished that today). I'm happy to hold off on them until we're ready if that helps. Bastien - let me know if it would be useful to have a version to test against to see what will work best for you, or if you need something different. Definitely want to make sure user space gets the best option as my understanding is changing this later becomes a pain :) Thanks Mark
On Wed, 2020-11-25 at 14:41 -0500, Mark Pearson wrote: > On 25/11/2020 11:42, Hans de Goede wrote: > > Hi, > > > > On 11/24/20 4:30 PM, Bastien Nocera wrote: > > > On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote: > > > > Hi, > > > > On 11/20/20 8:50 PM, Barnabás Pőcze wrote: > > > > > Hi > > > > > 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson > > > > > írta: > > > > > > > > > Bastien, what do you think about Barnabás' suggestion to always > > > > have the files present and use poll (POLL_PRI) on it to see > > > > when > > > > it changes, listing maybe "none" as available profiles when > > > > there > > > > is no provider? > > > > > > Whether the file exists or doesn't, we have ways to monitor it > > > appearing or disappearing. I can monitor the directory with > > > inotify > > > to see the file appearing or disappearing, or I can monitor the > > > file with inotify to see whether it changes. > > > > Ok, do you have a preference either way? I personally think having > > the file only appear if its functional is a bit cleaner. So if it > > does not matter much for userspace either way then I suggest we go > > that route. > > > My (limited) vote is having the file appear when the profile is > loaded > seems nicer (and to disappear if the profile is unloaded). No > profile, > no interface just seems elegant. > > I pretty much have v4 of the patches ready to go (I wanted to rebase > on > the update for the palm sensor and finished that today). I'm happy to > hold off on them until we're ready if that helps. Bastien - let me > know > if it would be useful to have a version to test against to see what > will > work best for you, or if you need something different. Definitely > want > to make sure user space gets the best option as my understanding is > changing this later becomes a pain :) I don't have the hardware. I agree with you that "no profile = no file" seems cleaner, but whichever is easier for you.
Hi, On 11/25/20 11:32 PM, Bastien Nocera wrote: > On Wed, 2020-11-25 at 14:41 -0500, Mark Pearson wrote: >> On 25/11/2020 11:42, Hans de Goede wrote: >>> Hi, >>> >>> On 11/24/20 4:30 PM, Bastien Nocera wrote: >>>> On Sat, 2020-11-21 at 15:27 +0100, Hans de Goede wrote: >>>>> Hi, >>>>> On 11/20/20 8:50 PM, Barnabás Pőcze wrote: >>>>>> Hi >>>>>> 2020. november 15., vasárnap 1:44 keltezéssel, Mark Pearson >>>>>> írta: >>>>>> >>>>> Bastien, what do you think about Barnabás' suggestion to always >>>>> have the files present and use poll (POLL_PRI) on it to see >>>>> when >>>>> it changes, listing maybe "none" as available profiles when >>>>> there >>>>> is no provider? >>>> >>>> Whether the file exists or doesn't, we have ways to monitor it >>>> appearing or disappearing. I can monitor the directory with >>>> inotify >>>> to see the file appearing or disappearing, or I can monitor the >>>> file with inotify to see whether it changes. >>> >>> Ok, do you have a preference either way? I personally think having >>> the file only appear if its functional is a bit cleaner. So if it >>> does not matter much for userspace either way then I suggest we go >>> that route. >>> >> My (limited) vote is having the file appear when the profile is >> loaded >> seems nicer (and to disappear if the profile is unloaded). No >> profile, >> no interface just seems elegant. >> >> I pretty much have v4 of the patches ready to go (I wanted to rebase >> on >> the update for the palm sensor and finished that today). I'm happy to >> hold off on them until we're ready if that helps. Bastien - let me >> know >> if it would be useful to have a version to test against to see what >> will >> work best for you, or if you need something different. Definitely >> want >> to make sure user space gets the best option as my understanding is >> changing this later becomes a pain :) > > I don't have the hardware. I agree with you that "no profile = no file" > seems cleaner, but whichever is easier for you. Ok, lets go with that then (so same as in v3). Mark, if you can submit v4 upstream that would be great. Regards, Hans
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index edf1558c1105..73a99af5ec2c 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 "" @@ -538,3 +552,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 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..e4bbee48c0f8 --- /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/device.h> +#include <linux/fs.h> +#include <linux/init.h> +#include <linux/kobject.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/platform_profile.h> +#include <linux/printk.h> +#include <linux/string.h> +#include <linux/sysfs.h> + +static struct platform_profile_handler *cur_profile; +static DEFINE_MUTEX(profile_lock); + +/* Ensure the first char of each profile is unique */ +static const char * const profile_names[] = { + [profile_low] = "low-power", + [profile_cool] = "cool", + [profile_quiet] = "quiet", + [profile_balance] = "balance", + [profile_perform] = "performance", +}; +static_assert(ARRAY_SIZE(profile_names) == profile_perform+1); + +static ssize_t platform_profile_choices_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int len = 0; + int i; + + mutex_lock(&profile_lock); + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -ENODEV; + } + + if (!cur_profile->choices) { + mutex_unlock(&profile_lock); + return sysfs_emit(buf, "\n"); + } + + for (i = profile_low; i <= profile_perform; 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; + int err; + + mutex_lock(&profile_lock); + if (!cur_profile) { + mutex_unlock(&profile_lock); + return -EOPNOTSUPP; + } + + 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; + + 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; + + mutex_lock(&profile_lock); + if (!cur_profile) { + 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; + } + + if (!cur_profile->profile_set) { + 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 +}; + +int platform_profile_notify(void) +{ + if (!cur_profile) + return -EOPNOTSUPP; + sysfs_notify(acpi_kobj, NULL, "platform_profile"); + return 0; +} +EXPORT_SYMBOL_GPL(platform_profile_notify); + +int platform_profile_register(struct platform_profile_handler *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_group); +} +EXPORT_SYMBOL_GPL(platform_profile_register); + +int platform_profile_unregister(void) +{ + mutex_lock(&profile_lock); + 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) +{ + 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..f6592434c8ce --- /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 { + profile_low, + profile_cool, + profile_quiet, + profile_balance, + 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(struct platform_profile_handler *pprof); +int platform_profile_unregister(void); +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> --- 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) Big thank you to reviewers for all the suggestions. Changes in v3: Add missed platform_profile.h file drivers/acpi/Kconfig | 33 ++++++ drivers/acpi/Makefile | 1 + drivers/acpi/platform_profile.c | 181 +++++++++++++++++++++++++++++++ include/linux/platform_profile.h | 36 ++++++ 4 files changed, 251 insertions(+) create mode 100644 drivers/acpi/platform_profile.c create mode 100644 include/linux/platform_profile.h