Re: [PATCH v2] clockevents: Introduce mode specific callbacks

[Preeti U Murthy]

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 <tglx@xxxxxxxxxxxxx> [2]
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
> 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 <preeti@xxxxxxxxxxxxxxxxxx>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/