v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Patrik Lundquist]

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

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Viresh Kumar]

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

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Patrik Lundquist]

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

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Dirk Brandewie]

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()


A return of zero from cpufreq_driver->get() is a warning at best for
intel_pstate at init time.  In fact zero is a valid return value AFAICT.
I should be doing something rational in any case.

>
> 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);
>

The truncation from the integer math is the likely culprit.

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

This commit and the follow-on to fix a performance regression it introduced
are on my list to get into stable.

If you could file a bugzilla and add me to the CC list it would help me out
when I update stable.
>
>
> 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
>

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

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()
> 
> 
> A return of zero from cpufreq_driver->get() is a warning at best for
> intel_pstate at init time.  In fact zero is a valid return value AFAICT.

Well, is it?  So what is the 0 supposed to mean?

Rafael

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Dirk Brandewie]

On 03/11/2014 12:50 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()
>>
>>
>> A return of zero from cpufreq_driver->get() is a warning at best for
>> intel_pstate at init time.  In fact zero is a valid return value AFAICT.
>
> Well, is it?  So what is the 0 supposed to mean?

Zero frequency the core is not running.  I said it is a valid answer not that I 
should be returning it.  There is nothing in the docs or headers that I can
find that says Zero is a failure value.

In this case the the error should be a warning maybe __cpufreq_add_dev() the 
only use of policy->cur is the bootloader workaround.


>
> Rafael
>

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

On Tuesday, March 11, 2014 01:08:13 PM Dirk Brandewie wrote:
> On 03/11/2014 12:50 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()
> >>
> >>
> >> A return of zero from cpufreq_driver->get() is a warning at best for
> >> intel_pstate at init time.  In fact zero is a valid return value AFAICT.
> >
> > Well, is it?  So what is the 0 supposed to mean?
> 
> Zero frequency the core is not running.  I said it is a valid answer not that I 
> should be returning it.  There is nothing in the docs or headers that I can
> find that says Zero is a failure value.
> 
> In this case the the error should be a warning maybe __cpufreq_add_dev() the 
> only use of policy->cur is the bootloader workaround.

There's another check in cpufreq_update_policy() that also fails if 0 is
returned from ->get().

Definitely, 0 is not the right answer to the "What frequency is the CPU running at?"
question.  It doesn't have a defined meaning either.

Rafael

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Viresh Kumar]

On 12 March 2014 04:08, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
> Zero frequency the core is not running.  I said it is a valid answer not
> that I should be returning it.  There is nothing in the docs or headers that
> I can
> find that says Zero is a failure value.

In this case cpufreq_add_dev() shouldn't have been called for this CPU
then. And so its obviously better to return error in such cases.

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

On Wednesday, March 12, 2014 01:21:33 PM Viresh Kumar wrote:
> On 12 March 2014 04:08, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
> > Zero frequency the core is not running.  I said it is a valid answer not
> > that I should be returning it.  There is nothing in the docs or headers that
> > I can
> > find that says Zero is a failure value.
> 
> In this case cpufreq_add_dev() shouldn't have been called for this CPU
> then. And so its obviously better to return error in such cases.

Why isn't it documented, then?

In fact, zero returned by ->get() has no defined meaning as I said before.

The point, however, is that at least intel_pstate could return 0 *before*
your changes and you made up that whole "0 can't be returned from ->get()"
argument.  It *could* be returned and it *was* returned sometimes.

That's what the report is about.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

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?

Rafael

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Dirk Brandewie]

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.




second sample hasn't been taken
> Rafael
>

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

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.

Rafael

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

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;

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Dirk Brandewie]

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()

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

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?

---
 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 && has_target()) {
 		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 && has_target()) {
 		new_policy.cur = cpufreq_driver->get(cpu);
 		if (WARN_ON(!new_policy.cur)) {
 			ret = -EIO;

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

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;

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

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

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

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);

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Viresh Kumar]

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>

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Patrik Lundquist]

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

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

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.

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Patrik Lundquist]

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.

[PATCH] cpufreq: Skip current frequency initialization for ->setpolicy drivers

[Rafael J. Wysocki]

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");

Re: [PATCH] cpufreq: Skip current frequency initialization for ->setpolicy drivers

[Dirk Brandewie]

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");
>

Re: [PATCH] cpufreq: Skip current frequency initialization for ->setpolicy drivers

[Srivatsa S. Bhat]

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");
> 

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Viresh Kumar]

On Wed, Mar 12, 2014 at 4:57 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> 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?

I don't think its a good idea to handle ->setpolicy specially here.
Zero shouldn't
be allowed as a return value of any online CPU. And doesn't matter at all with
->setpolicy or ->target.

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Rafael J. Wysocki]

On Wednesday, March 12, 2014 01:25:17 PM Viresh Kumar wrote:
> On Wed, Mar 12, 2014 at 4:57 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> 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?
> 
> I don't think its a good idea to handle ->setpolicy specially here.
> Zero shouldn't
> be allowed as a return value of any online CPU. And doesn't matter at all with
> ->setpolicy or ->target.

I agree that returning 0 from ->get() is not a good idea, but that happened
already before your commit and you broke working code.

Also it doesn't make sense to update policy->cur before calling
cpufreq_set_policy() for ->setpolicy drivers, because they don't need that.
And that's what my patch is about.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: v3.13.5 intel_pstate: cpufreq: __cpufreq_add_dev: ->get() failed

[Patrik Lundquist]

On 11 March 2014 18:58, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
>
> Sorry for the slow response you caught me taking a few days off :-)

No worries. :-)


> If you could file a bugzilla and add me to the CC list it would help me out when I update stable.

Done.