ARM: EXYNOS: Fix coupled CPU idle freeze on Exynos4210

Message ID 20180321094505.25494-1-m.szyprowski@samsung.com
State New
Headers show
Series
  • ARM: EXYNOS: Fix coupled CPU idle freeze on Exynos4210
Related show

Commit Message

Marek Szyprowski March 21, 2018, 9:45 a.m.
Since commit 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only
feature") coupled CPU idle freezes from time to time on Exynos4210. Later
commit 313c8c16ee62 ("PM / CPU: replace raw_notifier with atomic_notifier")
changed the context in which the CPU idle code is executed, what results
in fully reproducible freeze all the time. However, almost the same coupled
CPU idle code works fine on Exynos3250 regarless of the changes made in
the mentioned commits.

It turned out that the IPI call used on Exynos4210 is conflicting with the
change done in the first mentioned commit in GIC. Fix this by using the
same code path as for Exynos3250, instead of the IPI call for
synchronization with second CPU core, call dsb_sev() directly.

Tested on Exynos4210-based Trats and Origen boards.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

CC: stable@vger.kernel.org # v4.13+
---
 arch/arm/mach-exynos/pm.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

-- 
2.15.0

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Marc Zyngier March 21, 2018, 10:05 a.m. | #1
On 21/03/18 09:45, Marek Szyprowski wrote:
> Since commit 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only

> feature") coupled CPU idle freezes from time to time on Exynos4210. Later

> commit 313c8c16ee62 ("PM / CPU: replace raw_notifier with atomic_notifier")

> changed the context in which the CPU idle code is executed, what results

> in fully reproducible freeze all the time. However, almost the same coupled

> CPU idle code works fine on Exynos3250 regarless of the changes made in

> the mentioned commits.

> 

> It turned out that the IPI call used on Exynos4210 is conflicting with the

> change done in the first mentioned commit in GIC. Fix this by using the

> same code path as for Exynos3250, instead of the IPI call for

> synchronization with second CPU core, call dsb_sev() directly.

> 

> Tested on Exynos4210-based Trats and Origen boards.

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> CC: stable@vger.kernel.org # v4.13+

> ---

>  arch/arm/mach-exynos/pm.c | 6 +-----

>  1 file changed, 1 insertion(+), 5 deletions(-)

> 

> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c

> index dc4346ecf16d..a1055a2b8d54 100644

> --- a/arch/arm/mach-exynos/pm.c

> +++ b/arch/arm/mach-exynos/pm.c

> @@ -271,11 +271,7 @@ static int exynos_cpu0_enter_aftr(void)

>  				goto fail;

>  

>  			call_firmware_op(cpu_boot, 1);

> -

> -			if (soc_is_exynos3250())

> -				dsb_sev();

> -			else

> -				arch_send_wakeup_ipi_mask(cpumask_of(1));

> +			dsb_sev();

>  		}

>  	}

>  fail:

> 


I'm a bit puzzled here.

If the GIC change broke something on your platform, it probably also
broke something for all other users of arch_send_wakeup_ipi_mask. But
nobody reported anything until now. Also, it doesn't look like 4210 is a
big-little platform, so it is unlikely to use the big-little switcher.

What is specific to this system that makes misbehave? Were you
implicitly relying on the BL lock to perform some serialization?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski March 21, 2018, 10:18 a.m. | #2
Hi Marc,

On 2018-03-21 11:05, Marc Zyngier wrote:
> On 21/03/18 09:45, Marek Szyprowski wrote:

>> Since commit 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only

>> feature") coupled CPU idle freezes from time to time on Exynos4210. Later

>> commit 313c8c16ee62 ("PM / CPU: replace raw_notifier with atomic_notifier")

>> changed the context in which the CPU idle code is executed, what results

>> in fully reproducible freeze all the time. However, almost the same coupled

>> CPU idle code works fine on Exynos3250 regarless of the changes made in

>> the mentioned commits.

>>

>> It turned out that the IPI call used on Exynos4210 is conflicting with the

>> change done in the first mentioned commit in GIC. Fix this by using the

>> same code path as for Exynos3250, instead of the IPI call for

>> synchronization with second CPU core, call dsb_sev() directly.

>>

>> Tested on Exynos4210-based Trats and Origen boards.

>>

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>> CC: stable@vger.kernel.org # v4.13+

>> ---

>>   arch/arm/mach-exynos/pm.c | 6 +-----

>>   1 file changed, 1 insertion(+), 5 deletions(-)

>>

>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c

>> index dc4346ecf16d..a1055a2b8d54 100644

>> --- a/arch/arm/mach-exynos/pm.c

>> +++ b/arch/arm/mach-exynos/pm.c

>> @@ -271,11 +271,7 @@ static int exynos_cpu0_enter_aftr(void)

>>   				goto fail;

>>   

>>   			call_firmware_op(cpu_boot, 1);

>> -

>> -			if (soc_is_exynos3250())

>> -				dsb_sev();

>> -			else

>> -				arch_send_wakeup_ipi_mask(cpumask_of(1));

>> +			dsb_sev();

>>   		}

>>   	}

>>   fail:

>>

> I'm a bit puzzled here.

>

> If the GIC change broke something on your platform, it probably also

> broke something for all other users of arch_send_wakeup_ipi_mask. But

> nobody reported anything until now. Also, it doesn't look like 4210 is a

> big-little platform, so it is unlikely to use the big-little switcher.

>

> What is specific to this system that makes misbehave? Were you

> implicitly relying on the BL lock to perform some serialization?


My hypothesis, after some internal discussion, is that dsb_sev() should be
there from the beginning, but instead there was an 
arch_send_wakeup_ipi_mask()
call, which indirectly called dsb_sev() as a part of GIC spinlock internals.
When spinlock has been removed from GIC, there is no dsb_sev() call anymore,
what causes synchronization issue.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki March 21, 2018, 1:49 p.m. | #3
On Wednesday, March 21, 2018 10:45:05 AM CET Marek Szyprowski wrote:
> Since commit 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only

> feature") coupled CPU idle freezes from time to time on Exynos4210. Later

> commit 313c8c16ee62 ("PM / CPU: replace raw_notifier with atomic_notifier")

> changed the context in which the CPU idle code is executed, what results

> in fully reproducible freeze all the time. However, almost the same coupled

> CPU idle code works fine on Exynos3250 regarless of the changes made in

> the mentioned commits.

> 

> It turned out that the IPI call used on Exynos4210 is conflicting with the

> change done in the first mentioned commit in GIC. Fix this by using the

> same code path as for Exynos3250, instead of the IPI call for

> synchronization with second CPU core, call dsb_sev() directly.

> 

> Tested on Exynos4210-based Trats and Origen boards.

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> CC: stable@vger.kernel.org # v4.13+

> ---

>  arch/arm/mach-exynos/pm.c | 6 +-----

>  1 file changed, 1 insertion(+), 5 deletions(-)

> 

> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c

> index dc4346ecf16d..a1055a2b8d54 100644

> --- a/arch/arm/mach-exynos/pm.c

> +++ b/arch/arm/mach-exynos/pm.c

> @@ -271,11 +271,7 @@ static int exynos_cpu0_enter_aftr(void)

>  				goto fail;

>  

>  			call_firmware_op(cpu_boot, 1);

> -

> -			if (soc_is_exynos3250())

> -				dsb_sev();

> -			else

> -				arch_send_wakeup_ipi_mask(cpumask_of(1));

> +			dsb_sev();

>  		}

>  	}

>  fail:

> 


That will be -stable material I believe?


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index dc4346ecf16d..a1055a2b8d54 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -271,11 +271,7 @@  static int exynos_cpu0_enter_aftr(void)
 				goto fail;
 
 			call_firmware_op(cpu_boot, 1);
-
-			if (soc_is_exynos3250())
-				dsb_sev();
-			else
-				arch_send_wakeup_ipi_mask(cpumask_of(1));
+			dsb_sev();
 		}
 	}
 fail: