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

Commit Message

Mark Rutland Jan. 9, 2013, 2:46 p.m. UTC

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 <[e-mail]>
Reviewed-by: Santosh Shilimkar <[e-mail]>
Tested-by: Santosh Shilimkar <[e-mail]>
---
 include/linux/clockchips.h   |    9 +++++++++
 kernel/time/tick-broadcast.c |   12 ++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

Comments

Thomas Gleixner Jan. 14, 2013, 11:06 a.m. UTC | #1

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

Mark Rutland Jan. 14, 2013, 11:29 a.m. UTC | #2

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

Thomas Gleixner Jan. 14, 2013, 11:50 a.m. UTC | #3

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

Mark Rutland Jan. 14, 2013, 12:12 p.m. UTC | #4

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

Thomas Gleixner Jan. 14, 2013, 2:17 p.m. UTC | #5

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

Mark Rutland Jan. 14, 2013, 3:36 p.m. UTC | #6

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.

Santosh Shilimkar Jan. 15, 2013, 6:40 a.m. UTC | #7

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

Thomas Gleixner Jan. 15, 2013, 11:24 a.m. UTC | #8

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

Mark Rutland Jan. 15, 2013, noon UTC | #9

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.

Patch

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).
  */