[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[ning.n.jiang at gmail.com]

From: Ning Jiang <ning.n.jiang@gmail.com>

Currently there are two problems when we try to stop local timer.
First, it calls set_mode function directly so mode state is not
updated for the clock event device. Second, it makes the device
unused instead of shutdown.

A subtle error will happen because of it. When a cpu is plugged out
it will stop the local timer. It will call tick_nohz_idle_enter()
in idle thread afterwards. It will cancel the sched timer and try
to reprogram the next event. This is wrong since the local timer
is supposed to be stopped.

The right way to stop the local timer is to shutdown it by calling
clockevents_set_mode(). Thus when we try to reprogram the clock
event device, it will return directly without doing anything since
the clock mode is CLOCK_EVT_MODE_SHUTDOWN.

Signed-off-by: Ning Jiang <ning.n.jiang@gmail.com>
---
 arch/arm/kernel/smp_twd.c                |    2 +-
 arch/arm/mach-exynos/mct.c               |    2 +-
 arch/arm/mach-msm/timer.c                |    2 +-
 drivers/clocksource/arm_arch_timer.c     |    2 +-
 drivers/clocksource/time-armada-370-xp.c |    2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 3f25650..c1d4ab4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -92,7 +92,7 @@ static int twd_timer_ack(void)
 
 static void twd_timer_stop(struct clock_event_device *clk)
 {
-	twd_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
+	clockevents_set_mode(clk, CLOCK_EVT_MODE_SHUTDOWN);
 	disable_percpu_irq(clk->irq);
 }
 
diff --git a/arch/arm/mach-exynos/mct.c b/arch/arm/mach-exynos/mct.c
index c9d6650..b2d0d46 100644
--- a/arch/arm/mach-exynos/mct.c
+++ b/arch/arm/mach-exynos/mct.c
@@ -429,7 +429,7 @@ static int __cpuinit exynos4_local_timer_setup(struct clock_event_device *evt)
 static void exynos4_local_timer_stop(struct clock_event_device *evt)
 {
 	unsigned int cpu = smp_processor_id();
-	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
+	clockevents_set_mode(evt, CLOCK_EVT_MODE_SHUTDOWN);
 	if (mct_int_type == MCT_INT_SPI)
 		if (cpu == 0)
 			remove_irq(evt->irq, &mct_tick0_event_irq);
diff --git a/arch/arm/mach-msm/timer.c b/arch/arm/mach-msm/timer.c
index 2969027..9b0cc66 100644
--- a/arch/arm/mach-msm/timer.c
+++ b/arch/arm/mach-msm/timer.c
@@ -152,7 +152,7 @@ static int __cpuinit msm_local_timer_setup(struct clock_event_device *evt)
 
 static void msm_local_timer_stop(struct clock_event_device *evt)
 {
-	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
+	clockevents_set_mode(evt, CLOCK_EVT_MODE_SHUTDOWN);
 	disable_percpu_irq(evt->irq);
 }
 
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index d7ad425..beecb26 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -242,7 +242,7 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
 			disable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI]);
 	}
 
-	clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk);
+	clockevents_set_mode(clk, CLOCK_EVT_MODE_SHUTDOWN);
 }
 
 static int __cpuinit arch_timer_cpu_notify(struct notifier_block *self,
diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
index 47a6730..bfb2971 100644
--- a/drivers/clocksource/time-armada-370-xp.c
+++ b/drivers/clocksource/time-armada-370-xp.c
@@ -201,7 +201,7 @@ static int __cpuinit armada_370_xp_timer_setup(struct clock_event_device *evt)
 
 static void  armada_370_xp_timer_stop(struct clock_event_device *evt)
 {
-	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
+	clockevents_set_mode(evt, CLOCK_EVT_MODE_SHUTDOWN);
 	disable_percpu_irq(evt->irq);
 }
 
-- 
1.7.1

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[Stephen Boyd]

On 03/29/13 02:24, ning.n.jiang at gmail.com wrote:
> From: Ning Jiang <ning.n.jiang@gmail.com>
>
> Currently there are two problems when we try to stop local timer.
> First, it calls set_mode function directly so mode state is not
> updated for the clock event device. Second, it makes the device
> unused instead of shutdown.

What device is this a problem on? I believe this only matters to drivers
which enable their timer in their set_next_event() callback? But even
then, does anything actually happen because the interrupt should have
been disabled in the local timer stop callback.

>
> A subtle error will happen because of it. When a cpu is plugged out
> it will stop the local timer. It will call tick_nohz_idle_enter()
> in idle thread afterwards. It will cancel the sched timer and try
> to reprogram the next event. This is wrong since the local timer
> is supposed to be stopped.
>
> The right way to stop the local timer is to shutdown it by calling
> clockevents_set_mode(). Thus when we try to reprogram the clock
> event device, it will return directly without doing anything since
> the clock mode is CLOCK_EVT_MODE_SHUTDOWN.

While this prevents the set_next_event() callback from being called on a
dying CPU, wouldn't it be better to fix this problem in the core code
once instead of fixing it many times in each local timer driver? It
doesn't seem to make much sense to program an event on a CPU that is
about to die, so why do we do that?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[Ning Jiang]

2013/3/30 Stephen Boyd <sboyd@codeaurora.org>:
> On 03/29/13 02:24, ning.n.jiang at gmail.com wrote:
>> From: Ning Jiang <ning.n.jiang@gmail.com>
>>
>> Currently there are two problems when we try to stop local timer.
>> First, it calls set_mode function directly so mode state is not
>> updated for the clock event device. Second, it makes the device
>> unused instead of shutdown.
>
> What device is this a problem on? I believe this only matters to drivers
> which enable their timer in their set_next_event() callback? But even
> then, does anything actually happen because the interrupt should have
> been disabled in the local timer stop callback.
>

Right. Drivers which enable timer in set_next_event() will have this problem.
It will not have functional problem in my case. But my device cannot enter
low power mode with a pending interrupt even if it is disabled.

>>
>> A subtle error will happen because of it. When a cpu is plugged out
>> it will stop the local timer. It will call tick_nohz_idle_enter()
>> in idle thread afterwards. It will cancel the sched timer and try
>> to reprogram the next event. This is wrong since the local timer
>> is supposed to be stopped.
>>
>> The right way to stop the local timer is to shutdown it by calling
>> clockevents_set_mode(). Thus when we try to reprogram the clock
>> event device, it will return directly without doing anything since
>> the clock mode is CLOCK_EVT_MODE_SHUTDOWN.
>
> While this prevents the set_next_event() callback from being called on a
> dying CPU, wouldn't it be better to fix this problem in the core code
> once instead of fixing it many times in each local timer driver? It
> doesn't seem to make much sense to program an event on a CPU that is
> about to die, so why do we do that?
>

Actually, I was trying to fix it in the core code like this, but I
thought it is not
that good and we still need to fix the local timer driver problem even
with this fix.

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index c6d6400..e22e268 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -210,6 +210,9 @@ int clockevents_program_event(struct
clock_event_device *dev, ktime_t expires,
                return -ETIME;
        }

+       if (cpu_is_offline(smp_processor_id()))
+               return 0;
+
        dev->next_event = expires;

        if (dev->mode == CLOCK_EVT_MODE_SHUTDOWN)

> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[Russell King - ARM Linux]

On Sat, Mar 30, 2013 at 05:57:38PM +0800, Ning Jiang wrote:
> 2013/3/30 Stephen Boyd <sboyd@codeaurora.org>:
> > On 03/29/13 02:24, ning.n.jiang at gmail.com wrote:
> >> From: Ning Jiang <ning.n.jiang@gmail.com>
> >>
> >> Currently there are two problems when we try to stop local timer.
> >> First, it calls set_mode function directly so mode state is not
> >> updated for the clock event device. Second, it makes the device
> >> unused instead of shutdown.
> >
> > What device is this a problem on? I believe this only matters to drivers
> > which enable their timer in their set_next_event() callback? But even
> > then, does anything actually happen because the interrupt should have
> > been disabled in the local timer stop callback.
> >
> 
> Right. Drivers which enable timer in set_next_event() will have this problem.
> It will not have functional problem in my case. But my device cannot enter
> low power mode with a pending interrupt even if it is disabled.

You're not telling us what you have discovered.  How does set_next_event()
get called after we've set the mode to UNUSED in the current code?

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[Ning Jiang]

2013/3/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sat, Mar 30, 2013 at 05:57:38PM +0800, Ning Jiang wrote:
>> 2013/3/30 Stephen Boyd <sboyd@codeaurora.org>:
>> > On 03/29/13 02:24, ning.n.jiang at gmail.com wrote:
>> >> From: Ning Jiang <ning.n.jiang@gmail.com>
>> >>
>> >> Currently there are two problems when we try to stop local timer.
>> >> First, it calls set_mode function directly so mode state is not
>> >> updated for the clock event device. Second, it makes the device
>> >> unused instead of shutdown.
>> >
>> > What device is this a problem on? I believe this only matters to drivers
>> > which enable their timer in their set_next_event() callback? But even
>> > then, does anything actually happen because the interrupt should have
>> > been disabled in the local timer stop callback.
>> >
>>
>> Right. Drivers which enable timer in set_next_event() will have this problem.
>> It will not have functional problem in my case. But my device cannot enter
>> low power mode with a pending interrupt even if it is disabled.
>
> You're not telling us what you have discovered.  How does set_next_event()
> get called after we've set the mode to UNUSED in the current code?

In the current code we did not set the mode to UNUSED but only call
set_mode callback function for the clock event device. This normally
disables current clock event device. The dying CPU eventually will
switch to idle thread, call tick_nohz_idle_enter(), try to cancel the
sched_timer and reprogram the next event. Then set_next_event() gets
called. The call stack will be like:

tick_nohz_idle_enter
  -> __tick_nohz_idle_enter
    -> tick_nohz_stop_sched_tick
      -> hrtimer_cancel
        -> hrtimer_try_to_cancel
          -> remove_hrtimer
            -> __remove_hrtimer
              -> hrtimer_force_reprogram
                -> tick_program_event
                  -> clockevents_program_event
                    -> set_next_event

In set_next_event() we'll re-enable and re-program the clock event device.

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[Ning Jiang]

2013/3/30 Ning Jiang <ning.n.jiang@gmail.com>:
> 2013/3/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> On Sat, Mar 30, 2013 at 05:57:38PM +0800, Ning Jiang wrote:
>>> 2013/3/30 Stephen Boyd <sboyd@codeaurora.org>:
>>> > On 03/29/13 02:24, ning.n.jiang at gmail.com wrote:
>>> >> From: Ning Jiang <ning.n.jiang@gmail.com>
>>> >>
>>> >> Currently there are two problems when we try to stop local timer.
>>> >> First, it calls set_mode function directly so mode state is not
>>> >> updated for the clock event device. Second, it makes the device
>>> >> unused instead of shutdown.
>>> >
>>> > What device is this a problem on? I believe this only matters to drivers
>>> > which enable their timer in their set_next_event() callback? But even
>>> > then, does anything actually happen because the interrupt should have
>>> > been disabled in the local timer stop callback.
>>> >
>>>
>>> Right. Drivers which enable timer in set_next_event() will have this problem.
>>> It will not have functional problem in my case. But my device cannot enter
>>> low power mode with a pending interrupt even if it is disabled.
>>
>> You're not telling us what you have discovered.  How does set_next_event()
>> get called after we've set the mode to UNUSED in the current code?
>
> In the current code we did not set the mode to UNUSED but only call
> set_mode callback function for the clock event device. This normally
> disables current clock event device. The dying CPU eventually will
> switch to idle thread, call tick_nohz_idle_enter(), try to cancel the
> sched_timer and reprogram the next event. Then set_next_event() gets
> called. The call stack will be like:
>
> tick_nohz_idle_enter
>   -> __tick_nohz_idle_enter
>     -> tick_nohz_stop_sched_tick
>       -> hrtimer_cancel
>         -> hrtimer_try_to_cancel
>           -> remove_hrtimer
>             -> __remove_hrtimer
>               -> hrtimer_force_reprogram
>                 -> tick_program_event
>                   -> clockevents_program_event
>                     -> set_next_event
>
> In set_next_event() we'll re-enable and re-program the clock event device.

I think there are two problems here:
1. We should use clockevents_set_mode() instead of calling set_mode
callback directly. This is the issue my patch was trying to fix.
2. We shouldn't program a clock event device for a dying CPU anyway. I
can submit another patch if agreed.

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[Daniel Lezcano]

On 03/29/2013 10:24 AM, ning.n.jiang at gmail.com wrote:
> From: Ning Jiang <ning.n.jiang@gmail.com>
> 
> Currently there are two problems when we try to stop local timer.
> First, it calls set_mode function directly so mode state is not
> updated for the clock event device. Second, it makes the device
> unused instead of shutdown.
> 
> A subtle error will happen because of it. When a cpu is plugged out
> it will stop the local timer. It will call tick_nohz_idle_enter()
> in idle thread afterwards. It will cancel the sched timer and try
> to reprogram the next event. This is wrong since the local timer
> is supposed to be stopped.
> 
> The right way to stop the local timer is to shutdown it by calling
> clockevents_set_mode(). Thus when we try to reprogram the clock
> event device, it will return directly without doing anything since
> the clock mode is CLOCK_EVT_MODE_SHUTDOWN.
> 
> Signed-off-by: Ning Jiang <ning.n.jiang@gmail.com>
> ---
>  arch/arm/kernel/smp_twd.c                |    2 +-
>  arch/arm/mach-exynos/mct.c               |    2 +-
>  arch/arm/mach-msm/timer.c                |    2 +-
>  drivers/clocksource/arm_arch_timer.c     |    2 +-
>  drivers/clocksource/time-armada-370-xp.c |    2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index 3f25650..c1d4ab4 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -92,7 +92,7 @@ static int twd_timer_ack(void)
>  
>  static void twd_timer_stop(struct clock_event_device *clk)
>  {
> -	twd_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
> +	clockevents_set_mode(clk, CLOCK_EVT_MODE_SHUTDOWN);
>  	disable_percpu_irq(clk->irq);

Wouldn't be clockevents_shutdown more adequate here ? The next event
will be also set.



-- 
 <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

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[Ning Jiang]

2013/4/1 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 03/29/2013 10:24 AM, ning.n.jiang at gmail.com wrote:
>> From: Ning Jiang <ning.n.jiang@gmail.com>
>>
>> Currently there are two problems when we try to stop local timer.
>> First, it calls set_mode function directly so mode state is not
>> updated for the clock event device. Second, it makes the device
>> unused instead of shutdown.
>>
>> A subtle error will happen because of it. When a cpu is plugged out
>> it will stop the local timer. It will call tick_nohz_idle_enter()
>> in idle thread afterwards. It will cancel the sched timer and try
>> to reprogram the next event. This is wrong since the local timer
>> is supposed to be stopped.
>>
>> The right way to stop the local timer is to shutdown it by calling
>> clockevents_set_mode(). Thus when we try to reprogram the clock
>> event device, it will return directly without doing anything since
>> the clock mode is CLOCK_EVT_MODE_SHUTDOWN.
>>
>> Signed-off-by: Ning Jiang <ning.n.jiang@gmail.com>
>> ---
>>  arch/arm/kernel/smp_twd.c                |    2 +-
>>  arch/arm/mach-exynos/mct.c               |    2 +-
>>  arch/arm/mach-msm/timer.c                |    2 +-
>>  drivers/clocksource/arm_arch_timer.c     |    2 +-
>>  drivers/clocksource/time-armada-370-xp.c |    2 +-
>>  5 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
>> index 3f25650..c1d4ab4 100644
>> --- a/arch/arm/kernel/smp_twd.c
>> +++ b/arch/arm/kernel/smp_twd.c
>> @@ -92,7 +92,7 @@ static int twd_timer_ack(void)
>>
>>  static void twd_timer_stop(struct clock_event_device *clk)
>>  {
>> -     twd_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
>> +     clockevents_set_mode(clk, CLOCK_EVT_MODE_SHUTDOWN);
>>       disable_percpu_irq(clk->irq);
>
> Wouldn't be clockevents_shutdown more adequate here ? The next event
> will be also set.

You're right. clockevents_shutdown seems more appropriate here. I'll
submit a revised patch for it.

> --
>  <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
>

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[Ning Jiang]

2013/4/1 Ning Jiang <ning.n.jiang@gmail.com>:
> 2013/4/1 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 03/29/2013 10:24 AM, ning.n.jiang at gmail.com wrote:
>>> From: Ning Jiang <ning.n.jiang@gmail.com>
>>>
>>> Currently there are two problems when we try to stop local timer.
>>> First, it calls set_mode function directly so mode state is not
>>> updated for the clock event device. Second, it makes the device
>>> unused instead of shutdown.
>>>
>>> A subtle error will happen because of it. When a cpu is plugged out
>>> it will stop the local timer. It will call tick_nohz_idle_enter()
>>> in idle thread afterwards. It will cancel the sched timer and try
>>> to reprogram the next event. This is wrong since the local timer
>>> is supposed to be stopped.
>>>
>>> The right way to stop the local timer is to shutdown it by calling
>>> clockevents_set_mode(). Thus when we try to reprogram the clock
>>> event device, it will return directly without doing anything since
>>> the clock mode is CLOCK_EVT_MODE_SHUTDOWN.
>>>
>>> Signed-off-by: Ning Jiang <ning.n.jiang@gmail.com>
>>> ---
>>>  arch/arm/kernel/smp_twd.c                |    2 +-
>>>  arch/arm/mach-exynos/mct.c               |    2 +-
>>>  arch/arm/mach-msm/timer.c                |    2 +-
>>>  drivers/clocksource/arm_arch_timer.c     |    2 +-
>>>  drivers/clocksource/time-armada-370-xp.c |    2 +-
>>>  5 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
>>> index 3f25650..c1d4ab4 100644
>>> --- a/arch/arm/kernel/smp_twd.c
>>> +++ b/arch/arm/kernel/smp_twd.c
>>> @@ -92,7 +92,7 @@ static int twd_timer_ack(void)
>>>
>>>  static void twd_timer_stop(struct clock_event_device *clk)
>>>  {
>>> -     twd_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
>>> +     clockevents_set_mode(clk, CLOCK_EVT_MODE_SHUTDOWN);
>>>       disable_percpu_irq(clk->irq);
>>
>> Wouldn't be clockevents_shutdown more adequate here ? The next event
>> will be also set.
>
> You're right. clockevents_shutdown seems more appropriate here. I'll
> submit a revised patch for it.
>

Here attached the revised patch. Any more comments?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ARM-timer-Shutdown-clock-event-device-when-stopping-.patch
Type: application/octet-stream
Size: 3765 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130402/5312debc/attachment.obj>

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[Daniel Lezcano]

On 03/29/2013 10:24 AM, ning.n.jiang at gmail.com wrote:
> From: Ning Jiang <ning.n.jiang@gmail.com>
> 
> Currently there are two problems when we try to stop local timer.
> First, it calls set_mode function directly so mode state is not
> updated for the clock event device. Second, it makes the device
> unused instead of shutdown.
> 
> A subtle error will happen because of it. When a cpu is plugged out
> it will stop the local timer. It will call tick_nohz_idle_enter()
> in idle thread afterwards. It will cancel the sched timer and try
> to reprogram the next event. This is wrong since the local timer
> is supposed to be stopped.
> 
> The right way to stop the local timer is to shutdown it by calling
> clockevents_set_mode(). Thus when we try to reprogram the clock
> event device, it will return directly without doing anything since
> the clock mode is CLOCK_EVT_MODE_SHUTDOWN.
> 
> Signed-off-by: Ning Jiang <ning.n.jiang@gmail.com>
> ---


Don't you see a kernel BUG with this patch ?

In the clockevents code, there is:

/**
 * clockevents_notify - notification about relevant events
 */
void clockevents_notify(unsigned long reason, void *arg)
{
	...
	case CLOCK_EVT_NOTIFY_CPU_DEAD:
	...
                cpu = *((int *)arg);
                list_for_each_entry_safe(dev, tmp, &clockevent_devices,
					list) {
                       if (cpumask_test_cpu(cpu, dev->cpumask) &&
                            cpumask_weight(dev->cpumask) == 1 &&
                            !tick_is_broadcast_device(dev)) {
                                BUG_ON(dev->mode !=
					CLOCK_EVT_MODE_UNUSED);

				^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

                                list_del(&dev->list);
                        }
                }
                break;
	...
}

This is called triggered from hrtimer_cpu_notify with the CPU_DEAD event.


-- 
 <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

[PATCH] ARM: timer: Shutdown clock event device when stopping local timer

[Ning Jiang]

2013/4/1 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 03/29/2013 10:24 AM, ning.n.jiang at gmail.com wrote:
>> From: Ning Jiang <ning.n.jiang@gmail.com>
>>
>> Currently there are two problems when we try to stop local timer.
>> First, it calls set_mode function directly so mode state is not
>> updated for the clock event device. Second, it makes the device
>> unused instead of shutdown.
>>
>> A subtle error will happen because of it. When a cpu is plugged out
>> it will stop the local timer. It will call tick_nohz_idle_enter()
>> in idle thread afterwards. It will cancel the sched timer and try
>> to reprogram the next event. This is wrong since the local timer
>> is supposed to be stopped.
>>
>> The right way to stop the local timer is to shutdown it by calling
>> clockevents_set_mode(). Thus when we try to reprogram the clock
>> event device, it will return directly without doing anything since
>> the clock mode is CLOCK_EVT_MODE_SHUTDOWN.
>>
>> Signed-off-by: Ning Jiang <ning.n.jiang@gmail.com>
>> ---
>
>
> Don't you see a kernel BUG with this patch ?
>
> In the clockevents code, there is:
>
> /**
>  * clockevents_notify - notification about relevant events
>  */
> void clockevents_notify(unsigned long reason, void *arg)
> {
>         ...
>         case CLOCK_EVT_NOTIFY_CPU_DEAD:
>         ...
>                 cpu = *((int *)arg);
>                 list_for_each_entry_safe(dev, tmp, &clockevent_devices,
>                                         list) {
>                        if (cpumask_test_cpu(cpu, dev->cpumask) &&
>                             cpumask_weight(dev->cpumask) == 1 &&
>                             !tick_is_broadcast_device(dev)) {
>                                 BUG_ON(dev->mode !=
>                                         CLOCK_EVT_MODE_UNUSED);
>
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>                                 list_del(&dev->list);
>                         }
>                 }
>                 break;
>         ...
> }
>
> This is called triggered from hrtimer_cpu_notify with the CPU_DEAD event.
>

The clockevents_do_notify() in clockevents_notify() will call
tick_notify() which will call tick_shutdown() in turn. tick_shutdown()
will set clock event device mode to UNUSED. So no panic afterwards.

void clockevents_notify(unsigned long reason, void *arg)
{
        ...
        clockevents_do_notify(reason, arg);

        switch (reason) {
        case CLOCK_EVT_NOTIFY_CPU_DEAD:
                ...
                list_for_each_entry_safe(dev, tmp, &clockevent_devices, list) {
                        if (cpumask_test_cpu(cpu, dev->cpumask) &&
                            cpumask_weight(dev->cpumask) == 1 &&
                            !tick_is_broadcast_device(dev)) {
                                BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
                                list_del(&dev->list);
                        }
                }
                ....
        }
        raw_spin_unlock_irqrestore(&clockevents_lock, flags);
}