diff mbox series

[v6,2/3] ACPI: platform-profile: Add platform profile support

Message ID 20201211020630.305905-2-markpearson@lenovo.com (mailing list archive)
State Changes Requested, archived
Headers show
Series [v6,1/3] Documentation: Add documentation for new platform_profile sysfs attribute | expand

Commit Message

Mark Pearson Dec. 11, 2020, 2:06 a.m. UTC
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

Changed in v6:
 - Change default build option to 'm' and clean in formating in Kconfig
 - Change enums to be capitalised as requested
 - Rename unregister function to remove

 drivers/acpi/Kconfig             |  17 +++
 drivers/acpi/Makefile            |   1 +
 drivers/acpi/platform_profile.c  | 181 +++++++++++++++++++++++++++++++
 include/linux/platform_profile.h |  39 +++++++
 4 files changed, 238 insertions(+)
 create mode 100644 drivers/acpi/platform_profile.c
 create mode 100644 include/linux/platform_profile.h

Comments

Rafael J. Wysocki Dec. 16, 2020, 6:13 p.m. UTC | #1
On Fri, Dec 11, 2020 at 3:15 AM 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>

[cut]

> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> new file mode 100644
> index 000000000000..9a1e2abd7602
> --- /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);

I'm not sure why this callback is necessary and, provided that there
is a good enough reason, why it cannot return an enum
platform_profile_option value.

In principle, if ->profile_set() returns 0, the requested profile can
be saved in a static var and then returned by subsequent "read"
operations.

> +       int (*profile_set)(enum platform_profile_option profile);
> +};
> +
> +int platform_profile_register(const struct platform_profile_handler *pprof);
> +int platform_profile_remove(void);
> +void platform_profile_notify(void);
> +
> +#endif  /*_PLATFORM_PROFILE_H_*/
> --
Barnabás Pőcze Dec. 16, 2020, 6:42 p.m. UTC | #2
2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:

> On Fri, Dec 11, 2020 at 3:15 AM 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>
> [...]
> > +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);
>
> I'm not sure why this callback is necessary and, provided that there
> is a good enough reason, why it cannot return an enum
> platform_profile_option value.
>
> In principle, if ->profile_set() returns 0, the requested profile can
> be saved in a static var and then returned by subsequent "read"
> operations.
>

It is possible that the platform profile can be changed with (e.g.) a dedicated
button (commonly found on laptops), in which case there needs to be a mechanism
to retrieve the new profile, which would not be possible without introducing
something else in place of that getter - unless I'm missing something obvious.


Regards,
Barnabás Pőcze
Rafael J. Wysocki Dec. 16, 2020, 6:47 p.m. UTC | #3
On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
>
> > On Fri, Dec 11, 2020 at 3:15 AM 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>
> > [...]
> > > +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);
> >
> > I'm not sure why this callback is necessary and, provided that there
> > is a good enough reason, why it cannot return an enum
> > platform_profile_option value.
> >
> > In principle, if ->profile_set() returns 0, the requested profile can
> > be saved in a static var and then returned by subsequent "read"
> > operations.
> >
>
> It is possible that the platform profile can be changed with (e.g.) a dedicated
> button (commonly found on laptops), in which case there needs to be a mechanism
> to retrieve the new profile, which would not be possible without introducing
> something else in place of that getter - unless I'm missing something obvious.

Fair enough.

The other question remains, then.
Mark Pearson Dec. 16, 2020, 7:18 p.m. UTC | #4
Hi Rafael,

On 16/12/2020 13:47, Rafael J. Wysocki wrote:
> On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>
>> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
>>
>>> On Fri, Dec 11, 2020 at 3:15 AM 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>
>>> [...]
>>>> +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);
>>>
>>> I'm not sure why this callback is necessary and, provided that there
>>> is a good enough reason, why it cannot return an enum
>>> platform_profile_option value.
>>>
>>> In principle, if ->profile_set() returns 0, the requested profile can
>>> be saved in a static var and then returned by subsequent "read"
>>> operations.
>>>
>>
>> It is possible that the platform profile can be changed with (e.g.) a dedicated
>> button (commonly found on laptops), in which case there needs to be a mechanism
>> to retrieve the new profile, which would not be possible without introducing
>> something else in place of that getter - unless I'm missing something obvious.
> 
> Fair enough.
> 
> The other question remains, then.
> 
My thinking here that I shouldn't make assumptions for future platform
implementations - there may be valid cases in the future where being
able to return an error condition if there was an error would be useful.

Just trying to keep this somewhat future proof. Returning an error
condition seemed a useful thing to have available.

Mark
Rafael J. Wysocki Dec. 16, 2020, 7:50 p.m. UTC | #5
On Wed, Dec 16, 2020 at 8:19 PM Mark Pearson <markpearson@lenovo.com> wrote:
>
> Hi Rafael,
>
> On 16/12/2020 13:47, Rafael J. Wysocki wrote:
> > On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>
> >> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
> >>
> >>> On Fri, Dec 11, 2020 at 3:15 AM 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>
> >>> [...]
> >>>> +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);
> >>>
> >>> I'm not sure why this callback is necessary and, provided that there
> >>> is a good enough reason, why it cannot return an enum
> >>> platform_profile_option value.
> >>>
> >>> In principle, if ->profile_set() returns 0, the requested profile can
> >>> be saved in a static var and then returned by subsequent "read"
> >>> operations.
> >>>
> >>
> >> It is possible that the platform profile can be changed with (e.g.) a dedicated
> >> button (commonly found on laptops), in which case there needs to be a mechanism
> >> to retrieve the new profile, which would not be possible without introducing
> >> something else in place of that getter - unless I'm missing something obvious.
> >
> > Fair enough.
> >
> > The other question remains, then.
> >
> My thinking here that I shouldn't make assumptions for future platform
> implementations - there may be valid cases in the future where being
> able to return an error condition if there was an error would be useful.
>
> Just trying to keep this somewhat future proof. Returning an error
> condition seemed a useful thing to have available.

You can still return an error while returning a platform_profile_option value.

Just add a special value representing an error to that set.
Mark Pearson Dec. 16, 2020, 9:36 p.m. UTC | #6
On 16/12/2020 14:50, Rafael J. Wysocki wrote:
> On Wed, Dec 16, 2020 at 8:19 PM Mark Pearson <markpearson@lenovo.com> wrote:
>>
>> Hi Rafael,
>>
>> On 16/12/2020 13:47, Rafael J. Wysocki wrote:
>>> On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>>>
>>>> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
>>>>
>>>>> On Fri, Dec 11, 2020 at 3:15 AM 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>
>>>>> [...]
>>>>>> +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);
>>>>>
>>>>> I'm not sure why this callback is necessary and, provided that there
>>>>> is a good enough reason, why it cannot return an enum
>>>>> platform_profile_option value.
>>>>>
>>>>> In principle, if ->profile_set() returns 0, the requested profile can
>>>>> be saved in a static var and then returned by subsequent "read"
>>>>> operations.
>>>>>
>>>>
>>>> It is possible that the platform profile can be changed with (e.g.) a dedicated
>>>> button (commonly found on laptops), in which case there needs to be a mechanism
>>>> to retrieve the new profile, which would not be possible without introducing
>>>> something else in place of that getter - unless I'm missing something obvious.
>>>
>>> Fair enough.
>>>
>>> The other question remains, then.
>>>
>> My thinking here that I shouldn't make assumptions for future platform
>> implementations - there may be valid cases in the future where being
>> able to return an error condition if there was an error would be useful.
>>
>> Just trying to keep this somewhat future proof. Returning an error
>> condition seemed a useful thing to have available.
> 
> You can still return an error while returning a platform_profile_option value.
> 
> Just add a special value representing an error to that set.
> 
I'd like to understand what is better about that approach than having an
error and a returnable parameter?

I'm probably missing something obvious but if I add an extra
platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
error case (and return just an enum platform_profile_option) it seems I
lose the granularity of being able to return a specific error condition.
It doesn't feel like an improvement.

Let me know what I'm missing.

Thanks
Mark
Rafael J. Wysocki Dec. 17, 2020, 1:36 p.m. UTC | #7
On Wed, Dec 16, 2020 at 10:36 PM Mark Pearson <markpearson@lenovo.com> wrote:
>
>
> On 16/12/2020 14:50, Rafael J. Wysocki wrote:
> > On Wed, Dec 16, 2020 at 8:19 PM Mark Pearson <markpearson@lenovo.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >> On 16/12/2020 13:47, Rafael J. Wysocki wrote:
> >>> On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>>>
> >>>> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
> >>>>
> >>>>> On Fri, Dec 11, 2020 at 3:15 AM 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>
> >>>>> [...]
> >>>>>> +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);
> >>>>>
> >>>>> I'm not sure why this callback is necessary and, provided that there
> >>>>> is a good enough reason, why it cannot return an enum
> >>>>> platform_profile_option value.
> >>>>>
> >>>>> In principle, if ->profile_set() returns 0, the requested profile can
> >>>>> be saved in a static var and then returned by subsequent "read"
> >>>>> operations.
> >>>>>
> >>>>
> >>>> It is possible that the platform profile can be changed with (e.g.) a dedicated
> >>>> button (commonly found on laptops), in which case there needs to be a mechanism
> >>>> to retrieve the new profile, which would not be possible without introducing
> >>>> something else in place of that getter - unless I'm missing something obvious.
> >>>
> >>> Fair enough.
> >>>
> >>> The other question remains, then.
> >>>
> >> My thinking here that I shouldn't make assumptions for future platform
> >> implementations - there may be valid cases in the future where being
> >> able to return an error condition if there was an error would be useful.
> >>
> >> Just trying to keep this somewhat future proof. Returning an error
> >> condition seemed a useful thing to have available.
> >
> > You can still return an error while returning a platform_profile_option value.
> >
> > Just add a special value representing an error to that set.
> >
> I'd like to understand what is better about that approach than having an
> error and a returnable parameter?
>
> I'm probably missing something obvious but if I add an extra
> platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
> error case (and return just an enum platform_profile_option) it seems I
> lose the granularity of being able to return a specific error condition.
> It doesn't feel like an improvement.

And what's the user expected to do about the different error codes
that can be returned?
Mark Pearson Dec. 17, 2020, 2:38 p.m. UTC | #8
On 17/12/2020 08:36, Rafael J. Wysocki wrote:
> On Wed, Dec 16, 2020 at 10:36 PM Mark Pearson <markpearson@lenovo.com> wrote:
>>
>>
>> On 16/12/2020 14:50, Rafael J. Wysocki wrote:
>>> On Wed, Dec 16, 2020 at 8:19 PM Mark Pearson <markpearson@lenovo.com> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> On 16/12/2020 13:47, Rafael J. Wysocki wrote:
>>>>> On Wed, Dec 16, 2020 at 7:42 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>>>>>
>>>>>> 2020. december 16., szerda 19:13 keltezéssel, Rafael J. Wysocki írta:
>>>>>>
>>>>>>> On Fri, Dec 11, 2020 at 3:15 AM 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>
>>>>>>> [...]
>>>>>>>> +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);
>>>>>>>
>>>>>>> I'm not sure why this callback is necessary and, provided that there
>>>>>>> is a good enough reason, why it cannot return an enum
>>>>>>> platform_profile_option value.
>>>>>>>
>>>>>>> In principle, if ->profile_set() returns 0, the requested profile can
>>>>>>> be saved in a static var and then returned by subsequent "read"
>>>>>>> operations.
>>>>>>>
>>>>>>
>>>>>> It is possible that the platform profile can be changed with (e.g.) a dedicated
>>>>>> button (commonly found on laptops), in which case there needs to be a mechanism
>>>>>> to retrieve the new profile, which would not be possible without introducing
>>>>>> something else in place of that getter - unless I'm missing something obvious.
>>>>>
>>>>> Fair enough.
>>>>>
>>>>> The other question remains, then.
>>>>>
>>>> My thinking here that I shouldn't make assumptions for future platform
>>>> implementations - there may be valid cases in the future where being
>>>> able to return an error condition if there was an error would be useful.
>>>>
>>>> Just trying to keep this somewhat future proof. Returning an error
>>>> condition seemed a useful thing to have available.
>>>
>>> You can still return an error while returning a platform_profile_option value.
>>>
>>> Just add a special value representing an error to that set.
>>>
>> I'd like to understand what is better about that approach than having an
>> error and a returnable parameter?
>>
>> I'm probably missing something obvious but if I add an extra
>> platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
>> error case (and return just an enum platform_profile_option) it seems I
>> lose the granularity of being able to return a specific error condition.
>> It doesn't feel like an improvement.
> 
> And what's the user expected to do about the different error codes
> that can be returned?
> 

From my experiences when something fails I would rather have more
information than less. I'm probably over thinking it though.

Looking at it again, from my Lenovo platform profile implementation
perspective I have no objections. It's hard to argue a point without
having a hard requirement or example :) I'll go ahead with your
suggestion of just returning the profile.

Thank you for the review
Mark
Barnabás Pőcze Dec. 17, 2020, 3:02 p.m. UTC | #9
2020. december 17., csütörtök 14:36 keltezéssel, Rafael J. Wysocki <rafael@kernel.org> írta:

> [...]
> > >> My thinking here that I shouldn't make assumptions for future platform
> > >> implementations - there may be valid cases in the future where being
> > >> able to return an error condition if there was an error would be useful.
> > >>
> > >> Just trying to keep this somewhat future proof. Returning an error
> > >> condition seemed a useful thing to have available.
> > >
> > > You can still return an error while returning a platform_profile_option value.
> > >
> > > Just add a special value representing an error to that set.
> > >
> > I'd like to understand what is better about that approach than having an
> > error and a returnable parameter?
> >
> > I'm probably missing something obvious but if I add an extra
> > platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
> > error case (and return just an enum platform_profile_option) it seems I
> > lose the granularity of being able to return a specific error condition.
> > It doesn't feel like an improvement.
>
> And what's the user expected to do about the different error codes
> that can be returned?

Even assuming the users of the API cannot or will not handle different errors
differently, who would benefit from getting rid of this information which may be
an aid in debugging for those who know what they're looking at?

And if a new enum value is introduced to signal an error condition, how is that
going to be communicated to the users? A magic string like "error" or "-1"?
Or under a single errno like -EIO? Personally, I believe neither of these two
solutions are better than returning the actual errno which is deemed most
appropriate by the platform profile handler. Or am I missing a third way?


Regards,
Barnabás Pőcze
Rafael J. Wysocki Dec. 17, 2020, 3:09 p.m. UTC | #10
On Thu, Dec 17, 2020 at 4:02 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> 2020. december 17., csütörtök 14:36 keltezéssel, Rafael J. Wysocki <rafael@kernel.org> írta:
>
> > [...]
> > > >> My thinking here that I shouldn't make assumptions for future platform
> > > >> implementations - there may be valid cases in the future where being
> > > >> able to return an error condition if there was an error would be useful.
> > > >>
> > > >> Just trying to keep this somewhat future proof. Returning an error
> > > >> condition seemed a useful thing to have available.
> > > >
> > > > You can still return an error while returning a platform_profile_option value.
> > > >
> > > > Just add a special value representing an error to that set.
> > > >
> > > I'd like to understand what is better about that approach than having an
> > > error and a returnable parameter?
> > >
> > > I'm probably missing something obvious but if I add an extra
> > > platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
> > > error case (and return just an enum platform_profile_option) it seems I
> > > lose the granularity of being able to return a specific error condition.
> > > It doesn't feel like an improvement.
> >
> > And what's the user expected to do about the different error codes
> > that can be returned?
>
> Even assuming the users of the API cannot or will not handle different errors
> differently, who would benefit from getting rid of this information which may be
> an aid in debugging for those who know what they're looking at?
>
> And if a new enum value is introduced to signal an error condition, how is that
> going to be communicated to the users? A magic string like "error" or "-1"?
> Or under a single errno like -EIO?

Yes.

> Personally, I believe neither of these two
> solutions are better than returning the actual errno which is deemed most
> appropriate by the platform profile handler. Or am I missing a third way?

Can we please defer this until we actually have a driver needing to
return different error values from ->get_profile() (and I find it hard
to believe that there will be any drivers like that)?

Let's do the simpler thing until we have a real need to do the more
complicated one.

Otherwise we're preparing for things that may never happen.
Maximilian Luz Dec. 17, 2020, 3:18 p.m. UTC | #11
On 12/17/20 4:09 PM, Rafael J. Wysocki wrote:
> On Thu, Dec 17, 2020 at 4:02 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>
>> 2020. december 17., csütörtök 14:36 keltezéssel, Rafael J. Wysocki <rafael@kernel.org> írta:
>>
>>> [...]
>>>>>> My thinking here that I shouldn't make assumptions for future platform
>>>>>> implementations - there may be valid cases in the future where being
>>>>>> able to return an error condition if there was an error would be useful.
>>>>>>
>>>>>> Just trying to keep this somewhat future proof. Returning an error
>>>>>> condition seemed a useful thing to have available.
>>>>>
>>>>> You can still return an error while returning a platform_profile_option value.
>>>>>
>>>>> Just add a special value representing an error to that set.
>>>>>
>>>> I'd like to understand what is better about that approach than having an
>>>> error and a returnable parameter?
>>>>
>>>> I'm probably missing something obvious but if I add an extra
>>>> platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
>>>> error case (and return just an enum platform_profile_option) it seems I
>>>> lose the granularity of being able to return a specific error condition.
>>>> It doesn't feel like an improvement.
>>>
>>> And what's the user expected to do about the different error codes
>>> that can be returned?
>>
>> Even assuming the users of the API cannot or will not handle different errors
>> differently, who would benefit from getting rid of this information which may be
>> an aid in debugging for those who know what they're looking at?
>>
>> And if a new enum value is introduced to signal an error condition, how is that
>> going to be communicated to the users? A magic string like "error" or "-1"?
>> Or under a single errno like -EIO?
> 
> Yes.
> 
>> Personally, I believe neither of these two
>> solutions are better than returning the actual errno which is deemed most
>> appropriate by the platform profile handler. Or am I missing a third way?
> 
> Can we please defer this until we actually have a driver needing to
> return different error values from ->get_profile() (and I find it hard
> to believe that there will be any drivers like that)?

I can maybe give an example. On Microsoft Surface devices, the
performance mode / platform profile is set via a request to the embedded
controller and can be queried the same way. This request can fail (most
notably with ETIMEDOUT and EREMOTEIO). I think at least being able to
show users an error in this case would be helpful.

A driver for that is currently at [1], but I haven't adapted that yet to
the platform profile patchset.

On the other hand, I can also query and store the profile when loading
the driver and then update it when it's changed. That'd require that no
one else changes the profile, but I think we can safely assume that.

[1]: https://github.com/linux-surface/surface-aggregator-module/blob/master/module/src/clients/surface_perfmode.c,

Regards,
Max

> 
> Let's do the simpler thing until we have a real need to do the more
> complicated one.
> 
> Otherwise we're preparing for things that may never happen.
>
Rafael J. Wysocki Dec. 17, 2020, 3:20 p.m. UTC | #12
On Thu, Dec 17, 2020 at 4:18 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
>
>
> On 12/17/20 4:09 PM, Rafael J. Wysocki wrote:
> > On Thu, Dec 17, 2020 at 4:02 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>
> >> 2020. december 17., csütörtök 14:36 keltezéssel, Rafael J. Wysocki <rafael@kernel.org> írta:
> >>
> >>> [...]
> >>>>>> My thinking here that I shouldn't make assumptions for future platform
> >>>>>> implementations - there may be valid cases in the future where being
> >>>>>> able to return an error condition if there was an error would be useful.
> >>>>>>
> >>>>>> Just trying to keep this somewhat future proof. Returning an error
> >>>>>> condition seemed a useful thing to have available.
> >>>>>
> >>>>> You can still return an error while returning a platform_profile_option value.
> >>>>>
> >>>>> Just add a special value representing an error to that set.
> >>>>>
> >>>> I'd like to understand what is better about that approach than having an
> >>>> error and a returnable parameter?
> >>>>
> >>>> I'm probably missing something obvious but if I add an extra
> >>>> platform_profile option (e.g PLATFORM_PROFILE_ERROR) to be used in an
> >>>> error case (and return just an enum platform_profile_option) it seems I
> >>>> lose the granularity of being able to return a specific error condition.
> >>>> It doesn't feel like an improvement.
> >>>
> >>> And what's the user expected to do about the different error codes
> >>> that can be returned?
> >>
> >> Even assuming the users of the API cannot or will not handle different errors
> >> differently, who would benefit from getting rid of this information which may be
> >> an aid in debugging for those who know what they're looking at?
> >>
> >> And if a new enum value is introduced to signal an error condition, how is that
> >> going to be communicated to the users? A magic string like "error" or "-1"?
> >> Or under a single errno like -EIO?
> >
> > Yes.
> >
> >> Personally, I believe neither of these two
> >> solutions are better than returning the actual errno which is deemed most
> >> appropriate by the platform profile handler. Or am I missing a third way?
> >
> > Can we please defer this until we actually have a driver needing to
> > return different error values from ->get_profile() (and I find it hard
> > to believe that there will be any drivers like that)?
>
> I can maybe give an example. On Microsoft Surface devices, the
> performance mode / platform profile is set via a request to the embedded
> controller and can be queried the same way. This request can fail (most
> notably with ETIMEDOUT and EREMOTEIO). I think at least being able to
> show users an error in this case would be helpful.

OK, fair enough, let's keep it the way it is.
diff mbox series

Patch

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index edf1558c1105..5ddff93e38c2 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -326,6 +326,23 @@  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 m
+	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..567b2320693a
--- /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_remove(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_remove);
+
+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..9a1e2abd7602
--- /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_remove(void);
+void platform_profile_notify(void);
+
+#endif  /*_PLATFORM_PROFILE_H_*/