Re: intel_pstate oopses and lockdep report with Linux v4.5-1822-g63e30271b04c

[Stephane Gasparini <stephane.gasparini@linux.intel.com>]

—
Steph




> On Mar 18, 2016, at 10:44 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> 
> On Fri, Mar 18, 2016 at 7:32 PM, Stephane Gasparini
> <stephane.gasparini@linux.intel.com> wrote:
>> 
>> —
>> Steph
>> 
>> 
>> 
>> 
>>> On Mar 18, 2016, at 6:52 PM, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:
>>> 
>>> On Fri, 2016-03-18 at 17:13 +0100, Stephane Gasparini wrote:
>>>> Rafael,
>>>> 
>>>> Why in step 3) both atom_set_pstate() and atom_set_pstate() were not
>>>> both
>>>> changed to use wrmsrl ?
>>> Initial Atom support was experimental as there were no users, till
>>> Chrome started using. So it was just a miss.
>>> 
>>> We should never have to use wrmsrl_on_cpu. But looks like
>>> cpufreq_driver.init() can't guarantee that.
>>> 
>>>> 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.
>>> This is a different issue. BIOS has different configuration option to
>>> enable fast boot modes which are not necessarily optimized for Linux.
>>> Some aggressive setting will force system to reboot on boot. So I will
>>> leave the way it is.
>>> 
>>> Thanks,
>>> Srinivas
>>> 
>> 
>> Still it does not answer my question, why when implementing a4675fbc4a7a
>> we did changed core for wrmsrl and not atom ?
> 
> By mistake?
> 
>> My point is that the issue was more due to a miss in the patch a4675fbc4a7a
>> rather than a difference of behavior between atom and core.
> 
> The issue is due to the fact that wrmsrl_on_cpu() is used in atom_set_pstate().
> 
> Moreover, core_set_pstate() doesn't use wrmsrl_on_cpu(), so in fact it
> is different from atom_set_pstate() in that respect.
> 
> Now, why and when that difference was introduced doesn't really
> matter.  What matters is whether or not it makes sense and what to do
> about it.
> 
> To me, it doesn't make sense.  wrmsrl() should be used on both Core
> and Atom to update the MSR in intel_pstate_adjust_busy_pstate().  In
> turn, wrmsrl_on_cpu() should be used by both of them on init/exit.
> That's exactly what happens with my patch applied, with a twist that
> on init/exit the P-state is always set to the minimum, so it is not
> even necessary to pass the pstate argument between functions in that
> case.
> 
>> The commit message is a bit misleading around this.
>> The wrmrl_on_cpu() is needed on both core and atom during init.
> 
> Yes, it is, but how is that related to the changelog of this patch?

Telling what you are saying in this email in answer to me would make the thing 
more clear IMO. 
1) the error seen is a side effect of the previous change, so the issue 
   was not existing before
2) the explanation would be more clear that during the init/exit wmsrl_on_cpu()
   and other wise wmsrl is the one to be used.


> 
> Thanks,
> Rafael