* Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute
2020-10-27 16:42 [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute Mark Pearson
@ 2020-10-27 18:11 ` Elia Devito
2020-10-28 11:54 ` Hans de Goede
2020-10-28 13:45 ` Bastien Nocera
2 siblings, 0 replies; 10+ messages in thread
From: Elia Devito @ 2020-10-27 18:11 UTC (permalink / raw)
To: markpearson, Mark Pearson
Cc: dvhart, mgross, mario.limonciello, hadess, bberg, linux-pm,
linux-acpi, platform-driver-x86, linux-kernel, Hans de Goede
Hi to all,
In data martedì 27 ottobre 2020 17:42:19 CET, Mark Pearson ha scritto:
> 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
> consumption or towards performance (and higher power consumption and
> thermals).
>
> 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
> +this API uses strings to describe the various profiles. To make sure that
> +userspace gets a consistent experience when using this API this API
> +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. Drivers which wish to introduce new
> profile-names must: +1. Have very good reasons to do so.
> +2. Add the new profile-name to this document, so that future drivers which
> also + have a similar problem can use the same name.
> +
> +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.
> +
> + 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)
> +
> + Userspace may expect drivers to offer at least several of
these
> + standard profile-names.
> +
> +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.
> --
> 2.28.0
From my perspective now is perfect, thanks to all for the work.
Regards
Elia
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute
2020-10-27 16:42 [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute Mark Pearson
2020-10-27 18:11 ` Elia Devito
@ 2020-10-28 11:54 ` Hans de Goede
2020-10-28 13:45 ` Bastien Nocera
2 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2020-10-28 11:54 UTC (permalink / raw)
To: Mark Pearson
Cc: dvhart, mgross, mario.limonciello, eliadevito, hadess, bberg,
linux-pm, linux-acpi, platform-driver-x86, linux-kernel
Hi,
A few minor nitpicks below, mostly stuff which I missed before, sorry.
I suggest you make v2 part of the series where you actually add the
drivers/acpi/... and the thinkpad_acpi.c bits to implement this.
On 10/27/20 5:42 PM, 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
s/somei/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).
I think it is better to also drop the " (and higher power consumption and
thermals)" bit here (and below) like you did for the cool and quiet parts.
Regards,
Hans
> 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
> +this API uses strings to describe the various profiles. To make sure that
> +userspace gets a consistent experience when using this API this API
> +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. Drivers which wish to introduce new profile-names must:
> +1. Have very good reasons to do so.
> +2. Add the new profile-name to this document, so that future drivers which also
> + have a similar problem can use the same name.
> +
> +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.
> +
> + 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)
> +
> + Userspace may expect drivers to offer at least several of these
> + standard profile-names.
> +
> +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.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute
2020-10-27 16:42 [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute Mark Pearson
2020-10-27 18:11 ` Elia Devito
2020-10-28 11:54 ` Hans de Goede
@ 2020-10-28 13:45 ` Bastien Nocera
2020-10-28 17:23 ` Hans de Goede
2020-10-29 11:25 ` Rafael J. Wysocki
2 siblings, 2 replies; 10+ messages in thread
From: Bastien Nocera @ 2020-10-28 13:45 UTC (permalink / raw)
To: Mark Pearson
Cc: dvhart, mgross, mario.limonciello, eliadevito, bberg, linux-pm,
linux-acpi, platform-driver-x86, linux-kernel, Hans de Goede
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.
> consumption or towards performance (and higher power consumption and
> thermals).
s/thermal/temperature/
"A thermal" is something else (it's seasonal underwear for me ;)
> 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?
> +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".
> +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.
> 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"
> +
> +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..."
> +
> + 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".
> + Userspace may expect drivers to offer at least
> several of these
> + standard profile-names.
Replce "at least several" with "more than one".
> +
> +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?
Cheers
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute
2020-10-28 13:45 ` Bastien Nocera
@ 2020-10-28 17:23 ` Hans de Goede
2020-10-29 0:55 ` [External] " Mark Pearson
2020-10-29 12:33 ` Bastien Nocera
2020-10-29 11:25 ` Rafael J. Wysocki
1 sibling, 2 replies; 10+ messages in thread
From: Hans de Goede @ 2020-10-28 17:23 UTC (permalink / raw)
To: Bastien Nocera, Mark Pearson
Cc: dvhart, mgross, mario.limonciello, eliadevito, bberg, linux-pm,
linux-acpi, platform-driver-x86, linux-kernel
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.
>
>> consumption or towards performance (and higher power consumption and
>> thermals).
>
> s/thermal/temperature/
>
> "A thermal" is something else (it's seasonal underwear for me ;)
>
>> 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.
>
>> +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".
>
>> +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.
>
>> 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"
>
>> +
>> +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..."
>
>> +
>> + 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".
>
>> + Userspace may expect drivers to offer at least
>> several of these
>> + standard profile-names.
>
> Replce "at least several" with "more than one".
>
>> +
>> +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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute
2020-10-28 17:23 ` Hans de Goede
@ 2020-10-29 0:55 ` Mark Pearson
2020-10-29 9:46 ` Hans de Goede
2020-10-29 12:33 ` Bastien Nocera
1 sibling, 1 reply; 10+ messages in thread
From: Mark Pearson @ 2020-10-29 0:55 UTC (permalink / raw)
To: Hans de Goede, Bastien Nocera
Cc: dvhart, mgross, mario.limonciello, eliadevito, bberg, linux-pm,
linux-acpi, platform-driver-x86, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute
2020-10-29 0:55 ` [External] " Mark Pearson
@ 2020-10-29 9:46 ` Hans de Goede
2020-10-29 14:21 ` Bastien Nocera
0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2020-10-29 9:46 UTC (permalink / raw)
To: Mark Pearson, Bastien Nocera
Cc: dvhart, mgross, mario.limonciello, eliadevito, bberg, linux-pm,
linux-acpi, platform-driver-x86, linux-kernel
Hi,
On 10/29/20 1:55 AM, Mark Pearson wrote:
> Thanks Hans and Bastien,
>
> On 28/10/2020 13:23, Hans de Goede wrote:
<big snip>
>>> 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?
IMHO it does not belong in the sysfs API docs for the platform_profile
stuff. But I guess it would be good to document it somewhere in some
generic syfs API rules/expectations document (with a note that their
might be exceptions).
Ideally we would already have such a file somewhere, but I don't know
if we do (I did not look). So if you feel like it (and such a file does
not exist yet) then I guess a patch adding such a doc file would be good.
Regards,
Hans
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [External] Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute
2020-10-29 9:46 ` Hans de Goede
@ 2020-10-29 14:21 ` Bastien Nocera
0 siblings, 0 replies; 10+ messages in thread
From: Bastien Nocera @ 2020-10-29 14:21 UTC (permalink / raw)
To: Hans de Goede, Mark Pearson
Cc: dvhart, mgross, mario.limonciello, eliadevito, bberg, linux-pm,
linux-acpi, platform-driver-x86, linux-kernel
On Thu, 2020-10-29 at 10:46 +0100, Hans de Goede wrote:
> <
> <snip>
> IMHO it does not belong in the sysfs API docs for the
> platform_profile
> stuff. But I guess it would be good to document it somewhere in some
> generic syfs API rules/expectations document (with a note that their
> might be exceptions).
>
> Ideally we would already have such a file somewhere, but I don't know
> if we do (I did not look). So if you feel like it (and such a file
> does
> not exist yet) then I guess a patch adding such a doc file would be
> good.
I don't know enough about the helpers and the code around it to know
whether documenting this would be needed, but I'm fine with knowing
that we're not breaking new ground here.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute
2020-10-28 17:23 ` Hans de Goede
2020-10-29 0:55 ` [External] " Mark Pearson
@ 2020-10-29 12:33 ` Bastien Nocera
1 sibling, 0 replies; 10+ messages in thread
From: Bastien Nocera @ 2020-10-29 12:33 UTC (permalink / raw)
To: Hans de Goede, Mark Pearson
Cc: dvhart, mgross, mario.limonciello, eliadevito, bberg, linux-pm,
linux-acpi, platform-driver-x86, linux-kernel
On Wed, 2020-10-28 at 18:23 +0100, Hans de Goede wrote:
>
> > 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.
What do you think of:
> +Since numbers are a rather meaningless way to describe platform-
profiles
"Since numbers on their own cannot represent the multiple variables
that a profile will adjust (power consumption, heat generation, etc.)
..."
> +this API uses strings to describe the various profiles. To make sure that
> +userspace gets a consistent experience when using this API this API
> +document defines a fixed set of profile-names. Drivers *must* map their
> +internal profile representation/names onto this fixed set.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Documentation: Add documentation for new platform_profile sysfs attribute
2020-10-28 13:45 ` Bastien Nocera
2020-10-28 17:23 ` Hans de Goede
@ 2020-10-29 11:25 ` Rafael J. Wysocki
1 sibling, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2020-10-29 11:25 UTC (permalink / raw)
To: Bastien Nocera, Mark Pearson
Cc: Darren Hart, Mark Gross, Mario Limonciello, Elia Devito,
Benjamin Berg, Linux PM, ACPI Devel Maling List, Platform Driver,
Linux Kernel Mailing List, Hans de Goede
On Thu, Oct 29, 2020 at 2:53 AM Bastien Nocera <hadess@hadess.net> 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
Strictly speaking, power is a rate, so it cannot be consumed.
So I would say "low-power operation" here.
> 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.
>
> > consumption or towards performance (and higher power consumption and
And analogously here.
> > thermals).
>
> s/thermal/temperature/
Right.
> "A thermal" is something else (it's seasonal underwear for me ;)
>
> > 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 just is not useful. :-)
> > +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".
>
> > +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.
Right.
> > 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"
>
> > +
> > +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..."
>
> > +
> > + Drivers must use the following standard profile-
> > names:
> > +
> > + low-power: Emphasises low power
> > consumption
Let's be precise, so "low-power operation", please (see above for the reason).
> > + cool: Emphasises cooler operation
> > + quiet: Emphasises quieter operation
> > + balanced: Balance between low power
> > consumption
And same here and analogously everywhere.
> > + 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".
>
> > + Userspace may expect drivers to offer at least
> > several of these
> > + standard profile-names.
>
> Replce "at least several" with "more than one".
>
> > +
> > +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?
>
> Cheers
>
^ permalink raw reply [flat|nested] 10+ messages in thread