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

[Rafael J. Wysocki]

On Monday, March 21, 2016 10:31:37 AM Stephane Gasparini wrote:
> 
> â
> Steph
> 
> 
> 
> 
> > On Mar 18, 2016, at 10:44 PM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > 
> > 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?
> 
> 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

That's moot.

Of course, you can argue that being inconsistent about using wmsrl_on_cpu()
vs wmsrl() was a bug by itself, but did it really lead to any user-visible
problems?

Honestly, I doubt it.

The issue has become visible after commit a4675fbc4a7a and that's the reason
for my patch.

> 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.

So to me "initialization and cleanup" means exactly "init/exit", so I don't
think it really needs to be more clear than that.

I've added a line about making things consistent between Core and Atom to the
changelog, BTW.  Please have a look at it in my tree.

Thanks,
Rafael