Re: [PATCH] ARM: timer: Shutdown clock event device when stopping local timer
2013/3/30 Ning Jiang ning.n.ji...@gmail.com: 2013/3/30 Russell King - ARM Linux li...@arm.linux.org.uk: On Sat, Mar 30, 2013 at 05:57:38PM +0800, Ning Jiang wrote: 2013/3/30 Stephen Boyd sb...@codeaurora.org: On 03/29/13 02:24, ning.n.ji...@gmail.com wrote: From: Ning Jiang ning.n.ji...@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. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GENERIC_GPIO considered deprecated
Hi Romain, On Sat, Mar 30, 2013 at 3:07 PM, Romain Naour romain.na...@openwide.fr wrote: Hi Alex, When I read your mail, I was surprised that you were speaking about GPIOs, my pathes for samsung CPUs are intended for timer sub-system. As you can see in this thread, when I send my patches ARM: S3C24XX: Add samsung-time support for s3c24xx and Add samsung-time support for s5pc100. They didn't add select GENERIC_GPIO. http://thread.gmane.org/gmane.linux.kernel.samsung-soc/13432/focus=14980 http://thread.gmane.org/gmane.linux.kernel.samsung-soc/13432/focus=14982 There is something wrong with the commit, I see that select GENERIC_GPIO was added in my patches by mistake. I recommend you to speak directly with samsung's kernel maintainer that I CC in this mail. Indeed, it seems like these select GENERIC_GPIO have been added during the merge of your patches, since I can see the line is here in your patch (but not added by it). Kim, on the current next there are two of these select GENERIC_GPIO that are added from your branch, could you amend the patches that adds them such as they get changed into select ARCH_REQUIRE_GPIOLIB instead? You can grep for select GENERIC_GPIO in arch/arm to find the offending lines. We are removing GENERIC_GPIO and this work cannot be merged until you do this since it would break ARM builds. Thanks! Alex. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: timer: Shutdown clock event device when stopping local timer
On 03/29/2013 10:24 AM, ning.n.ji...@gmail.com wrote:
From: Ning Jiang ning.n.ji...@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.ji...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: timer: Shutdown clock event device when stopping local timer
On 03/29/2013 10:24 AM, ning.n.ji...@gmail.com wrote:
From: Ning Jiang ning.n.ji...@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.ji...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: timer: Shutdown clock event device when stopping local timer
2013/4/1 Daniel Lezcano daniel.lezc...@linaro.org:
On 03/29/2013 10:24 AM, ning.n.ji...@gmail.com wrote:
From: Ning Jiang ning.n.ji...@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.ji...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: timer: Shutdown clock event device when stopping local timer
2013/4/1 Daniel Lezcano daniel.lezc...@linaro.org:
On 03/29/2013 10:24 AM, ning.n.ji...@gmail.com wrote:
From: Ning Jiang ning.n.ji...@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.ji...@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);
}
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
