[PATCH v2] clockevents: Introduce mode specific callbacks

[PATCH v2] clockevents: Introduce mode specific callbacks

[Viresh Kumar]

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@linutronix.de> [2]
Signed-off-by: Viresh Kumar <viresh.kumar@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);
-- 
2.3.0.rc0.44.ga94655d

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

[Rafael J. Wysocki]

On Friday, February 13, 2015 08:54:56 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.

This is not going to fly AFAICS if you don't say what exacly you need it for.

Rafael

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

[Viresh Kumar]

On 13 February 2015 at 10:11, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, February 13, 2015 08:54:56 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.
>
> This is not going to fly AFAICS if you don't say what exacly you need it for.

For this: https://lkml.org/lkml/2014/5/9/508

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

[Rafael J. Wysocki]

On Friday, February 13, 2015 11:01:18 AM Viresh Kumar wrote:
> On 13 February 2015 at 10:11, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, February 13, 2015 08:54:56 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.
> >
> > This is not going to fly AFAICS if you don't say what exacly you need it for.
> 
> For this: https://lkml.org/lkml/2014/5/9/508

OK, I see.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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@linutronix.de> [2]
> Signed-off-by: Viresh Kumar <viresh.kumar@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 <preeti@linux.vnet.ibm.com>

[tip:timers/core] clockevents: Introduce mode specific callbacks

[tip-bot for Viresh Kumar]

Commit-ID:  bd624d75db21ea5402f9ecf4450b311794d80352
Gitweb:     http://git.kernel.org/tip/bd624d75db21ea5402f9ecf4450b311794d80352
Author:     Viresh Kumar <viresh.kumar@linaro.org>
AuthorDate: Fri, 13 Feb 2015 08:54:56 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 18 Feb 2015 15:16:23 +0100

clockevents: Introduce mode specific callbacks

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 benefit from 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@linutronix.de> [2]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: linaro-kernel@lists.linaro.org
Cc: linaro-networking@linaro.org
Link: http://lkml.kernel.org/r/792d59a40423f0acffc9bb0bec9de1341a06fa02.1423788565.git.viresh.kumar@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 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 2e4cb67..59af26b 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 5544990..489642b 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 61ed862..2cfd194 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);