[PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Thierry Reding]

This fixes the following crash when booting on Tegra:

	[  123.346233] Unable to handle kernel NULL pointer dereference at virtual address 00000000
	[  123.354346] pgd = c0004000
	[  123.357071] [00000000] *pgd=00000000
	[  123.360687] Internal error: Oops: 80000007 [#1] PREEMPT SMP ARM
	[  123.366614] Modules linked in:
	[  123.369702] CPU: 0    Not tainted  (3.8.0-rc7-next-20130212-00002-ga12b34b-dirty #21)
	[  123.377541] PC is at 0x0
	[  123.380123] LR is at tick_do_broadcast.constprop.3+0x74/0xb0
	[  123.385802] pc : [<00000000>]    lr : [<c0064344>]    psr: 20000193
	[  123.385802] sp : c06ede18  ip : 00000002  fp : 00000004
	[  123.397285] r10: c06e8a60  r9 : c06f4f10  r8 : c06f4ca0
	[  123.402520] r7 : 0000001c  r6 : b7bf7c40  r5 : c07537d8  r4 : 00000000
	[  123.409055] r3 : 00000000  r2 : 004ac000  r1 : 00000004  r0 : c07537e4
	[  123.415594] Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
	[  123.422999] Control: 10c5387d  Table: 1ada804a  DAC: 00000015
	[  123.428756] Process swapper/0 (pid: 0, stack limit = 0xc06ec238)
	[  123.434771] Stack: (0xc06ede18 to 0xc06ee000)
	[  123.439145] de00:                                                       00000002 ffffffff
	[  123.447348] de20: 7fffffff c0064434 b7bec890 c0707840 b7bf7c40 0000001c 00000000 c0707800
	[  123.455547] de40: df815ad0 00000000 00000000 00000049 c072e8b3 df815a80 00000001 c022db34
	[  123.463749] de60: c022db10 c0079a14 0000001c c0b8ca68 b7bf3dc0 df815a80 df815ad0 c0707800
	[  123.471950] de80: fe000100 df9cb380 c06ec000 c04eb1ac 00000000 c0079b94 df815a80 df815ad0
	[  123.480148] dea0: 00000000 c007c7b0 c007c734 00000049 00000049 c0079410 c06e91f0 c000ec38
	[  123.488349] dec0: fe00010c c06f5048 c06edee8 c00086d4 c005dd20 c032c13c 60000113 ffffffff
	[  123.496549] dee0: c06edf1c c000e000 c06edf30 3b9aca00 b7bf2650 0000001c b718bcc0 0000001c
	[  123.504747] df00: c0b8c3e8 00000001 df9cb380 c06ec000 c04eb1ac 00000000 00000015 c06edf30
	[  123.512945] df20: c005dd20 c032c13c 60000113 ffffffff b7bf2650 0000001c 00000000 c04eb1ac
	[  123.521144] df40: 00000000 df9cb394 c0b8c3e8 c0b8c3e8 c06f9de0 c032be10 df9cb394 c0b8c3e8
	[  123.529341] df60: 00000001 c032dd30 0000004c c0b8c3e8 c0777c94 00000001 c06f9de0 00000000
	[  123.537541] df80: c072ea88 c032bf40 c06ec000 c072ea88 c06ec000 c06f43f4 c04eb1ac c000f008
	[  123.545740] dfa0: c06f4d00 00000000 c072e9c0 c0b89140 ffffffff 411fc090 3fffffff c06b47bc
	[  123.553939] dfc0: ffffffff ffffffff c06b42ec 00000000 00000000 c06ddcd0 00000000 10c5387d
	[  123.562138] dfe0: c06f43f0 c06ddccc c06f8aa4 0000406a 00000000 00008074 00000000 00000000
	[  123.570369] [<c0064344>] (tick_do_broadcast.constprop.3+0x74/0xb0) from [<c0064434>] (tick_handle_oneshot_broadcast+0xb4/0x108)
	[  123.581897] [<c0064434>] (tick_handle_oneshot_broadcast+0xb4/0x108) from [<c022db34>] (tegra_timer_interrupt+0x24/0x2c)
	[  123.592727] [<c022db34>] (tegra_timer_interrupt+0x24/0x2c) from [<c0079a14>] (handle_irq_event_percpu+0x50/0x194)
	[  123.603022] [<c0079a14>] (handle_irq_event_percpu+0x50/0x194) from [<c0079b94>] (handle_irq_event+0x3c/0x5c)
	[  123.612887] [<c0079b94>] (handle_irq_event+0x3c/0x5c) from [<c007c7b0>] (handle_fasteoi_irq+0x7c/0x138)
	[  123.622314] [<c007c7b0>] (handle_fasteoi_irq+0x7c/0x138) from [<c0079410>] (generic_handle_irq+0x20/0x30)
	[  123.631923] [<c0079410>] (generic_handle_irq+0x20/0x30) from [<c000ec38>] (handle_IRQ+0x38/0x94)
	[  123.640740] [<c000ec38>] (handle_IRQ+0x38/0x94) from [<c00086d4>] (gic_handle_irq+0x28/0x5c)
	[  123.649204] [<c00086d4>] (gic_handle_irq+0x28/0x5c) from [<c000e000>] (__irq_svc+0x40/0x70)
	[  123.657561] Exception stack(0xc06edee8 to 0xc06edf30)
	[  123.662637] dee0:                   c06edf30 3b9aca00 b7bf2650 0000001c b718bcc0 0000001c
	[  123.670839] df00: c0b8c3e8 00000001 df9cb380 c06ec000 c04eb1ac 00000000 00000015 c06edf30
	[  123.679027] df20: c005dd20 c032c13c 60000113 ffffffff
	[  123.684125] [<c000e000>] (__irq_svc+0x40/0x70) from [<c032c13c>] (cpuidle_wrap_enter+0x48/0x94)
	[  123.692858] [<c032c13c>] (cpuidle_wrap_enter+0x48/0x94) from [<c032be10>] (cpuidle_enter_state+0x14/0x70)
	[  123.702461] [<c032be10>] (cpuidle_enter_state+0x14/0x70) from [<c032dd30>] (cpuidle_enter_state_coupled+0x208/0x2a0)
	[  123.713013] [<c032dd30>] (cpuidle_enter_state_coupled+0x208/0x2a0) from [<c032bf40>] (cpuidle_idle_call+0xd4/0x124)
	[  123.723475] [<c032bf40>] (cpuidle_idle_call+0xd4/0x124) from [<c000f008>] (cpu_idle+0xb0/0x124)
	[  123.732228] [<c000f008>] (cpu_idle+0xb0/0x124) from [<c06b47bc>] (start_kernel+0x2a4/0x2f4)
	[  123.740586] Code: bad PC value
	[  123.743668] ---[ end trace ceed8db1ca3c192e ]---
	[  123.748297] Kernel panic - not syncing: Fatal exception in interrupt
	[  125.143549] SMP: failed to stop secondary CPUs

Note that I have close to no clue what I'm doing, so the patch might be
the completely wrong thing to do. An alternative I had initially thought
about was to check for NULL before calling the clock_event_device's
.broadcast() function.

The reason why I chose to always assign .broadcast instead is that a
previous patch (3d06770: Add generic timer broadcast support) aims at
generically implementing broadcast support on ARM and providing a
tick_broadcast() implementation for this purpose so it seemed like the
right thing to do.

The above-mentioned patch for some reason removed the assignment to the
.broadcast() member for apparently no reason, so maybe that was just
done by mistake?

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 arch/arm/kernel/smp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 5f73f70..472ba9d 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -467,7 +467,7 @@ void tick_broadcast(const struct cpumask *mask)
 	smp_cross_call(mask, IPI_TIMER);
 }
 #else
-#define smp_timer_broadcast	NULL
+#define tick_broadcast NULL
 #endif
 
 static void broadcast_timer_set_mode(enum clock_event_mode mode,
@@ -513,6 +513,8 @@ static void __cpuinit percpu_timer_setup(void)
 
 	if (!lt_ops || lt_ops->setup(evt))
 		broadcast_timer_setup(evt);
+
+	evt->broadcast = tick_broadcast;
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
-- 
1.8.1.2

[PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Thierry Reding]

This fixes the following crash when booting on Tegra:

	[  123.346233] Unable to handle kernel NULL pointer dereference at virtual address 00000000
	[  123.354346] pgd = c0004000
	[  123.357071] [00000000] *pgd=00000000
	[  123.360687] Internal error: Oops: 80000007 [#1] PREEMPT SMP ARM
	[  123.366614] Modules linked in:
	[  123.369702] CPU: 0    Not tainted  (3.8.0-rc7-next-20130212-00002-ga12b34b-dirty #21)
	[  123.377541] PC is at 0x0
	[  123.380123] LR is at tick_do_broadcast.constprop.3+0x74/0xb0
	[  123.385802] pc : [<00000000>]    lr : [<c0064344>]    psr: 20000193
	[  123.385802] sp : c06ede18  ip : 00000002  fp : 00000004
	[  123.397285] r10: c06e8a60  r9 : c06f4f10  r8 : c06f4ca0
	[  123.402520] r7 : 0000001c  r6 : b7bf7c40  r5 : c07537d8  r4 : 00000000
	[  123.409055] r3 : 00000000  r2 : 004ac000  r1 : 00000004  r0 : c07537e4
	[  123.415594] Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
	[  123.422999] Control: 10c5387d  Table: 1ada804a  DAC: 00000015
	[  123.428756] Process swapper/0 (pid: 0, stack limit = 0xc06ec238)
	[  123.434771] Stack: (0xc06ede18 to 0xc06ee000)
	[  123.439145] de00:                                                       00000002 ffffffff
	[  123.447348] de20: 7fffffff c0064434 b7bec890 c0707840 b7bf7c40 0000001c 00000000 c0707800
	[  123.455547] de40: df815ad0 00000000 00000000 00000049 c072e8b3 df815a80 00000001 c022db34
	[  123.463749] de60: c022db10 c0079a14 0000001c c0b8ca68 b7bf3dc0 df815a80 df815ad0 c0707800
	[  123.471950] de80: fe000100 df9cb380 c06ec000 c04eb1ac 00000000 c0079b94 df815a80 df815ad0
	[  123.480148] dea0: 00000000 c007c7b0 c007c734 00000049 00000049 c0079410 c06e91f0 c000ec38
	[  123.488349] dec0: fe00010c c06f5048 c06edee8 c00086d4 c005dd20 c032c13c 60000113 ffffffff
	[  123.496549] dee0: c06edf1c c000e000 c06edf30 3b9aca00 b7bf2650 0000001c b718bcc0 0000001c
	[  123.504747] df00: c0b8c3e8 00000001 df9cb380 c06ec000 c04eb1ac 00000000 00000015 c06edf30
	[  123.512945] df20: c005dd20 c032c13c 60000113 ffffffff b7bf2650 0000001c 00000000 c04eb1ac
	[  123.521144] df40: 00000000 df9cb394 c0b8c3e8 c0b8c3e8 c06f9de0 c032be10 df9cb394 c0b8c3e8
	[  123.529341] df60: 00000001 c032dd30 0000004c c0b8c3e8 c0777c94 00000001 c06f9de0 00000000
	[  123.537541] df80: c072ea88 c032bf40 c06ec000 c072ea88 c06ec000 c06f43f4 c04eb1ac c000f008
	[  123.545740] dfa0: c06f4d00 00000000 c072e9c0 c0b89140 ffffffff 411fc090 3fffffff c06b47bc
	[  123.553939] dfc0: ffffffff ffffffff c06b42ec 00000000 00000000 c06ddcd0 00000000 10c5387d
	[  123.562138] dfe0: c06f43f0 c06ddccc c06f8aa4 0000406a 00000000 00008074 00000000 00000000
	[  123.570369] [<c0064344>] (tick_do_broadcast.constprop.3+0x74/0xb0) from [<c0064434>] (tick_handle_oneshot_broadcast+0xb4/0x108)
	[  123.581897] [<c0064434>] (tick_handle_oneshot_broadcast+0xb4/0x108) from [<c022db34>] (tegra_timer_interrupt+0x24/0x2c)
	[  123.592727] [<c022db34>] (tegra_timer_interrupt+0x24/0x2c) from [<c0079a14>] (handle_irq_event_percpu+0x50/0x194)
	[  123.603022] [<c0079a14>] (handle_irq_event_percpu+0x50/0x194) from [<c0079b94>] (handle_irq_event+0x3c/0x5c)
	[  123.612887] [<c0079b94>] (handle_irq_event+0x3c/0x5c) from [<c007c7b0>] (handle_fasteoi_irq+0x7c/0x138)
	[  123.622314] [<c007c7b0>] (handle_fasteoi_irq+0x7c/0x138) from [<c0079410>] (generic_handle_irq+0x20/0x30)
	[  123.631923] [<c0079410>] (generic_handle_irq+0x20/0x30) from [<c000ec38>] (handle_IRQ+0x38/0x94)
	[  123.640740] [<c000ec38>] (handle_IRQ+0x38/0x94) from [<c00086d4>] (gic_handle_irq+0x28/0x5c)
	[  123.649204] [<c00086d4>] (gic_handle_irq+0x28/0x5c) from [<c000e000>] (__irq_svc+0x40/0x70)
	[  123.657561] Exception stack(0xc06edee8 to 0xc06edf30)
	[  123.662637] dee0:                   c06edf30 3b9aca00 b7bf2650 0000001c b718bcc0 0000001c
	[  123.670839] df00: c0b8c3e8 00000001 df9cb380 c06ec000 c04eb1ac 00000000 00000015 c06edf30
	[  123.679027] df20: c005dd20 c032c13c 60000113 ffffffff
	[  123.684125] [<c000e000>] (__irq_svc+0x40/0x70) from [<c032c13c>] (cpuidle_wrap_enter+0x48/0x94)
	[  123.692858] [<c032c13c>] (cpuidle_wrap_enter+0x48/0x94) from [<c032be10>] (cpuidle_enter_state+0x14/0x70)
	[  123.702461] [<c032be10>] (cpuidle_enter_state+0x14/0x70) from [<c032dd30>] (cpuidle_enter_state_coupled+0x208/0x2a0)
	[  123.713013] [<c032dd30>] (cpuidle_enter_state_coupled+0x208/0x2a0) from [<c032bf40>] (cpuidle_idle_call+0xd4/0x124)
	[  123.723475] [<c032bf40>] (cpuidle_idle_call+0xd4/0x124) from [<c000f008>] (cpu_idle+0xb0/0x124)
	[  123.732228] [<c000f008>] (cpu_idle+0xb0/0x124) from [<c06b47bc>] (start_kernel+0x2a4/0x2f4)
	[  123.740586] Code: bad PC value
	[  123.743668] ---[ end trace ceed8db1ca3c192e ]---
	[  123.748297] Kernel panic - not syncing: Fatal exception in interrupt
	[  125.143549] SMP: failed to stop secondary CPUs

Note that I have close to no clue what I'm doing, so the patch might be
the completely wrong thing to do. An alternative I had initially thought
about was to check for NULL before calling the clock_event_device's
.broadcast() function.

The reason why I chose to always assign .broadcast instead is that a
previous patch (3d06770: Add generic timer broadcast support) aims at
generically implementing broadcast support on ARM and providing a
tick_broadcast() implementation for this purpose so it seemed like the
right thing to do.

The above-mentioned patch for some reason removed the assignment to the
.broadcast() member for apparently no reason, so maybe that was just
done by mistake?

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 arch/arm/kernel/smp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 5f73f70..472ba9d 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -467,7 +467,7 @@ void tick_broadcast(const struct cpumask *mask)
 	smp_cross_call(mask, IPI_TIMER);
 }
 #else
-#define smp_timer_broadcast	NULL
+#define tick_broadcast NULL
 #endif
 
 static void broadcast_timer_set_mode(enum clock_event_mode mode,
@@ -513,6 +513,8 @@ static void __cpuinit percpu_timer_setup(void)
 
 	if (!lt_ops || lt_ops->setup(evt))
 		broadcast_timer_setup(evt);
+
+	evt->broadcast = tick_broadcast;
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
-- 
1.8.1.2

Re: [PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Russell King - ARM Linux]

On Tue, Feb 12, 2013 at 04:49:54PM +0100, Thierry Reding wrote:
> Note that I have close to no clue what I'm doing, so the patch might be
> the completely wrong thing to do. An alternative I had initially thought
> about was to check for NULL before calling the clock_event_device's
> .broadcast() function.
> 
> The reason why I chose to always assign .broadcast instead is that a
> previous patch (3d06770: Add generic timer broadcast support) aims at
> generically implementing broadcast support on ARM and providing a
> tick_broadcast() implementation for this purpose so it seemed like the
> right thing to do.
> 
> The above-mentioned patch for some reason removed the assignment to the
> .broadcast() member for apparently no reason, so maybe that was just
> done by mistake?

This is now supposed to be handled by the timer core stuff.  It
looks to me like 3d06770eef43eaad606e77246bfcc7e82b1d9fb4
(arm: Add generic timer broadcast support) is slightly busted in
that it doesn't deal with the #else case you identified below.

Certainly, initializing evt->broadcast after the setup is not the
right solution - it must be done before, but this was removed by
the above commit.

So, things to be done here:
1. Fix the #else part of the code.
2. Fix the reported oops.

I think this falls to Mark, being the last one to touch this and cause
this breakage. :)

[PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Russell King - ARM Linux]

On Tue, Feb 12, 2013 at 04:49:54PM +0100, Thierry Reding wrote:
> Note that I have close to no clue what I'm doing, so the patch might be
> the completely wrong thing to do. An alternative I had initially thought
> about was to check for NULL before calling the clock_event_device's
> .broadcast() function.
> 
> The reason why I chose to always assign .broadcast instead is that a
> previous patch (3d06770: Add generic timer broadcast support) aims at
> generically implementing broadcast support on ARM and providing a
> tick_broadcast() implementation for this purpose so it seemed like the
> right thing to do.
> 
> The above-mentioned patch for some reason removed the assignment to the
> .broadcast() member for apparently no reason, so maybe that was just
> done by mistake?

This is now supposed to be handled by the timer core stuff.  It
looks to me like 3d06770eef43eaad606e77246bfcc7e82b1d9fb4
(arm: Add generic timer broadcast support) is slightly busted in
that it doesn't deal with the #else case you identified below.

Certainly, initializing evt->broadcast after the setup is not the
right solution - it must be done before, but this was removed by
the above commit.

So, things to be done here:
1. Fix the #else part of the code.
2. Fix the reported oops.

I think this falls to Mark, being the last one to touch this and cause
this breakage. :)

Re: [PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Stephen Warren]

On 02/12/2013 08:55 AM, Russell King - ARM Linux wrote:
> On Tue, Feb 12, 2013 at 04:49:54PM +0100, Thierry Reding wrote:
>> Note that I have close to no clue what I'm doing, so the patch might be
>> the completely wrong thing to do. An alternative I had initially thought
>> about was to check for NULL before calling the clock_event_device's
>> .broadcast() function.
>>
>> The reason why I chose to always assign .broadcast instead is that a
>> previous patch (3d06770: Add generic timer broadcast support) aims at
>> generically implementing broadcast support on ARM and providing a
>> tick_broadcast() implementation for this purpose so it seemed like the
>> right thing to do.
>>
>> The above-mentioned patch for some reason removed the assignment to the
>> .broadcast() member for apparently no reason, so maybe that was just
>> done by mistake?
> 
> This is now supposed to be handled by the timer core stuff.  It
> looks to me like 3d06770eef43eaad606e77246bfcc7e82b1d9fb4
> (arm: Add generic timer broadcast support) is slightly busted in
> that it doesn't deal with the #else case you identified below.

This is fixed by:

"clockevents: fix generic broadcast for FEAT_C3STOP"

http://www.spinics.net/lists/arm-kernel/msg223568.html

For some reason this hasn't made it into linux-next yet, at least not by
next-20130211.

It's in:

git://nv-tegra.nvidia.com/user/swarren/linux-2.6 next-20130211-fixed

[PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Stephen Warren]

On 02/12/2013 08:55 AM, Russell King - ARM Linux wrote:
> On Tue, Feb 12, 2013 at 04:49:54PM +0100, Thierry Reding wrote:
>> Note that I have close to no clue what I'm doing, so the patch might be
>> the completely wrong thing to do. An alternative I had initially thought
>> about was to check for NULL before calling the clock_event_device's
>> .broadcast() function.
>>
>> The reason why I chose to always assign .broadcast instead is that a
>> previous patch (3d06770: Add generic timer broadcast support) aims at
>> generically implementing broadcast support on ARM and providing a
>> tick_broadcast() implementation for this purpose so it seemed like the
>> right thing to do.
>>
>> The above-mentioned patch for some reason removed the assignment to the
>> .broadcast() member for apparently no reason, so maybe that was just
>> done by mistake?
> 
> This is now supposed to be handled by the timer core stuff.  It
> looks to me like 3d06770eef43eaad606e77246bfcc7e82b1d9fb4
> (arm: Add generic timer broadcast support) is slightly busted in
> that it doesn't deal with the #else case you identified below.

This is fixed by:

"clockevents: fix generic broadcast for FEAT_C3STOP"

http://www.spinics.net/lists/arm-kernel/msg223568.html

For some reason this hasn't made it into linux-next yet, at least not by
next-20130211.

It's in:

git://nv-tegra.nvidia.com/user/swarren/linux-2.6 next-20130211-fixed

Re: [PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Mark Rutland]

Hi,

On Tue, Feb 12, 2013 at 03:55:16PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 12, 2013 at 04:49:54PM +0100, Thierry Reding wrote:
> > Note that I have close to no clue what I'm doing, so the patch might be
> > the completely wrong thing to do. An alternative I had initially thought
> > about was to check for NULL before calling the clock_event_device's
> > .broadcast() function.
> > 
> > The reason why I chose to always assign .broadcast instead is that a
> > previous patch (3d06770: Add generic timer broadcast support) aims at
> > generically implementing broadcast support on ARM and providing a
> > tick_broadcast() implementation for this purpose so it seemed like the
> > right thing to do.
> > 
> > The above-mentioned patch for some reason removed the assignment to the
> > .broadcast() member for apparently no reason, so maybe that was just
> > done by mistake?

Apologies for this. I believe you've hit the same problem as Stephen Warren [1].

The broadcast function is now meant to be set by timer core, but unfortunately
I made a mistake when adding the functionality such that it isn't set for
timers with CLOCK_EVT_FEAT_C3STOP. I posted a fix [2] on Friday, but so far
I've had no reply.

Thomas, are you happy to take the fix at [2] ?

> 
> This is now supposed to be handled by the timer core stuff.  It
> looks to me like 3d06770eef43eaad606e77246bfcc7e82b1d9fb4
> (arm: Add generic timer broadcast support) is slightly busted in
> that it doesn't deal with the #else case you identified below.

That's a minor cosmetic defect. We shouldn't touch smp_timer_broadcast at all
now.

> 
> Certainly, initializing evt->broadcast after the setup is not the
> right solution - it must be done before, but this was removed by
> the above commit.
> 
> So, things to be done here:
> 1. Fix the #else part of the code.
> 2. Fix the reported oops.
> 
> I think this falls to Mark, being the last one to touch this and cause
> this breakage. :)
> 

Hopefully with [2], I've fixed 2. I'll fix 1 shortly. :)

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/148065.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/148471.html

[PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Mark Rutland]

Hi,

On Tue, Feb 12, 2013 at 03:55:16PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 12, 2013 at 04:49:54PM +0100, Thierry Reding wrote:
> > Note that I have close to no clue what I'm doing, so the patch might be
> > the completely wrong thing to do. An alternative I had initially thought
> > about was to check for NULL before calling the clock_event_device's
> > .broadcast() function.
> > 
> > The reason why I chose to always assign .broadcast instead is that a
> > previous patch (3d06770: Add generic timer broadcast support) aims at
> > generically implementing broadcast support on ARM and providing a
> > tick_broadcast() implementation for this purpose so it seemed like the
> > right thing to do.
> > 
> > The above-mentioned patch for some reason removed the assignment to the
> > .broadcast() member for apparently no reason, so maybe that was just
> > done by mistake?

Apologies for this. I believe you've hit the same problem as Stephen Warren [1].

The broadcast function is now meant to be set by timer core, but unfortunately
I made a mistake when adding the functionality such that it isn't set for
timers with CLOCK_EVT_FEAT_C3STOP. I posted a fix [2] on Friday, but so far
I've had no reply.

Thomas, are you happy to take the fix at [2] ?

> 
> This is now supposed to be handled by the timer core stuff.  It
> looks to me like 3d06770eef43eaad606e77246bfcc7e82b1d9fb4
> (arm: Add generic timer broadcast support) is slightly busted in
> that it doesn't deal with the #else case you identified below.

That's a minor cosmetic defect. We shouldn't touch smp_timer_broadcast at all
now.

> 
> Certainly, initializing evt->broadcast after the setup is not the
> right solution - it must be done before, but this was removed by
> the above commit.
> 
> So, things to be done here:
> 1. Fix the #else part of the code.
> 2. Fix the reported oops.
> 
> I think this falls to Mark, being the last one to touch this and cause
> this breakage. :)
> 

Hopefully with [2], I've fixed 2. I'll fix 1 shortly. :)

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/148065.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/148471.html

Re: [PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Mark Rutland]

[...]

> So, things to be done here:
> 1. Fix the #else part of the code.
> 2. Fix the reported oops.

I believe the patch below solves point 1. Nothing refers to smp_timer_broadcast
any more (as I've tested with git grep), so removing it shouldn't create any
additional problems.

Russell, are you happy for me to drop this in the patch system?

Thanks,
Mark.

---->8----
From 1954a075d0ce7f5ce5466b20f7aee9a0ac044cda Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Date: Tue, 12 Feb 2013 16:50:18 +0000
Subject: [PATCH] arm: remove unused smp_timer_broadcast #define

The assignment of clock_event_device::broadcast can be done by timer
core as of 12ad100046: "clockevents: Add generic timer broadcast
function", and the arm code moved over to this as of 3d06770eef: "arm:
Add generic timer broadcast support", but left a dangling #define when
!CONFIG_GENERIC_TIMER_BROADCAST.

This patch removes the now unused #define.

Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm/kernel/smp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index b7e3b50..ab9458d 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -480,8 +480,6 @@ void tick_broadcast(const struct cpumask *mask)
 {
 	smp_cross_call(mask, IPI_TIMER);
 }
-#else
-#define smp_timer_broadcast	NULL
 #endif
 
 static void broadcast_timer_set_mode(enum clock_event_mode mode,
-- 
1.8.1.1

[PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Mark Rutland]

[...]

> So, things to be done here:
> 1. Fix the #else part of the code.
> 2. Fix the reported oops.

I believe the patch below solves point 1. Nothing refers to smp_timer_broadcast
any more (as I've tested with git grep), so removing it shouldn't create any
additional problems.

Russell, are you happy for me to drop this in the patch system?

Thanks,
Mark.

---->8----
>From 1954a075d0ce7f5ce5466b20f7aee9a0ac044cda Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Tue, 12 Feb 2013 16:50:18 +0000
Subject: [PATCH] arm: remove unused smp_timer_broadcast #define

The assignment of clock_event_device::broadcast can be done by timer
core as of 12ad100046: "clockevents: Add generic timer broadcast
function", and the arm code moved over to this as of 3d06770eef: "arm:
Add generic timer broadcast support", but left a dangling #define when
!CONFIG_GENERIC_TIMER_BROADCAST.

This patch removes the now unused #define.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm/kernel/smp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index b7e3b50..ab9458d 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -480,8 +480,6 @@ void tick_broadcast(const struct cpumask *mask)
 {
 	smp_cross_call(mask, IPI_TIMER);
 }
-#else
-#define smp_timer_broadcast	NULL
 #endif
 
 static void broadcast_timer_set_mode(enum clock_event_mode mode,
-- 
1.8.1.1

Re: [PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Russell King - ARM Linux]

On Tue, Feb 12, 2013 at 05:12:17PM +0000, Mark Rutland wrote:
> [...]
> 
> > So, things to be done here:
> > 1. Fix the #else part of the code.
> > 2. Fix the reported oops.
> 
> I believe the patch below solves point 1. Nothing refers to smp_timer_broadcast
> any more (as I've tested with git grep), so removing it shouldn't create any
> additional problems.
> 
> Russell, are you happy for me to drop this in the patch system?

Yes, it might as well go into the devel-stable branch along with your
other patches.  Thanks.

[PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Russell King - ARM Linux]

On Tue, Feb 12, 2013 at 05:12:17PM +0000, Mark Rutland wrote:
> [...]
> 
> > So, things to be done here:
> > 1. Fix the #else part of the code.
> > 2. Fix the reported oops.
> 
> I believe the patch below solves point 1. Nothing refers to smp_timer_broadcast
> any more (as I've tested with git grep), so removing it shouldn't create any
> additional problems.
> 
> Russell, are you happy for me to drop this in the patch system?

Yes, it might as well go into the devel-stable branch along with your
other patches.  Thanks.

Re: [PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Mark Rutland]

On Tue, Feb 12, 2013 at 05:15:01PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 12, 2013 at 05:12:17PM +0000, Mark Rutland wrote:
> > [...]
> > 
> > > So, things to be done here:
> > > 1. Fix the #else part of the code.
> > > 2. Fix the reported oops.
> > 
> > I believe the patch below solves point 1. Nothing refers to smp_timer_broadcast
> > any more (as I've tested with git grep), so removing it shouldn't create any
> > additional problems.
> > 
> > Russell, are you happy for me to drop this in the patch system?
> 
> Yes, it might as well go into the devel-stable branch along with your
> other patches.  Thanks.
> 

Submitted as 7651/1.

Thanks,
Mark.

[PATCH] HACK: ARM: Fix generic timer broadcast for TWD

[Mark Rutland]

On Tue, Feb 12, 2013 at 05:15:01PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 12, 2013 at 05:12:17PM +0000, Mark Rutland wrote:
> > [...]
> > 
> > > So, things to be done here:
> > > 1. Fix the #else part of the code.
> > > 2. Fix the reported oops.
> > 
> > I believe the patch below solves point 1. Nothing refers to smp_timer_broadcast
> > any more (as I've tested with git grep), so removing it shouldn't create any
> > additional problems.
> > 
> > Russell, are you happy for me to drop this in the patch system?
> 
> Yes, it might as well go into the devel-stable branch along with your
> other patches.  Thanks.
> 

Submitted as 7651/1.

Thanks,
Mark.