On 02/13/2015 06:24 AM, Viresh Kumar wrote: > It is not possible for the clockevents core to know which modes (other than > those with a corresponding feature flag) are supported by a particular > implementation. And drivers are expected to handle transition to all modes > elegantly, as ->set_mode() would be issued for them unconditionally. > > Now, adding support for a new mode complicates things a bit if we want to use > the legacy ->set_mode() callback. We need to closely review all clockevents > drivers to see if they would break on addition of a new mode. And after such > reviews, it is found that we have to do non-trivial changes to most of the > drivers [1]. > > Introduce mode-specific set_mode_*() callbacks, some of which the drivers may > or > may not implement. A missing callback would clearly convey the message that > the > corresponding mode isn't supported. > > A driver may still choose to keep supporting the legacy ->set_mode() callback, > but ->set_mode() wouldn't be supporting any new modes beyond RESUME. If a > driver > wants to get benefited by using a new mode, it would be required to migrate to > the mode specific callbacks. > > The legacy ->set_mode() callback and the newly introduced mode-specific > callbacks are mutually exclusive. Only one of them should be supported by the > driver. > > Sanity check is done at the time of registration to distinguish between > optional > and required callbacks and to make error recovery and handling simpler. If the > legacy ->set_mode() callback is provided, all mode specific ones would be > ignored by the core but a warning is thrown if they are present. > > Call sites calling ->set_mode() directly are also updated to use > __clockevents_set_mode() instead, as ->set_mode() may not be available anymore > for few drivers. > > [1] https://lkml.org/lkml/2014/12/9/605 > [2] https://lkml.org/lkml/2015/1/23/255 > > Suggested-by: Thomas Gleixner <t...@linutronix.de> [2] > Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org> > --- > V1->V2: Stricter sanity checks. > > include/linux/clockchips.h | 21 +++++++++-- > kernel/time/clockevents.c | 88 > ++++++++++++++++++++++++++++++++++++++++++++-- > kernel/time/timer_list.c | 32 +++++++++++++++-- > 3 files changed, 134 insertions(+), 7 deletions(-) > > diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h > index 2e4cb67f6e56..59af26b54d15 100644 > --- a/include/linux/clockchips.h > +++ b/include/linux/clockchips.h > @@ -39,6 +39,8 @@ enum clock_event_mode { > CLOCK_EVT_MODE_PERIODIC, > CLOCK_EVT_MODE_ONESHOT, > CLOCK_EVT_MODE_RESUME, > + > + /* Legacy ->set_mode() callback doesn't support below modes */ > }; > > /* > @@ -81,7 +83,11 @@ enum clock_event_mode { > * @mode: operating mode assigned by the management code > * @features: features > * @retries: number of forced programming retries > - * @set_mode: set mode function > + * @set_mode: legacy set mode function, only for modes <= > CLOCK_EVT_MODE_RESUME. > + * @set_mode_periodic: switch mode to periodic, if !set_mode > + * @set_mode_oneshot: switch mode to oneshot, if !set_mode > + * @set_mode_shutdown: switch mode to shutdown, if !set_mode > + * @set_mode_resume: resume clkevt device, if !set_mode > * @broadcast: function to broadcast events > * @min_delta_ticks: minimum delta value in ticks stored for reconfiguration > * @max_delta_ticks: maximum delta value in ticks stored for reconfiguration > @@ -108,9 +114,20 @@ struct clock_event_device { > unsigned int features; > unsigned long retries; > > - void (*broadcast)(const struct cpumask *mask); > + /* > + * Mode transition callback(s): Only one of the two groups should be > + * defined: > + * - set_mode(), only for modes <= CLOCK_EVT_MODE_RESUME. > + * - set_mode_{shutdown|periodic|oneshot|resume}(). > + */ > void (*set_mode)(enum clock_event_mode mode, > struct clock_event_device *); > + int (*set_mode_periodic)(struct clock_event_device > *); > + int (*set_mode_oneshot)(struct clock_event_device > *); > + int (*set_mode_shutdown)(struct clock_event_device > *); > + int (*set_mode_resume)(struct clock_event_device *); > + > + void (*broadcast)(const struct cpumask *mask); > void (*suspend)(struct clock_event_device *); > void (*resume)(struct clock_event_device *); > unsigned long min_delta_ticks; > diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c > index 55449909f114..489642b08d64 100644 > --- a/kernel/time/clockevents.c > +++ b/kernel/time/clockevents.c > @@ -94,6 +94,57 @@ u64 clockevent_delta2ns(unsigned long latch, struct > clock_event_device *evt) > } > EXPORT_SYMBOL_GPL(clockevent_delta2ns); > > +static int __clockevents_set_mode(struct clock_event_device *dev, > + enum clock_event_mode mode) > +{ > + /* Transition with legacy set_mode() callback */ > + if (dev->set_mode) { > + /* Legacy callback doesn't support new modes */ > + if (mode > CLOCK_EVT_MODE_RESUME) > + return -ENOSYS; > + dev->set_mode(mode, dev); > + return 0; > + } > + > + if (dev->features & CLOCK_EVT_FEAT_DUMMY) > + return 0; > + > + /* Transition with new mode-specific callbacks */ > + switch (mode) { > + case CLOCK_EVT_MODE_UNUSED: > + /* > + * This is an internal state, which is guaranteed to go from > + * SHUTDOWN to UNUSED. No driver interaction required. > + */ > + return 0; > + > + case CLOCK_EVT_MODE_SHUTDOWN: > + return dev->set_mode_shutdown(dev); > + > + case CLOCK_EVT_MODE_PERIODIC: > + /* Core internal bug */ > + if (!(dev->features & CLOCK_EVT_FEAT_PERIODIC)) > + return -ENOSYS; > + return dev->set_mode_periodic(dev); > + > + case CLOCK_EVT_MODE_ONESHOT: > + /* Core internal bug */ > + if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT)) > + return -ENOSYS; > + return dev->set_mode_oneshot(dev); > + > + case CLOCK_EVT_MODE_RESUME: > + /* Optional callback */ > + if (dev->set_mode_resume) > + return dev->set_mode_resume(dev); > + else > + return 0; > + > + default: > + return -ENOSYS; > + } > +} > + > /** > * clockevents_set_mode - set the operating mode of a clock event device > * @dev: device to modify > @@ -105,7 +156,9 @@ void clockevents_set_mode(struct clock_event_device *dev, > enum clock_event_mode mode) > { > if (dev->mode != mode) { > - dev->set_mode(mode, dev); > + if (__clockevents_set_mode(dev, mode)) > + return; > + > dev->mode = mode; > > /* > @@ -373,6 +426,35 @@ int clockevents_unbind_device(struct clock_event_device > *ced, int cpu) > } > EXPORT_SYMBOL_GPL(clockevents_unbind); > > +/* Sanity check of mode transition callbacks */ > +static int clockevents_sanity_check(struct clock_event_device *dev) > +{ > + /* Legacy set_mode() callback */ > + if (dev->set_mode) { > + /* We shouldn't be supporting new modes now */ > + WARN_ON(dev->set_mode_periodic || dev->set_mode_oneshot || > + dev->set_mode_shutdown || dev->set_mode_resume); > + return 0; > + } > + > + if (dev->features & CLOCK_EVT_FEAT_DUMMY) > + return 0; > + > + /* New mode-specific callbacks */ > + if (!dev->set_mode_shutdown) > + return -EINVAL; > + > + if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) && > + !dev->set_mode_periodic) > + return -EINVAL; > + > + if ((dev->features & CLOCK_EVT_FEAT_ONESHOT) && > + !dev->set_mode_oneshot) > + return -EINVAL; > + > + return 0; > +} > + > /** > * clockevents_register_device - register a clock event device > * @dev: device to register > @@ -382,6 +464,8 @@ void clockevents_register_device(struct > clock_event_device *dev) > unsigned long flags; > > BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED); > + BUG_ON(clockevents_sanity_check(dev)); > + > if (!dev->cpumask) { > WARN_ON(num_possible_cpus() > 1); > dev->cpumask = cpumask_of(smp_processor_id()); > @@ -449,7 +533,7 @@ int __clockevents_update_freq(struct clock_event_device > *dev, u32 freq) > return clockevents_program_event(dev, dev->next_event, false); > > if (dev->mode == CLOCK_EVT_MODE_PERIODIC) > - dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev); > + return __clockevents_set_mode(dev, CLOCK_EVT_MODE_PERIODIC); > > return 0; > } > diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c > index 61ed862cdd37..2cfd19485824 100644 > --- a/kernel/time/timer_list.c > +++ b/kernel/time/timer_list.c > @@ -228,9 +228,35 @@ print_tickdevice(struct seq_file *m, struct tick_device > *td, int cpu) > print_name_offset(m, dev->set_next_event); > SEQ_printf(m, "\n"); > > - SEQ_printf(m, " set_mode: "); > - print_name_offset(m, dev->set_mode); > - SEQ_printf(m, "\n"); > + if (dev->set_mode) { > + SEQ_printf(m, " set_mode: "); > + print_name_offset(m, dev->set_mode); > + SEQ_printf(m, "\n"); > + } else { > + if (dev->set_mode_shutdown) { > + SEQ_printf(m, " shutdown: "); > + print_name_offset(m, dev->set_mode_shutdown); > + SEQ_printf(m, "\n"); > + } > + > + if (dev->set_mode_periodic) { > + SEQ_printf(m, " periodic: "); > + print_name_offset(m, dev->set_mode_periodic); > + SEQ_printf(m, "\n"); > + } > + > + if (dev->set_mode_oneshot) { > + SEQ_printf(m, " oneshot: "); > + print_name_offset(m, dev->set_mode_oneshot); > + SEQ_printf(m, "\n"); > + } > + > + if (dev->set_mode_resume) { > + SEQ_printf(m, " resume: "); > + print_name_offset(m, dev->set_mode_resume); > + SEQ_printf(m, "\n"); > + } > + } > > SEQ_printf(m, " event_handler: "); > print_name_offset(m, dev->event_handler); >
Reviewed-by: Preeti U Murthy <pre...@linux.vnet.ibm.com> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
