ARM: imx6q: cpuidle: fix bug that CPU may not wake up
diff mbox series

Message ID 20190222.174953.431124434951497467.okuno.kohji@jp.panasonic.com
State New
Headers show
Series
  • ARM: imx6q: cpuidle: fix bug that CPU may not wake up
Related show

Commit Message

Kohji Okuno Feb. 22, 2019, 8:49 a.m. UTC
Hi,
I found the bug in cpuidle-imx6q.c. Could you check my patch?

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

Best regards,
 Kohji Okuno

Comments

Lucas Stach Feb. 22, 2019, 9:14 a.m. UTC | #1
Hi,

Am Freitag, den 22.02.2019, 17:49 +0900 schrieb Kohji Okuno:
> Hi,
> I found the bug in cpuidle-imx6q.c. Could you check my patch?
> 
>  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".

Please send patches inline, instead of attachments. That makes review a
lot easier.

I agree that the patch does the right thing, while simplifying the code
at the same time, which is always a good sign. The only (rather minor)
issue that I could spot is that the "master" variable name isn't
matching what it does anymore, as there is no master CPU anymore. It is
more like the last man standing turning the clock off, while the first
one to wake up will switch it back on. But then I also fail to come up
with a better naming.

Regards,
Lucas
Fabio Estevam Feb. 22, 2019, 12:25 p.m. UTC | #2
Hi Kohji,

On Fri, Feb 22, 2019 at 6:14 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi,
>
> Am Freitag, den 22.02.2019, 17:49 +0900 schrieb Kohji Okuno:
> > Hi,
> > I found the bug in cpuidle-imx6q.c. Could you check my patch?
> >
> >  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".
>
> Please send patches inline, instead of attachments. That makes review a
> lot easier.

Yes, please re-send it with "git send-email" and also include your
Signed-off-by tag.

It looks like a good fix.

Thanks

Patch
diff mbox series

From 2cfeb5f68c2dd0de4854dbac091bf330e25f00d1 Mon Sep 17 00:00:00 2001
From: Kohji Okuno <okuno.kohji@jp.panasonic.com>
Date: Fri, 22 Feb 2019 17:45:26 +0900
Subject: [PATCH] ARM: imx6q: cpuidle: fix bug that CPU may not wake up at
 expected time

 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".
---
 arch/arm/mach-imx/cpuidle-imx6q.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
index bfeb25aaf9a2..22d982d451d1 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 int master = 0;
 static DEFINE_SPINLOCK(master_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(&master_lock);
+	if (++master == num_online_cpus()) {
 		imx6_set_lpm(WAIT_UNCLOCKED);
-		cpu_do_idle();
-		imx6_set_lpm(WAIT_CLOCKED);
-		spin_unlock(&master_lock);
-		goto done;
 	}
+	spin_unlock(&master_lock);
 
-idle:
 	cpu_do_idle();
-done:
-	atomic_dec(&master);
+
+	spin_lock(&master_lock);
+	if (master-- == num_online_cpus()) {
+		imx6_set_lpm(WAIT_CLOCKED);
+	}
+	spin_unlock(&master_lock);
 
 	return index;
 }
-- 
2.17.1