Re: [PATCH v8 13/13] Documentation: amd-pstate: introduce new global sysfs attributes

[Limonciello, Mario]

On 12/19/2022 00:40, Perry Yuan wrote:
> The new amd-pstate driver support to switch the driver working mode and

Words like "new" don't age well.

> use can switch the driver mode within the sysfs attributes in the below
> path and check current mode

Perhaps "The amd-pstate driver supports switching working modes at runtime.
Users can view and change modes by interacting with the "status" sysfs 
attribute.

> $ cd /sys/devices/system/cpu/amd-pstate

No need to change directories; you're demonstrating below using full paths.

> check driver mode:
> $ cat /sys/devices/system/cpu/amd-pstate/status
>
> switch mode:
> $ sudo bash -c "echo passive  > /sys/devices/system/cpu/amd-pstate/status"
> or
> $ sudo bash -c "echo active  > /sys/devices/system/cpu/amd-pstate/status"

Another way you could suggest this:

# echo "passive" | sudo tee /sys/devices/system/cpu/amd-pstate/status
or
# echo "active" | sudo tee /sys/devices/system/cpu/amd-pstate/status

I don't feel strongly which way to suggest though.

> Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
> ---
>   Documentation/admin-guide/pm/amd-pstate.rst | 28 +++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index 62744dae3c5f..f3a8f8a66783 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -339,6 +339,34 @@ processor must provide at least nominal performance requested and go higher if c
>   operating conditions allow.
>   
>   
> +User Space Interface in ``sysfs``
> +=================================
> +
> +Global Attributes
> +-----------------
> +
> +``amd-pstate`` exposes several global attributes (files) in ``sysfs`` to
> +control its functionality at the system level.  They are located in the
> +``/sys/devices/system/cpu/amd-pstate/`` directory and affect all CPUs.
> +
> +``status``
> +	Operation mode of the driver: "active", "passive" or "off".
> +
> +	"active"
> +		The driver is functional and in the ``active mode``
> +
> +	"passive"
> +		The driver is functional and in the ``passive mode``
> +
> +	"off"
> +		The driver is unregistered and not functional now.
> +
> +	This attribute can be written to in order to change the driver's
> +	operation mode or to unregister it.  The string written to it must be
> +	one of the possible values of it and, if successful, the write will

I think this is implied that you wrote a possible value and if it 
returns success something happens.  That's how all sysfs files work and 
it's needlessly wordy.

> +	cause the driver to switch over to the operation mode represented by
> +	that string - or to be unregistered in the "off" case.
Considering the implication of my above comment I think you can reword 
this as:

"Writing one of these values to the sysfs file will cause the driver to..."

> +
>   ``cpupower`` tool support for ``amd-pstate``
>   ===============================================
>