From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wang, Wei W" Subject: Re: [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline Date: Tue, 23 Jun 2015 08:21:34 +0000 Message-ID: <286AC319A985734F985F78AFA26841F7996FBD@shsmsx102.ccr.corp.intel.com> References: <1434011290-17415-1-git-send-email-wei.w.wang@intel.com> <558400740200007800086F05@mail.emea.novell.com> <286AC319A985734F985F78AFA26841F7996C24@shsmsx102.ccr.corp.intel.com> <558930100200007800087F26@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <558930100200007800087F26@mail.emea.novell.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: "andrew.cooper3@citrix.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 23/06/2015 16:08, Jan Beulich wrote: > >>> On 23.06.15 at 08:16, wrote: > > On 19/06/2015 17:44, Jan Beulich wrote: > >> >>> On 11.06.15 at 10:28, wrote: > >> > cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we > >> > change to NULL it after the call of cpufreq_driver->exit. > >> > Otherwise, a calltrace will show up on your screen due to the > >> > reference of a NULL pointer when you power down the system. > >> > >> Apart from what was already said on this patch, I think it is placed > >> too late in this series (should precede patch 6) or, even better, not > >> needed at all: ->exit() gets passed the policy being cleaned up, so I > >> don't follow why the driver needs to consult the per-CPU field to obtain it. > > > >> Plus, if the patch is to be kept, the description suggesting this to > >> be a problem at system shutdown only is wrong - it can equally well > >> happen while offlining a CPU. Just saying that the pointer is still needed > would be sufficient. > > > > It's needed. When unplugging a CPU, the intel_pstate driver sets it to > > run with the lowest P-state. > > I understand the latter, but this doesn't explain why you can't do what I > suggested above. Because the "->exit()" needs to call "intel_pstate_set_pstate()" which does not receive "policy" as a parameter. "intel_pstate_set_pstate()" is also called by another function without passing "policy". So I think it is simpler to just change the order of NULLing the pointer, instead of changing more code. Best, Wei