| Message ID | 20190222.174953.431124434951497467.okuno.kohji@jp.panasonic.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: imx6q: cpuidle: fix bug that CPU may not wake up | expand |
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
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
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