On Tue, Dec 8, 2020 at 7:54 PM Mark Pearson <markpearson@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>");
> >> +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_*/
> >> --
