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

[Rafael J. Wysocki]

On Fri, Mar 18, 2016 at 7:32 PM, Stephane Gasparini
<stephane.gasparini@xxxxxxxxxxxxxxx> wrote:
>
> â
> Steph
>
>
>
>
>> On Mar 18, 2016, at 6:52 PM, Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> 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?

Thanks,
Rafael