Re: [External] Re: [PATCH v5 2/3] ACPI: platform-profile: Add platform profile support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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_*/
> >> --



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux