ARM: EXYNOS: Fix coupled CPU idle freeze on Exynos4210

PATCH New Marek Szyprowski 5 participants 7 messages Mar 21, 2018 1 message not in the archive

Subsystems (5)

Stable branch Arm (32-bit) Arm sub-architectures Arm Samsung Exynos +1 more subsystems

Share & tools

A message that is not in the archive (sent off-list or not yet received): CGME20180321094523eucas1p1a87d146d65d4191ccdb37f1723977011@eucas1p1.samsung.com

Marek Szyprowski

author

2018-03-21 09:45 UTC #1 lore raw

PATCH 1/1 ARM: EXYNOS: Fix coupled CPU idle freeze on Exynos4210

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 <[email protected]>

CC stable at vger.kernel.org # v4.13+

---
 arch/arm/mach-exynos/pm.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

arch/arm/mach-exynos/pm.c +1 -5

index dc4346ecf16d..a1055a2b8d54 100644
@@ -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:

Signature · 1 line

2.15.0

Marc Zyngier

2018-03-21 10:05 UTC #2 lore raw

PATCH 1/1 ARM: EXYNOS: Fix coupled CPU idle freeze on Exynos4210

Quoted 29 lines · Marek Szyprowski

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 <[email protected]>
CC stable at vger.kernel.org # v4.13+
---
 arch/arm/mach-exynos/pm.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
arch/arm/mach-exynos/pm.c +0 -0
index dc4346ecf16d..a1055a2b8d54 100644
@@ -271,11 +271,7 @@ static int exynos_cpu0_enter_aftr(void)
 				goto fail;
 

arch/arm/mach-exynos/pm.c static int exynos_cpu0_enter_aftr(void) · line 277

Quoted 10 lines

arch/arm/mach-exynos/pm.c +1 -5

 			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.

Signature · 1 line

Jazz is not dead. It just smells funny...

Marek Szyprowski

author

2018-03-21 10:18 UTC #3 lore raw

PATCH 1/1 ARM: EXYNOS: Fix coupled CPU idle freeze on Exynos4210

Hi Marc,

Quoted 42 lines · Marc Zyngier

Quoted 29 lines · Marek Szyprowski
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 <[email protected]>
CC stable at vger.kernel.org # v4.13+
---
  arch/arm/mach-exynos/pm.c | 6 +-----
  1 file changed, 1 insertion(+), 5 deletions(-)
arch/arm/mach-exynos/pm.c +0 -0
index dc4346ecf16d..a1055a2b8d54 100644
@@ -271,11 +271,7 @@ static int exynos_cpu0_enter_aftr(void)
  				goto fail;
 
arch/arm/mach-exynos/pm.c static int exynos_cpu0_enter_aftr(void) · line 277
Quoted 10 lines
arch/arm/mach-exynos/pm.c +1 -5
  			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.

Quoted 7 lines

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

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland

Marc Zyngier

2018-03-21 11:15 UTC #4 lore raw

PATCH 1/1 ARM: EXYNOS: Fix coupled CPU idle freeze on Exynos4210

Quoted 53 lines · Marek Szyprowski

Hi Marc,
Quoted 42 lines · Marc Zyngier
Quoted 29 lines · Marek Szyprowski
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 <[email protected]>
CC stable at vger.kernel.org # v4.13+
---
  arch/arm/mach-exynos/pm.c | 6 +-----
  1 file changed, 1 insertion(+), 5 deletions(-)
arch/arm/mach-exynos/pm.c +0 -0
index dc4346ecf16d..a1055a2b8d54 100644
@@ -271,11 +271,7 @@ static int exynos_cpu0_enter_aftr(void)
  				goto fail;
 
arch/arm/mach-exynos/pm.c static int exynos_cpu0_enter_aftr(void) · line 277
Quoted 10 lines
arch/arm/mach-exynos/pm.c +1 -5
  			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.
Quoted 7 lines
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?

Quoted 6 lines

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.

That's pretty scary. Glad you pinpointed it in the end.

FWIW:

Acked-by Marc Zyngier <[email protected]>

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Bartlomiej Zolnierkiewicz

2018-03-21 11:39 UTC #5 lore raw

PATCH 1/1 ARM: EXYNOS: Fix coupled CPU idle freeze on Exynos4210

In reply to Marek Szyprowski

Quoted 13 lines · Marek Szyprowski

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.

Quoted 4 lines

Tested on Exynos4210-based Trats and Origen boards.
Signed-off-by Marek Szyprowski <[email protected]>
CC stable at vger.kernel.org # v4.13+

Thanks for spotting and fixing this.

Acked-by Bartlomiej Zolnierkiewicz <[email protected]>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Rafael J. Wysocki

2018-03-21 13:49 UTC #6 lore raw

PATCH 1/1 ARM: EXYNOS: Fix coupled CPU idle freeze on Exynos4210

In reply to Marek Szyprowski

Quoted 29 lines · CET Marek Szyprowski

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 <[email protected]>
CC [email protected] # v4.13+
---
 arch/arm/mach-exynos/pm.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
arch/arm/mach-exynos/pm.c +0 -0
index dc4346ecf16d..a1055a2b8d54 100644
@@ -271,11 +271,7 @@ static int exynos_cpu0_enter_aftr(void)
 				goto fail;
 

arch/arm/mach-exynos/pm.c static int exynos_cpu0_enter_aftr(void) · line 277

Quoted 10 lines

arch/arm/mach-exynos/pm.c +1 -5

 			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?

Krzysztof Kozlowski

maintainer

2018-03-21 17:55 UTC #7 lore raw

PATCH 1/1 ARM: EXYNOS: Fix coupled CPU idle freeze on Exynos4210

Quoted 29 lines · Rafael J. Wysocki

Quoted 28 lines · CET Marek Szyprowski
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 <[email protected]>
CC stable at vger.kernel.org # v4.13+
---
 arch/arm/mach-exynos/pm.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
arch/arm/mach-exynos/pm.c +0 -0
index dc4346ecf16d..a1055a2b8d54 100644
@@ -271,11 +271,7 @@ static int exynos_cpu0_enter_aftr(void)
 				goto fail;
 

Quoted 13 lines

arch/arm/mach-exynos/pm.c static int exynos_cpu0_enter_aftr(void) · line 277
Quoted 11 lines
arch/arm/mach-exynos/pm.c +1 -5
 			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?

Yes, it is CC-stable.

Marek,
Thanks, applied (with adjustment of stable address - I believe if some text follows it, it makes more readable to put <>).

Best regards,
Krzysztof