Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

From: Shawn Guo <shawnguo@kernel.org>
To: Kohji Okuno <okuno.kohji@jp.panasonic.com>
Cc: Fabio Estevam <fabio.estevam@nxp.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	linux-arm-kernel@lists.infradead.org,
	NXP Linux Team <linux-imx@nxp.com>
Subject: Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time
Date: Fri, 1 Mar 2019 17:23:42 +0800	[thread overview]
Message-ID: <20190301092341.GP26041@dragon> (raw)
In-Reply-To: <20190226023413.19531-1-okuno.kohji@jp.panasonic.com>

Hi Kohji,

On Tue, Feb 26, 2019 at 11:34:13AM +0900, Kohji Okuno wrote:
>  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'.

Just out of curiosity, how did you observe the bug?  I'm trying to
understand the impact of the bug.

Shawn

> 
> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
> ---
> 
> Changes for v3:
>  - remove the braces for a single statement inside the if block.
> 
> Changes for v2:
>  - change "master" variable name.
> 
>  arch/arm/mach-imx/cpuidle-imx6q.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
> index bfeb25aaf9a2..326e870d7123 100644
> --- a/arch/arm/mach-imx/cpuidle-imx6q.c
> +++ b/arch/arm/mach-imx/cpuidle-imx6q.c
> @@ -16,30 +16,23 @@
>  #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;
>  }
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel