Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756265AbcCUOHO (ORCPT ); Mon, 21 Mar 2016 10:07:14 -0400 Received: from v094114.home.net.pl ([79.96.170.134]:62868 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752482AbcCUOHK convert rfc822-to-8bit (ORCPT ); Mon, 21 Mar 2016 10:07:10 -0400 From: "Rafael J. Wysocki" To: Stephane Gasparini Cc: "Rafael J. Wysocki" , Srinivas Pandruvada , Josh Boyer , Philippe Longepe , Len Brown , Viresh Kumar , Linux PM list , "Linux-Kernel@Vger. Kernel. Org" Subject: Re: intel_pstate oopses and lockdep report with Linux v4.5-1822-g63e30271b04c Date: Mon, 21 Mar 2016 15:09:19 +0100 Message-ID: <3083355.SxRo8HXI0T@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/4.5.0-rc1+; KDE/4.11.5; x86_64; ; ) In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3659 Lines: 104 On Monday, March 21, 2016 10:31:37 AM Stephane Gasparini wrote: > > — > Steph > > > > > > On Mar 18, 2016, at 10:44 PM, Rafael J. Wysocki wrote: > > > > On Fri, Mar 18, 2016 at 7:32 PM, Stephane Gasparini > > wrote: > >> > >> — > >> Steph > >> > >> > >> > >> > >>> On Mar 18, 2016, at 6:52 PM, Srinivas Pandruvada 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