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

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

[Mark Rutland]

This is an updated version of the series I posted back in December:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137929.html

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 go to Stephen Boyd and Santosh Shilimkar 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
  arm: Use generic timer broadcast receiver
  clockevents: Add generic timer broadcast function
  arm: Add generic timer broadcast support

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

[PATCHv2 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>
---
 include/linux/clockchips.h   |    9 +++++++++
 kernel/time/tick-broadcast.c |   12 ++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 8a7096f..6537114 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -161,6 +161,15 @@ 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);
+#else
+static inline int tick_receive_broadcast(void)
+{
+	return 0;
+}
+#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..5079bb7 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -125,6 +125,18 @@ 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;
+
+	evt->event_handler(evt);
+	return 0;
+}
+
 /*
  * Broadcast the event to the cpus, which are set in the mask (mangled).
  */
-- 
1.7.0.4

[PATCHv2 2/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>
---
 arch/arm/kernel/smp.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 84f4cbf..a9d7e70 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)
 {
@@ -598,7 +592,7 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 
 	case IPI_TIMER:
 		irq_enter();
-		ipi_timer();
+		tick_receive_broadcast();
 		irq_exit();
 		break;
 
-- 
1.7.0.4

[PATCHv2 3/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>
---
 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 6537114..921568b 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);
 #else
 static inline int tick_receive_broadcast(void)
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 5079bb7..b8aa122 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

[PATCHv2 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>
---
 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 f95ba14..b051266 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 a9d7e70..a901054 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

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

[Stephen Boyd]

On 01/09/13 06:46, Mark Rutland wrote:
> This is an updated version of the series I posted back in December:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137929.html
>
> 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 go to Stephen Boyd and Santosh Shilimkar 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.
>

For the series:

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

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

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

[Mark Rutland]

On Wed, Jan 09, 2013 at 08:46:18PM +0000, Stephen Boyd wrote:
> On 01/09/13 06:46, Mark Rutland wrote:
> > This is an updated version of the series I posted back in December:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137929.html
> >
> > 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 go to Stephen Boyd and Santosh Shilimkar 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.
> >
> 
> For the series:
> 
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

Thanks!

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

Mark.

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

[Thomas Gleixner]

On Wed, 9 Jan 2013, Mark Rutland wrote:
> +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> +extern int tick_receive_broadcast(void);
> +#else
> +static inline int tick_receive_broadcast(void)
> +{
> +	return 0;
> +}

What's the inline function for? If an arch does not have broadcasting
support it should not have a receive broadcast function call either.

> +#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..5079bb7 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -125,6 +125,18 @@ 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;

Is anything going to use the return value?

Thanks,

	tglx

[PATCHv2 3/4] clockevents: Add generic timer broadcast function

[Thomas Gleixner]

On Wed, 9 Jan 2013, Mark Rutland wrote:
> 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.

Looks good.

If you fixup the first one of the series, I'll take the generic
patches and provide a branch to pull for the ARM folks, so the ARM
part can go via their tree.

Thanks,

	tglx

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

[Mark Rutland]

On Mon, Jan 14, 2013 at 11:06:31AM +0000, Thomas Gleixner wrote:
> On Wed, 9 Jan 2013, Mark Rutland wrote:
> > +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> > +extern int tick_receive_broadcast(void);
> > +#else
> > +static inline int tick_receive_broadcast(void)
> > +{
> > +	return 0;
> > +}
> 
> What's the inline function for? If an arch does not have broadcasting
> support it should not have a receive broadcast function call either.

That was how this was originally structured [1], but Santosh suggested this
would break the build for !GENERIC_CLOCKEVENTS_BROADCAST [1]. It means that the
arch-specific receive path (i.e. IPI handler) doesn't have to be #ifdef'd,
which makes it less ugly.

I'm happy to have it the other way, with #ifdef'd IPI handlers.

> 
> > +#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..5079bb7 100644
> > --- a/kernel/time/tick-broadcast.c
> > +++ b/kernel/time/tick-broadcast.c
> > @@ -125,6 +125,18 @@ 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;
> 
> Is anything going to use the return value?

I'd added this after looking at the x86 lapic timers, where interrupts might
remain pending over a kexec, and lapic interrupts come up before timers are
registered. The return value is useful for shutting down the timer in that case
(see x86's local_apic_timer_interrupt).

If you don't agree I'll remove the return value.

> 
> Thanks,
> 
> 	tglx
>

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/138486.html

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

[Thomas Gleixner]

On Mon, 14 Jan 2013, Mark Rutland wrote:

> On Mon, Jan 14, 2013 at 11:06:31AM +0000, Thomas Gleixner wrote:
> > On Wed, 9 Jan 2013, Mark Rutland wrote:
> > > +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> > > +extern int tick_receive_broadcast(void);
> > > +#else
> > > +static inline int tick_receive_broadcast(void)
> > > +{
> > > +	return 0;
> > > +}
> > 
> > What's the inline function for? If an arch does not have broadcasting
> > support it should not have a receive broadcast function call either.
> 
> That was how this was originally structured [1], but Santosh suggested this
> would break the build for !GENERIC_CLOCKEVENTS_BROADCAST [1]. It means that the
> arch-specific receive path (i.e. IPI handler) doesn't have to be #ifdef'd,
> which makes it less ugly.

Hmm. If you want to keep the IPI around unconditionally the inline
makes some sense, though the question is whether keeping an unused IPI
around makes sense in the first place. I'd rather see a warning that
an unexpected IPI happened than a silent inline function being called.

> > Is anything going to use the return value?
> 
> I'd added this after looking at the x86 lapic timers, where interrupts might
> remain pending over a kexec, and lapic interrupts come up before timers are
> registered. The return value is useful for shutting down the timer in that case
> (see x86's local_apic_timer_interrupt).

Right, though then you need to check for evt->event_handler as well.

Thanks,

	tglx

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

[Mark Rutland]

On Mon, Jan 14, 2013 at 11:50:55AM +0000, Thomas Gleixner wrote:
> On Mon, 14 Jan 2013, Mark Rutland wrote:
> 
> > On Mon, Jan 14, 2013 at 11:06:31AM +0000, Thomas Gleixner wrote:
> > > On Wed, 9 Jan 2013, Mark Rutland wrote:
> > > > +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> > > > +extern int tick_receive_broadcast(void);
> > > > +#else
> > > > +static inline int tick_receive_broadcast(void)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > 
> > > What's the inline function for? If an arch does not have broadcasting
> > > support it should not have a receive broadcast function call either.
> > 
> > That was how this was originally structured [1], but Santosh suggested this
> > would break the build for !GENERIC_CLOCKEVENTS_BROADCAST [1]. It means that the
> > arch-specific receive path (i.e. IPI handler) doesn't have to be #ifdef'd,
> > which makes it less ugly.
> 
> Hmm. If you want to keep the IPI around unconditionally the inline
> makes some sense, though the question is whether keeping an unused IPI
> around makes sense in the first place. I'd rather see a warning that
> an unexpected IPI happened than a silent inline function being called.

How about I add a warning (e.g. "Impossible timer broadcast received.") and
return -EOPNOTSUPP when !GENERIC_CLOCKEVENTS_BROADCAST?

> > > Is anything going to use the return value?
> > 
> > I'd added this after looking at the x86 lapic timers, where interrupts might
> > remain pending over a kexec, and lapic interrupts come up before timers are
> > registered. The return value is useful for shutting down the timer in that case
> > (see x86's local_apic_timer_interrupt).
> 
> Right, though then you need to check for evt->event_handler as well.

I thought this previously also [1], but I couldn't find any path such that a
tick_cpu_device would have an evtdev without an event_handler. We always set the
handler before setting evtdev, and alway wipe evtdev before wiping the handler.

Have I missed something?

> Thanks,
> 
> 	tglx
> 

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/138092.html

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

[Thomas Gleixner]

On Mon, 14 Jan 2013, Mark Rutland wrote:
> On Mon, Jan 14, 2013 at 11:50:55AM +0000, Thomas Gleixner wrote:
> > On Mon, 14 Jan 2013, Mark Rutland wrote:
> > 
> > > On Mon, Jan 14, 2013 at 11:06:31AM +0000, Thomas Gleixner wrote:
> > > > On Wed, 9 Jan 2013, Mark Rutland wrote:
> > > > > +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> > > > > +extern int tick_receive_broadcast(void);
> > > > > +#else
> > > > > +static inline int tick_receive_broadcast(void)
> > > > > +{
> > > > > +	return 0;
> > > > > +}
> > > > 
> > > > What's the inline function for? If an arch does not have broadcasting
> > > > support it should not have a receive broadcast function call either.
> > > 
> > > That was how this was originally structured [1], but Santosh suggested this
> > > would break the build for !GENERIC_CLOCKEVENTS_BROADCAST [1]. It means that the
> > > arch-specific receive path (i.e. IPI handler) doesn't have to be #ifdef'd,
> > > which makes it less ugly.
> > 
> > Hmm. If you want to keep the IPI around unconditionally the inline
> > makes some sense, though the question is whether keeping an unused IPI
> > around makes sense in the first place. I'd rather see a warning that
> > an unexpected IPI happened than a silent inline function being called.
> 
> How about I add a warning (e.g. "Impossible timer broadcast received.") and
> return -EOPNOTSUPP when !GENERIC_CLOCKEVENTS_BROADCAST?

You still need to do something with the return value in the arch IPI
code, right?

> > > > Is anything going to use the return value?
> > > 
> > > I'd added this after looking at the x86 lapic timers, where interrupts might
> > > remain pending over a kexec, and lapic interrupts come up before timers are
> > > registered. The return value is useful for shutting down the timer in that case
> > > (see x86's local_apic_timer_interrupt).
> > 
> > Right, though then you need to check for evt->event_handler as well.
> 
> I thought this previously also [1], but I couldn't find any path such that a
> tick_cpu_device would have an evtdev without an event_handler. We always set the
> handler before setting evtdev, and alway wipe evtdev before wiping the handler.
> 
> Have I missed something?

That's an x86 specific issue. Though we could try and make that
functionality completely generic.

Thanks,

	tglx

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

[Mark Rutland]

On Mon, Jan 14, 2013 at 02:17:26PM +0000, Thomas Gleixner wrote:
> On Mon, 14 Jan 2013, Mark Rutland wrote:
> > On Mon, Jan 14, 2013 at 11:50:55AM +0000, Thomas Gleixner wrote:
> > > On Mon, 14 Jan 2013, Mark Rutland wrote:
> > > 
> > > > On Mon, Jan 14, 2013 at 11:06:31AM +0000, Thomas Gleixner wrote:
> > > > > On Wed, 9 Jan 2013, Mark Rutland wrote:
> > > > > > +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> > > > > > +extern int tick_receive_broadcast(void);
> > > > > > +#else
> > > > > > +static inline int tick_receive_broadcast(void)
> > > > > > +{
> > > > > > +	return 0;
> > > > > > +}
> > > > > 
> > > > > What's the inline function for? If an arch does not have broadcasting
> > > > > support it should not have a receive broadcast function call either.
> > > > 
> > > > That was how this was originally structured [1], but Santosh suggested this
> > > > would break the build for !GENERIC_CLOCKEVENTS_BROADCAST [1]. It means that the
> > > > arch-specific receive path (i.e. IPI handler) doesn't have to be #ifdef'd,
> > > > which makes it less ugly.
> > > 
> > > Hmm. If you want to keep the IPI around unconditionally the inline
> > > makes some sense, though the question is whether keeping an unused IPI
> > > around makes sense in the first place. I'd rather see a warning that
> > > an unexpected IPI happened than a silent inline function being called.
> > 
> > How about I add a warning (e.g. "Impossible timer broadcast received.") and
> > return -EOPNOTSUPP when !GENERIC_CLOCKEVENTS_BROADCAST?
> 
> You still need to do something with the return value in the arch IPI
> code, right?

Good point. Having the stub when !CONFIG_GENERIC_CLOCKEVENTS_BROADCAST is
clearly problematic.

I'll go with your original suggestion, removing the tick_receive_broadcast stub
for !CONFIG_GENERIC_CLOCKEVENTS_BROADCAST and I'll #idef the IPI_TIMER handler.
That way it'll fall down to the standard warning for an unexpected/unknown IPI
for arch/arm at least.

> > > > > Is anything going to use the return value?
> > > > 
> > > > I'd added this after looking at the x86 lapic timers, where interrupts might
> > > > remain pending over a kexec, and lapic interrupts come up before timers are
> > > > registered. The return value is useful for shutting down the timer in that case
> > > > (see x86's local_apic_timer_interrupt).
> > > 
> > > Right, though then you need to check for evt->event_handler as well.
> > 
> > I thought this previously also [1], but I couldn't find any path such that a
> > tick_cpu_device would have an evtdev without an event_handler. We always set the
> > handler before setting evtdev, and alway wipe evtdev before wiping the handler.
> > 
> > Have I missed something?
> 
> That's an x86 specific issue. Though we could try and make that
> functionality completely generic.

Just to check: is the evt->event_handler check necessary?

> Thanks,
> 
> 	tglx
> 

Thanks,
Mark.

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

[Santosh Shilimkar]

On Monday 14 January 2013 09:06 PM, Mark Rutland wrote:
> On Mon, Jan 14, 2013 at 02:17:26PM +0000, Thomas Gleixner wrote:
>> On Mon, 14 Jan 2013, Mark Rutland wrote:
>>> On Mon, Jan 14, 2013 at 11:50:55AM +0000, Thomas Gleixner wrote:
>>>> On Mon, 14 Jan 2013, Mark Rutland wrote:
>>>>
>>>>> On Mon, Jan 14, 2013 at 11:06:31AM +0000, Thomas Gleixner wrote:
>>>>>> On Wed, 9 Jan 2013, Mark Rutland wrote:
>>>>>>> +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
>>>>>>> +extern int tick_receive_broadcast(void);
>>>>>>> +#else
>>>>>>> +static inline int tick_receive_broadcast(void)
>>>>>>> +{
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>
>>>>>> What's the inline function for? If an arch does not have broadcasting
>>>>>> support it should not have a receive broadcast function call either.
>>>>>
>>>>> That was how this was originally structured [1], but Santosh suggested this
>>>>> would break the build for !GENERIC_CLOCKEVENTS_BROADCAST [1]. It means that the
>>>>> arch-specific receive path (i.e. IPI handler) doesn't have to be #ifdef'd,
>>>>> which makes it less ugly.
>>>>
>>>> Hmm. If you want to keep the IPI around unconditionally the inline
>>>> makes some sense, though the question is whether keeping an unused IPI
>>>> around makes sense in the first place. I'd rather see a warning that
>>>> an unexpected IPI happened than a silent inline function being called.
>>>
>>> How about I add a warning (e.g. "Impossible timer broadcast received.") and
>>> return -EOPNOTSUPP when !GENERIC_CLOCKEVENTS_BROADCAST?
>>
>> You still need to do something with the return value in the arch IPI
>> code, right?
>
> Good point. Having the stub when !CONFIG_GENERIC_CLOCKEVENTS_BROADCAST is
> clearly problematic.
>
> I'll go with your original suggestion, removing the tick_receive_broadcast stub
> for !CONFIG_GENERIC_CLOCKEVENTS_BROADCAST and I'll #idef the IPI_TIMER handler.
> That way it'll fall down to the standard warning for an unexpected/unknown IPI
> for arch/arm at least.
>
The alternative is fine by me.

Regards
santosh

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

[Thomas Gleixner]

On Mon, 14 Jan 2013, Mark Rutland wrote:
> On Mon, Jan 14, 2013 at 02:17:26PM +0000, Thomas Gleixner wrote:
> > > I thought this previously also [1], but I couldn't find any path such that a
> > > tick_cpu_device would have an evtdev without an event_handler. We always set the
> > > handler before setting evtdev, and alway wipe evtdev before wiping the handler.
> > > 
> > > Have I missed something?
> > 
> > That's an x86 specific issue. Though we could try and make that
> > functionality completely generic.
> 
> Just to check: is the evt->event_handler check necessary?

For x86 yes. See the comment.

Thanks,

	tglx

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

[Mark Rutland]

On Tue, Jan 15, 2013 at 11:24:53AM +0000, Thomas Gleixner wrote:
> On Mon, 14 Jan 2013, Mark Rutland wrote:
> > On Mon, Jan 14, 2013 at 02:17:26PM +0000, Thomas Gleixner wrote:
> > > > I thought this previously also [1], but I couldn't find any path such that a
> > > > tick_cpu_device would have an evtdev without an event_handler. We always set the
> > > > handler before setting evtdev, and alway wipe evtdev before wiping the handler.
> > > > 
> > > > Have I missed something?
> > > 
> > > That's an x86 specific issue. Though we could try and make that
> > > functionality completely generic.
> > 
> > Just to check: is the evt->event_handler check necessary?
> 
> For x86 yes. See the comment.

Ah, sorry. I misunderstood on my initial reading.

I've posted a version with the evt->event_handler check restored, and the
tick_receive_broadcast stub removed when !CONFIG_GENERIC_CLOCKEVENTS_BROADCAST:

https://lkml.org/lkml/2013/1/14/276

The generic clockevents patches (based on v3.8-rc3) can also be found at
git://linux-arm.org/linux-mr.git tags/timer-broadcast-v3-core

> Thanks,
> 
> 	tglx
> 

Thanks,
Mark.