* Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed [not found] <CAA7pwKNmosWaWPR0x3kNzL7eNLk8vdCB=NknU9ayLT9s1fJQYA@mail.gmail.com> @ 2014-03-10 5:23 ` Viresh Kumar 2014-03-10 12:15 ` Patrik Lundquist [not found] ` <531F783B.7010100@gmail.com> 1 sibling, 1 reply; 12+ messages in thread From: Viresh Kumar @ 2014-03-10 5:23 UTC (permalink / raw) To: Patrik Lundquist, Dirk Brandewie, Linux PM list, Rafael J. Wysocki Cc: cpufreq@vger.kernel.org Cc'ing relevant people.. On Fri, Mar 7, 2014 at 11:49 PM, Patrik Lundquist <patrik.lundquist@gmail.com> wrote: > Hi, > > booting 3.13.5 on a dual socket Ivy Bridge-EP resulted in this error: > > [ 0.194139] smpboot: CPU0: Intel(R) Xeon(R) CPU E5-2687W v2 @ > 3.40GHz (fam: 06, model: 3e, stepping: 04) > ... > [ 0.246755] x86: Booting SMP configuration: > [ 0.250935] .... node #0, CPUs: #1 #2 #3 #4 #5 #6 #7 > [ 0.357648] .... node #1, CPUs: #8 #9 #10 #11 #12 #13 #14 #15 > [ 0.553293] x86: Booted up 2 nodes, 16 CPUs > [ 0.557666] smpboot: Total of 16 processors activated (108850.19 BogoMIPS) > ... > [ 5.210204] Intel P-state driver initializing. > [ 5.232407] Intel pstate controlling: cpu 0 > [ 5.253628] Intel pstate controlling: cpu 1 > [ 5.274899] cpufreq: __cpufreq_add_dev: ->get() failed > [ 5.294856] Intel pstate controlling: cpu 2 > [ 5.313553] Intel pstate controlling: cpu 3 > [ 5.332526] Intel pstate controlling: cpu 4 > [ 5.352347] Intel pstate controlling: cpu 5 > [ 5.372112] Intel pstate controlling: cpu 6 > [ 5.391097] Intel pstate controlling: cpu 7 > [ 5.410272] Intel pstate controlling: cpu 8 > [ 5.429092] Intel pstate controlling: cpu 9 > [ 5.447714] Intel pstate controlling: cpu 10 > [ 5.465872] Intel pstate controlling: cpu 11 > [ 5.482942] Intel pstate controlling: cpu 12 > [ 5.498414] Intel pstate controlling: cpu 13 > [ 5.513586] Intel pstate controlling: cpu 14 > [ 5.529200] Intel pstate controlling: cpu 15 > > CPU 1 is alive and well but missing the cpufreq driver. The system is > running fine otherwise. > > Looking closer at the problem gives that intel_pstate_init_cpu() is > successful but intel_pstate_get(), which is called right after by > cpufreq, fails. > > Since all_cpu_data[1] is initialized it gives that sample->freq must > be zero. So the bug should be in intel_pstate_calc_busy() which > incorrectly sets sample->freq to zero. > > I guess cpu->pstate.max_pstate == 4000000 since that's what > cpuinfo_max_freq and scaling_max_freq is on the other cores. > > So the error is likely that core_pct is calculated to 0 in > intel_pstate.c:intel_pstate_calc_busy(): > > core_pct = div64_u64(int_tofp(sample->aperf * 100), > sample->mperf); > > > > Might be fixed by this commit but should be backported in that case: > > commit fcb6a15c2e7e76d493e6f91ea889ab40e1c643a4 > Author: Dirk Brandewie <dirk.j.brandewie@intel.com> > Date: Mon Feb 3 08:55:31 2014 -0800 > > intel_pstate: Take core C0 time into account for core busy calculation > > > > My options to explore the problem further by backporting patches and > continuous reboots are a bit limited at the moment. > > Regards, > Patrik > -- > To unsubscribe from this list: send the line "unsubscribe cpufreq" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed 2014-03-10 5:23 ` v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed Viresh Kumar @ 2014-03-10 12:15 ` Patrik Lundquist 0 siblings, 0 replies; 12+ messages in thread From: Patrik Lundquist @ 2014-03-10 12:15 UTC (permalink / raw) To: Viresh Kumar Cc: Dirk Brandewie, Linux PM list, Rafael J. Wysocki, cpufreq@vger.kernel.org Some MSR values for the troubling CPU 1 (I haven't rebooted since the error): Min P-state: 12 Max P-state: 34 Turbo P-state: 40 TSC: 1105403635263536 APERF: 153644110734887 MPERF: 142432205366417 Maybe a bit surprising that APERF is larger than MPERF since turbo isn't working for CPU 1. The APERF/MPERF ratio should be close to 1.0 at boot time. Are the counters reset by Linux? Can there be a race condition that throws off the ratio? On 10 March 2014 06:23, Viresh Kumar <viresh.kumar@linaro.org> wrote: > Cc'ing relevant people.. > > On Fri, Mar 7, 2014 at 11:49 PM, Patrik Lundquist > <patrik.lundquist@gmail.com> wrote: >> Hi, >> >> booting 3.13.5 on a dual socket Ivy Bridge-EP resulted in this error: >> >> [ 0.194139] smpboot: CPU0: Intel(R) Xeon(R) CPU E5-2687W v2 @ >> 3.40GHz (fam: 06, model: 3e, stepping: 04) >> ... >> [ 0.246755] x86: Booting SMP configuration: >> [ 0.250935] .... node #0, CPUs: #1 #2 #3 #4 #5 #6 #7 >> [ 0.357648] .... node #1, CPUs: #8 #9 #10 #11 #12 #13 #14 #15 >> [ 0.553293] x86: Booted up 2 nodes, 16 CPUs >> [ 0.557666] smpboot: Total of 16 processors activated (108850.19 BogoMIPS) >> ... >> [ 5.210204] Intel P-state driver initializing. >> [ 5.232407] Intel pstate controlling: cpu 0 >> [ 5.253628] Intel pstate controlling: cpu 1 >> [ 5.274899] cpufreq: __cpufreq_add_dev: ->get() failed >> [ 5.294856] Intel pstate controlling: cpu 2 >> [ 5.313553] Intel pstate controlling: cpu 3 >> [ 5.332526] Intel pstate controlling: cpu 4 >> [ 5.352347] Intel pstate controlling: cpu 5 >> [ 5.372112] Intel pstate controlling: cpu 6 >> [ 5.391097] Intel pstate controlling: cpu 7 >> [ 5.410272] Intel pstate controlling: cpu 8 >> [ 5.429092] Intel pstate controlling: cpu 9 >> [ 5.447714] Intel pstate controlling: cpu 10 >> [ 5.465872] Intel pstate controlling: cpu 11 >> [ 5.482942] Intel pstate controlling: cpu 12 >> [ 5.498414] Intel pstate controlling: cpu 13 >> [ 5.513586] Intel pstate controlling: cpu 14 >> [ 5.529200] Intel pstate controlling: cpu 15 >> >> CPU 1 is alive and well but missing the cpufreq driver. The system is >> running fine otherwise. >> >> Looking closer at the problem gives that intel_pstate_init_cpu() is >> successful but intel_pstate_get(), which is called right after by >> cpufreq, fails. >> >> Since all_cpu_data[1] is initialized it gives that sample->freq must >> be zero. So the bug should be in intel_pstate_calc_busy() which >> incorrectly sets sample->freq to zero. >> >> I guess cpu->pstate.max_pstate == 4000000 since that's what >> cpuinfo_max_freq and scaling_max_freq is on the other cores. >> >> So the error is likely that core_pct is calculated to 0 in >> intel_pstate.c:intel_pstate_calc_busy(): >> >> core_pct = div64_u64(int_tofp(sample->aperf * 100), >> sample->mperf); >> >> >> >> Might be fixed by this commit but should be backported in that case: >> >> commit fcb6a15c2e7e76d493e6f91ea889ab40e1c643a4 >> Author: Dirk Brandewie <dirk.j.brandewie@intel.com> >> Date: Mon Feb 3 08:55:31 2014 -0800 >> >> intel_pstate: Take core C0 time into account for core busy calculation >> >> >> >> My options to explore the problem further by backporting patches and >> continuous reboots are a bit limited at the moment. >> >> Regards, >> Patrik >> -- >> To unsubscribe from this list: send the line "unsubscribe cpufreq" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <531F783B.7010100@gmail.com>]
[parent not found: <2792846.28xTH7a9zW@vostro.rjw.lan>]
* Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed [not found] ` <2792846.28xTH7a9zW@vostro.rjw.lan> @ 2014-03-11 23:07 ` Rafael J. Wysocki 2014-03-11 23:09 ` Rafael J. Wysocki 2014-03-12 11:42 ` Patrik Lundquist 0 siblings, 2 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2014-03-11 23:07 UTC (permalink / raw) To: Dirk Brandewie, Patrik Lundquist; +Cc: cpufreq, linux-pm On Tuesday, March 11, 2014 11:48:30 PM Rafael J. Wysocki wrote: > On Tuesday, March 11, 2014 01:55:23 PM Dirk Brandewie wrote: > > On 03/11/2014 01:57 PM, Rafael J. Wysocki wrote: > > > On Tuesday, March 11, 2014 09:52:42 PM Rafael J. Wysocki wrote: > > >> On Tuesday, March 11, 2014 01:17:20 PM Dirk Brandewie wrote: > > >>> On 03/11/2014 01:20 PM, Rafael J. Wysocki wrote: > > >>>> On Tuesday, March 11, 2014 10:58:59 AM Dirk Brandewie wrote: > > >>>>> Hi Patrick, > > >>>>> > > >>>>> Sorry for the slow response you caught me taking a few days off :-) > > >>>>> > > >>>>> On 03/07/2014 07:49 AM, Patrik Lundquist wrote: > > >>>>>> Hi, > > >>>>>> > > >>>>>> booting 3.13.5 on a dual socket Ivy Bridge-EP resulted in this error: > > >>>>>> > > >>>>>> [ 0.194139] smpboot: CPU0: Intel(R) Xeon(R) CPU E5-2687W v2 @ > > >>>>>> 3.40GHz (fam: 06, model: 3e, stepping: 04) > > >>>>>> ... > > >>>>>> [ 0.246755] x86: Booting SMP configuration: > > >>>>>> [ 0.250935] .... node #0, CPUs: #1 #2 #3 #4 #5 #6 #7 > > >>>>>> [ 0.357648] .... node #1, CPUs: #8 #9 #10 #11 #12 #13 #14 #15 > > >>>>>> [ 0.553293] x86: Booted up 2 nodes, 16 CPUs > > >>>>>> [ 0.557666] smpboot: Total of 16 processors activated (108850.19 BogoMIPS) > > >>>>>> ... > > >>>>>> [ 5.210204] Intel P-state driver initializing. > > >>>>>> [ 5.232407] Intel pstate controlling: cpu 0 > > >>>>>> [ 5.253628] Intel pstate controlling: cpu 1 > > >>>>>> [ 5.274899] cpufreq: __cpufreq_add_dev: ->get() failed > > >>>>>> [ 5.294856] Intel pstate controlling: cpu 2 > > >>>>>> [ 5.313553] Intel pstate controlling: cpu 3 > > >>>>>> [ 5.332526] Intel pstate controlling: cpu 4 > > >>>>>> [ 5.352347] Intel pstate controlling: cpu 5 > > >>>>>> [ 5.372112] Intel pstate controlling: cpu 6 > > >>>>>> [ 5.391097] Intel pstate controlling: cpu 7 > > >>>>>> [ 5.410272] Intel pstate controlling: cpu 8 > > >>>>>> [ 5.429092] Intel pstate controlling: cpu 9 > > >>>>>> [ 5.447714] Intel pstate controlling: cpu 10 > > >>>>>> [ 5.465872] Intel pstate controlling: cpu 11 > > >>>>>> [ 5.482942] Intel pstate controlling: cpu 12 > > >>>>>> [ 5.498414] Intel pstate controlling: cpu 13 > > >>>>>> [ 5.513586] Intel pstate controlling: cpu 14 > > >>>>>> [ 5.529200] Intel pstate controlling: cpu 15 > > >>>>>> > > >>>>>> CPU 1 is alive and well but missing the cpufreq driver. The system is > > >>>>>> running fine otherwise. > > >>>>> > > >>>>> This is a regression introduced by commit > > >>>>> da60ce9f2fa cpufreq: call cpufreq_driver->get() after calling ->init() > > >>>> > > >>>> So the problem is that ->get() may return 0 in intel_pstate and that causes > > >>>> the core's _add function to abort? That would mean sample->freq equal to 0, > > >>>> which shouldn't happen after intel_pstate_sample() called by intel_pstate_init_cpu(). > > >>>> > > >>>> Or am I missing anything? > > >>>> > > >>> > > >>> The problem is that the core has been running less than 1% of the time based on > > >>> the absolute values of aperf/mperf and the second sample has not been taken to > > >>> get a more precise delta. > > >>> > > >>> I thought about running sample twice during init but didn't want to propose it > > >>> until I made sure I was not going to break anything else. > > >> > > >> Well, ->setpolicy drivers are a special case anyway, so we can simply skip the > > >> current frequency updates in __cpufreq_add_dev() and cpufreq_update_policy() > > >> for them. > > > > > > In other words, we can do something like in the patch below I suppose? > > > > > > Rafael > > > > > > > > > --- > > > drivers/cpufreq/cpufreq.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > > =================================================================== > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > > @@ -1137,7 +1137,7 @@ static int __cpufreq_add_dev(struct devi > > > per_cpu(cpufreq_cpu_data, j) = policy; > > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > > > > > - if (cpufreq_driver->get) { > > > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > > > policy->cur = cpufreq_driver->get(policy->cpu); > > > if (!policy->cur) { > > > pr_err("%s: ->get() failed\n", __func__); > > > @@ -2150,7 +2150,7 @@ int cpufreq_update_policy(unsigned int c > > > * BIOS might change freq behind our back > > > * -> ask driver for current freq and notify governors about a change > > > */ > > > - if (cpufreq_driver->get) { > > > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > > > new_policy.cur = cpufreq_driver->get(cpu); > > > if (WARN_ON(!new_policy.cur)) { > > > ret = -EIO; > > > > > or use has_target() > > Yes. > > Modified patch is appended. Patrik, can you please check if it helps? Well, actually, I think that checking ->setpolicy is more appropriate, because both places modified by the patch above are before calling cpufreq_set_policy() and that quite explicitly handles ->setpolicy drivers in a special way. It may be equivalent, but that's not obvious from the way the code is written. So Patrik, please test this one (resending, so that it gets to linux-pm): --- drivers/cpufreq/cpufreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Index: linux-pm/drivers/cpufreq/cpufreq.c =================================================================== --- linux-pm.orig/drivers/cpufreq/cpufreq.c +++ linux-pm/drivers/cpufreq/cpufreq.c @@ -1137,7 +1137,7 @@ static int __cpufreq_add_dev(struct devi per_cpu(cpufreq_cpu_data, j) = policy; write_unlock_irqrestore(&cpufreq_driver_lock, flags); - if (cpufreq_driver->get) { + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { policy->cur = cpufreq_driver->get(policy->cpu); if (!policy->cur) { pr_err("%s: ->get() failed\n", __func__); @@ -2150,7 +2150,7 @@ int cpufreq_update_policy(unsigned int c * BIOS might change freq behind our back * -> ask driver for current freq and notify governors about a change */ - if (cpufreq_driver->get) { + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { new_policy.cur = cpufreq_driver->get(cpu); if (WARN_ON(!new_policy.cur)) { ret = -EIO; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed 2014-03-11 23:07 ` Rafael J. Wysocki @ 2014-03-11 23:09 ` Rafael J. Wysocki 2014-03-11 23:53 ` Rafael J. Wysocki 2014-03-12 11:42 ` Patrik Lundquist 1 sibling, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2014-03-11 23:09 UTC (permalink / raw) To: Dirk Brandewie; +Cc: Patrik Lundquist, cpufreq, linux-pm, Viresh Kumar On Wednesday, March 12, 2014 12:07:03 AM Rafael J. Wysocki wrote: > On Tuesday, March 11, 2014 11:48:30 PM Rafael J. Wysocki wrote: > > On Tuesday, March 11, 2014 01:55:23 PM Dirk Brandewie wrote: > > > On 03/11/2014 01:57 PM, Rafael J. Wysocki wrote: > > > > On Tuesday, March 11, 2014 09:52:42 PM Rafael J. Wysocki wrote: > > > >> On Tuesday, March 11, 2014 01:17:20 PM Dirk Brandewie wrote: > > > >>> On 03/11/2014 01:20 PM, Rafael J. Wysocki wrote: > > > >>>> On Tuesday, March 11, 2014 10:58:59 AM Dirk Brandewie wrote: > > > >>>>> Hi Patrick, > > > >>>>> > > > >>>>> Sorry for the slow response you caught me taking a few days off :-) > > > >>>>> > > > >>>>> On 03/07/2014 07:49 AM, Patrik Lundquist wrote: > > > >>>>>> Hi, > > > >>>>>> > > > >>>>>> booting 3.13.5 on a dual socket Ivy Bridge-EP resulted in this error: > > > >>>>>> > > > >>>>>> [ 0.194139] smpboot: CPU0: Intel(R) Xeon(R) CPU E5-2687W v2 @ > > > >>>>>> 3.40GHz (fam: 06, model: 3e, stepping: 04) > > > >>>>>> ... > > > >>>>>> [ 0.246755] x86: Booting SMP configuration: > > > >>>>>> [ 0.250935] .... node #0, CPUs: #1 #2 #3 #4 #5 #6 #7 > > > >>>>>> [ 0.357648] .... node #1, CPUs: #8 #9 #10 #11 #12 #13 #14 #15 > > > >>>>>> [ 0.553293] x86: Booted up 2 nodes, 16 CPUs > > > >>>>>> [ 0.557666] smpboot: Total of 16 processors activated (108850.19 BogoMIPS) > > > >>>>>> ... > > > >>>>>> [ 5.210204] Intel P-state driver initializing. > > > >>>>>> [ 5.232407] Intel pstate controlling: cpu 0 > > > >>>>>> [ 5.253628] Intel pstate controlling: cpu 1 > > > >>>>>> [ 5.274899] cpufreq: __cpufreq_add_dev: ->get() failed > > > >>>>>> [ 5.294856] Intel pstate controlling: cpu 2 > > > >>>>>> [ 5.313553] Intel pstate controlling: cpu 3 > > > >>>>>> [ 5.332526] Intel pstate controlling: cpu 4 > > > >>>>>> [ 5.352347] Intel pstate controlling: cpu 5 > > > >>>>>> [ 5.372112] Intel pstate controlling: cpu 6 > > > >>>>>> [ 5.391097] Intel pstate controlling: cpu 7 > > > >>>>>> [ 5.410272] Intel pstate controlling: cpu 8 > > > >>>>>> [ 5.429092] Intel pstate controlling: cpu 9 > > > >>>>>> [ 5.447714] Intel pstate controlling: cpu 10 > > > >>>>>> [ 5.465872] Intel pstate controlling: cpu 11 > > > >>>>>> [ 5.482942] Intel pstate controlling: cpu 12 > > > >>>>>> [ 5.498414] Intel pstate controlling: cpu 13 > > > >>>>>> [ 5.513586] Intel pstate controlling: cpu 14 > > > >>>>>> [ 5.529200] Intel pstate controlling: cpu 15 > > > >>>>>> > > > >>>>>> CPU 1 is alive and well but missing the cpufreq driver. The system is > > > >>>>>> running fine otherwise. > > > >>>>> > > > >>>>> This is a regression introduced by commit > > > >>>>> da60ce9f2fa cpufreq: call cpufreq_driver->get() after calling ->init() > > > >>>> > > > >>>> So the problem is that ->get() may return 0 in intel_pstate and that causes > > > >>>> the core's _add function to abort? That would mean sample->freq equal to 0, > > > >>>> which shouldn't happen after intel_pstate_sample() called by intel_pstate_init_cpu(). > > > >>>> > > > >>>> Or am I missing anything? > > > >>>> > > > >>> > > > >>> The problem is that the core has been running less than 1% of the time based on > > > >>> the absolute values of aperf/mperf and the second sample has not been taken to > > > >>> get a more precise delta. > > > >>> > > > >>> I thought about running sample twice during init but didn't want to propose it > > > >>> until I made sure I was not going to break anything else. > > > >> > > > >> Well, ->setpolicy drivers are a special case anyway, so we can simply skip the > > > >> current frequency updates in __cpufreq_add_dev() and cpufreq_update_policy() > > > >> for them. > > > > > > > > In other words, we can do something like in the patch below I suppose? > > > > > > > > Rafael > > > > > > > > > > > > --- > > > > drivers/cpufreq/cpufreq.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > > > =================================================================== > > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > > > @@ -1137,7 +1137,7 @@ static int __cpufreq_add_dev(struct devi > > > > per_cpu(cpufreq_cpu_data, j) = policy; > > > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > > > > > > > - if (cpufreq_driver->get) { > > > > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > > > > policy->cur = cpufreq_driver->get(policy->cpu); > > > > if (!policy->cur) { > > > > pr_err("%s: ->get() failed\n", __func__); > > > > @@ -2150,7 +2150,7 @@ int cpufreq_update_policy(unsigned int c > > > > * BIOS might change freq behind our back > > > > * -> ask driver for current freq and notify governors about a change > > > > */ > > > > - if (cpufreq_driver->get) { > > > > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > > > > new_policy.cur = cpufreq_driver->get(cpu); > > > > if (WARN_ON(!new_policy.cur)) { > > > > ret = -EIO; > > > > > > > or use has_target() > > > > Yes. > > > > Modified patch is appended. Patrik, can you please check if it helps? > > Well, actually, I think that checking ->setpolicy is more appropriate, because > both places modified by the patch above are before calling cpufreq_set_policy() > and that quite explicitly handles ->setpolicy drivers in a special way. > > It may be equivalent, but that's not obvious from the way the code is written. And by the way, it would be good to clarify this particular thing. Is having ->target set mutually exclusive with having ->setpolicy set? Rafael ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed 2014-03-11 23:09 ` Rafael J. Wysocki @ 2014-03-11 23:53 ` Rafael J. Wysocki 2014-03-12 5:22 ` Viresh Kumar 0 siblings, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2014-03-11 23:53 UTC (permalink / raw) To: Dirk Brandewie; +Cc: Patrik Lundquist, cpufreq, linux-pm, Viresh Kumar On Wednesday, March 12, 2014 12:09:13 AM Rafael J. Wysocki wrote: > On Wednesday, March 12, 2014 12:07:03 AM Rafael J. Wysocki wrote: > > On Tuesday, March 11, 2014 11:48:30 PM Rafael J. Wysocki wrote: > > > On Tuesday, March 11, 2014 01:55:23 PM Dirk Brandewie wrote: > > > > On 03/11/2014 01:57 PM, Rafael J. Wysocki wrote: > > > > > On Tuesday, March 11, 2014 09:52:42 PM Rafael J. Wysocki wrote: > > > > >> On Tuesday, March 11, 2014 01:17:20 PM Dirk Brandewie wrote: > > > > >>> On 03/11/2014 01:20 PM, Rafael J. Wysocki wrote: > > > > >>>> On Tuesday, March 11, 2014 10:58:59 AM Dirk Brandewie wrote: > > > > >>>>> Hi Patrick, > > > > >>>>> > > > > >>>>> Sorry for the slow response you caught me taking a few days off :-) > > > > >>>>> > > > > >>>>> On 03/07/2014 07:49 AM, Patrik Lundquist wrote: > > > > >>>>>> Hi, > > > > >>>>>> > > > > >>>>>> booting 3.13.5 on a dual socket Ivy Bridge-EP resulted in this error: > > > > >>>>>> > > > > >>>>>> [ 0.194139] smpboot: CPU0: Intel(R) Xeon(R) CPU E5-2687W v2 @ > > > > >>>>>> 3.40GHz (fam: 06, model: 3e, stepping: 04) > > > > >>>>>> ... > > > > >>>>>> [ 0.246755] x86: Booting SMP configuration: > > > > >>>>>> [ 0.250935] .... node #0, CPUs: #1 #2 #3 #4 #5 #6 #7 > > > > >>>>>> [ 0.357648] .... node #1, CPUs: #8 #9 #10 #11 #12 #13 #14 #15 > > > > >>>>>> [ 0.553293] x86: Booted up 2 nodes, 16 CPUs > > > > >>>>>> [ 0.557666] smpboot: Total of 16 processors activated (108850.19 BogoMIPS) > > > > >>>>>> ... > > > > >>>>>> [ 5.210204] Intel P-state driver initializing. > > > > >>>>>> [ 5.232407] Intel pstate controlling: cpu 0 > > > > >>>>>> [ 5.253628] Intel pstate controlling: cpu 1 > > > > >>>>>> [ 5.274899] cpufreq: __cpufreq_add_dev: ->get() failed > > > > >>>>>> [ 5.294856] Intel pstate controlling: cpu 2 > > > > >>>>>> [ 5.313553] Intel pstate controlling: cpu 3 > > > > >>>>>> [ 5.332526] Intel pstate controlling: cpu 4 > > > > >>>>>> [ 5.352347] Intel pstate controlling: cpu 5 > > > > >>>>>> [ 5.372112] Intel pstate controlling: cpu 6 > > > > >>>>>> [ 5.391097] Intel pstate controlling: cpu 7 > > > > >>>>>> [ 5.410272] Intel pstate controlling: cpu 8 > > > > >>>>>> [ 5.429092] Intel pstate controlling: cpu 9 > > > > >>>>>> [ 5.447714] Intel pstate controlling: cpu 10 > > > > >>>>>> [ 5.465872] Intel pstate controlling: cpu 11 > > > > >>>>>> [ 5.482942] Intel pstate controlling: cpu 12 > > > > >>>>>> [ 5.498414] Intel pstate controlling: cpu 13 > > > > >>>>>> [ 5.513586] Intel pstate controlling: cpu 14 > > > > >>>>>> [ 5.529200] Intel pstate controlling: cpu 15 > > > > >>>>>> > > > > >>>>>> CPU 1 is alive and well but missing the cpufreq driver. The system is > > > > >>>>>> running fine otherwise. > > > > >>>>> > > > > >>>>> This is a regression introduced by commit > > > > >>>>> da60ce9f2fa cpufreq: call cpufreq_driver->get() after calling ->init() > > > > >>>> > > > > >>>> So the problem is that ->get() may return 0 in intel_pstate and that causes > > > > >>>> the core's _add function to abort? That would mean sample->freq equal to 0, > > > > >>>> which shouldn't happen after intel_pstate_sample() called by intel_pstate_init_cpu(). > > > > >>>> > > > > >>>> Or am I missing anything? > > > > >>>> > > > > >>> > > > > >>> The problem is that the core has been running less than 1% of the time based on > > > > >>> the absolute values of aperf/mperf and the second sample has not been taken to > > > > >>> get a more precise delta. > > > > >>> > > > > >>> I thought about running sample twice during init but didn't want to propose it > > > > >>> until I made sure I was not going to break anything else. > > > > >> > > > > >> Well, ->setpolicy drivers are a special case anyway, so we can simply skip the > > > > >> current frequency updates in __cpufreq_add_dev() and cpufreq_update_policy() > > > > >> for them. > > > > > > > > > > In other words, we can do something like in the patch below I suppose? > > > > > > > > > > Rafael > > > > > > > > > > > > > > > --- > > > > > drivers/cpufreq/cpufreq.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > > > > =================================================================== > > > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > > > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > > > > @@ -1137,7 +1137,7 @@ static int __cpufreq_add_dev(struct devi > > > > > per_cpu(cpufreq_cpu_data, j) = policy; > > > > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > > > > > > > > > - if (cpufreq_driver->get) { > > > > > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > > > > > policy->cur = cpufreq_driver->get(policy->cpu); > > > > > if (!policy->cur) { > > > > > pr_err("%s: ->get() failed\n", __func__); > > > > > @@ -2150,7 +2150,7 @@ int cpufreq_update_policy(unsigned int c > > > > > * BIOS might change freq behind our back > > > > > * -> ask driver for current freq and notify governors about a change > > > > > */ > > > > > - if (cpufreq_driver->get) { > > > > > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > > > > > new_policy.cur = cpufreq_driver->get(cpu); > > > > > if (WARN_ON(!new_policy.cur)) { > > > > > ret = -EIO; > > > > > > > > > or use has_target() > > > > > > Yes. > > > > > > Modified patch is appended. Patrik, can you please check if it helps? > > > > Well, actually, I think that checking ->setpolicy is more appropriate, because > > both places modified by the patch above are before calling cpufreq_set_policy() > > and that quite explicitly handles ->setpolicy drivers in a special way. > > > > It may be equivalent, but that's not obvious from the way the code is written. > > And by the way, it would be good to clarify this particular thing. > > Is having ->target set mutually exclusive with having ->setpolicy set? It is quite clear that having ->setpolicy implies having ->target unset, because both drivers with ->setpolicy (intel_pstate and longrun) don't have ->target set. Now, are there any *other* cpufreq drivers without ->target? In either case, in my opinion we should just make cpufreq_register_driver() fail for drivers having both ->set_policy and ->target set at the same time. Like in the patch below. Rafael --- drivers/cpufreq/cpufreq.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Index: linux-pm/drivers/cpufreq/cpufreq.c =================================================================== --- linux-pm.orig/drivers/cpufreq/cpufreq.c +++ linux-pm/drivers/cpufreq/cpufreq.c @@ -2306,7 +2306,9 @@ int cpufreq_register_driver(struct cpufr if (!driver_data || !driver_data->verify || !driver_data->init || !(driver_data->setpolicy || driver_data->target_index || - driver_data->target)) + driver_data->target) || + (driver_data->setpolicy && (driver_data->target_index || + driver_data->target))) return -EINVAL; pr_debug("trying to register driver %s\n", driver_data->name); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed 2014-03-11 23:53 ` Rafael J. Wysocki @ 2014-03-12 5:22 ` Viresh Kumar 0 siblings, 0 replies; 12+ messages in thread From: Viresh Kumar @ 2014-03-12 5:22 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Dirk Brandewie, Patrik Lundquist, cpufreq@vger.kernel.org, linux-pm@vger.kernel.org On 12 March 2014 07:53, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> Is having ->target set mutually exclusive with having ->setpolicy set? > > It is quite clear that having ->setpolicy implies having ->target unset, > because both drivers with ->setpolicy (intel_pstate and longrun) don't > have ->target set. > > Now, are there any *other* cpufreq drivers without ->target? > > In either case, in my opinion we should just make cpufreq_register_driver() > fail for drivers having both ->set_policy and ->target set at the same time. > Like in the patch below. > > Rafael > > > --- > drivers/cpufreq/cpufreq.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/cpufreq/cpufreq.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > +++ linux-pm/drivers/cpufreq/cpufreq.c > @@ -2306,7 +2306,9 @@ int cpufreq_register_driver(struct cpufr > > if (!driver_data || !driver_data->verify || !driver_data->init || > !(driver_data->setpolicy || driver_data->target_index || > - driver_data->target)) > + driver_data->target) || > + (driver_data->setpolicy && (driver_data->target_index || > + driver_data->target))) > return -EINVAL; > > pr_debug("trying to register driver %s\n", driver_data->name); Acked-by: Viresh Kumar <viresh.kumar@linaro.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed 2014-03-11 23:07 ` Rafael J. Wysocki 2014-03-11 23:09 ` Rafael J. Wysocki @ 2014-03-12 11:42 ` Patrik Lundquist 2014-03-12 13:27 ` Rafael J. Wysocki 2014-03-12 14:14 ` Patrik Lundquist 1 sibling, 2 replies; 12+ messages in thread From: Patrik Lundquist @ 2014-03-12 11:42 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Dirk Brandewie, cpufreq@vger.kernel.org, Linux PM list On 12 March 2014 00:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > So Patrik, please test this one (resending, so that it gets to linux-pm): Will do. Might take a couple of days. cpufreq_quick_get() caught my eye when applying the patch. "This is the last known freq, without actually getting it from the driver." doesn't make sense since it is calling the driver. And shouldn't it get the frequency from the policy when possible? @@ -1484,7 +1484,7 @@ unsigned int cpufreq_quick_get(unsigned int cpu) struct cpufreq_policy *policy; unsigned int ret_freq = 0; - if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get) + if (cpufreq_driver && !cpufreq_driver->setpolicy && cpufreq_driver->get) return cpufreq_driver->get(cpu); policy = cpufreq_cpu_get(cpu); > --- > drivers/cpufreq/cpufreq.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > Index: linux-pm/drivers/cpufreq/cpufreq.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > +++ linux-pm/drivers/cpufreq/cpufreq.c > @@ -1137,7 +1137,7 @@ static int __cpufreq_add_dev(struct devi > per_cpu(cpufreq_cpu_data, j) = policy; > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > - if (cpufreq_driver->get) { > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > policy->cur = cpufreq_driver->get(policy->cpu); > if (!policy->cur) { > pr_err("%s: ->get() failed\n", __func__); > @@ -2150,7 +2150,7 @@ int cpufreq_update_policy(unsigned int c > * BIOS might change freq behind our back > * -> ask driver for current freq and notify governors about a change > */ > - if (cpufreq_driver->get) { > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > new_policy.cur = cpufreq_driver->get(cpu); > if (WARN_ON(!new_policy.cur)) { > ret = -EIO; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed 2014-03-12 11:42 ` Patrik Lundquist @ 2014-03-12 13:27 ` Rafael J. Wysocki 2014-03-12 14:14 ` Patrik Lundquist 1 sibling, 0 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2014-03-12 13:27 UTC (permalink / raw) To: Patrik Lundquist; +Cc: Dirk Brandewie, cpufreq@vger.kernel.org, Linux PM list On Wednesday, March 12, 2014 12:42:26 PM Patrik Lundquist wrote: > On 12 March 2014 00:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > So Patrik, please test this one (resending, so that it gets to linux-pm): > > Will do. Might take a couple of days. > > cpufreq_quick_get() caught my eye when applying the patch. "This is > the last known freq, without actually getting it from the driver." > doesn't make sense since it is calling the driver. And shouldn't it > get the frequency from the policy when possible? > > > @@ -1484,7 +1484,7 @@ unsigned int cpufreq_quick_get(unsigned int cpu) > struct cpufreq_policy *policy; > unsigned int ret_freq = 0; > > - if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get) > + if (cpufreq_driver && !cpufreq_driver->setpolicy && cpufreq_driver->get) > return cpufreq_driver->get(cpu); No, that one is intentional, because ->setpolicy drivers are supposed to implement a fast mechanism to obtain the frequency via ->get(). It also isn't related to the problem at hand. The kerneldoc is outdated, though. > > policy = cpufreq_cpu_get(cpu); > > > > > --- > > drivers/cpufreq/cpufreq.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > =================================================================== > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > @@ -1137,7 +1137,7 @@ static int __cpufreq_add_dev(struct devi > > per_cpu(cpufreq_cpu_data, j) = policy; > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > > > - if (cpufreq_driver->get) { > > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > > policy->cur = cpufreq_driver->get(policy->cpu); > > if (!policy->cur) { > > pr_err("%s: ->get() failed\n", __func__); > > @@ -2150,7 +2150,7 @@ int cpufreq_update_policy(unsigned int c > > * BIOS might change freq behind our back > > * -> ask driver for current freq and notify governors about a change > > */ > > - if (cpufreq_driver->get) { > > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > > new_policy.cur = cpufreq_driver->get(cpu); > > if (WARN_ON(!new_policy.cur)) { > > ret = -EIO; > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed 2014-03-12 11:42 ` Patrik Lundquist 2014-03-12 13:27 ` Rafael J. Wysocki @ 2014-03-12 14:14 ` Patrik Lundquist 2014-03-12 23:30 ` [PATCH] cpufreq: Skip current frequency initialization for ->setpolicy drivers Rafael J. Wysocki 1 sibling, 1 reply; 12+ messages in thread From: Patrik Lundquist @ 2014-03-12 14:14 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Dirk Brandewie, cpufreq@vger.kernel.org, Linux PM list On 12 March 2014 12:42, Patrik Lundquist <patrik.lundquist@gmail.com> wrote: > On 12 March 2014 00:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> >> So Patrik, please test this one (resending, so that it gets to linux-pm): > > Will do. Might take a couple of days. Come to think of it, there's not much to test besides verifying that cpufreq_driver->get() isn't called like before (i.e. no need to test on the server). So I inserted pr_err()s and they aren't printed when booting my Intel Xeon CPU E3-1240 V2 while the intel_pstate driver still is used. The patch works for me. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] cpufreq: Skip current frequency initialization for ->setpolicy drivers 2014-03-12 14:14 ` Patrik Lundquist @ 2014-03-12 23:30 ` Rafael J. Wysocki 2014-03-12 23:30 ` Dirk Brandewie 2014-03-18 11:53 ` Srivatsa S. Bhat 0 siblings, 2 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2014-03-12 23:30 UTC (permalink / raw) To: Patrik Lundquist, Dirk Brandewie; +Cc: cpufreq@vger.kernel.org, Linux PM list On Wednesday, March 12, 2014 03:14:51 PM Patrik Lundquist wrote: > On 12 March 2014 12:42, Patrik Lundquist <patrik.lundquist@gmail.com> wrote: > > On 12 March 2014 00:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> > >> So Patrik, please test this one (resending, so that it gets to linux-pm): > > > > Will do. Might take a couple of days. > > Come to think of it, there's not much to test besides verifying that > cpufreq_driver->get() isn't called like before (i.e. no need to test > on the server). > > So I inserted pr_err()s and they aren't printed when booting my Intel > Xeon CPU E3-1240 V2 while the intel_pstate driver still is used. > > The patch works for me. Awesome, thanks! Appended again with a proper changelog and tags. Dirk, please let me know if you're fine with it. Rafael --- From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Subject: cpufreq: Skip current frequency initialization for ->setpolicy drivers After commit da60ce9f2fac (cpufreq: call cpufreq_driver->get() after calling ->init()) __cpufreq_add_dev() sometimes fails for CPUs handled by intel_pstate, because that driver may return 0 from its ->get() callback if it has not run long enough to collect enough samples on the given CPU. That didn't happen before commit da60ce9f2fac which added policy->cur initialization to __cpufreq_add_dev() to help reduce code duplication in other cpufreq drivers. However, the code added by commit da60ce9f2fac need not be executed for cpufreq drivers having the ->setpolicy callback defined, because the subsequent invocation of cpufreq_set_policy() will use that callback to initialize the policy anyway and it doesn't need policy->cur to be initialized upfront. The analogous code in cpufreq_update_policy() is also unnecessary for cpufreq drivers having ->setpolicy set and may be skipped for them as well. Since intel_pstate provides ->setpolicy, skipping the upfront policy->cur initialization for cpufreq drivers with that callback set will cover intel_pstate and the problem it's been having after commit da60ce9f2fac will be addressed. Fixes: da60ce9f2fac (cpufreq: call cpufreq_driver->get() after calling ->init()) References: https://bugzilla.kernel.org/show_bug.cgi?id=71931 Reported-and-tested-by: Patrik Lundquist <patrik.lundquist@gmail.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: 3.13+ <stable@vger.kernel.org> # 3.13+ --- drivers/cpufreq/cpufreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Index: linux-pm/drivers/cpufreq/cpufreq.c =================================================================== --- linux-pm.orig/drivers/cpufreq/cpufreq.c +++ linux-pm/drivers/cpufreq/cpufreq.c @@ -1129,7 +1129,7 @@ static int __cpufreq_add_dev(struct devi per_cpu(cpufreq_cpu_data, j) = policy; write_unlock_irqrestore(&cpufreq_driver_lock, flags); - if (cpufreq_driver->get) { + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { policy->cur = cpufreq_driver->get(policy->cpu); if (!policy->cur) { pr_err("%s: ->get() failed\n", __func__); @@ -2143,7 +2143,7 @@ int cpufreq_update_policy(unsigned int c * BIOS might change freq behind our back * -> ask driver for current freq and notify governors about a change */ - if (cpufreq_driver->get) { + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { new_policy.cur = cpufreq_driver->get(cpu); if (!policy->cur) { pr_debug("Driver did not initialize current freq"); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cpufreq: Skip current frequency initialization for ->setpolicy drivers 2014-03-12 23:30 ` [PATCH] cpufreq: Skip current frequency initialization for ->setpolicy drivers Rafael J. Wysocki @ 2014-03-12 23:30 ` Dirk Brandewie 2014-03-18 11:53 ` Srivatsa S. Bhat 1 sibling, 0 replies; 12+ messages in thread From: Dirk Brandewie @ 2014-03-12 23:30 UTC (permalink / raw) To: Rafael J. Wysocki, Patrik Lundquist Cc: dirk.brandewie, cpufreq@vger.kernel.org, Linux PM list On 03/12/2014 04:30 PM, Rafael J. Wysocki wrote: > Dirk, please let me know if you're fine with it. > > --- > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Subject: cpufreq: Skip current frequency initialization for ->setpolicy drivers > > After commit da60ce9f2fac (cpufreq: call cpufreq_driver->get() after > calling ->init()) __cpufreq_add_dev() sometimes fails for CPUs handled > by intel_pstate, because that driver may return 0 from its ->get() > callback if it has not run long enough to collect enough samples on the > given CPU. That didn't happen before commit da60ce9f2fac which added > policy->cur initialization to __cpufreq_add_dev() to help reduce code > duplication in other cpufreq drivers. > > However, the code added by commit da60ce9f2fac need not be executed > for cpufreq drivers having the ->setpolicy callback defined, because > the subsequent invocation of cpufreq_set_policy() will use that > callback to initialize the policy anyway and it doesn't need > policy->cur to be initialized upfront. The analogous code in > cpufreq_update_policy() is also unnecessary for cpufreq drivers > having ->setpolicy set and may be skipped for them as well. > > Since intel_pstate provides ->setpolicy, skipping the upfront > policy->cur initialization for cpufreq drivers with that callback > set will cover intel_pstate and the problem it's been having after > commit da60ce9f2fac will be addressed. > > Fixes: da60ce9f2fac (cpufreq: call cpufreq_driver->get() after calling ->init()) > References: https://bugzilla.kernel.org/show_bug.cgi?id=71931 > Reported-and-tested-by: Patrik Lundquist <patrik.lundquist@gmail.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Acked-by: Dirk Brandewie <dirk.j.brandewie@intel.com> > Cc: 3.13+ <stable@vger.kernel.org> # 3.13+ > --- > drivers/cpufreq/cpufreq.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > Index: linux-pm/drivers/cpufreq/cpufreq.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > +++ linux-pm/drivers/cpufreq/cpufreq.c > @@ -1129,7 +1129,7 @@ static int __cpufreq_add_dev(struct devi > per_cpu(cpufreq_cpu_data, j) = policy; > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > - if (cpufreq_driver->get) { > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > policy->cur = cpufreq_driver->get(policy->cpu); > if (!policy->cur) { > pr_err("%s: ->get() failed\n", __func__); > @@ -2143,7 +2143,7 @@ int cpufreq_update_policy(unsigned int c > * BIOS might change freq behind our back > * -> ask driver for current freq and notify governors about a change > */ > - if (cpufreq_driver->get) { > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > new_policy.cur = cpufreq_driver->get(cpu); > if (!policy->cur) { > pr_debug("Driver did not initialize current freq"); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cpufreq: Skip current frequency initialization for ->setpolicy drivers 2014-03-12 23:30 ` [PATCH] cpufreq: Skip current frequency initialization for ->setpolicy drivers Rafael J. Wysocki 2014-03-12 23:30 ` Dirk Brandewie @ 2014-03-18 11:53 ` Srivatsa S. Bhat 1 sibling, 0 replies; 12+ messages in thread From: Srivatsa S. Bhat @ 2014-03-18 11:53 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Patrik Lundquist, Dirk Brandewie, cpufreq@vger.kernel.org, Linux PM list On 03/13/2014 05:00 AM, Rafael J. Wysocki wrote: > On Wednesday, March 12, 2014 03:14:51 PM Patrik Lundquist wrote: >> On 12 March 2014 12:42, Patrik Lundquist <patrik.lundquist@gmail.com> wrote: >>> On 12 March 2014 00:07, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>>> >>>> So Patrik, please test this one (resending, so that it gets to linux-pm): >>> >>> Will do. Might take a couple of days. >> >> Come to think of it, there's not much to test besides verifying that >> cpufreq_driver->get() isn't called like before (i.e. no need to test >> on the server). >> >> So I inserted pr_err()s and they aren't printed when booting my Intel >> Xeon CPU E3-1240 V2 while the intel_pstate driver still is used. >> >> The patch works for me. > > Awesome, thanks! Appended again with a proper changelog and tags. > > Dirk, please let me know if you're fine with it. > > Rafael > > > --- > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Subject: cpufreq: Skip current frequency initialization for ->setpolicy drivers > > After commit da60ce9f2fac (cpufreq: call cpufreq_driver->get() after > calling ->init()) __cpufreq_add_dev() sometimes fails for CPUs handled > by intel_pstate, because that driver may return 0 from its ->get() > callback if it has not run long enough to collect enough samples on the > given CPU. That didn't happen before commit da60ce9f2fac which added > policy->cur initialization to __cpufreq_add_dev() to help reduce code > duplication in other cpufreq drivers. > > However, the code added by commit da60ce9f2fac need not be executed > for cpufreq drivers having the ->setpolicy callback defined, because > the subsequent invocation of cpufreq_set_policy() will use that > callback to initialize the policy anyway and it doesn't need > policy->cur to be initialized upfront. The analogous code in > cpufreq_update_policy() is also unnecessary for cpufreq drivers > having ->setpolicy set and may be skipped for them as well. > > Since intel_pstate provides ->setpolicy, skipping the upfront > policy->cur initialization for cpufreq drivers with that callback > set will cover intel_pstate and the problem it's been having after > commit da60ce9f2fac will be addressed. > > Fixes: da60ce9f2fac (cpufreq: call cpufreq_driver->get() after calling ->init()) > References: https://bugzilla.kernel.org/show_bug.cgi?id=71931 > Reported-and-tested-by: Patrik Lundquist <patrik.lundquist@gmail.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: 3.13+ <stable@vger.kernel.org> # 3.13+ > --- Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> Regards, Srivatsa S. Bhat > drivers/cpufreq/cpufreq.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > Index: linux-pm/drivers/cpufreq/cpufreq.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > +++ linux-pm/drivers/cpufreq/cpufreq.c > @@ -1129,7 +1129,7 @@ static int __cpufreq_add_dev(struct devi > per_cpu(cpufreq_cpu_data, j) = policy; > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > - if (cpufreq_driver->get) { > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > policy->cur = cpufreq_driver->get(policy->cpu); > if (!policy->cur) { > pr_err("%s: ->get() failed\n", __func__); > @@ -2143,7 +2143,7 @@ int cpufreq_update_policy(unsigned int c > * BIOS might change freq behind our back > * -> ask driver for current freq and notify governors about a change > */ > - if (cpufreq_driver->get) { > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) { > new_policy.cur = cpufreq_driver->get(cpu); > if (!policy->cur) { > pr_debug("Driver did not initialize current freq"); > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-03-18 11:53 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAA7pwKNmosWaWPR0x3kNzL7eNLk8vdCB=NknU9ayLT9s1fJQYA@mail.gmail.com>
2014-03-10 5:23 ` v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed Viresh Kumar
2014-03-10 12:15 ` Patrik Lundquist
[not found] ` <531F783B.7010100@gmail.com>
[not found] ` <2792846.28xTH7a9zW@vostro.rjw.lan>
2014-03-11 23:07 ` Rafael J. Wysocki
2014-03-11 23:09 ` Rafael J. Wysocki
2014-03-11 23:53 ` Rafael J. Wysocki
2014-03-12 5:22 ` Viresh Kumar
2014-03-12 11:42 ` Patrik Lundquist
2014-03-12 13:27 ` Rafael J. Wysocki
2014-03-12 14:14 ` Patrik Lundquist
2014-03-12 23:30 ` [PATCH] cpufreq: Skip current frequency initialization for ->setpolicy drivers Rafael J. Wysocki
2014-03-12 23:30 ` Dirk Brandewie
2014-03-18 11:53 ` Srivatsa S. Bhat
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).