[PATCHv3 0/4] clockevents: decouple broadcast mechanism from drivers

[PATCHv3 0/4] clockevents: decouple broadcast mechanism from drivers

[Mark Rutland]

This is an updated version of the series I posted earlier this month:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/140528.html

The core patches can be found at:
git://linux-arm.org/linux-mr.git tags/timer-broadcast-v3-core

And the full series can be found at:
git://linux-arm.org/linux-mr.git tags/timer-broadcast-v3-arm

Changes since v2:
* Add evt->event_handler check in tick_receive_broadcast
* Remove tick_receive_broadcast stub for !GENERIC_CLOCKEVENTS_BROADCAST
* #ifdef IPI_TIMER handler for unexpected IPI warning
* Reorder patches (generic first, then arm implementation)
Changes since v1:
* Drop removal of guards in smp.c
* Removed useless evt->evt_handler check in tick_receive_broadcast
* Fix up tick_receive_broadcast when !GENERIC_CLOCKEVENTS_BROADCAST
* Fix checkpatch issues (multi-line strings)

Thanks to Stephen Boyd, Santosh Shilimkar, and Thomas Gleixner for their
commments.

In some SMP systems, cpu-local timers may stop delivering interrupts
when in low power states, or not all CPUs may have local timers. To
support these systems we have a mechanism for broadcasting timer ticks
to other CPUs. This mechanism relies on the struct
clock_event_device::broadcast function pointer, which is a
driver-specific mechanism for broadcasting ticks to other CPUs.

As the broadcast mechanism is architecture-specific, placing the
broadcast function on struct clock_event_device ties each driver to a
single architecture. Additionally the driver or architecture backend
must handle the routing of broadcast ticks to the correct
clock_event_device, leading to duplication of the list of active
clock_event_devices.

These patches introduce a generic mechanism for handling the receipt of
timer broadcasts, and an optional architecture-specific broadcast
function which allows drivers to be decoupled from a particular
architecture will retaining support for timer tick broadcasts. These
mechanisms are wired up for the arm port, and have been boot-tested on a
pandaboard.

Thanks,
Mark.

Mark Rutland (4):
  clockevents: Add generic timer broadcast receiver
  clockevents: Add generic timer broadcast function
  arm: Use generic timer broadcast receiver
  arm: Add generic timer broadcast support

 arch/arm/Kconfig             |    1 +
 arch/arm/kernel/smp.c        |   13 ++++---------
 include/linux/clockchips.h   |    9 +++++++++
 kernel/time/Kconfig          |    4 ++++
 kernel/time/tick-broadcast.c |   28 ++++++++++++++++++++++++++++
 5 files changed, 46 insertions(+), 9 deletions(-)

[PATCHv3 1/4] clockevents: Add generic timer broadcast receiver

[Mark Rutland]

Currently the broadcast mechanism used for timers is abstracted by a
function pointer on struct clock_event_device. As the fundamental
mechanism for broadcast is architecture-specific, this ties each
clock_event_device driver to a single architecture, even where the
driver is otherwise generic.

This patch adds a standard path for the receipt of timer broadcasts, so
drivers and/or architecture backends need not manage redundant lists of
timers for the purpose of routing broadcast timer ticks.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 include/linux/clockchips.h   |    4 ++++
 kernel/time/tick-broadcast.c |   15 +++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 8a7096f..e1089aa 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -161,6 +161,10 @@ clockevents_calc_mult_shift(struct clock_event_device *ce, u32 freq, u32 minsec)
 extern void clockevents_suspend(void);
 extern void clockevents_resume(void);
 
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+extern int tick_receive_broadcast(void);
+#endif
+
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 extern void clockevents_notify(unsigned long reason, void *arg);
 #else
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index f113755..1d6767d 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -125,6 +125,21 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 	return ret;
 }
 
+int tick_receive_broadcast(void)
+{
+	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+	struct clock_event_device *evt = td->evtdev;
+
+	if (!evt)
+		return -ENODEV;
+
+	if (!evt->evt_handler)
+		return -EINVAL;
+
+	evt->event_handler(evt);
+	return 0;
+}
+
 /*
  * Broadcast the event to the cpus, which are set in the mask (mangled).
  */
-- 
1.7.0.4

[PATCHv3 2/4] clockevents: Add generic timer broadcast function

[Mark Rutland]

Currently, the timer broadcast mechanism is defined by a function
pointer on struct clock_event_device. As the fundamental mechanism for
broadcast is architecture-specific, this means that clock_event_device
drivers cannot be shared across multiple architectures.

This patch adds an (optional) architecture-specific function for timer
tick broadcast, allowing drivers which may require broadcast
functionality to be shared across multiple architectures.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 include/linux/clockchips.h   |    5 +++++
 kernel/time/Kconfig          |    4 ++++
 kernel/time/tick-broadcast.c |   13 +++++++++++++
 3 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index e1089aa..6634652 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -162,6 +162,11 @@ extern void clockevents_suspend(void);
 extern void clockevents_resume(void);
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+#ifdef CONFIG_ARCH_HAS_TICK_BROADCAST
+extern void tick_broadcast(const struct cpumask *mask);
+#else
+#define tick_broadcast	NULL
+#endif
 extern int tick_receive_broadcast(void);
 #endif
 
diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index 8601f0d..b696922 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -38,6 +38,10 @@ config GENERIC_CLOCKEVENTS_BUILD
 	default y
 	depends on GENERIC_CLOCKEVENTS
 
+# Architecture can handle broadcast in a driver-agnostic way
+config ARCH_HAS_TICK_BROADCAST
+	bool
+
 # Clockevents broadcasting infrastructure
 config GENERIC_CLOCKEVENTS_BROADCAST
 	bool
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 1d6767d..9a70c6c 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -18,6 +18,7 @@
 #include <linux/percpu.h>
 #include <linux/profile.h>
 #include <linux/sched.h>
+#include <linux/smp.h>
 
 #include "tick-internal.h"
 
@@ -86,6 +87,11 @@ int tick_is_broadcast_device(struct clock_event_device *dev)
 	return (dev && tick_broadcast_device.evtdev == dev);
 }
 
+static void err_broadcast(const struct cpumask *mask)
+{
+	pr_crit_once("Failed to broadcast timer tick. Some CPUs may be unresponsive.\n");
+}
+
 /*
  * Check, if the device is disfunctional and a place holder, which
  * needs to be handled by the broadcast device.
@@ -105,6 +111,13 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 	 */
 	if (!tick_device_is_functional(dev)) {
 		dev->event_handler = tick_handle_periodic;
+		if (!dev->broadcast)
+			dev->broadcast = tick_broadcast;
+		if (!dev->broadcast) {
+			pr_warn_once("%s depends on broadcast, but no broadcast function available\n",
+				     dev->name);
+			dev->broadcast = err_broadcast;
+		}
 		cpumask_set_cpu(cpu, tick_get_broadcast_mask());
 		tick_broadcast_start_periodic(tick_broadcast_device.evtdev);
 		ret = 1;
-- 
1.7.0.4

[PATCHv3 3/4] arm: Use generic timer broadcast receiver

[Mark Rutland]

Currently, the ARM backend must maintain a redundant list of timers for
the purpose of centralising timer broadcast functionality. This prevents
sharing timer drivers across architectures.

This patch moves the pain of dealing with timer broadcasts to the core
clockevents tick broadcast code, which already maintains its own list
of timers.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/kernel/smp.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 84f4cbf..9308519 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -475,12 +475,6 @@ u64 smp_irq_stat_cpu(unsigned int cpu)
  */
 static DEFINE_PER_CPU(struct clock_event_device, percpu_clockevent);
 
-static void ipi_timer(void)
-{
-	struct clock_event_device *evt = &__get_cpu_var(percpu_clockevent);
-	evt->event_handler(evt);
-}
-
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 static void smp_timer_broadcast(const struct cpumask *mask)
 {
@@ -596,11 +590,13 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 	case IPI_WAKEUP:
 		break;
 
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 	case IPI_TIMER:
 		irq_enter();
-		ipi_timer();
+		tick_receive_broadcast();
 		irq_exit();
 		break;
+#endif
 
 	case IPI_RESCHEDULE:
 		scheduler_ipi();
-- 
1.7.0.4

[PATCHv3 4/4] arm: Add generic timer broadcast support

[Mark Rutland]

Implement timer_broadcast for the arm architecture, allowing for the use
of clock_event_device_drivers decoupled from the timer tick broadcast
mechanism.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/Kconfig      |    1 +
 arch/arm/kernel/smp.c |    3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 67874b8..65ae737 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -4,6 +4,7 @@ config ARM
 	select ARCH_BINFMT_ELF_RANDOMIZE_PIE
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_HAVE_CUSTOM_GPIO_H
+	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
 	select ARCH_WANT_IPC_PARSE_VERSION
 	select BUILDTIME_EXTABLE_SORT if MMU
 	select CPU_PM if (SUSPEND || CPU_IDLE)
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 9308519..b7e3b50 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -476,7 +476,7 @@ u64 smp_irq_stat_cpu(unsigned int cpu)
 static DEFINE_PER_CPU(struct clock_event_device, percpu_clockevent);
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
-static void smp_timer_broadcast(const struct cpumask *mask)
+void tick_broadcast(const struct cpumask *mask)
 {
 	smp_cross_call(mask, IPI_TIMER);
 }
@@ -524,7 +524,6 @@ static void __cpuinit percpu_timer_setup(void)
 	struct clock_event_device *evt = &per_cpu(percpu_clockevent, cpu);
 
 	evt->cpumask = cpumask_of(cpu);
-	evt->broadcast = smp_timer_broadcast;
 
 	if (!lt_ops || lt_ops->setup(evt))
 		broadcast_timer_setup(evt);
-- 
1.7.0.4

[PATCHv3 4/4] arm: Add generic timer broadcast support

[Stephen Warren]

On 01/14/2013 10:05 AM, Mark Rutland wrote:
> Implement timer_broadcast for the arm architecture, allowing for the use
> of clock_event_device_drivers decoupled from the timer tick broadcast
> mechanism.

Mark, this patch is now in next-20130206 and causes a crash during boot
on Tegra. The reason appears to be because of:

> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c

> @@ -524,7 +524,6 @@ static void __cpuinit percpu_timer_setup(void)
>  	struct clock_event_device *evt = &per_cpu(percpu_clockevent, cpu);
>  
>  	evt->cpumask = cpumask_of(cpu);
> -	evt->broadcast = smp_timer_broadcast;

After that change, evt->broadcast is never assigned, and hence is NULL.
Yet elsewhere in kernel/time/tick-broadcast.c it's used unconditionally:

static void tick_do_broadcast(struct cpumask *mask)
...
	if (!cpumask_empty(mask)) {
...
		td = &per_cpu(tick_cpu_device, cpumask_first(mask));
		td->evtdev->broadcast(mask);

Now perhaps the Tegra timer driver simply isn't being set up correctly,
so the bug is there... But the only other place I can find where
->broadcast is assigned is in tick_device_uses_broadcast() which only
does it for "non-functional" timers, which doesn't apply to Tegra's timer.

[PATCHv3 4/4] arm: Add generic timer broadcast support

[Mark Rutland]

Hi Stephen,

Sorry about this; I'm to blame for the bug.

On Wed, Feb 06, 2013 at 08:51:43PM +0000, Stephen Warren wrote:
> On 01/14/2013 10:05 AM, Mark Rutland wrote:
> > Implement timer_broadcast for the arm architecture, allowing for the use
> > of clock_event_device_drivers decoupled from the timer tick broadcast
> > mechanism.
> 
> Mark, this patch is now in next-20130206 and causes a crash during boot
> on Tegra. The reason appears to be because of:
> 
> > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> 
> > @@ -524,7 +524,6 @@ static void __cpuinit percpu_timer_setup(void)
> >  	struct clock_event_device *evt = &per_cpu(percpu_clockevent, cpu);
> >  
> >  	evt->cpumask = cpumask_of(cpu);
> > -	evt->broadcast = smp_timer_broadcast;
> 
> After that change, evt->broadcast is never assigned, and hence is NULL.
> Yet elsewhere in kernel/time/tick-broadcast.c it's used unconditionally:
> 
> static void tick_do_broadcast(struct cpumask *mask)
> ...
> 	if (!cpumask_empty(mask)) {
> ...
> 		td = &per_cpu(tick_cpu_device, cpumask_first(mask));
> 		td->evtdev->broadcast(mask);
> 
> Now perhaps the Tegra timer driver simply isn't being set up correctly,
> so the bug is there... But the only other place I can find where
> ->broadcast is assigned is in tick_device_uses_broadcast() which only
> does it for "non-functional" timers, which doesn't apply to Tegra's timer.
> 

The intent of 12ad100046: "clockevents: Add generic timer broadcast function"
was to setup the broadcast function both for non-functional/dummy timers and
those that stop in low-power states (CLOCK_EVT_FEAT_C3STOP). I missed the
CLOCK_EVT_FEAT_C3STOP case.

I believe the patch below will fix this for Tegra and any other platforms where
broadcast is required in low power states.

Stephen, could you test this for Tegra?

Thanks,
Mark.

---->8----
>From a93b7fa8b23464a69d0fb88dce0528ab1abd276d Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Thu, 7 Feb 2013 09:35:29 +0000
Subject: [PATCH] clockevents: fix generic broadcast for FEAT_C3STOP

Commit 12ad100046: "clockevents: Add generic timer broadcast function"
made tick_device_uses_broadcast set up the generic broadcast function
for dummy devices (where !tick_device_is_functional(dev)), but neglected
to set up the broadcast function for devices that stop in low power
states (with the CLOCK_EVT_FEAT_C3STOP flag).

When these devices enter low power states they will not have the generic
broadcast function assigned, and will bring down the system when an
attempt is made to broadcast to them.

This patch ensures that the broadcast function is also assigned for
devices which require broadcast in low power states.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 kernel/time/tick-broadcast.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index f726537..2fb8cb8 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -92,6 +92,17 @@ static void err_broadcast(const struct cpumask *mask)
 	pr_crit_once("Failed to broadcast timer tick. Some CPUs may be unresponsive.\n");
 }
 
+static void tick_device_setup_broadcast_func(struct clock_event_device *dev)
+{
+	if (!dev->broadcast)
+		dev->broadcast = tick_broadcast;
+	if (!dev->broadcast) {
+		pr_warn_once("%s depends on broadcast, but no broadcast function available\n",
+			     dev->name);
+		dev->broadcast = err_broadcast;
+	}
+}
+
 /*
  * Check, if the device is disfunctional and a place holder, which
  * needs to be handled by the broadcast device.
@@ -111,13 +122,7 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 	 */
 	if (!tick_device_is_functional(dev)) {
 		dev->event_handler = tick_handle_periodic;
-		if (!dev->broadcast)
-			dev->broadcast = tick_broadcast;
-		if (!dev->broadcast) {
-			pr_warn_once("%s depends on broadcast, but no broadcast function available\n",
-				     dev->name);
-			dev->broadcast = err_broadcast;
-		}
+		tick_device_setup_broadcast_func(dev);
 		cpumask_set_cpu(cpu, tick_get_broadcast_mask());
 		tick_broadcast_start_periodic(tick_broadcast_device.evtdev);
 		ret = 1;
@@ -129,9 +134,10 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 		 */
 		if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) {
 			int cpu = smp_processor_id();
-
 			cpumask_clear_cpu(cpu, tick_get_broadcast_mask());
 			tick_broadcast_clear_oneshot(cpu);
+		} else {
+			tick_device_setup_broadcast_func(dev);
 		}
 	}
 	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
-- 
1.8.1.1

[PATCHv3 4/4] arm: Add generic timer broadcast support

[Santosh Shilimkar]

Mark,

On Thursday 07 February 2013 03:34 PM, Mark Rutland wrote:
> Hi Stephen,
>
> Sorry about this; I'm to blame for the bug.
>
> On Wed, Feb 06, 2013 at 08:51:43PM +0000, Stephen Warren wrote:
>> On 01/14/2013 10:05 AM, Mark Rutland wrote:
>>> Implement timer_broadcast for the arm architecture, allowing for the use
>>> of clock_event_device_drivers decoupled from the timer tick broadcast
>>> mechanism.
>>
>> Mark, this patch is now in next-20130206 and causes a crash during boot
>> on Tegra. The reason appears to be because of:
>>
>>> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
>>
>>> @@ -524,7 +524,6 @@ static void __cpuinit percpu_timer_setup(void)
>>>   	struct clock_event_device *evt = &per_cpu(percpu_clockevent, cpu);
>>>
>>>   	evt->cpumask = cpumask_of(cpu);
>>> -	evt->broadcast = smp_timer_broadcast;
>>
>> After that change, evt->broadcast is never assigned, and hence is NULL.
>> Yet elsewhere in kernel/time/tick-broadcast.c it's used unconditionally:
>>
>> static void tick_do_broadcast(struct cpumask *mask)
>> ...
>> 	if (!cpumask_empty(mask)) {
>> ...
>> 		td = &per_cpu(tick_cpu_device, cpumask_first(mask));
>> 		td->evtdev->broadcast(mask);
>>
>> Now perhaps the Tegra timer driver simply isn't being set up correctly,
>> so the bug is there... But the only other place I can find where
>> ->broadcast is assigned is in tick_device_uses_broadcast() which only
>> does it for "non-functional" timers, which doesn't apply to Tegra's timer.
>>
>
> The intent of 12ad100046: "clockevents: Add generic timer broadcast function"
> was to setup the broadcast function both for non-functional/dummy timers and
> those that stop in low-power states (CLOCK_EVT_FEAT_C3STOP). I missed the
> CLOCK_EVT_FEAT_C3STOP case.
>
I am not sure this exactly the case. Because in my testing, the C3STOP 
path was exercised already. And if the C3STOP is used then notifiers
calls are expected for switching of clock-events to broadcast mode.

And dummy broad-cast hook should come into picture only if the per CPU
local timer clock-event are not registered.

I just tried "next-20130207" on OMAP and I see the broad-cast mechanism
works and didn't observe any crash.

------------------------------
Tick Device: mode:     1
Broadcast device
Clock Event Device: gp_timer
  max_delta_ns:   131071523464981
  min_delta_ns:   91552
  mult:           70369
  shift:          31
  mode:           3
  next_event:     89984375000 nsecs
  set_next_event: omap2_gp_timer_set_next_event
  set_mode:       omap2_gp_timer_set_mode
  event_handler:  tick_handle_oneshot_broadcast
  retries:        0

tick_broadcast_mask: 00000003
tick_broadcast_oneshot_mask: 00000000


Tick Device: mode:     1
Per CPU device: 0
Clock Event Device: local_timer
  max_delta_ns:   10737418240
  min_delta_ns:   1000
  mult:           858993459
  shift:          31
  mode:           3
  next_event:     125250000000 nsecs
  set_next_event: twd_set_next_event
  set_mode:       twd_set_mode
  event_handler:  hrtimer_interrupt
  retries:        346

Tick Device: mode:     1
Per CPU device: 1
Clock Event Device: local_timer
  max_delta_ns:   10737418240
  min_delta_ns:   1000
  mult:           858993459
  shift:          31
  mode:           3
  next_event:     89921875000 nsecs
  set_next_event: twd_set_next_event
  set_mode:       twd_set_mode
  event_handler:  hrtimer_interrupt
  retries:        258

#

------------------------------

> I believe the patch below will fix this for Tegra and any other platforms where
> broadcast is required in low power states.
>
Am not sure you really need that patch unless and until am missing a
scenario in my test.

Stephan,

Do we have crash log seen on Tegra ? Which tegra CPUIDLE driver
is being used when crash is seen ?

Regards
Santosh

[PATCHv3 4/4] arm: Add generic timer broadcast support

[Stephen Warren]

On 02/07/2013 04:40 AM, Santosh Shilimkar wrote:
> Mark,
> 
> On Thursday 07 February 2013 03:34 PM, Mark Rutland wrote:
>> Hi Stephen,
>>
>> Sorry about this; I'm to blame for the bug.
>>
>> On Wed, Feb 06, 2013 at 08:51:43PM +0000, Stephen Warren wrote:
>>> On 01/14/2013 10:05 AM, Mark Rutland wrote:
>>>> Implement timer_broadcast for the arm architecture, allowing for the
>>>> use
>>>> of clock_event_device_drivers decoupled from the timer tick broadcast
>>>> mechanism.
>>>
>>> Mark, this patch is now in next-20130206 and causes a crash during boot
>>> on Tegra. The reason appears to be because of:
>>>
>>>> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
>>>
>>>> @@ -524,7 +524,6 @@ static void __cpuinit percpu_timer_setup(void)
>>>>       struct clock_event_device *evt = &per_cpu(percpu_clockevent,
>>>> cpu);
>>>>
>>>>       evt->cpumask = cpumask_of(cpu);
>>>> -    evt->broadcast = smp_timer_broadcast;
>>>
>>> After that change, evt->broadcast is never assigned, and hence is NULL.
>>> Yet elsewhere in kernel/time/tick-broadcast.c it's used unconditionally:
>>>
>>> static void tick_do_broadcast(struct cpumask *mask)
>>> ...
>>>     if (!cpumask_empty(mask)) {
>>> ...
>>>         td = &per_cpu(tick_cpu_device, cpumask_first(mask));
>>>         td->evtdev->broadcast(mask);
>>>
>>> Now perhaps the Tegra timer driver simply isn't being set up correctly,
>>> so the bug is there... But the only other place I can find where
>>> ->broadcast is assigned is in tick_device_uses_broadcast() which only
>>> does it for "non-functional" timers, which doesn't apply to Tegra's
>>> timer.
>>
>> The intent of 12ad100046: "clockevents: Add generic timer broadcast
>> function"
>> was to setup the broadcast function both for non-functional/dummy
>> timers and
>> those that stop in low-power states (CLOCK_EVT_FEAT_C3STOP). I missed the
>> CLOCK_EVT_FEAT_C3STOP case.
>
> I am not sure this exactly the case. Because in my testing, the C3STOP
> path was exercised already. And if the C3STOP is used then notifiers
> calls are expected for switching of clock-events to broadcast mode.
> 
> And dummy broad-cast hook should come into picture only if the per CPU
> local timer clock-event are not registered.
> 
> I just tried "next-20130207" on OMAP and I see the broad-cast mechanism
> works and didn't observe any crash.
...
>> I believe the patch below will fix this for Tegra and any other
>> platforms where
>> broadcast is required in low power states.
>>
> Am not sure you really need that patch unless and until am missing a
> scenario in my test.
> 
> Stephan,
> 
> Do we have crash log seen on Tegra ? Which tegra CPUIDLE driver
> is being used when crash is seen ?

The crash log is below. It's simply because td->evt->broadcast is NULL
because it wasn't ever set.

I'm using the CPUIDLE driver in arch/arm/mach-tegra/cpuidle*.c.

> [ 4.179156] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [ 4.211200] pgd = c0004000
> [ 4.237688] [00000000] *pgd=00000000
> [ 4.264868] Internal error: Oops: 80000005 [#1] PREEMPT SMP ARM
> [ 4.294396] Modules linked in:
> [ 4.320940] CPU: 0 Not tainted (3.8.0-rc6-next-20130206-00002-g2aaa5ff #6)
> [ 4.352125] PC is at 0x0
> [ 4.378346] LR is at tick_do_broadcast.constprop.3+0x74/0xb0
> [ 4.407786] pc : [<00000000>] lr : [<c0063b38>] psr: 20000193
> [ 4.407786] sp : c06d7d60 ip : 00000002 fp : 00000004
> [ 4.466633] r10: c06d2ef8 r9 : c06def08 r8 : c06dec98
> [ 4.495457] r7 : 00000000 r6 : f8b8d230 r5 : c073d598 r4 : 00000000
> [ 4.525825] r3 : 00000000 r2 : 008ac000 r1 : 00000004 r0 : c073d5a4
> [ 4.556192] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
> [ 4.587865] Control: 10c5387d Table: 0000404a DAC: 00000015
> [ 4.617983] Process swapper/0 (pid: 0, stack limit = 0xc06d6238)
> [ 4.648314] Stack: (0xc06d7d60 to 0xc06d8000)
> [ 4.677145] 7d60: 00000002 ffffffff 7fffffff c0063c28 00000000 c06f17c0 f8b8d230 00000000
> [ 4.710387] 7d80: c0029b40 c06f1780 ee015ad0 00000000 00000000 00000049 c07186a7 ee015a80
> [ 4.743652] 7da0: 00000001 c0228c2c c0228c08 c0079184 f8b66130 00000000 ee2dc8e8 ee015a80
> [ 4.776806] 7dc0: ee015ad0 c06f1780 fe000100 c06d8080 c06d6000 c04e01ac 00000000 c0079304
> [ 4.810088] 7de0: ee015a80 ee015ad0 c06d7ee8 c007bf38 c007bebc 00000049 00000049 c0078b8c
> [ 4.843304] 7e00: c06d3678 c000eb74 fe00010c c06df040 c06d7e30 c00086d8 c0029acc c0029b40
> [ 4.876440] 7e20: 20000113 ffffffff c06d7e64 c000df40 00000001 c073a100 00000000 00000000
> [ 4.909580] 7e40: 00000202 0000001d 00000000 c06d6000 c06d8080 c06d6000 c04e01ac 00000000
> [ 4.942655] 7e60: 00000000 c06d7e78 c0029acc c0029b40 20000113 ffffffff c06ede80 ee012380
> [ 4.975697] 7e80: c0f7a834 00000000 ffff8c6e 00200000 c01f4290 c06d6000 0000001d 00000000
> [ 5.008939] 7ea0: fe000100 ee1a8b80 c06d6000 c04e01ac 00000000 c0029fa4 c06d3678 c000eb78
> [ 5.042564] 7ec0: fe00010c c06df040 c06d7ee8 c00086d8 c005d5f8 c0325700 60000113 ffffffff
> [ 5.076394] 7ee0: c06d7f1c c000df40 c06d7f30 3b9aca00 f8b59610 00000000 f8105948 00000000
> [ 5.110339] 7f00: c0f763e8 00000001 ee1a8b80 c06d6000 c04e01ac 00000000 00000015 c06d7f30
> [ 5.144383] 7f20: c005d5f8 c0325700 60000113 ffffffff f8b59610 00000000 00000000 c04e01ac
> [ 5.178363] 7f40: 00000000 c0f763e8 c0f763e8 00000001 c06e3de0 c03253ec 00000004 ee1a8b94
> [ 5.212337] 7f60: c0f763e8 c0327290 0000004c c0f763e8 c0761a0c 00000001 c06e3de0 00000000
> [ 5.246703] 7f80: c06e2ab0 c0325504 c06d6000 c06d6000 c0718888 c06de3f4 c04e01ac c000ef74
> [ 5.281026] 7fa0: c06decf8 c06d6000 c07187c0 c0f73d40 ffffffff 411fc090 3fffffff c069f7b4
> [ 5.315263] 7fc0: ffffffff ffffffff c069f2ec 00000000 00000000 c06c86a8 00000000 10c5387d
> [ 5.349498] 7fe0: c06de3f0 c06c86a4 c06e2aa4 0000406a 00000000 00008074 00000000 00000000
> [ 5.383733] [<c0063b38>] (tick_do_broadcast.constprop.3+0x74/0xb0) from [<c0063c28>] (tick_handle_oneshot_broadcast+0xb4/0x108)
> [ 5.421789] [<c0063c28>] (tick_handle_oneshot_broadcast+0xb4/0x108) from [<c0228c2c>] (tegra_timer_interrupt+0x24/0x2c)
> [ 5.459485] [<c0228c2c>] (tegra_timer_interrupt+0x24/0x2c) from [<c0079184>] (handle_irq_event_percpu+0x50/0x194)
> [ 5.497012] [<c0079184>] (handle_irq_event_percpu+0x50/0x194) from [<c0079304>] (handle_irq_event+0x3c/0x5c)
> [ 5.534558] [<c0079304>] (handle_irq_event+0x3c/0x5c) from [<c007bf38>] (handle_fasteoi_irq+0x7c/0x138)
> [ 5.572007] [<c007bf38>] (handle_fasteoi_irq+0x7c/0x138) from [<c0078b8c>] (generic_handle_irq+0x20/0x30)
> [ 5.609920] [<c0078b8c>] (generic_handle_irq+0x20/0x30) from [<c000eb74>] (handle_IRQ+0x38/0x94)
> [ 5.647209] [<c000eb74>] (handle_IRQ+0x38/0x94) from [<c00086d8>] (gic_handle_irq+0x28/0x5c)
> [ 5.684385] [<c00086d8>] (gic_handle_irq+0x28/0x5c) from [<c000df40>] (__irq_svc+0x40/0x70)
> [ 5.721713] Exception stack(0xc06d7e30 to 0xc06d7e78)
> [ 5.755567] 7e20: 00000001 c073a100 00000000 00000000
> [ 5.792782] 7e40: 00000202 0000001d 00000000 c06d6000 c06d8080 c06d6000 c04e01ac 00000000
> [ 5.829856] 7e60: 00000000 c06d7e78 c0029acc c0029b40 20000113 ffffffff
> [ 5.865532] [<c000df40>] (__irq_svc+0x40/0x70) from [<c0029b40>] (__do_softirq+0x84/0x1b8)
> [ 5.903191] [<c0029b40>] (__do_softirq+0x84/0x1b8) from [<c0029fa4>] (irq_exit+0x90/0x98)
> [ 5.940843] [<c0029fa4>] (irq_exit+0x90/0x98) from [<c000eb78>] (handle_IRQ+0x3c/0x94)
> [ 5.978365] [<c000eb78>] (handle_IRQ+0x3c/0x94) from [<c00086d8>] (gic_handle_irq+0x28/0x5c)
> [ 6.016482] [<c00086d8>] (gic_handle_irq+0x28/0x5c) from [<c000df40>] (__irq_svc+0x40/0x70)
> [ 6.054601] Exception stack(0xc06d7ee8 to 0xc06d7f30)
> [ 6.089523] 7ee0: c06d7f30 3b9aca00 f8b59610 00000000 f8105948 00000000
> [ 6.127885] 7f00: c0f763e8 00000001 ee1a8b80 c06d6000 c04e01ac 00000000 00000015 c06d7f30
> [ 6.166169] 7f20: c005d5f8 c0325700 60000113 ffffffff
> [ 6.201233] [<c000df40>] (__irq_svc+0x40/0x70) from [<c0325700>] (cpuidle_wrap_enter+0x48/0x98)
> [ 6.240573] [<c0325700>] (cpuidle_wrap_enter+0x48/0x98) from [<c03253ec>] (cpuidle_enter_state+0x14/0x60)
> [ 6.281197] [<c03253ec>] (cpuidle_enter_state+0x14/0x60) from [<c0327290>] (cpuidle_enter_state_coupled+0x208/0x2a0)
> [ 6.323163] [<c0327290>] (cpuidle_enter_state_coupled+0x208/0x2a0) from [<c0325504>] (cpuidle_idle_call+0xcc/0x11c)
> [ 6.365258] [<c0325504>] (cpuidle_idle_call+0xcc/0x11c) from [<c000ef74>] (cpu_idle+0xb0/0x114)
> [ 6.405790] [<c000ef74>] (cpu_idle+0xb0/0x114) from [<c069f7b4>] (start_kernel+0x2a4/0x2f4)
> [ 6.446051] Code: bad PC value
> [ 6.480872] ---[ end trace 319bbea8753aac77 ]---
> [ 6.517329] Kernel panic- not syncing: Fatal exception in interrupt
> [ 7.944703] SMP: failed to stop secondary CPUs 

[PATCHv3 4/4] arm: Add generic timer broadcast support

[Santosh Shilimkar]

On Thursday 07 February 2013 10:21 PM, Stephen Warren wrote:
> On 02/07/2013 04:40 AM, Santosh Shilimkar wrote:
[...]

>> Stephan,
>>
>> Do we have crash log seen on Tegra ? Which tegra CPUIDLE driver
>> is being used when crash is seen ?
>
> The crash log is below. It's simply because td->evt->broadcast is NULL
> because it wasn't ever set.
>
Thanks for the log. Its clear from the log that broadcast event wasn't
set.

Regards,
Santosh

[PATCHv3 4/4] arm: Add generic timer broadcast support

[Mark Rutland]

[...]

> > The intent of 12ad100046: "clockevents: Add generic timer broadcast function"
> > was to setup the broadcast function both for non-functional/dummy timers and
> > those that stop in low-power states (CLOCK_EVT_FEAT_C3STOP). I missed the
> > CLOCK_EVT_FEAT_C3STOP case.
> >
> I am not sure this exactly the case. Because in my testing, the C3STOP 
> path was exercised already. And if the C3STOP is used then notifiers
> calls are expected for switching of clock-events to broadcast mode.

I don't thing the C3STOP path was fully exercised.

The notifier calls sets up the broadcast source and adds cpus to
tick_broadcast_oneshot_mask, but they don't set up the broadcast function on
the local timer clock_event_devices. While everything else was set up for broadcast,
the CPU in idle's tick_cpu_device->evtdev had a NULL broadcast function pointer.

As soon as the broadcast device receives its interrupt, it attempts to
broadcast to all cpus in tick_broadcast_oneshot_mask, calling the broadcast
function pointer on their local timers' struct clock_event_device. As this was
never set up, it explodes, attempting to branch to NULL.

> 
> And dummy broad-cast hook should come into picture only if the per CPU
> local timer clock-event are not registered.

This is irrelevant here. The issue is that the broadcast function pointer isn't
set up for clocks with CLOCK_EVT_FEAT_C3STOP.

> 
> I just tried "next-20130207" on OMAP and I see the broad-cast mechanism
> works and didn't observe any crash.
> 
> ------------------------------
> Tick Device: mode:     1
> Broadcast device
> Clock Event Device: gp_timer
>   max_delta_ns:   131071523464981
>   min_delta_ns:   91552
>   mult:           70369
>   shift:          31
>   mode:           3
>   next_event:     89984375000 nsecs
>   set_next_event: omap2_gp_timer_set_next_event
>   set_mode:       omap2_gp_timer_set_mode
>   event_handler:  tick_handle_oneshot_broadcast
>   retries:        0
> 
> tick_broadcast_mask: 00000003
> tick_broadcast_oneshot_mask: 00000000

The broadcast clocks' next broadcast is in ~90 seconds time.

[...]

> 
> Tick Device: mode:     1
> Per CPU device: 1
> Clock Event Device: local_timer
>   max_delta_ns:   10737418240
>   min_delta_ns:   1000
>   mult:           858993459
>   shift:          31
>   mode:           3
>   next_event:     89921875000 nsecs
>   set_next_event: twd_set_next_event
>   set_mode:       twd_set_mode
>   event_handler:  hrtimer_interrupt
>   retries:        258

And this clock next expects a tick in ~90 seconds time.

Did you leave the board on for at least 90 seconds after grabbing this output,
with the cpu staying in idle?

If not, the kernel won't have attempted to broadcast.

> 
> #
> 
> ------------------------------
> 
> > I believe the patch below will fix this for Tegra and any other platforms where
> > broadcast is required in low power states.
> >
> Am not sure you really need that patch unless and until am missing a
> scenario in my test.

I suspect that with the time before broadcast being so large, broadcast never
actually occurred (even though the global timer was ready for it to).

I've don't have a suitable OMAP platform to test that theory on, unfortunately.

> 
> Stephan,
> 
> Do we have crash log seen on Tegra ? Which tegra CPUIDLE driver
> is being used when crash is seen ?

I've acquired a Harmony board, and running next-20130207, I encounter the exact
issue I described above (with the PC at NULL). With my suggested patch applied,
the problem disappears.

I believe the idle driver would be tegra20-cpuidle, and I believe it's doing
the right thing.

Thanks,
Mark.

[PATCHv3 4/4] arm: Add generic timer broadcast support

[Santosh Shilimkar]

Mark,

On Thursday 07 February 2013 10:47 PM, Mark Rutland wrote:

[..]

>>> I believe the patch below will fix this for Tegra and any other platforms where
>>> broadcast is required in low power states.
>>>
 From the Stephan's crash the issue is pretty clear and your patch make
sense. Some how I didn't manage to reproduce the issue on my board and
hence assumed everything is just fine. Will have a look at it later why
it didn't explode since it was so obvious.

Sorry for the noise.

Regards
Santosh

[PATCHv3 4/4] arm: Add generic timer broadcast support

[Stephen Warren]

On 02/07/2013 03:04 AM, Mark Rutland wrote:
> Hi Stephen,
> 
> Sorry about this; I'm to blame for the bug.
> 
> On Wed, Feb 06, 2013 at 08:51:43PM +0000, Stephen Warren wrote:
>> On 01/14/2013 10:05 AM, Mark Rutland wrote:
>>> Implement timer_broadcast for the arm architecture, allowing for the use
>>> of clock_event_device_drivers decoupled from the timer tick broadcast
>>> mechanism.
>>
>> Mark, this patch is now in next-20130206 and causes a crash during boot
>> on Tegra. The reason appears to be because of:
>>
>>> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
>>
>>> @@ -524,7 +524,6 @@ static void __cpuinit percpu_timer_setup(void)
>>>  	struct clock_event_device *evt = &per_cpu(percpu_clockevent, cpu);
>>>  
>>>  	evt->cpumask = cpumask_of(cpu);
>>> -	evt->broadcast = smp_timer_broadcast;
>>
>> After that change, evt->broadcast is never assigned, and hence is NULL.
>> Yet elsewhere in kernel/time/tick-broadcast.c it's used unconditionally:
>>
>> static void tick_do_broadcast(struct cpumask *mask)
>> ...
>> 	if (!cpumask_empty(mask)) {
>> ...
>> 		td = &per_cpu(tick_cpu_device, cpumask_first(mask));
>> 		td->evtdev->broadcast(mask);
>>
>> Now perhaps the Tegra timer driver simply isn't being set up correctly,
>> so the bug is there... But the only other place I can find where
>> ->broadcast is assigned is in tick_device_uses_broadcast() which only
>> does it for "non-functional" timers, which doesn't apply to Tegra's timer.
>>
> 
> The intent of 12ad100046: "clockevents: Add generic timer broadcast function"
> was to setup the broadcast function both for non-functional/dummy timers and
> those that stop in low-power states (CLOCK_EVT_FEAT_C3STOP). I missed the
> CLOCK_EVT_FEAT_C3STOP case.
> 
> I believe the patch below will fix this for Tegra and any other platforms where
> broadcast is required in low power states.
> 
> Stephen, could you test this for Tegra?

Tested-by: Stephen Warren <swarren@nvidia.com>

Thanks.

[PATCHv3 4/4] arm: Add generic timer broadcast support

[Mark Rutland]

[...]

> > The intent of 12ad100046: "clockevents: Add generic timer broadcast function"
> > was to setup the broadcast function both for non-functional/dummy timers and
> > those that stop in low-power states (CLOCK_EVT_FEAT_C3STOP). I missed the
> > CLOCK_EVT_FEAT_C3STOP case.
> > 
> > I believe the patch below will fix this for Tegra and any other platforms where
> > broadcast is required in low power states.
> > 
> > Stephen, could you test this for Tegra?
> 
> Tested-by: Stephen Warren <swarren@nvidia.com>

Thanks. Sorry for the bother.

Mark.