| Message ID | 20190226020623.18324-1-okuno.kohji@jp.panasonic.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time | expand |
Hi Kohji, On Mon, Feb 25, 2019 at 11:08 PM Kohji Okuno <okuno.kohji@jp.panasonic.com> wrote: > + if (++num_idle_cpus == num_online_cpus()) { > imx6_set_lpm(WAIT_UNCLOCKED); You should remove the braces for a single statement inside the if block. > - cpu_do_idle(); > - imx6_set_lpm(WAIT_CLOCKED); > - spin_unlock(&master_lock); > - goto done; > } > + spin_unlock(&cpuidle_lock); > > -idle: > cpu_do_idle(); > -done: > - atomic_dec(&master); > + > + spin_lock(&cpuidle_lock); > + if (num_idle_cpus-- == num_online_cpus()) { > + imx6_set_lpm(WAIT_CLOCKED); > + } Same here.
Hi Fabio,
Than you for your comment.
Just to be sure, let me check.
Should we change as follows?
- if (++num_idle_cpus == num_online_cpus()) {
+ num_idle_cpus++
+ if (num_idle_cpus == num_online_cpus()) {
Fabio Estevam <festevam@gmail.com> wrote:
> Hi Kohji,
>
> On Mon, Feb 25, 2019 at 11:08 PM Kohji Okuno
> <okuno.kohji@jp.panasonic.com> wrote:
>
>> + if (++num_idle_cpus == num_online_cpus()) {
>> imx6_set_lpm(WAIT_UNCLOCKED);
>
> You should remove the braces for a single statement inside the if block.
>
>> - cpu_do_idle();
>> - imx6_set_lpm(WAIT_CLOCKED);
>> - spin_unlock(&master_lock);
>> - goto done;
>> }
>> + spin_unlock(&cpuidle_lock);
>>
>> -idle:
>> cpu_do_idle();
>> -done:
>> - atomic_dec(&master);
>> +
>> + spin_lock(&cpuidle_lock);
>> + if (num_idle_cpus-- == num_online_cpus()) {
>> + imx6_set_lpm(WAIT_CLOCKED);
>> + }
>
> Same here.
Hi Kohji, On Mon, Feb 25, 2019 at 11:19 PM Kohji Okuno <okuno.kohji@jp.panasonic.com> wrote: > > Hi Fabio, > > Than you for your comment. > Just to be sure, let me check. > > Should we change as follows? > > - if (++num_idle_cpus == num_online_cpus()) { > + num_idle_cpus++ > + if (num_idle_cpus == num_online_cpus()) { Instead of doing: if (num_idle_cpus-- == num_online_cpus()) { imx6_set_lpm(WAIT_CLOCKED); } You should do: if (num_idle_cpus-- == num_online_cpus()) imx6_set_lpm(WAIT_CLOCKED); No need for adding { for an if block if only one statement is used. Thanks
Hi Fabio,
Was your meaning as follows?
- if (++num_idle_cpus == num_online_cpus()) {
+ if (++num_idle_cpus == num_online_cpus())
imx6_set_lpm(WAIT_UNCLOCKED);
- }
Best regards,
Kohji Okuno
Kohji Okuno <okuno.kohji@jp.panasonic.com> wrote:
> Hi Fabio,
>
> Than you for your comment.
> Just to be sure, let me check.
>
> Should we change as follows?
>
> - if (++num_idle_cpus == num_online_cpus()) {
> + num_idle_cpus++
> + if (num_idle_cpus == num_online_cpus()) {
>
> Fabio Estevam <festevam@gmail.com> wrote:
>> Hi Kohji,
>>
>> On Mon, Feb 25, 2019 at 11:08 PM Kohji Okuno
>> <okuno.kohji@jp.panasonic.com> wrote:
>>
>>> + if (++num_idle_cpus == num_online_cpus()) {
>>> imx6_set_lpm(WAIT_UNCLOCKED);
>>
>> You should remove the braces for a single statement inside the if block.
>>
>>> - cpu_do_idle();
>>> - imx6_set_lpm(WAIT_CLOCKED);
>>> - spin_unlock(&master_lock);
>>> - goto done;
>>> }
>>> + spin_unlock(&cpuidle_lock);
>>>
>>> -idle:
>>> cpu_do_idle();
>>> -done:
>>> - atomic_dec(&master);
>>> +
>>> + spin_lock(&cpuidle_lock);
>>> + if (num_idle_cpus-- == num_online_cpus()) {
>>> + imx6_set_lpm(WAIT_CLOCKED);
>>> + }
>>
>> Same here.
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c index bfeb25aaf9a2..9d005b589498 100644 --- a/arch/arm/mach-imx/cpuidle-imx6q.c +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -16,30 +16,25 @@ #include "cpuidle.h" #include "hardware.h" -static atomic_t master = ATOMIC_INIT(0); -static DEFINE_SPINLOCK(master_lock); +static int num_idle_cpus = 0; +static DEFINE_SPINLOCK(cpuidle_lock); static int imx6q_enter_wait(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - if (atomic_inc_return(&master) == num_online_cpus()) { - /* - * With this lock, we prevent other cpu to exit and enter - * this function again and become the master. - */ - if (!spin_trylock(&master_lock)) - goto idle; + spin_lock(&cpuidle_lock); + if (++num_idle_cpus == num_online_cpus()) { imx6_set_lpm(WAIT_UNCLOCKED); - cpu_do_idle(); - imx6_set_lpm(WAIT_CLOCKED); - spin_unlock(&master_lock); - goto done; } + spin_unlock(&cpuidle_lock); -idle: cpu_do_idle(); -done: - atomic_dec(&master); + + spin_lock(&cpuidle_lock); + if (num_idle_cpus-- == num_online_cpus()) { + imx6_set_lpm(WAIT_CLOCKED); + } + spin_unlock(&cpuidle_lock); return index; }
In the current cpuidle implementation for i.MX6q, the CPU that sets 'WAIT_UNCLOCKED' and the CPU that returns to 'WAIT_CLOCKED' are always the same. While the CPU that sets 'WAIT_UNCLOCKED' is in IDLE state of "WAIT", if the other CPU wakes up and enters IDLE state of "WFI" istead of "WAIT", this CPU can not wake up at expired time. Because, in the case of "WFI", the CPU must be waked up by the local timer interrupt. But, while 'WAIT_UNCLOCKED' is set, the local timer is stopped, when all CPUs execute "wfi" instruction. As a result, the local timer interrupt is not fired. In this situation, this CPU will wake up by IRQ different from local timer. (e.g. broacast tiemr) So, this fix changes CPU to return to 'WAIT_CLOCKED'. Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com> --- Changes for v2: - change "master" variable name. arch/arm/mach-imx/cpuidle-imx6q.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)