| Message ID | 1355832418-31692-5-git-send-email-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Minor nit On 12/18/12 04:06, Mark Rutland wrote: > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > index c2dd022..ec22a80 100644 > --- a/kernel/time/tick-broadcast.c > +++ b/kernel/time/tick-broadcast.c > @@ -86,6 +87,12 @@ 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("Attempted to broadcast tick, but no broadcast mechanism " > + "present. Some CPUs may be unresponsive."); This is missing a newline. You may also want to put the string on a single line so we can easily grep for it in the sources. > @@ -105,6 +112,14 @@ 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", Same one line comment here. I thought checkpatch didn't complain anymore.
On Tue, Dec 18, 2012 at 10:17:13PM +0000, Stephen Boyd wrote: > Minor nit > > On 12/18/12 04:06, Mark Rutland wrote: > > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > > index c2dd022..ec22a80 100644 > > --- a/kernel/time/tick-broadcast.c > > +++ b/kernel/time/tick-broadcast.c > > @@ -86,6 +87,12 @@ 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("Attempted to broadcast tick, but no broadcast mechanism " > > + "present. Some CPUs may be unresponsive."); > > This is missing a newline. You may also want to put the string on a > single line so we can easily grep for it in the sources. Whoops, fixed. I'll change both strings to be single line. > > @@ -105,6 +112,14 @@ 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", > > Same one line comment here. I thought checkpatch didn't complain anymore. In fact it actively warns. Not sure how I missed that. Thanks, Mark.
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 c2dd022..ec22a80 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,12 @@ 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("Attempted to broadcast tick, but no broadcast mechanism " + "present. Some CPUs may be unresponsive."); +} + /* * Check, if the device is disfunctional and a place holder, which * needs to be handled by the broadcast device. @@ -105,6 +112,14 @@ 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;
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> --- include/linux/clockchips.h | 5 +++++ kernel/time/Kconfig | 4 ++++ kernel/time/tick-broadcast.c | 15 +++++++++++++++ 3 files changed, 24 insertions(+), 0 deletions(-)