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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bc79f3dd-425f-8f91-bccb-69bf263ed8b2@amd.com>
Date:   Mon, 19 Dec 2022 16:35:11 -0600
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     Perry Yuan <perry.yuan@....com>, rafael.j.wysocki@...el.com,
        ray.huang@....com, viresh.kumar@...aro.org
Cc:     Deepak.Sharma@....com, Nathan.Fontenot@....com,
        Alexander.Deucher@....com, Shimmer.Huang@....com,
        Xiaojian.Du@....com, Li.Meng@....com, wyes.karny@....com,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 13/13] Documentation: amd-pstate: introduce new global
 sysfs attributes

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@....com>
> ---
>   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``
>   ===============================================
>