Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] cpufreq: intel_pstate: Fix EPP setting via sysfs in active mode
@ 2020-07-28 17:09 Rafael J. Wysocki
  2020-07-29  5:47 ` Francisco Jerez
  0 siblings, 1 reply; 2+ messages in thread
From: Rafael J. Wysocki @ 2020-07-28 17:09 UTC (permalink / raw)
  To: Linux PM, Srinivas Pandruvada; +Cc: LKML, Francisco Jerez

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because intel_pstate_set_energy_pref_index() reads and writes the
MSR_HWP_REQUEST register without using the cached value of it used by
intel_pstate_hwp_boost_up() and intel_pstate_hwp_boost_down(), those
functions may overwrite the value written by it and so the EPP value
set via sysfs may be lost.

To avoid that, make intel_pstate_set_energy_pref_index() take the
cached value of MSR_HWP_REQUEST just like the other two routines
mentioned above and update it with the new EPP value coming from
user space in addition to updating the MSR.

Note that the MSR itself still needs to be updated too in case
hwp_boost is unset or the boosting mechanism is not active at the
EPP change time.

Fixes: e0efd5be63e8 ("cpufreq: intel_pstate: Add HWP boost utility and sched util hooks")
Reported-by: Francisco Jerez <currojerez@riseup.net>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

This patch is on top of https://patchwork.kernel.org/patch/11689347/

---
 drivers/cpufreq/intel_pstate.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -653,11 +653,12 @@ static int intel_pstate_set_energy_pref_
 		epp = cpu_data->epp_default;
 
 	if (boot_cpu_has(X86_FEATURE_HWP_EPP)) {
-		u64 value;
-
-		ret = rdmsrl_on_cpu(cpu_data->cpu, MSR_HWP_REQUEST, &value);
-		if (ret)
-			return ret;
+		/*
+		 * Use the cached HWP Request MSR value, because the register
+		 * itself may be updated by intel_pstate_hwp_boost_up() or
+		 * intel_pstate_hwp_boost_down() at any time.
+		 */
+		u64 value = READ_ONCE(cpu_data->hwp_req_cached);
 
 		value &= ~GENMASK_ULL(31, 24);
 
@@ -667,6 +668,12 @@ static int intel_pstate_set_energy_pref_
 			epp = epp_values[pref_index - 1];
 
 		value |= (u64)epp << 24;
+		/*
+		 * The only other updater of hwp_req_cached in the active mode,
+		 * intel_pstate_hwp_set(), is called under the same lock as this
+		 * function, so it cannot run in parallel with the update below.
+		 */
+		WRITE_ONCE(cpu_data->hwp_req_cached, value);
 		ret = wrmsrl_on_cpu(cpu_data->cpu, MSR_HWP_REQUEST, value);
 	} else {
 		if (epp == -EINVAL)




^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] cpufreq: intel_pstate: Fix EPP setting via sysfs in active mode
  2020-07-28 17:09 [PATCH] cpufreq: intel_pstate: Fix EPP setting via sysfs in active mode Rafael J. Wysocki
@ 2020-07-29  5:47 ` Francisco Jerez
  0 siblings, 0 replies; 2+ messages in thread
From: Francisco Jerez @ 2020-07-29  5:47 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux PM, Srinivas Pandruvada; +Cc: LKML

[-- Attachment #1.1: Type: text/plain, Size: 2653 bytes --]

"Rafael J. Wysocki" <rjw@rjwysocki.net> writes:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Because intel_pstate_set_energy_pref_index() reads and writes the
> MSR_HWP_REQUEST register without using the cached value of it used by
> intel_pstate_hwp_boost_up() and intel_pstate_hwp_boost_down(), those
> functions may overwrite the value written by it and so the EPP value
> set via sysfs may be lost.
>
> To avoid that, make intel_pstate_set_energy_pref_index() take the
> cached value of MSR_HWP_REQUEST just like the other two routines
> mentioned above and update it with the new EPP value coming from
> user space in addition to updating the MSR.
>
> Note that the MSR itself still needs to be updated too in case
> hwp_boost is unset or the boosting mechanism is not active at the
> EPP change time.
>
> Fixes: e0efd5be63e8 ("cpufreq: intel_pstate: Add HWP boost utility and sched util hooks")
> Reported-by: Francisco Jerez <currojerez@riseup.net>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Francisco Jerez <currojerez@riseup.net>

> ---
>
> This patch is on top of https://patchwork.kernel.org/patch/11689347/
>
> ---
>  drivers/cpufreq/intel_pstate.c |   17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -653,11 +653,12 @@ static int intel_pstate_set_energy_pref_
>  		epp = cpu_data->epp_default;
>  
>  	if (boot_cpu_has(X86_FEATURE_HWP_EPP)) {
> -		u64 value;
> -
> -		ret = rdmsrl_on_cpu(cpu_data->cpu, MSR_HWP_REQUEST, &value);
> -		if (ret)
> -			return ret;
> +		/*
> +		 * Use the cached HWP Request MSR value, because the register
> +		 * itself may be updated by intel_pstate_hwp_boost_up() or
> +		 * intel_pstate_hwp_boost_down() at any time.
> +		 */
> +		u64 value = READ_ONCE(cpu_data->hwp_req_cached);
>  
>  		value &= ~GENMASK_ULL(31, 24);
>  
> @@ -667,6 +668,12 @@ static int intel_pstate_set_energy_pref_
>  			epp = epp_values[pref_index - 1];
>  
>  		value |= (u64)epp << 24;
> +		/*
> +		 * The only other updater of hwp_req_cached in the active mode,
> +		 * intel_pstate_hwp_set(), is called under the same lock as this
> +		 * function, so it cannot run in parallel with the update below.
> +		 */
> +		WRITE_ONCE(cpu_data->hwp_req_cached, value);
>  		ret = wrmsrl_on_cpu(cpu_data->cpu, MSR_HWP_REQUEST, value);
>  	} else {
>  		if (epp == -EINVAL)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 17:09 [PATCH] cpufreq: intel_pstate: Fix EPP setting via sysfs in active mode Rafael J. Wysocki
2020-07-29  5:47 ` Francisco Jerez

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git