[PATCH 1/3] clocksource: exynos_mct: Fix stall after CPU hotplugging

[PATCH 1/3] clocksource: exynos_mct: Fix stall after CPU hotplugging

[Krzysztof Kozlowski]

Fix stall after hotplugging CPU1. Affected are SoCs where Multi Core Timer
interrupts are shared (SPI), e.g. Exynos 4210. The stall was a result of
starting the CPU1 local timer not in L1 timer but in L0 (which is used
by CPU0).

Trigger:
$ echo 0 > /sys/bus/cpu/devices/cpu1/online && echo 1 > /sys/bus/cpu/devices/cpu1/online

Stall information:
[  530.045259] INFO: rcu_preempt detected stalls on CPUs/tasks:
[  530.045618]  1: (6 GPs behind) idle=6d0/0/0 softirq=369/369
[  530.050987]  (detected by 0, t=6589 jiffies, g=33, c=32, q=0)
[  530.056721] Task dump for CPU 1:
[  530.059928] swapper/1       R running      0     0      1 0x00001000
[  530.066377] [<c0524e14>] (__schedule+0x414/0x9b4) from [<c00b6610>] (rcu_idle_enter+0x18/0x38)
[  530.074955] [<c00b6610>] (rcu_idle_enter+0x18/0x38) from [<c0079a18>] (cpu_startup_entry+0x60/0x3bc)
[  530.084069] [<c0079a18>] (cpu_startup_entry+0x60/0x3bc) from [<c0517d34>] (secondary_start_kernel+0x164/0x1a0)
[  530.094029] [<c0517d34>] (secondary_start_kernel+0x164/0x1a0) from [<40517244>] (0x40517244)

The timers for CPU1 were missed:
[  591.668436] cpu: 1
[  591.670430]  clock 0:
[  591.672691]   .base:       c0ab7750
[  591.676160]   .index:      0
[  591.679025]   .resolution: 1 nsecs
[  591.682404]   .get_time:   ktime_get
[  591.685970]   .offset:     0 nsecs
[  591.689349] active timers:
[  591.692045]  #0: <dfb51f40>, hrtimer_wakeup, S:01
[  591.696759]  # expires at 454687834257-454687884257 nsecs [in -136770537232 to -136770487232 nsecs]

And the event_handler for next event was wrong:
[  591.917120] Tick Device: mode:     1
[  591.920676] Per CPU device: 0
[  591.923621] Clock Event Device: mct_tick0
[  591.927623]  max_delta_ns:   178956969027
[  591.931613]  min_delta_ns:   1249
[  591.934913]  mult:           51539608
[  591.938557]  shift:          32
[  591.941681]  mode:           3
[  591.944724]  next_event:     595025000000 nsecs
[  591.949227]  set_next_event: exynos4_tick_set_next_event
[  591.954522]  set_mode:       exynos4_tick_set_mode
[  591.959296]  event_handler:  hrtimer_interrupt
[  591.963730]  retries:        0
[  591.966761]
[  591.968245] Tick Device: mode:     0
[  591.971801] Per CPU device: 1
[  591.974746] Clock Event Device: mct_tick1
[  591.978750]  max_delta_ns:   178956969027
[  591.982739]  min_delta_ns:   1249
[  591.986037]  mult:           51539608
[  591.989681]  shift:          32
[  591.992806]  mode:           3
[  591.995848]  next_event:     453685000000 nsecs
[  592.000353]  set_next_event: exynos4_tick_set_next_event
[  592.005648]  set_mode:       exynos4_tick_set_mode
[  592.010421]  event_handler:  tick_handle_periodic
[  592.015115]  retries:        0
[  592.018145]

After turning off the CPU1, the MCT L1 local timer was disabled but the
interrupt was not cleared. Turning on the CPU1 enabled the IRQ
with setup_irq() but, before setting affinity to CPU1, the pending L1 timer
interrupt was processed by CPU0 in exynos4_mct_tick_isr().

The ISR then called event handler which set up the next timer event for
current CPU (CPU0). Therefore the MCT L1 timer wasn't actually started.

Fix the stall by:
1. Setting next timer event not on current CPU but on the CPU indicated
   by cpumask in 'clock_event_device'.
2. Clearing the timer interrupt upon stopping the local timer.

The patch also moves around the call to exynos4_mct_tick_stop() but this
is done only for the code readability as it is not essential for the fix.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: <stable@vger.kernel.org>
---
 drivers/clocksource/exynos_mct.c |   33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 48f76bc05da0..0b49b09dd1a9 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -339,7 +339,14 @@ static void exynos4_mct_tick_start(unsigned long cycles,
 static int exynos4_tick_set_next_event(unsigned long cycles,
 				       struct clock_event_device *evt)
 {
-	struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick);
+	/*
+	 * In case of hotplugging non-boot CPU, the set_next_event could be
+	 * called on CPU0 by ISR before IRQ affinity is set to proper CPU.
+	 * Thus for accessing proper MCT Lx timer, 'per_cpu' for cpumask
+	 * in event must be used instead of 'this_cpu_ptr'.
+	 */
+	struct mct_clock_event_device *mevt = &per_cpu(percpu_mct_tick,
+			cpumask_first(evt->cpumask));
 
 	exynos4_mct_tick_start(cycles, mevt);
 
@@ -371,23 +378,13 @@ static inline void exynos4_tick_set_mode(enum clock_event_mode mode,
 
 static int exynos4_mct_tick_clear(struct mct_clock_event_device *mevt)
 {
-	struct clock_event_device *evt = &mevt->evt;
-
-	/*
-	 * This is for supporting oneshot mode.
-	 * Mct would generate interrupt periodically
-	 * without explicit stopping.
-	 */
-	if (evt->mode != CLOCK_EVT_MODE_PERIODIC)
-		exynos4_mct_tick_stop(mevt);
-
 	/* Clear the MCT tick interrupt */
 	if (__raw_readl(reg_base + mevt->base + MCT_L_INT_CSTAT_OFFSET) & 1) {
 		exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
 		return 1;
-	} else {
-		return 0;
 	}
+
+	return 0;
 }
 
 static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
@@ -395,6 +392,13 @@ static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
 	struct mct_clock_event_device *mevt = dev_id;
 	struct clock_event_device *evt = &mevt->evt;
 
+	/*
+	 * This is for supporting oneshot mode.
+	 * Mct would generate interrupt periodically
+	 * without explicit stopping.
+	 */
+	if (evt->mode != CLOCK_EVT_MODE_PERIODIC)
+		exynos4_mct_tick_stop(mevt);
 	exynos4_mct_tick_clear(mevt);
 
 	evt->event_handler(evt);
@@ -441,7 +445,10 @@ static int exynos4_local_timer_setup(struct clock_event_device *evt)
 
 static void exynos4_local_timer_stop(struct clock_event_device *evt)
 {
+	struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick);
+
 	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
+	exynos4_mct_tick_clear(mevt);
 	if (mct_int_type == MCT_INT_SPI)
 		free_irq(evt->irq, this_cpu_ptr(&percpu_mct_tick));
 	else
-- 
1.7.9.5

[PATCH 2/3] clocksource: exynos_mct: Change exynos4_mct_tick_clear return type to void

[Krzysztof Kozlowski]

Return value of exynos4_mct_tick_clear() was never checked so it can
be safely changed to void.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/clocksource/exynos_mct.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 0b49b09dd1a9..2ac7d228743a 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -376,15 +376,11 @@ static inline void exynos4_tick_set_mode(enum clock_event_mode mode,
 	}
 }
 
-static int exynos4_mct_tick_clear(struct mct_clock_event_device *mevt)
+static void exynos4_mct_tick_clear(struct mct_clock_event_device *mevt)
 {
 	/* Clear the MCT tick interrupt */
-	if (__raw_readl(reg_base + mevt->base + MCT_L_INT_CSTAT_OFFSET) & 1) {
+	if (__raw_readl(reg_base + mevt->base + MCT_L_INT_CSTAT_OFFSET) & 1)
 		exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
-		return 1;
-	}
-
-	return 0;
 }
 
 static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
-- 
1.7.9.5

[PATCH 3/3] clocksource: exynos_mct: Fix too early ISR fire up on wrong CPU

[Krzysztof Kozlowski]

After hotplugging CPU1 the first call of interrupt handler for CPU1
oneshot timer was called on CPU0 because it fired up before setting IRQ
affinity. Affected are SoCs where Multi Core Timer interrupts are shared
(SPI), e.g. Exynos 4210.

During setup of the MCT timers the clock event device should be
registered after setting the affinity for interrupt. This will prevent
starting the timer too early.

Additionally, if clock event device has interrupt set up, the
clockevents_config_and_register() will also set the affinity for it.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: <stable@vger.kernel.org>
---
 drivers/clocksource/exynos_mct.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 2ac7d228743a..f9c9a1d41f2a 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -418,8 +418,6 @@ static int exynos4_local_timer_setup(struct clock_event_device *evt)
 	evt->set_mode = exynos4_tick_set_mode;
 	evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
 	evt->rating = 450;
-	clockevents_config_and_register(evt, clk_rate / (TICK_BASE_CNT + 1),
-					0xf, 0x7fffffff);
 
 	exynos4_mct_write(TICK_BASE_CNT, mevt->base + MCT_L_TCNTB_OFFSET);
 
@@ -435,6 +433,8 @@ static int exynos4_local_timer_setup(struct clock_event_device *evt)
 	} else {
 		enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0);
 	}
+	clockevents_config_and_register(evt, clk_rate / (TICK_BASE_CNT + 1),
+					0xf, 0x7fffffff);
 
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH 1/3] clocksource: exynos_mct: Fix stall after CPU hotplugging

[Krzysztof Kozlowski]

On pią, 2014-03-28 at 14:06 +0100, Krzysztof Kozlowski wrote:
> Fix stall after hotplugging CPU1. Affected are SoCs where Multi Core Timer
> interrupts are shared (SPI), e.g. Exynos 4210. The stall was a result of
> starting the CPU1 local timer not in L1 timer but in L0 (which is used
> by CPU0).

Hi,

Do you have any comments on these 3 patches? They fix the CPU stall on
Exynos4210 and also on Exynos3250 (Chanwoo Choi sent patches for it
recently).

Best regards,
Krzysztof



> Trigger:
> $ echo 0 > /sys/bus/cpu/devices/cpu1/online && echo 1 > /sys/bus/cpu/devices/cpu1/online
> 
> Stall information:
> [  530.045259] INFO: rcu_preempt detected stalls on CPUs/tasks:
> [  530.045618]  1: (6 GPs behind) idle=6d0/0/0 softirq=369/369
> [  530.050987]  (detected by 0, t=6589 jiffies, g=33, c=32, q=0)
> [  530.056721] Task dump for CPU 1:
> [  530.059928] swapper/1       R running      0     0      1 0x00001000
> [  530.066377] [<c0524e14>] (__schedule+0x414/0x9b4) from [<c00b6610>] (rcu_idle_enter+0x18/0x38)
> [  530.074955] [<c00b6610>] (rcu_idle_enter+0x18/0x38) from [<c0079a18>] (cpu_startup_entry+0x60/0x3bc)
> [  530.084069] [<c0079a18>] (cpu_startup_entry+0x60/0x3bc) from [<c0517d34>] (secondary_start_kernel+0x164/0x1a0)
> [  530.094029] [<c0517d34>] (secondary_start_kernel+0x164/0x1a0) from [<40517244>] (0x40517244)
> 
> The timers for CPU1 were missed:
> [  591.668436] cpu: 1
> [  591.670430]  clock 0:
> [  591.672691]   .base:       c0ab7750
> [  591.676160]   .index:      0
> [  591.679025]   .resolution: 1 nsecs
> [  591.682404]   .get_time:   ktime_get
> [  591.685970]   .offset:     0 nsecs
> [  591.689349] active timers:
> [  591.692045]  #0: <dfb51f40>, hrtimer_wakeup, S:01
> [  591.696759]  # expires at 454687834257-454687884257 nsecs [in -136770537232 to -136770487232 nsecs]
> 
> And the event_handler for next event was wrong:
> [  591.917120] Tick Device: mode:     1
> [  591.920676] Per CPU device: 0
> [  591.923621] Clock Event Device: mct_tick0
> [  591.927623]  max_delta_ns:   178956969027
> [  591.931613]  min_delta_ns:   1249
> [  591.934913]  mult:           51539608
> [  591.938557]  shift:          32
> [  591.941681]  mode:           3
> [  591.944724]  next_event:     595025000000 nsecs
> [  591.949227]  set_next_event: exynos4_tick_set_next_event
> [  591.954522]  set_mode:       exynos4_tick_set_mode
> [  591.959296]  event_handler:  hrtimer_interrupt
> [  591.963730]  retries:        0
> [  591.966761]
> [  591.968245] Tick Device: mode:     0
> [  591.971801] Per CPU device: 1
> [  591.974746] Clock Event Device: mct_tick1
> [  591.978750]  max_delta_ns:   178956969027
> [  591.982739]  min_delta_ns:   1249
> [  591.986037]  mult:           51539608
> [  591.989681]  shift:          32
> [  591.992806]  mode:           3
> [  591.995848]  next_event:     453685000000 nsecs
> [  592.000353]  set_next_event: exynos4_tick_set_next_event
> [  592.005648]  set_mode:       exynos4_tick_set_mode
> [  592.010421]  event_handler:  tick_handle_periodic
> [  592.015115]  retries:        0
> [  592.018145]
> 
> After turning off the CPU1, the MCT L1 local timer was disabled but the
> interrupt was not cleared. Turning on the CPU1 enabled the IRQ
> with setup_irq() but, before setting affinity to CPU1, the pending L1 timer
> interrupt was processed by CPU0 in exynos4_mct_tick_isr().
> 
> The ISR then called event handler which set up the next timer event for
> current CPU (CPU0). Therefore the MCT L1 timer wasn't actually started.
> 
> Fix the stall by:
> 1. Setting next timer event not on current CPU but on the CPU indicated
>    by cpumask in 'clock_event_device'.
> 2. Clearing the timer interrupt upon stopping the local timer.
> 
> The patch also moves around the call to exynos4_mct_tick_stop() but this
> is done only for the code readability as it is not essential for the fix.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/clocksource/exynos_mct.c |   33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 48f76bc05da0..0b49b09dd1a9 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -339,7 +339,14 @@ static void exynos4_mct_tick_start(unsigned long cycles,
>  static int exynos4_tick_set_next_event(unsigned long cycles,
>  				       struct clock_event_device *evt)
>  {
> -	struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick);
> +	/*
> +	 * In case of hotplugging non-boot CPU, the set_next_event could be
> +	 * called on CPU0 by ISR before IRQ affinity is set to proper CPU.
> +	 * Thus for accessing proper MCT Lx timer, 'per_cpu' for cpumask
> +	 * in event must be used instead of 'this_cpu_ptr'.
> +	 */
> +	struct mct_clock_event_device *mevt = &per_cpu(percpu_mct_tick,
> +			cpumask_first(evt->cpumask));
>  
>  	exynos4_mct_tick_start(cycles, mevt);
>  
> @@ -371,23 +378,13 @@ static inline void exynos4_tick_set_mode(enum clock_event_mode mode,
>  
>  static int exynos4_mct_tick_clear(struct mct_clock_event_device *mevt)
>  {
> -	struct clock_event_device *evt = &mevt->evt;
> -
> -	/*
> -	 * This is for supporting oneshot mode.
> -	 * Mct would generate interrupt periodically
> -	 * without explicit stopping.
> -	 */
> -	if (evt->mode != CLOCK_EVT_MODE_PERIODIC)
> -		exynos4_mct_tick_stop(mevt);
> -
>  	/* Clear the MCT tick interrupt */
>  	if (__raw_readl(reg_base + mevt->base + MCT_L_INT_CSTAT_OFFSET) & 1) {
>  		exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
>  		return 1;
> -	} else {
> -		return 0;
>  	}
> +
> +	return 0;
>  }
>  
>  static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
> @@ -395,6 +392,13 @@ static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
>  	struct mct_clock_event_device *mevt = dev_id;
>  	struct clock_event_device *evt = &mevt->evt;
>  
> +	/*
> +	 * This is for supporting oneshot mode.
> +	 * Mct would generate interrupt periodically
> +	 * without explicit stopping.
> +	 */
> +	if (evt->mode != CLOCK_EVT_MODE_PERIODIC)
> +		exynos4_mct_tick_stop(mevt);
>  	exynos4_mct_tick_clear(mevt);
>  
>  	evt->event_handler(evt);
> @@ -441,7 +445,10 @@ static int exynos4_local_timer_setup(struct clock_event_device *evt)
>  
>  static void exynos4_local_timer_stop(struct clock_event_device *evt)
>  {
> +	struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick);
> +
>  	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
> +	exynos4_mct_tick_clear(mevt);
>  	if (mct_int_type == MCT_INT_SPI)
>  		free_irq(evt->irq, this_cpu_ptr(&percpu_mct_tick));
>  	else

Re: [PATCH 1/3] clocksource: exynos_mct: Fix stall after CPU hotplugging

[Daniel Lezcano]

On 04/15/2014 11:34 AM, Krzysztof Kozlowski wrote:
> On pią, 2014-03-28 at 14:06 +0100, Krzysztof Kozlowski wrote:
>> Fix stall after hotplugging CPU1. Affected are SoCs where Multi Core Timer
>> interrupts are shared (SPI), e.g. Exynos 4210. The stall was a result of
>> starting the CPU1 local timer not in L1 timer but in L0 (which is used
>> by CPU0).
>
> Hi,
>
> Do you have any comments on these 3 patches? They fix the CPU stall on
> Exynos4210 and also on Exynos3250 (Chanwoo Choi sent patches for it
> recently).

You describe this issue as impacting different SoC not only the exynos, 
right ?

Do you know what other SoCs are impacted by this ?

I guess this issue is not reproducible just with the line below, we need 
a timer to expire right at the moment CPU1 is hotplugged, right ?

>> Trigger:
>> $ echo 0 > /sys/bus/cpu/devices/cpu1/online && echo 1 > /sys/bus/cpu/devices/cpu1/online
>>
>> Stall information:
>> [  530.045259] INFO: rcu_preempt detected stalls on CPUs/tasks:
>> [  530.045618]  1: (6 GPs behind) idle=6d0/0/0 softirq=369/369
>> [  530.050987]  (detected by 0, t=6589 jiffies, g=33, c=32, q=0)
>> [  530.056721] Task dump for CPU 1:
>> [  530.059928] swapper/1       R running      0     0      1 0x00001000
>> [  530.066377] [<c0524e14>] (__schedule+0x414/0x9b4) from [<c00b6610>] (rcu_idle_enter+0x18/0x38)
>> [  530.074955] [<c00b6610>] (rcu_idle_enter+0x18/0x38) from [<c0079a18>] (cpu_startup_entry+0x60/0x3bc)
>> [  530.084069] [<c0079a18>] (cpu_startup_entry+0x60/0x3bc) from [<c0517d34>] (secondary_start_kernel+0x164/0x1a0)
>> [  530.094029] [<c0517d34>] (secondary_start_kernel+0x164/0x1a0) from [<40517244>] (0x40517244)
>>
>> The timers for CPU1 were missed:
>> [  591.668436] cpu: 1
>> [  591.670430]  clock 0:
>> [  591.672691]   .base:       c0ab7750
>> [  591.676160]   .index:      0
>> [  591.679025]   .resolution: 1 nsecs
>> [  591.682404]   .get_time:   ktime_get
>> [  591.685970]   .offset:     0 nsecs
>> [  591.689349] active timers:
>> [  591.692045]  #0: <dfb51f40>, hrtimer_wakeup, S:01
>> [  591.696759]  # expires at 454687834257-454687884257 nsecs [in -136770537232 to -136770487232 nsecs]
>>
>> And the event_handler for next event was wrong:
>> [  591.917120] Tick Device: mode:     1
>> [  591.920676] Per CPU device: 0
>> [  591.923621] Clock Event Device: mct_tick0
>> [  591.927623]  max_delta_ns:   178956969027
>> [  591.931613]  min_delta_ns:   1249
>> [  591.934913]  mult:           51539608
>> [  591.938557]  shift:          32
>> [  591.941681]  mode:           3
>> [  591.944724]  next_event:     595025000000 nsecs
>> [  591.949227]  set_next_event: exynos4_tick_set_next_event
>> [  591.954522]  set_mode:       exynos4_tick_set_mode
>> [  591.959296]  event_handler:  hrtimer_interrupt
>> [  591.963730]  retries:        0
>> [  591.966761]
>> [  591.968245] Tick Device: mode:     0
>> [  591.971801] Per CPU device: 1
>> [  591.974746] Clock Event Device: mct_tick1
>> [  591.978750]  max_delta_ns:   178956969027
>> [  591.982739]  min_delta_ns:   1249
>> [  591.986037]  mult:           51539608
>> [  591.989681]  shift:          32
>> [  591.992806]  mode:           3
>> [  591.995848]  next_event:     453685000000 nsecs
>> [  592.000353]  set_next_event: exynos4_tick_set_next_event
>> [  592.005648]  set_mode:       exynos4_tick_set_mode
>> [  592.010421]  event_handler:  tick_handle_periodic
>> [  592.015115]  retries:        0
>> [  592.018145]
>>
>> After turning off the CPU1, the MCT L1 local timer was disabled but the
>> interrupt was not cleared. Turning on the CPU1 enabled the IRQ
>> with setup_irq() but, before setting affinity to CPU1, the pending L1 timer
>> interrupt was processed by CPU0 in exynos4_mct_tick_isr().
>>
>> The ISR then called event handler which set up the next timer event for
>> current CPU (CPU0). Therefore the MCT L1 timer wasn't actually started.
>>
>> Fix the stall by:
>> 1. Setting next timer event not on current CPU but on the CPU indicated
>>     by cpumask in 'clock_event_device'.
>> 2. Clearing the timer interrupt upon stopping the local timer.
>>
>> The patch also moves around the call to exynos4_mct_tick_stop() but this
>> is done only for the code readability as it is not essential for the fix.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>>   drivers/clocksource/exynos_mct.c |   33 ++++++++++++++++++++-------------
>>   1 file changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
>> index 48f76bc05da0..0b49b09dd1a9 100644
>> --- a/drivers/clocksource/exynos_mct.c
>> +++ b/drivers/clocksource/exynos_mct.c
>> @@ -339,7 +339,14 @@ static void exynos4_mct_tick_start(unsigned long cycles,
>>   static int exynos4_tick_set_next_event(unsigned long cycles,
>>   				       struct clock_event_device *evt)
>>   {
>> -	struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick);
>> +	/*
>> +	 * In case of hotplugging non-boot CPU, the set_next_event could be
>> +	 * called on CPU0 by ISR before IRQ affinity is set to proper CPU.
>> +	 * Thus for accessing proper MCT Lx timer, 'per_cpu' for cpumask
>> +	 * in event must be used instead of 'this_cpu_ptr'.
>> +	 */
>> +	struct mct_clock_event_device *mevt = &per_cpu(percpu_mct_tick,
>> +			cpumask_first(evt->cpumask));
>>
>>   	exynos4_mct_tick_start(cycles, mevt);
>>
>> @@ -371,23 +378,13 @@ static inline void exynos4_tick_set_mode(enum clock_event_mode mode,
>>
>>   static int exynos4_mct_tick_clear(struct mct_clock_event_device *mevt)
>>   {
>> -	struct clock_event_device *evt = &mevt->evt;
>> -
>> -	/*
>> -	 * This is for supporting oneshot mode.
>> -	 * Mct would generate interrupt periodically
>> -	 * without explicit stopping.
>> -	 */
>> -	if (evt->mode != CLOCK_EVT_MODE_PERIODIC)
>> -		exynos4_mct_tick_stop(mevt);
>> -
>>   	/* Clear the MCT tick interrupt */
>>   	if (__raw_readl(reg_base + mevt->base + MCT_L_INT_CSTAT_OFFSET) & 1) {
>>   		exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
>>   		return 1;
>> -	} else {
>> -		return 0;
>>   	}
>> +
>> +	return 0;
>>   }
>>
>>   static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
>> @@ -395,6 +392,13 @@ static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
>>   	struct mct_clock_event_device *mevt = dev_id;
>>   	struct clock_event_device *evt = &mevt->evt;
>>
>> +	/*
>> +	 * This is for supporting oneshot mode.
>> +	 * Mct would generate interrupt periodically
>> +	 * without explicit stopping.
>> +	 */
>> +	if (evt->mode != CLOCK_EVT_MODE_PERIODIC)
>> +		exynos4_mct_tick_stop(mevt);
>>   	exynos4_mct_tick_clear(mevt);
>>
>>   	evt->event_handler(evt);
>> @@ -441,7 +445,10 @@ static int exynos4_local_timer_setup(struct clock_event_device *evt)
>>
>>   static void exynos4_local_timer_stop(struct clock_event_device *evt)
>>   {
>> +	struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick);
>> +
>>   	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
>> +	exynos4_mct_tick_clear(mevt);
>>   	if (mct_int_type == MCT_INT_SPI)
>>   		free_irq(evt->irq, this_cpu_ptr(&percpu_mct_tick));
>>   	else
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/3] clocksource: exynos_mct: Fix stall after CPU hotplugging

[Krzysztof Kozlowski]

On wto, 2014-04-15 at 14:28 +0200, Daniel Lezcano wrote:
> On 04/15/2014 11:34 AM, Krzysztof Kozlowski wrote:
> > On pią, 2014-03-28 at 14:06 +0100, Krzysztof Kozlowski wrote:
> >> Fix stall after hotplugging CPU1. Affected are SoCs where Multi Core Timer
> >> interrupts are shared (SPI), e.g. Exynos 4210. The stall was a result of
> >> starting the CPU1 local timer not in L1 timer but in L0 (which is used
> >> by CPU0).
> >
> > Hi,
> >
> > Do you have any comments on these 3 patches? They fix the CPU stall on
> > Exynos4210 and also on Exynos3250 (Chanwoo Choi sent patches for it
> > recently).
> 
> You describe this issue as impacting different SoC not only the exynos, 
> right ?
>
> Do you know what other SoCs are impacted by this ?

No, affected are only Exynos SoC-s. It was confirmed on Exynos4210
(Trats board) and Exynos3250 (new SoC, patches for it were recently
posted by Chanwoo).

Other Exynos SoC-s where MCT local timers use shared interrupts (SPI)
can also be affected. Candidates are Exynos 5250 and 5420 but I haven't
tested them.

> I guess this issue is not reproducible just with the line below, we need 
> a timer to expire right at the moment CPU1 is hotplugged, right ?

Right. The timer must fire in short time between enabling local timer
for CPU1 and setting the affinity for IRQ.

In my case on ~1 GHz 2 core CPU (during idle state, system booted with
init=/bin/sh)  it was easily triggered with:

for i in `seq 200`; do
	echo 0 > /sys/bus/cpu/devices/cpu1/online
	echo 1 > /sys/bus/cpu/devices/cpu1/online
	sleep 1
done

The stall happened typically after 10-30 seconds of such test.

Best regards,
Krzysztof

> 
> >> Trigger:
> >> $ echo 0 > /sys/bus/cpu/devices/cpu1/online && echo 1 > /sys/bus/cpu/devices/cpu1/online
> >>
> >> Stall information:
> >> [  530.045259] INFO: rcu_preempt detected stalls on CPUs/tasks:
> >> [  530.045618]  1: (6 GPs behind) idle=6d0/0/0 softirq=369/369
> >> [  530.050987]  (detected by 0, t=6589 jiffies, g=33, c=32, q=0)
> >> [  530.056721] Task dump for CPU 1:
> >> [  530.059928] swapper/1       R running      0     0      1 0x00001000
> >> [  530.066377] [<c0524e14>] (__schedule+0x414/0x9b4) from [<c00b6610>] (rcu_idle_enter+0x18/0x38)
> >> [  530.074955] [<c00b6610>] (rcu_idle_enter+0x18/0x38) from [<c0079a18>] (cpu_startup_entry+0x60/0x3bc)
> >> [  530.084069] [<c0079a18>] (cpu_startup_entry+0x60/0x3bc) from [<c0517d34>] (secondary_start_kernel+0x164/0x1a0)
> >> [  530.094029] [<c0517d34>] (secondary_start_kernel+0x164/0x1a0) from [<40517244>] (0x40517244)
> >>
> >> The timers for CPU1 were missed:
> >> [  591.668436] cpu: 1
> >> [  591.670430]  clock 0:
> >> [  591.672691]   .base:       c0ab7750
> >> [  591.676160]   .index:      0
> >> [  591.679025]   .resolution: 1 nsecs
> >> [  591.682404]   .get_time:   ktime_get
> >> [  591.685970]   .offset:     0 nsecs
> >> [  591.689349] active timers:
> >> [  591.692045]  #0: <dfb51f40>, hrtimer_wakeup, S:01
> >> [  591.696759]  # expires at 454687834257-454687884257 nsecs [in -136770537232 to -136770487232 nsecs]
> >>
> >> And the event_handler for next event was wrong:
> >> [  591.917120] Tick Device: mode:     1
> >> [  591.920676] Per CPU device: 0
> >> [  591.923621] Clock Event Device: mct_tick0
> >> [  591.927623]  max_delta_ns:   178956969027
> >> [  591.931613]  min_delta_ns:   1249
> >> [  591.934913]  mult:           51539608
> >> [  591.938557]  shift:          32
> >> [  591.941681]  mode:           3
> >> [  591.944724]  next_event:     595025000000 nsecs
> >> [  591.949227]  set_next_event: exynos4_tick_set_next_event
> >> [  591.954522]  set_mode:       exynos4_tick_set_mode
> >> [  591.959296]  event_handler:  hrtimer_interrupt
> >> [  591.963730]  retries:        0
> >> [  591.966761]
> >> [  591.968245] Tick Device: mode:     0
> >> [  591.971801] Per CPU device: 1
> >> [  591.974746] Clock Event Device: mct_tick1
> >> [  591.978750]  max_delta_ns:   178956969027
> >> [  591.982739]  min_delta_ns:   1249
> >> [  591.986037]  mult:           51539608
> >> [  591.989681]  shift:          32
> >> [  591.992806]  mode:           3
> >> [  591.995848]  next_event:     453685000000 nsecs
> >> [  592.000353]  set_next_event: exynos4_tick_set_next_event
> >> [  592.005648]  set_mode:       exynos4_tick_set_mode
> >> [  592.010421]  event_handler:  tick_handle_periodic
> >> [  592.015115]  retries:        0
> >> [  592.018145]
> >>
> >> After turning off the CPU1, the MCT L1 local timer was disabled but the
> >> interrupt was not cleared. Turning on the CPU1 enabled the IRQ
> >> with setup_irq() but, before setting affinity to CPU1, the pending L1 timer
> >> interrupt was processed by CPU0 in exynos4_mct_tick_isr().
> >>
> >> The ISR then called event handler which set up the next timer event for
> >> current CPU (CPU0). Therefore the MCT L1 timer wasn't actually started.
> >>
> >> Fix the stall by:
> >> 1. Setting next timer event not on current CPU but on the CPU indicated
> >>     by cpumask in 'clock_event_device'.
> >> 2. Clearing the timer interrupt upon stopping the local timer.
> >>
> >> The patch also moves around the call to exynos4_mct_tick_stop() but this
> >> is done only for the code readability as it is not essential for the fix.
> >>
> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >> Cc: <stable@vger.kernel.org>
> >> ---
> >>   drivers/clocksource/exynos_mct.c |   33 ++++++++++++++++++++-------------
> >>   1 file changed, 20 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> >> index 48f76bc05da0..0b49b09dd1a9 100644
> >> --- a/drivers/clocksource/exynos_mct.c
> >> +++ b/drivers/clocksource/exynos_mct.c
> >> @@ -339,7 +339,14 @@ static void exynos4_mct_tick_start(unsigned long cycles,
> >>   static int exynos4_tick_set_next_event(unsigned long cycles,
> >>   				       struct clock_event_device *evt)
> >>   {
> >> -	struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick);
> >> +	/*
> >> +	 * In case of hotplugging non-boot CPU, the set_next_event could be
> >> +	 * called on CPU0 by ISR before IRQ affinity is set to proper CPU.
> >> +	 * Thus for accessing proper MCT Lx timer, 'per_cpu' for cpumask
> >> +	 * in event must be used instead of 'this_cpu_ptr'.
> >> +	 */
> >> +	struct mct_clock_event_device *mevt = &per_cpu(percpu_mct_tick,
> >> +			cpumask_first(evt->cpumask));
> >>
> >>   	exynos4_mct_tick_start(cycles, mevt);
> >>
> >> @@ -371,23 +378,13 @@ static inline void exynos4_tick_set_mode(enum clock_event_mode mode,
> >>
> >>   static int exynos4_mct_tick_clear(struct mct_clock_event_device *mevt)
> >>   {
> >> -	struct clock_event_device *evt = &mevt->evt;
> >> -
> >> -	/*
> >> -	 * This is for supporting oneshot mode.
> >> -	 * Mct would generate interrupt periodically
> >> -	 * without explicit stopping.
> >> -	 */
> >> -	if (evt->mode != CLOCK_EVT_MODE_PERIODIC)
> >> -		exynos4_mct_tick_stop(mevt);
> >> -
> >>   	/* Clear the MCT tick interrupt */
> >>   	if (__raw_readl(reg_base + mevt->base + MCT_L_INT_CSTAT_OFFSET) & 1) {
> >>   		exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
> >>   		return 1;
> >> -	} else {
> >> -		return 0;
> >>   	}
> >> +
> >> +	return 0;
> >>   }
> >>
> >>   static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
> >> @@ -395,6 +392,13 @@ static irqreturn_t exynos4_mct_tick_isr(int irq, void *dev_id)
> >>   	struct mct_clock_event_device *mevt = dev_id;
> >>   	struct clock_event_device *evt = &mevt->evt;
> >>
> >> +	/*
> >> +	 * This is for supporting oneshot mode.
> >> +	 * Mct would generate interrupt periodically
> >> +	 * without explicit stopping.
> >> +	 */
> >> +	if (evt->mode != CLOCK_EVT_MODE_PERIODIC)
> >> +		exynos4_mct_tick_stop(mevt);
> >>   	exynos4_mct_tick_clear(mevt);
> >>
> >>   	evt->event_handler(evt);
> >> @@ -441,7 +445,10 @@ static int exynos4_local_timer_setup(struct clock_event_device *evt)
> >>
> >>   static void exynos4_local_timer_stop(struct clock_event_device *evt)
> >>   {
> >> +	struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick);
> >> +
> >>   	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
> >> +	exynos4_mct_tick_clear(mevt);
> >>   	if (mct_int_type == MCT_INT_SPI)
> >>   		free_irq(evt->irq, this_cpu_ptr(&percpu_mct_tick));
> >>   	else
> >
> 
> 

Re: [PATCH 1/3] clocksource: exynos_mct: Fix stall after CPU hotplugging

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2438 bytes --]

On Tue, 15 Apr 2014, Krzysztof Kozlowski wrote:

> On wto, 2014-04-15 at 14:28 +0200, Daniel Lezcano wrote:
> > On 04/15/2014 11:34 AM, Krzysztof Kozlowski wrote:
> > > On pią, 2014-03-28 at 14:06 +0100, Krzysztof Kozlowski wrote:
> > >> Fix stall after hotplugging CPU1. Affected are SoCs where Multi Core Timer
> > >> interrupts are shared (SPI), e.g. Exynos 4210. The stall was a result of
> > >> starting the CPU1 local timer not in L1 timer but in L0 (which is used
> > >> by CPU0).
> > >
> > > Hi,
> > >
> > > Do you have any comments on these 3 patches? They fix the CPU stall on
> > > Exynos4210 and also on Exynos3250 (Chanwoo Choi sent patches for it
> > > recently).
> > 
> > You describe this issue as impacting different SoC not only the exynos, 
> > right ?
> >
> > Do you know what other SoCs are impacted by this ?
> 
> No, affected are only Exynos SoC-s. It was confirmed on Exynos4210
> (Trats board) and Exynos3250 (new SoC, patches for it were recently
> posted by Chanwoo).
> 
> Other Exynos SoC-s where MCT local timers use shared interrupts (SPI)
> can also be affected. Candidates are Exynos 5250 and 5420 but I haven't
> tested them.
> 
> > I guess this issue is not reproducible just with the line below, we need 
> > a timer to expire right at the moment CPU1 is hotplugged, right ?
> 
> Right. The timer must fire in short time between enabling local timer
> for CPU1 and setting the affinity for IRQ.

Why do you set the affinity in the CPU_ONLINE hotplug callback and not
right away when the interrupt is requested?

Thanks,

	tglx


Index: linux-2.6/drivers/clocksource/exynos_mct.c
===================================================================
--- linux-2.6.orig/drivers/clocksource/exynos_mct.c
+++ linux-2.6/drivers/clocksource/exynos_mct.c
@@ -430,6 +430,7 @@ static int exynos4_local_timer_setup(str
 				evt->irq);
 			return -EIO;
 		}
+		irq_set_affinity(mct_irqs[MCT_L0_IRQ + cpu], cpumask_of(cpu));
 	} else {
 		enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0);
 	}
@@ -461,12 +462,6 @@ static int exynos4_mct_cpu_notify(struct
 		mevt = this_cpu_ptr(&percpu_mct_tick);
 		exynos4_local_timer_setup(&mevt->evt);
 		break;
-	case CPU_ONLINE:
-		cpu = (unsigned long)hcpu;
-		if (mct_int_type == MCT_INT_SPI)
-			irq_set_affinity(mct_irqs[MCT_L0_IRQ + cpu],
-						cpumask_of(cpu));
-		break;
 	case CPU_DYING:
 		mevt = this_cpu_ptr(&percpu_mct_tick);
 		exynos4_local_timer_stop(&mevt->evt);

Re: [PATCH 1/3] clocksource: exynos_mct: Fix stall after CPU hotplugging

[Krzysztof Kozlowski]

On wto, 2014-04-15 at 17:20 +0200, Thomas Gleixner wrote:
> On Tue, 15 Apr 2014, Krzysztof Kozlowski wrote:
> 
> > On wto, 2014-04-15 at 14:28 +0200, Daniel Lezcano wrote:
> > > On 04/15/2014 11:34 AM, Krzysztof Kozlowski wrote:
> > > > On pią, 2014-03-28 at 14:06 +0100, Krzysztof Kozlowski wrote:
> > > >> Fix stall after hotplugging CPU1. Affected are SoCs where Multi Core Timer
> > > >> interrupts are shared (SPI), e.g. Exynos 4210. The stall was a result of
> > > >> starting the CPU1 local timer not in L1 timer but in L0 (which is used
> > > >> by CPU0).
> > > >
> > > > Hi,
> > > >
> > > > Do you have any comments on these 3 patches? They fix the CPU stall on
> > > > Exynos4210 and also on Exynos3250 (Chanwoo Choi sent patches for it
> > > > recently).
> > > 
> > > You describe this issue as impacting different SoC not only the exynos, 
> > > right ?
> > >
> > > Do you know what other SoCs are impacted by this ?
> > 
> > No, affected are only Exynos SoC-s. It was confirmed on Exynos4210
> > (Trats board) and Exynos3250 (new SoC, patches for it were recently
> > posted by Chanwoo).
> > 
> > Other Exynos SoC-s where MCT local timers use shared interrupts (SPI)
> > can also be affected. Candidates are Exynos 5250 and 5420 but I haven't
> > tested them.
> > 
> > > I guess this issue is not reproducible just with the line below, we need 
> > > a timer to expire right at the moment CPU1 is hotplugged, right ?
> > 
> > Right. The timer must fire in short time between enabling local timer
> > for CPU1 and setting the affinity for IRQ.
> 
> Why do you set the affinity in the CPU_ONLINE hotplug callback and not
> right away when the interrupt is requested?

Hi,

I think the problem in such code is in GIC. The gic_set_affinity() uses
cpu_online_mask:
	unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
In that time this CPU is not present in that mask so -EINVAL would be
returned.

The stall occurred also on 3.10 where the IRQ affinity is set just after
setup_irq():

if (cpu == 0) {
	mct_tick0_event_irq.dev_id = mevt;
	evt->irq = mct_irqs[MCT_L0_IRQ];
	setup_irq(evt->irq, &mct_tick0_event_irq);
} else {
	mct_tick1_event_irq.dev_id = mevt;
	evt->irq = mct_irqs[MCT_L1_IRQ];
	setup_irq(evt->irq, &mct_tick1_event_irq);
	irq_set_affinity(evt->irq, cpumask_of(1));
}

Best regards,
Krzysztof


> Thanks,
> 
> 	tglx
> 
> 
> Index: linux-2.6/drivers/clocksource/exynos_mct.c
> ===================================================================
> --- linux-2.6.orig/drivers/clocksource/exynos_mct.c
> +++ linux-2.6/drivers/clocksource/exynos_mct.c
> @@ -430,6 +430,7 @@ static int exynos4_local_timer_setup(str
>  				evt->irq);
>  			return -EIO;
>  		}
> +		irq_set_affinity(mct_irqs[MCT_L0_IRQ + cpu], cpumask_of(cpu));
>  	} else {
>  		enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0);
>  	}
> @@ -461,12 +462,6 @@ static int exynos4_mct_cpu_notify(struct
>  		mevt = this_cpu_ptr(&percpu_mct_tick);
>  		exynos4_local_timer_setup(&mevt->evt);
>  		break;
> -	case CPU_ONLINE:
> -		cpu = (unsigned long)hcpu;
> -		if (mct_int_type == MCT_INT_SPI)
> -			irq_set_affinity(mct_irqs[MCT_L0_IRQ + cpu],
> -						cpumask_of(cpu));
> -		break;
>  	case CPU_DYING:
>  		mevt = this_cpu_ptr(&percpu_mct_tick);
>  		exynos4_local_timer_stop(&mevt->evt);
> 
> 
> 

Re: [PATCH 1/3] clocksource: exynos_mct: Fix stall after CPU hotplugging

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7330 bytes --]

B1;3202;0cOn Tue, 15 Apr 2014, Krzysztof Kozlowski wrote:
> On wto, 2014-04-15 at 17:20 +0200, Thomas Gleixner wrote:
> > On Tue, 15 Apr 2014, Krzysztof Kozlowski wrote:
> > 
> > > On wto, 2014-04-15 at 14:28 +0200, Daniel Lezcano wrote:
> > > > On 04/15/2014 11:34 AM, Krzysztof Kozlowski wrote:
> > > > > On pią, 2014-03-28 at 14:06 +0100, Krzysztof Kozlowski wrote:
> > > > >> Fix stall after hotplugging CPU1. Affected are SoCs where Multi Core Timer
> > > > >> interrupts are shared (SPI), e.g. Exynos 4210. The stall was a result of
> > > > >> starting the CPU1 local timer not in L1 timer but in L0 (which is used
> > > > >> by CPU0).
> > > > >
> > > > > Hi,
> > > > >
> > > > > Do you have any comments on these 3 patches? They fix the CPU stall on
> > > > > Exynos4210 and also on Exynos3250 (Chanwoo Choi sent patches for it
> > > > > recently).
> > > > 
> > > > You describe this issue as impacting different SoC not only the exynos, 
> > > > right ?
> > > >
> > > > Do you know what other SoCs are impacted by this ?
> > > 
> > > No, affected are only Exynos SoC-s. It was confirmed on Exynos4210
> > > (Trats board) and Exynos3250 (new SoC, patches for it were recently
> > > posted by Chanwoo).
> > > 
> > > Other Exynos SoC-s where MCT local timers use shared interrupts (SPI)
> > > can also be affected. Candidates are Exynos 5250 and 5420 but I haven't
> > > tested them.
> > > 
> > > > I guess this issue is not reproducible just with the line below, we need 
> > > > a timer to expire right at the moment CPU1 is hotplugged, right ?
> > > 
> > > Right. The timer must fire in short time between enabling local timer
> > > for CPU1 and setting the affinity for IRQ.
> > 
> > Why do you set the affinity in the CPU_ONLINE hotplug callback and not
> > right away when the interrupt is requested?
> 
> Hi,
> 
> I think the problem in such code is in GIC. The gic_set_affinity() uses
> cpu_online_mask:
> 	unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
> In that time this CPU is not present in that mask so -EINVAL would be
> returned.

Hmm, indeed. Stupid me.

Here is a complete solution to the problem. We really want the drivers
to be fast and clean and not work around such issues.

I'm quite happy that I kept the 'force' argument of set_affinity
callbacks. I knew that I'd need it at some point.

So with the flag set we can disable the online mask check and force
the interrupt to the proper cpu.
 
Thanks,

	tglx

Index: linux-2.6/drivers/clocksource/exynos_mct.c
===================================================================
--- linux-2.6.orig/drivers/clocksource/exynos_mct.c
+++ linux-2.6/drivers/clocksource/exynos_mct.c
@@ -430,6 +430,7 @@ static int exynos4_local_timer_setup(str
 				evt->irq);
 			return -EIO;
 		}
+		irq_force_affinity(mct_irqs[MCT_L0_IRQ + cpu], cpumask_of(cpu));
 	} else {
 		enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0);
 	}
@@ -461,12 +462,6 @@ static int exynos4_mct_cpu_notify(struct
 		mevt = this_cpu_ptr(&percpu_mct_tick);
 		exynos4_local_timer_setup(&mevt->evt);
 		break;
-	case CPU_ONLINE:
-		cpu = (unsigned long)hcpu;
-		if (mct_int_type == MCT_INT_SPI)
-			irq_set_affinity(mct_irqs[MCT_L0_IRQ + cpu],
-						cpumask_of(cpu));
-		break;
 	case CPU_DYING:
 		mevt = this_cpu_ptr(&percpu_mct_tick);
 		exynos4_local_timer_stop(&mevt->evt);
Index: linux-2.6/drivers/irqchip/irq-gic.c
===================================================================
--- linux-2.6.orig/drivers/irqchip/irq-gic.c
+++ linux-2.6/drivers/irqchip/irq-gic.c
@@ -246,10 +246,14 @@ static int gic_set_affinity(struct irq_d
 			    bool force)
 {
 	void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
-	unsigned int shift = (gic_irq(d) % 4) * 8;
-	unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
+	unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
 	u32 val, mask, bit;
 
+	if (!force)
+		cpu = cpumask_any_and(mask_val, cpu_online_mask);
+	else
+		cpu = cpumask_first(mask_val);
+
 	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
 		return -EINVAL;
 
Index: linux-2.6/include/linux/interrupt.h
===================================================================
--- linux-2.6.orig/include/linux/interrupt.h
+++ linux-2.6/include/linux/interrupt.h
@@ -204,6 +204,7 @@ static inline int check_wakeup_irqs(void
 extern cpumask_var_t irq_default_affinity;
 
 extern int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask);
+extern int irq_force_affinity(unsigned int irq, const struct cpumask *cpumask);
 extern int irq_can_set_affinity(unsigned int irq);
 extern int irq_select_affinity(unsigned int irq);
 
Index: linux-2.6/kernel/irq/manage.c
===================================================================
--- linux-2.6.orig/kernel/irq/manage.c
+++ linux-2.6/kernel/irq/manage.c
@@ -180,7 +180,7 @@ int irq_do_set_affinity(struct irq_data
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 	int ret;
 
-	ret = chip->irq_set_affinity(data, mask, false);
+	ret = chip->irq_set_affinity(data, mask, force);
 	switch (ret) {
 	case IRQ_SET_MASK_OK:
 		cpumask_copy(data->affinity, mask);
@@ -192,7 +192,8 @@ int irq_do_set_affinity(struct irq_data
 	return ret;
 }
 
-int __irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask)
+static int irq_do_set_affinity_locked(struct irq_data *data,
+				      const struct cpumask *mask, bool force)
 {
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 	struct irq_desc *desc = irq_data_to_desc(data);
@@ -202,7 +203,7 @@ int __irq_set_affinity_locked(struct irq
 		return -EINVAL;
 
 	if (irq_can_move_pcntxt(data)) {
-		ret = irq_do_set_affinity(data, mask, false);
+		ret = irq_do_set_affinity(data, mask, force);
 	} else {
 		irqd_set_move_pending(data);
 		irq_copy_pending(desc, mask);
@@ -217,13 +218,13 @@ int __irq_set_affinity_locked(struct irq
 	return ret;
 }
 
-/**
- *	irq_set_affinity - Set the irq affinity of a given irq
- *	@irq:		Interrupt to set affinity
- *	@mask:		cpumask
- *
- */
-int irq_set_affinity(unsigned int irq, const struct cpumask *mask)
+int __irq_set_affinity_locked(struct irq_data *data,const struct cpumask *mask)
+{
+	return irq_do_set_affinity_locked(data, mask, false);
+}
+
+static int __irq_set_affinity(unsigned int irq, const struct cpumask *mask,
+			      bool force)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	unsigned long flags;
@@ -233,11 +234,33 @@ int irq_set_affinity(unsigned int irq, c
 		return -EINVAL;
 
 	raw_spin_lock_irqsave(&desc->lock, flags);
-	ret =  __irq_set_affinity_locked(irq_desc_get_irq_data(desc), mask);
+	ret = irq_do_set_affinity_locked(irq_desc_get_irq_data(desc), mask,
+					 force);
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 	return ret;
 }
 
+/**
+ *	irq_force_affinity - Force the irq affinity of a given irq
+ *	@irq:		Interrupt to set affinity
+ *	@mask:		cpumask
+ */
+int irq_force_affinity(unsigned int irq, const struct cpumask *mask)
+{
+	return __irq_set_affinity(irq, mask, true);
+}
+
+/**
+ *	irq_set_affinity - Set the irq affinity of a given irq
+ *	@irq:		Interrupt to set affinity
+ *	@mask:		cpumask
+ *
+ */
+int irq_set_affinity(unsigned int irq, const struct cpumask *mask)
+{
+	return __irq_set_affinity(irq, mask, false);
+}
+
 int irq_set_affinity_hint(unsigned int irq, const struct cpumask *m)
 {
 	unsigned long flags;

Re: [PATCH 1/3] clocksource: exynos_mct: Fix stall after CPU hotplugging

[Krzysztof Kozlowski]

On wto, 2014-04-15 at 18:20 +0200, Thomas Gleixner wrote:
> B1;3202;0cOn Tue, 15 Apr 2014, Krzysztof Kozlowski wrote:
> > On wto, 2014-04-15 at 17:20 +0200, Thomas Gleixner wrote:
> > > On Tue, 15 Apr 2014, Krzysztof Kozlowski wrote:
> > > 
> > > > On wto, 2014-04-15 at 14:28 +0200, Daniel Lezcano wrote:
> > > > > On 04/15/2014 11:34 AM, Krzysztof Kozlowski wrote:
> > > > > > On pią, 2014-03-28 at 14:06 +0100, Krzysztof Kozlowski wrote:
> > > > > >> Fix stall after hotplugging CPU1. Affected are SoCs where Multi Core Timer
> > > > > >> interrupts are shared (SPI), e.g. Exynos 4210. The stall was a result of
> > > > > >> starting the CPU1 local timer not in L1 timer but in L0 (which is used
> > > > > >> by CPU0).
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Do you have any comments on these 3 patches? They fix the CPU stall on
> > > > > > Exynos4210 and also on Exynos3250 (Chanwoo Choi sent patches for it
> > > > > > recently).
> > > > > 
> > > > > You describe this issue as impacting different SoC not only the exynos, 
> > > > > right ?
> > > > >
> > > > > Do you know what other SoCs are impacted by this ?
> > > > 
> > > > No, affected are only Exynos SoC-s. It was confirmed on Exynos4210
> > > > (Trats board) and Exynos3250 (new SoC, patches for it were recently
> > > > posted by Chanwoo).
> > > > 
> > > > Other Exynos SoC-s where MCT local timers use shared interrupts (SPI)
> > > > can also be affected. Candidates are Exynos 5250 and 5420 but I haven't
> > > > tested them.
> > > > 
> > > > > I guess this issue is not reproducible just with the line below, we need 
> > > > > a timer to expire right at the moment CPU1 is hotplugged, right ?
> > > > 
> > > > Right. The timer must fire in short time between enabling local timer
> > > > for CPU1 and setting the affinity for IRQ.
> > > 
> > > Why do you set the affinity in the CPU_ONLINE hotplug callback and not
> > > right away when the interrupt is requested?
> > 
> > Hi,
> > 
> > I think the problem in such code is in GIC. The gic_set_affinity() uses
> > cpu_online_mask:
> > 	unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
> > In that time this CPU is not present in that mask so -EINVAL would be
> > returned.
> 
> Hmm, indeed. Stupid me.
> 
> Here is a complete solution to the problem. We really want the drivers
> to be fast and clean and not work around such issues.
> 
> I'm quite happy that I kept the 'force' argument of set_affinity
> callbacks. I knew that I'd need it at some point.
> 
> So with the flag set we can disable the online mask check and force
> the interrupt to the proper cpu.

Thanks for the solution.

I tested your patch on Exynos 3250 and it is still not sufficient. After
hotplugging CPU1 ~10 times the stall happens (set_next_event is called
on wrong CPU).

The patch 3/3 ("clocksource: exynos_mct: Fix too early ISR fire up on
wrong CPU") is needed as the clockevents_config_and_register should be
called a little later.

Do you have rest of patches (2/3 and 3/3) or should I resend them?

And one minor nit in your patch: 'cpu' local variable in
exynos4_mct_cpu_notify() is no longer used so it can be removed.

Best regards,
Krzysztof


> Thanks,
> 
> 	tglx
> 
> Index: linux-2.6/drivers/clocksource/exynos_mct.c
> ===================================================================
> --- linux-2.6.orig/drivers/clocksource/exynos_mct.c
> +++ linux-2.6/drivers/clocksource/exynos_mct.c
> @@ -430,6 +430,7 @@ static int exynos4_local_timer_setup(str
>  				evt->irq);
>  			return -EIO;
>  		}
> +		irq_force_affinity(mct_irqs[MCT_L0_IRQ + cpu], cpumask_of(cpu));
>  	} else {
>  		enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0);
>  	}
> @@ -461,12 +462,6 @@ static int exynos4_mct_cpu_notify(struct
>  		mevt = this_cpu_ptr(&percpu_mct_tick);
>  		exynos4_local_timer_setup(&mevt->evt);
>  		break;
> -	case CPU_ONLINE:
> -		cpu = (unsigned long)hcpu;
> -		if (mct_int_type == MCT_INT_SPI)
> -			irq_set_affinity(mct_irqs[MCT_L0_IRQ + cpu],
> -						cpumask_of(cpu));
> -		break;
>  	case CPU_DYING:
>  		mevt = this_cpu_ptr(&percpu_mct_tick);
>  		exynos4_local_timer_stop(&mevt->evt);
> Index: linux-2.6/drivers/irqchip/irq-gic.c
> ===================================================================
> --- linux-2.6.orig/drivers/irqchip/irq-gic.c
> +++ linux-2.6/drivers/irqchip/irq-gic.c
> @@ -246,10 +246,14 @@ static int gic_set_affinity(struct irq_d
>  			    bool force)
>  {
>  	void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
> -	unsigned int shift = (gic_irq(d) % 4) * 8;
> -	unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
> +	unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
>  	u32 val, mask, bit;
>  
> +	if (!force)
> +		cpu = cpumask_any_and(mask_val, cpu_online_mask);
> +	else
> +		cpu = cpumask_first(mask_val);
> +
>  	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
>  		return -EINVAL;
>  
> Index: linux-2.6/include/linux/interrupt.h
> ===================================================================
> --- linux-2.6.orig/include/linux/interrupt.h
> +++ linux-2.6/include/linux/interrupt.h
> @@ -204,6 +204,7 @@ static inline int check_wakeup_irqs(void
>  extern cpumask_var_t irq_default_affinity;
>  
>  extern int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask);
> +extern int irq_force_affinity(unsigned int irq, const struct cpumask *cpumask);
>  extern int irq_can_set_affinity(unsigned int irq);
>  extern int irq_select_affinity(unsigned int irq);
>  
> Index: linux-2.6/kernel/irq/manage.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq/manage.c
> +++ linux-2.6/kernel/irq/manage.c
> @@ -180,7 +180,7 @@ int irq_do_set_affinity(struct irq_data
>  	struct irq_chip *chip = irq_data_get_irq_chip(data);
>  	int ret;
>  
> -	ret = chip->irq_set_affinity(data, mask, false);
> +	ret = chip->irq_set_affinity(data, mask, force);
>  	switch (ret) {
>  	case IRQ_SET_MASK_OK:
>  		cpumask_copy(data->affinity, mask);
> @@ -192,7 +192,8 @@ int irq_do_set_affinity(struct irq_data
>  	return ret;
>  }
>  
> -int __irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask)
> +static int irq_do_set_affinity_locked(struct irq_data *data,
> +				      const struct cpumask *mask, bool force)
>  {
>  	struct irq_chip *chip = irq_data_get_irq_chip(data);
>  	struct irq_desc *desc = irq_data_to_desc(data);
> @@ -202,7 +203,7 @@ int __irq_set_affinity_locked(struct irq
>  		return -EINVAL;
>  
>  	if (irq_can_move_pcntxt(data)) {
> -		ret = irq_do_set_affinity(data, mask, false);
> +		ret = irq_do_set_affinity(data, mask, force);
>  	} else {
>  		irqd_set_move_pending(data);
>  		irq_copy_pending(desc, mask);
> @@ -217,13 +218,13 @@ int __irq_set_affinity_locked(struct irq
>  	return ret;
>  }
>  
> -/**
> - *	irq_set_affinity - Set the irq affinity of a given irq
> - *	@irq:		Interrupt to set affinity
> - *	@mask:		cpumask
> - *
> - */
> -int irq_set_affinity(unsigned int irq, const struct cpumask *mask)
> +int __irq_set_affinity_locked(struct irq_data *data,const struct cpumask *mask)
> +{
> +	return irq_do_set_affinity_locked(data, mask, false);
> +}
> +
> +static int __irq_set_affinity(unsigned int irq, const struct cpumask *mask,
> +			      bool force)
>  {
>  	struct irq_desc *desc = irq_to_desc(irq);
>  	unsigned long flags;
> @@ -233,11 +234,33 @@ int irq_set_affinity(unsigned int irq, c
>  		return -EINVAL;
>  
>  	raw_spin_lock_irqsave(&desc->lock, flags);
> -	ret =  __irq_set_affinity_locked(irq_desc_get_irq_data(desc), mask);
> +	ret = irq_do_set_affinity_locked(irq_desc_get_irq_data(desc), mask,
> +					 force);
>  	raw_spin_unlock_irqrestore(&desc->lock, flags);
>  	return ret;
>  }
>  
> +/**
> + *	irq_force_affinity - Force the irq affinity of a given irq
> + *	@irq:		Interrupt to set affinity
> + *	@mask:		cpumask
> + */
> +int irq_force_affinity(unsigned int irq, const struct cpumask *mask)
> +{
> +	return __irq_set_affinity(irq, mask, true);
> +}
> +
> +/**
> + *	irq_set_affinity - Set the irq affinity of a given irq
> + *	@irq:		Interrupt to set affinity
> + *	@mask:		cpumask
> + *
> + */
> +int irq_set_affinity(unsigned int irq, const struct cpumask *mask)
> +{
> +	return __irq_set_affinity(irq, mask, false);
> +}
> +
>  int irq_set_affinity_hint(unsigned int irq, const struct cpumask *m)
>  {
>  	unsigned long flags;

Re: [PATCH 1/3] clocksource: exynos_mct: Fix stall after CPU hotplugging

[Thomas Gleixner]

On Wed, 16 Apr 2014, Krzysztof Kozlowski wrote:
> On wto, 2014-04-15 at 18:20 +0200, Thomas Gleixner wrote:
> > Here is a complete solution to the problem. We really want the drivers
> > to be fast and clean and not work around such issues.
> > 
> > I'm quite happy that I kept the 'force' argument of set_affinity
> > callbacks. I knew that I'd need it at some point.
> > 
> > So with the flag set we can disable the online mask check and force
> > the interrupt to the proper cpu.
> 
> Thanks for the solution.
> 
> I tested your patch on Exynos 3250 and it is still not sufficient. After
> hotplugging CPU1 ~10 times the stall happens (set_next_event is called
> on wrong CPU).
> 
> The patch 3/3 ("clocksource: exynos_mct: Fix too early ISR fire up on
> wrong CPU") is needed as the clockevents_config_and_register should be
> called a little later.

Ok. That makes sense.
 
> Do you have rest of patches (2/3 and 3/3) or should I resend them?
> 
> And one minor nit in your patch: 'cpu' local variable in
> exynos4_mct_cpu_notify() is no longer used so it can be removed.

Right.

I'm going to create a proper patch for the interrupt side and stick
that into irq/urgent. It's trivial enough to go in right away and I'll
tag it stable as well. You can build your changes on top then.

Thanks,

	tglx