Re: [External] Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute

[Mark Pearson <markpearson@lenovo.com>]

Thanks Hans and Bastien,

On 28/10/2020 13:23, Hans de Goede wrote:
> Hi,
> 
> On 10/28/20 2:45 PM, Bastien Nocera wrote:
>> Hey Hans, Mark,
>>
>> On Tue, 2020-10-27 at 12:42 -0400, Mark Pearson wrote:
>>> From: Hans de Goede <hdegoede@redhat.com>
>>>
>>> On modern systems the platform performance, temperature, fan and
>>> other
>>> hardware related characteristics are often dynamically configurable.
>>> The
>>> profile is often automatically adjusted to the load by somei
>>> automatic-mechanism (which may very well live outside the kernel).
>>>
>>> These auto platform-adjustment mechanisms often can be configured
>>> with
>>> one of several 'platform-profiles', with either a bias towards low-
>>> power
>>
>> Can you please make sure to quote 'platform-profile' and 'profile-name'
>> this way all through the document? They're not existing words, and
>> quoting them shows that they're attribute names, rather than English.
I'm leaning towards changing these to become "platform profile" and 
"profile name" (no quotes in the actual text). Any objections?

>>
>>> consumption or towards performance (and higher power consumption and
>>> thermals).
>>
>> s/thermal/temperature/
>>
>> "A thermal" is something else (it's seasonal underwear for me ;)
I'm removing that sentence from an earlier review so it's moot, but 
enjoy your underwear! (which reminds me that I need a new set of 
thermals for the winter...)

>>
>>> Introduce a new platform_profile sysfs API which offers a generic API
>>> for
>>> selecting the performance-profile of these automatic-mechanisms.
>>>
>>> Co-developed-by: Mark Pearson <markpearson@lenovo.com>
>>> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in V1:
>>>   - Moved from RFC to proposed patch
>>>   - Added cool profile as requested
>>>   - removed extra-profiles as no longer relevant
>>>
>>>   .../ABI/testing/sysfs-platform_profile        | 66
>>> +++++++++++++++++++
>>>   1 file changed, 66 insertions(+)
>>>   create mode 100644 Documentation/ABI/testing/sysfs-platform_profile
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-platform_profile
>>> b/Documentation/ABI/testing/sysfs-platform_profile
>>> new file mode 100644
>>> index 000000000000..240bd3d7532b
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-platform_profile
>>> @@ -0,0 +1,66 @@
>>> +Platform-profile selection (e.g.
>>> /sys/firmware/acpi/platform_profile)
>>> +
>>> +On modern systems the platform performance, temperature, fan and
>>> other
>>> +hardware related characteristics are often dynamically configurable.
>>> The
>>> +profile is often automatically adjusted to the load by some
>>> +automatic-mechanism (which may very well live outside the kernel).
>>> +
>>> +These auto platform-adjustment mechanisms often can be configured
>>> with
>>> +one of several 'platform-profiles', with either a bias towards low-
>>> power
>>> +consumption or towards performance (and higher power consumption and
>>> +thermals).
>>> +
>>> +The purpose of the platform_profile attribute is to offer a generic
>>> sysfs
>>> +API for selecting the platform-profile of these automatic-
>>> mechanisms.
>>> +
>>> +Note that this API is only for selecting the platform-profile, it is
>>> +NOT a goal of this API to allow monitoring the resulting performance
>>> +characteristics. Monitoring performance is best done with
>>> device/vendor
>>> +specific tools such as e.g. turbostat.
>>> +
>>> +Specifically when selecting a high-performance profile the actual
>>> achieved
>>> +performance may be limited by various factors such as: the heat
>>> generated
>>> +by other components, room temperature, free air flow at the bottom
>>> of a
>>> +laptop, etc. It is explicitly NOT a goal of this API to let
>>> userspace know
>>> +about any sub-optimal conditions which are impeding reaching the
>>> requested
>>> +performance level.
>>> +
>>> +Since numbers are a rather meaningless way to describe platform-
>>> profiles
>>
>> It's not meaningless, but rather ambiguous. For a range of 1 to 5, is 1
>> high performance, and 5 low power, or vice-versa?
> 
> It is meaningless because the space we are trying to describe with the
> profile-names is not 1 dimensional. E.g. as discussed before cool and
> low-power are not necessarily the same thing. If you have a better way
> to word this I'm definitely in favor of improving the text here.

I'm good with 'ambiguous' here as numbers are (interestingly) ambiguous. 
I've not thought of anything better
Any objections?

> 
>>
>>> +this API uses strings to describe the various profiles. To make sure
>>> that
>>> +userspace gets a consistent experience when using this API this API
>>
>> you can remove "when using this API".
Ack
>>
>>> +document defines a fixed set of profile-names. Drivers *must* map
>>> their
>>> +internal profile representation/names onto this fixed set.
>>> +
>>> +If for some reason there is no good match when mapping then a new
>>> profile-name
>>> +may be added.
>>
>> "for some reason" can be removed.
Ack
>>
>>>   Drivers which wish to introduce new profile-names must:
>>> +1. Have very good reasons to do so.
>>
>> "1. Explain why the existing 'profile-names' cannot be used"
>>
>>> +2. Add the new profile-name to this document, so that future drivers
>>> which also
>>> +   have a similar problem can use the same name.
>>
>> "2. Add the new 'profile-name' to the documentation so that other
>> drivers can use it, as well as user-space knowing clearly what
>> behaviour the 'profile-name' corresponds to"
How about just :
"2. Add the new profile name, along with a clear description of the 
behaviour, to the documentation."

It should be clear for all 'consumers' - regardless of origin
>>
>>> +
>>> +What:          /sys/firmware/acpi/platform_profile_choices
>>> +Date:          October 2020
>>> +Contact:       Hans de Goede <hdegoede@redhat.com>
>>> +Description:
>>> +               Reading this file gives a space separated list of
>>> profiles
>>> +               supported for this device.
>>
>> "This file contains a space-separated list of profiles..."
Ack
>>
>>> +
>>> +               Drivers must use the following standard profile-
>>> names:
>>> +
>>> +               low-power:              Emphasises low power
>>> consumption
>>> +               cool:                   Emphasises cooler operation
>>> +               quiet:                  Emphasises quieter operation
>>> +               balanced:               Balance between low power
>>> consumption
>>> +                                       and performance
>>> +               performance:            Emphasises performance (and
>>> may lead to
>>> +                                       higher temperatures and fan
>>> speeds)
>>
>> I'd replace "Emphasises" with either "Focus on" or the US English
>> spelling of "Emphasizes".
Darn - Google confirms that Emphasizes is more correct now. For some 
reason that's slightly disappointing :)
Ack.
>>
>>> +               Userspace may expect drivers to offer at least
>>> several of these
>>> +               standard profile-names.
>>
>> Replce "at least several" with "more than one".
Ack
>>
>>> +
>>> +What:          /sys/firmware/acpi/platform_profile
>>> +Date:          October 2020
>>> +Contact:       Hans de Goede <hdegoede@redhat.com>
>>> +Description:
>>> +               Reading this file gives the current selected profile
>>> for this
>>> +               device. Writing this file with one of the strings
>>> from
>>> +               available_profiles changes the profile to the new
>>> value.
>>
>> Is there another file which explains whether those sysfs value will
>> contain a trailing linefeed?
> 
> sysfs APIs are typically created so that they can be used from the shell,
> so on read a newline will be added. On write a newline at the end
> typically is allowed, but ignored. There are even special helper functions
> to deal with properly ignoring the newline on write.
> 
> Regards,
> 
> Hans
> 
> 
OK - does that need to actually be specified here? Or is that just 
something I keep in mind for the implementation?

Mark