Message-Id: <E8E228AA-B140-4C15-8984-EFACDC95FB9F@linux.intel.com>
Date:	Fri, 18 Mar 2016 17:13:41 +0100
From:	Stephane Gasparini <stephane.gasparini@...ux.intel.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:	Josh Boyer <jwboyer@...oraproject.org>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	Philippe Longepe <philippe.longepe@...ux.intel.com>,
	Len Brown <lenb@...nel.org>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Linux PM list <linux-pm@...r.kernel.org>,
	"Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>
Subject: Re: intel_pstate oopses and lockdep report with Linux v4.5-1822-g63e30271b04c

Rafael,

Something still obscure to me

1)
on commit 303ae7230751

core were using wrmsrl_on_cpu() in core_set_pstate()
atom were using wrmsrl() in byt_set_pstate()

call sequence is 


intel_state_init_cpu
 -> intel_pstate_get_cpu_pstates() 
   -> intel_pstate_set_pstate()
     -> pstate_funcs.set() +- byt_set_pstate()
                           +- core_set_pstate() 

2)
commit 0dd23f94251f aligned both core and atom 
so both used wrmsrl_on_cpu() in core_set_state() and byt_set_pstate()

Which means that during the init we are sure to always use wrmsrl_on_cpu()
on both atom and core.

The comment on this commit is not that precise, but we can guess that this 
issue solve here was happening during suspend_resume path that is off lining
on lining cpus (which happen a lot on Tablets and phone).

3)
at part of commit a4675fbc4a7a
the code of core_set_state  was changed to use wrmsrl instead 
of wrmsrl_on_cpu()
while the code of atom_set_pstate() (formerly byt_set_pstate() ) was kept
unchanged.


Why in step 3) both atom_set_pstate() and atom_set_pstate() were not both
changed to use wrmsrl ?
If it was OK to use wrmsrl on core, why it would not OK on atom ?

I assume that due to 0dd23f94251f commit, it was felt that the initialization 
issue as only been seen on BYT but this is not true as in step 1) core was 
already using wmsrl_on_cpu() vs byt that was using wrmsrl()

So either as part of commit a4675fbc4a7a both, core and atom should have kept 
using wrmsrl_on_cpu() or both should have been changed to use wrmsrl().

I still don’t see why atom would behave differently than core here.

If we go for using wmsrml only then reverting commit 0dd23f94251f is sufficient
compare to the patch you are proposing here.

If we agree that during initialization we need to use wrmsrl_on_cpu(), I would
change only the initialization on both core and atom.

BTW, what is the interest of setting the pstate to LFM during initialization ?
The BIOS is setting the pstate to either LFM, HFM or BFM, and why bothering
changing it.

Not setting the pstate at initialization will simply make the intel_state
starts from BIOS setting and will avoid all this complexity, i.e. calling
wrmsrl() in intel_pstate_set_pstate() and not calling it during init phase that
are prone to exhibit the issue.
 
—
Steph




> On Mar 18, 2016, at 3:36 PM, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> 
> On Friday, March 18, 2016 08:37:15 AM Josh Boyer wrote:
>> On Thu, Mar 17, 2016 at 8:20 PM, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
>>> On Thursday, March 17, 2016 12:44:54 PM Josh Boyer wrote:
>>>> On Thu, Mar 17, 2016 at 10:07 AM, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
>>>>> On Thursday, March 17, 2016 09:02:29 AM Josh Boyer wrote:
>>>>>> Hello,
>>>>> 
>>>>> Hi,
>>>>> 
>>>>>> I have an Intel Atom based NUC that is producing the following
>>>>>> backtraces on boot of Linus' tree as of last evening.  This does not
>>>>>> happen with a tree with top level commit 271ecc5253e2, but does happen
>>>>>> when using the tree mentioned in the subject with top level commit
>>>>>> 63e30271b04c.
>>>>>> 
>>>>>> The first backtrace appears to be a warning because the intel_pstate
>>>>>> driver is calling wrmsrl_on_cpu when interrupts are disabled?  Not
>>>>>> sure on that one.
>>>>>> 
>>>>>> The second backtrace is a lockdep report.  Both are from the same boot.
>>>>> 
>>>>> OK, thanks for the report.
>>>>> 
>>>>> Can you please try the patch below?
>>>>> 
>>>>> I'm actually unsure if we can do that safely in general for Atom because
>>>>> of the initialization, but that's what Core does anyway.
>>>>> 
>>>>> Srinivas, Philippe, why exactly do we need the wrmsrl_on_cpu() in
>>>>> atom_set_pstate()?  core_set_pstate() uses wrmsrl() and seems to be doing fine.
>>>>> 
>>>>> ---
>>>>> drivers/cpufreq/intel_pstate.c |    2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> 
>>>>> Index: linux-pm/drivers/cpufreq/intel_pstate.c
>>>>> ===================================================================
>>>>> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
>>>>> +++ linux-pm/drivers/cpufreq/intel_pstate.c
>>>>> @@ -587,7 +587,7 @@ static void atom_set_pstate(struct cpuda
>>>>> 
>>>>>        val |= vid;
>>>>> 
>>>>> -       wrmsrl_on_cpu(cpudata->cpu, MSR_IA32_PERF_CTL, val);
>>>>> +       wrmsrl(MSR_IA32_PERF_CTL, val);
>>>>> }
>>>>> 
>>>>> static int silvermont_get_scaling(void)
>>>>> 
>>>> 
>>>> I applied this on top of commit 09fd671ccb24 and the backtrace and
>>>> lockdep report both go away.  So yes, this seems to clear up the
>>>> issue.  I tested it on a variety of different CPU types and didn't
>>>> notice anything wrong on them either.
>>> 
>>> The problems may show up during initialization and cleanup where one CPU
>>> may be running code trying to configure a different one.  In those cases
>>> wrmsrl_on_cpu() needs to be used.
>>> 
>>> Let me cut a patch taking that into account.
>> 
>> OK.  Happy to test when you have it ready.
> 
> Thanks!
> 
> Please test the patch below.
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> Subject: [PATCH] intel_pstate: Do not call wrmsrl_on_cpu() with disabled interrupts
> 
> After commit a4675fbc4a7a (cpufreq: intel_pstate: Replace timers with
> utilization update callbacks) wrmsrl_on_cpu() cannot be called in the
> intel_pstate_adjust_busy_pstate() path as that is executed with
> disabled interrupts.  However, atom_set_pstate() called from there
> via intel_pstate_set_pstate() uses wrmsrl_on_cpu() to update the
> IA32_PERF_CTL MSR which triggers the WARN_ON_ONCE() in
> smp_call_function_single().
> 
> The reason why wrmsrl_on_cpu() is used by atom_set_pstate() is
> because intel_pstate_set_pstate() calling it is also invoked during
> the initialization and cleanup of the driver and in those cases it is
> not guaranteed to be run on the CPU that is being updated.  However,
> in the case when intel_pstate_set_pstate() is called by
> intel_pstate_adjust_busy_pstate(), wrmsrl() can be used to update
> the register safely.  Moreover, intel_pstate_set_pstate() already
> contains code that only is executed if the function is called by
> intel_pstate_adjust_busy_pstate() and there is a special argument
> passed to it because of that.
> 
> To fix the problem at hand, rearrange the code taking the above
> observations into account.
> 
> First, replace the ->set() callback in struct pstate_funcs with a
> ->get_val() one that will return the value to be written to the
> IA32_PERF_CTL MSR without updating the register.
> 
> Second, split intel_pstate_set_pstate() into two functions,
> intel_pstate_update_pstate() to be called by
> intel_pstate_adjust_busy_pstate() that will contain all of the
> intel_pstate_set_pstate() code which only needs to be executed in
> that case and will use wrmsrl() to update the MSR (after obtaining
> the value to write to it from the ->get_val() callback), and
> intel_pstate_set_min_pstate() to be invoked during the
> initialization and cleanup that will set the P-state to the
> minimum one and will update the MSR using wrmsrl_on_cpu().
> 
> Finally, move the code shared between intel_pstate_update_pstate()
> and intel_pstate_set_min_pstate() to a new static inline function
> intel_pstate_record_pstate() and make them both call it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> Fixes: a4675fbc4a7a (cpufreq: intel_pstate: Replace timers with utilization update callbacks)
> ---
> drivers/cpufreq/intel_pstate.c |   73 ++++++++++++++++++++++++-----------------
> 1 file changed, 43 insertions(+), 30 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -134,7 +134,7 @@ struct pstate_funcs {
> 	int (*get_min)(void);
> 	int (*get_turbo)(void);
> 	int (*get_scaling)(void);
> -	void (*set)(struct cpudata*, int pstate);
> +	u64 (*get_val)(struct cpudata*, int pstate);
> 	void (*get_vid)(struct cpudata *);
> 	int32_t (*get_target_pstate)(struct cpudata *);
> };
> @@ -565,7 +565,7 @@ static int atom_get_turbo_pstate(void)
> 	return value & 0x7F;
> }
> 
> -static void atom_set_pstate(struct cpudata *cpudata, int pstate)
> +static u64 atom_get_val(struct cpudata *cpudata, int pstate)
> {
> 	u64 val;
> 	int32_t vid_fp;
> @@ -585,9 +585,7 @@ static void atom_set_pstate(struct cpuda
> 	if (pstate > cpudata->pstate.max_pstate)
> 		vid = cpudata->vid.turbo;
> 
> -	val |= vid;
> -
> -	wrmsrl_on_cpu(cpudata->cpu, MSR_IA32_PERF_CTL, val);
> +	return val | vid;
> }
> 
> static int silvermont_get_scaling(void)
> @@ -711,7 +709,7 @@ static inline int core_get_scaling(void)
> 	return 100000;
> }
> 
> -static void core_set_pstate(struct cpudata *cpudata, int pstate)
> +static u64 core_get_val(struct cpudata *cpudata, int pstate)
> {
> 	u64 val;
> 
> @@ -719,7 +717,7 @@ static void core_set_pstate(struct cpuda
> 	if (limits->no_turbo && !limits->turbo_disabled)
> 		val |= (u64)1 << 32;
> 
> -	wrmsrl(MSR_IA32_PERF_CTL, val);
> +	return val;
> }
> 
> static int knl_get_turbo_pstate(void)
> @@ -750,7 +748,7 @@ static struct cpu_defaults core_params =
> 		.get_min = core_get_min_pstate,
> 		.get_turbo = core_get_turbo_pstate,
> 		.get_scaling = core_get_scaling,
> -		.set = core_set_pstate,
> +		.get_val = core_get_val,
> 		.get_target_pstate = get_target_pstate_use_performance,
> 	},
> };
> @@ -769,7 +767,7 @@ static struct cpu_defaults silvermont_pa
> 		.get_max_physical = atom_get_max_pstate,
> 		.get_min = atom_get_min_pstate,
> 		.get_turbo = atom_get_turbo_pstate,
> -		.set = atom_set_pstate,
> +		.get_val = atom_get_val,
> 		.get_scaling = silvermont_get_scaling,
> 		.get_vid = atom_get_vid,
> 		.get_target_pstate = get_target_pstate_use_cpu_load,
> @@ -790,7 +788,7 @@ static struct cpu_defaults airmont_param
> 		.get_max_physical = atom_get_max_pstate,
> 		.get_min = atom_get_min_pstate,
> 		.get_turbo = atom_get_turbo_pstate,
> -		.set = atom_set_pstate,
> +		.get_val = atom_get_val,
> 		.get_scaling = airmont_get_scaling,
> 		.get_vid = atom_get_vid,
> 		.get_target_pstate = get_target_pstate_use_cpu_load,
> @@ -812,7 +810,7 @@ static struct cpu_defaults knl_params =
> 		.get_min = core_get_min_pstate,
> 		.get_turbo = knl_get_turbo_pstate,
> 		.get_scaling = core_get_scaling,
> -		.set = core_set_pstate,
> +		.get_val = core_get_val,
> 		.get_target_pstate = get_target_pstate_use_performance,
> 	},
> };
> @@ -839,25 +837,24 @@ static void intel_pstate_get_min_max(str
> 	*min = clamp_t(int, min_perf, cpu->pstate.min_pstate, max_perf);
> }
> 
> -static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate, bool force)
> +static inline void intel_pstate_record_pstate(struct cpudata *cpu, int pstate)
> {
> -	int max_perf, min_perf;
> -
> -	if (force) {
> -		update_turbo_state();
> -
> -		intel_pstate_get_min_max(cpu, &min_perf, &max_perf);
> -
> -		pstate = clamp_t(int, pstate, min_perf, max_perf);
> -
> -		if (pstate == cpu->pstate.current_pstate)
> -			return;
> -	}
> 	trace_cpu_frequency(pstate * cpu->pstate.scaling, cpu->cpu);
> -
> 	cpu->pstate.current_pstate = pstate;
> +}
> 
> -	pstate_funcs.set(cpu, pstate);
> +static void intel_pstate_set_min_pstate(struct cpudata *cpu)
> +{
> +	int pstate = cpu->pstate.min_pstate;
> +
> +	intel_pstate_record_pstate(cpu, pstate);
> +	/*
> +	 * Generally, there is no guarantee that this code will always run on
> +	 * the CPU being updated, so force the register update to run on the
> +	 * right CPU.
> +	 */
> +	wrmsrl_on_cpu(cpu->cpu, MSR_IA32_PERF_CTL,
> +		      pstate_funcs.get_val(cpu, pstate));
> }
> 
> static void intel_pstate_get_cpu_pstates(struct cpudata *cpu)
> @@ -870,7 +867,8 @@ static void intel_pstate_get_cpu_pstates
> 
> 	if (pstate_funcs.get_vid)
> 		pstate_funcs.get_vid(cpu);
> -	intel_pstate_set_pstate(cpu, cpu->pstate.min_pstate, false);
> +
> +	intel_pstate_set_min_pstate(cpu);
> }
> 
> static inline void intel_pstate_calc_busy(struct cpudata *cpu)
> @@ -997,6 +995,21 @@ static inline int32_t get_target_pstate_
> 	return cpu->pstate.current_pstate - pid_calc(&cpu->pid, core_busy);
> }
> 
> +static inline void intel_pstate_update_pstate(struct cpudata *cpu, int pstate)
> +{
> +	int max_perf, min_perf;
> +
> +	update_turbo_state();
> +
> +	intel_pstate_get_min_max(cpu, &min_perf, &max_perf);
> +	pstate = clamp_t(int, pstate, min_perf, max_perf);
> +	if (pstate == cpu->pstate.current_pstate)
> +		return;
> +
> +	intel_pstate_record_pstate(cpu, pstate);
> +	wrmsrl(MSR_IA32_PERF_CTL, pstate_funcs.get_val(cpu, pstate));
> +}
> +
> static inline void intel_pstate_adjust_busy_pstate(struct cpudata *cpu)
> {
> 	int from, target_pstate;
> @@ -1006,7 +1019,7 @@ static inline void intel_pstate_adjust_b
> 
> 	target_pstate = pstate_funcs.get_target_pstate(cpu);
> 
> -	intel_pstate_set_pstate(cpu, target_pstate, true);
> +	intel_pstate_update_pstate(cpu, target_pstate);
> 
> 	sample = &cpu->sample;
> 	trace_pstate_sample(fp_toint(sample->core_pct_busy),
> @@ -1180,7 +1193,7 @@ static void intel_pstate_stop_cpu(struct
> 	if (hwp_active)
> 		return;
> 
> -	intel_pstate_set_pstate(cpu, cpu->pstate.min_pstate, false);
> +	intel_pstate_set_min_pstate(cpu);
> }
> 
> static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
> @@ -1255,7 +1268,7 @@ static void copy_cpu_funcs(struct pstate
> 	pstate_funcs.get_min   = funcs->get_min;
> 	pstate_funcs.get_turbo = funcs->get_turbo;
> 	pstate_funcs.get_scaling = funcs->get_scaling;
> -	pstate_funcs.set       = funcs->set;
> +	pstate_funcs.get_val   = funcs->get_val;
> 	pstate_funcs.get_vid   = funcs->get_vid;
> 	pstate_funcs.get_target_pstate = funcs->get_target_pstate;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists