| 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
| ||
|
Date: Tue, 31 Jan 2023 07:45:27 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Wyes Karny <wyes.karny@....com>,
Rafael J Wysocki <rafael@...nel.org>,
Huang Rui <ray.huang@....com>,
Jonathan Corbet <corbet@....net>,
Viresh Kumar <viresh.kumar@...aro.org>, Perry.Yuan@....com
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org, Bagas Sanjaya <bagasdotme@...il.com>,
santosh.shukla@....com, Len Brown <lenb@...nel.org>,
Robert Moore <robert.moore@...el.com>,
Borislav Petkov <bp@...en8.de>,
Ananth Narayan <ananth.narayan@....com>,
gautham.shenoy@....com, Tor Vic <torvic9@...lbox.org>
Subject: Re: [PATCH v4 5/6] cpufreq: amd_pstate: Add guided mode control
support via sysfs
On 1/30/23 23:21, Wyes Karny wrote:
> amd_pstate driver's `status` sysfs entry helps to control the driver's
> mode dynamically by user. After the addition of guided mode the
> combinations of mode transitions have been increased (16 combinations).
> Therefore optimise the amd_pstate_update_status function by implementing
> a state transition table.
>
> There are 4 states amd_pstate supports, namely: 'disable', 'passive',
> 'active', and 'guided'. The transition from any state to any other
> state is possible after this change. Only if the state requested matches
> with the current state then -EBUSY value is returned.
I realized this after I finished reviewing doc patch, but you probably
want to explain -EBUSY return code in documentation patch too.
>
> Sysfs interface:
>
> To disable amd_pstate driver:
> # echo disable > /sys/devices/system/cpu/amd_pstate/status
>
> To enable passive mode:
> # echo passive > /sys/devices/system/cpu/amd_pstate/status
>
> To change mode to active:
> # echo active > /sys/devices/system/cpu/amd_pstate/status
>
> To change mode to guided:
> # echo guided > /sys/devices/system/cpu/amd_pstate/status
>
> Signed-off-by: Wyes Karny <wyes.karny@....com>
> ---
> drivers/cpufreq/amd-pstate.c | 150 +++++++++++++++++++++++++----------
> 1 file changed, 108 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 48ab4684c3a5..6c522dec6967 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -65,6 +65,8 @@ static struct cpufreq_driver amd_pstate_epp_driver;
> static int cppc_state = AMD_PSTATE_DISABLE;
> struct kobject *amd_pstate_kobj;
>
> +typedef int (*cppc_mode_transition_fn)(int);
> +
> static inline int get_mode_idx_from_str(const char *str, size_t size)
> {
> int i;
> @@ -797,6 +799,105 @@ static ssize_t show_energy_performance_preference(
> return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]);
> }
>
> +static void amd_pstate_driver_cleanup(void)
> +{
> + amd_pstate_enable(false);
> + cppc_state = AMD_PSTATE_DISABLE;
> + current_pstate_driver = NULL;
> +}
> +
> +static int amd_pstate_register_driver(int mode)
> +{
> + int ret;
> +
> + if (mode == AMD_PSTATE_PASSIVE || mode == AMD_PSTATE_GUIDED)
> + current_pstate_driver = &amd_pstate_driver;
> + else if (mode == AMD_PSTATE_ACTIVE)
> + current_pstate_driver = &amd_pstate_epp_driver;
> + else
> + return -EINVAL;
> +
> + cppc_state = mode;
> + ret = cpufreq_register_driver(current_pstate_driver);
> + if (ret) {
> + amd_pstate_driver_cleanup();
> + return ret;
> + }
> + return 0;
> +}
> +
> +static int amd_pstate_unregister_driver(int dummy)
> +{
> + int ret;
> +
> + ret = cpufreq_unregister_driver(current_pstate_driver);
> +
> + if (ret)
> + return ret;
> +
> + amd_pstate_driver_cleanup();
> + return 0;
> +}
> +
> +static int amd_pstate_change_mode_without_dvr_change(int mode)
> +{
> + int cpu = 0;
> +
> + cppc_state = mode;
> +
> + if (boot_cpu_has(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE)
> + return 0;
> +
> + for_each_present_cpu(cpu) {
> + cppc_set_auto_sel(cpu, (cppc_state == AMD_PSTATE_PASSIVE) ? 0 : 1);
> + }
> +
> + return 0;
> +}
> +
> +static int amd_pstate_change_driver_mode(int mode)
> +{
> + int ret;
> +
> + ret = amd_pstate_unregister_driver(0);
> + if (ret)
> + return ret;
> +
> + ret = amd_pstate_register_driver(mode);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +/* Mode transition table */
This seems to be a pointless comment to me.
> +cppc_mode_transition_fn mode_state_machine[AMD_PSTATE_MAX][AMD_PSTATE_MAX] = {
> + [AMD_PSTATE_DISABLE] = {
> + [AMD_PSTATE_DISABLE] = NULL,
> + [AMD_PSTATE_PASSIVE] = amd_pstate_register_driver,
> + [AMD_PSTATE_ACTIVE] = amd_pstate_register_driver,
> + [AMD_PSTATE_GUIDED] = amd_pstate_register_driver,
> + },
> + [AMD_PSTATE_PASSIVE] = {
> + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver,
> + [AMD_PSTATE_PASSIVE] = NULL,
> + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode,
> + [AMD_PSTATE_GUIDED] = amd_pstate_change_mode_without_dvr_change,
> + },
> + [AMD_PSTATE_ACTIVE] = {
> + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver,
> + [AMD_PSTATE_PASSIVE] = amd_pstate_change_driver_mode,
> + [AMD_PSTATE_ACTIVE] = NULL,
> + [AMD_PSTATE_GUIDED] = amd_pstate_change_driver_mode,
> + },
> + [AMD_PSTATE_GUIDED] = {
> + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver,
> + [AMD_PSTATE_PASSIVE] = amd_pstate_change_mode_without_dvr_change,
> + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode,
> + [AMD_PSTATE_GUIDED] = NULL,
> + },
> +};
> +
> static ssize_t amd_pstate_show_status(char *buf)
> {
> if (!current_pstate_driver)
> @@ -805,57 +906,22 @@ static ssize_t amd_pstate_show_status(char *buf)
> return sysfs_emit(buf, "%s\n", amd_pstate_mode_string[cppc_state]);
> }
>
> -static void amd_pstate_driver_cleanup(void)
> -{
> - current_pstate_driver = NULL;
> -}
> -
> static int amd_pstate_update_status(const char *buf, size_t size)
> {
> - int ret;
> int mode_idx;
>
> - if (size > 7 || size < 6)
> + if (size > strlen("passive") || size < strlen("active"))
> return -EINVAL;
> - mode_idx = get_mode_idx_from_str(buf, size);
>
> - switch(mode_idx) {
> - case AMD_PSTATE_DISABLE:
> - if (!current_pstate_driver)
> - return -EINVAL;
> - if (cppc_state == AMD_PSTATE_ACTIVE)
> - return -EBUSY;
> - ret = cpufreq_unregister_driver(current_pstate_driver);
> - amd_pstate_driver_cleanup();
> - break;
> - case AMD_PSTATE_PASSIVE:
> - if (current_pstate_driver) {
> - if (current_pstate_driver == &amd_pstate_driver)
> - return 0;
> - cpufreq_unregister_driver(current_pstate_driver);
> - cppc_state = AMD_PSTATE_PASSIVE;
> - current_pstate_driver = &amd_pstate_driver;
> - }
> + mode_idx = get_mode_idx_from_str(buf, size);
>
> - ret = cpufreq_register_driver(current_pstate_driver);
> - break;
> - case AMD_PSTATE_ACTIVE:
> - if (current_pstate_driver) {
> - if (current_pstate_driver == &amd_pstate_epp_driver)
> - return 0;
> - cpufreq_unregister_driver(current_pstate_driver);
> - current_pstate_driver = &amd_pstate_epp_driver;
> - cppc_state = AMD_PSTATE_ACTIVE;
> - }
> + if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX)
> + return -EINVAL;
>
> - ret = cpufreq_register_driver(current_pstate_driver);
> - break;
> - default:
> - ret = -EINVAL;
> - break;
> - }
> + if (mode_state_machine[cppc_state][mode_idx])
> + return mode_state_machine[cppc_state][mode_idx](mode_idx);
>
> - return ret;
> + return -EBUSY;
> }
>
> static ssize_t show_status(struct kobject *kobj,
With one nit fixed,
Reviewed-by: Mario Limonciello <mario.limonciello@....com>
Powered by blists - more mailing lists