diff mbox

[1/2] clockevents: introduce ->set_dev_mode() which can return error

Message ID 1418162614-19182-2-git-send-email-khilman@kernel.org (mailing list archive)
State New, archived
Headers show

Commit Message

Kevin Hilman Dec. 9, 2014, 10:03 p.m. UTC
From: Viresh Kumar <viresh.kumar@linaro.org>

Currently, the ->set_mode() method of a clockevent device is not
allowed to fail, so it has no return value.  In order to add new
clockevent modes, and allow the setting of those modes to fail, we
need the clockevent core to be able to detect when setting a mode
fails.

To allow detection of mode setting failures, introduce a new method
->set_dev_mode() which can return an error if the given mode is not
supported or fails.

Since all current modes are still not allowed to fail, the core code
simply WARNs if any modes fail.  Future patches that add new mode
support will add proper error recovery based on failure conditions.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Kevin Hilman <khilman@linaro.org>
[khilman: rework changelogs, minor formatting/checkpatch cleanups]
Signed-off-by: Kevin Hilman <khilman@linaro.org>
---
 include/linux/clockchips.h |  5 ++++-
 kernel/time/clockevents.c  | 21 ++++++++++++++++++---
 kernel/time/timer_list.c   |  5 ++++-
 3 files changed, 26 insertions(+), 5 deletions(-)

Comments

preeti Dec. 10, 2014, 3:27 a.m. UTC | #1
On 12/10/2014 03:33 AM, Kevin Hilman wrote:
> From: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Currently, the ->set_mode() method of a clockevent device is not
> allowed to fail, so it has no return value.  In order to add new
> clockevent modes, and allow the setting of those modes to fail, we
> need the clockevent core to be able to detect when setting a mode
> fails.
> 
> To allow detection of mode setting failures, introduce a new method
> ->set_dev_mode() which can return an error if the given mode is not
> supported or fails.
> 
> Since all current modes are still not allowed to fail, the core code
> simply WARNs if any modes fail.  Future patches that add new mode
> support will add proper error recovery based on failure conditions.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Kevin Hilman <khilman@linaro.org>
> [khilman: rework changelogs, minor formatting/checkpatch cleanups]
> Signed-off-by: Kevin Hilman <khilman@linaro.org>
> ---
>  include/linux/clockchips.h |  5 ++++-
>  kernel/time/clockevents.c  | 21 ++++++++++++++++++---
>  kernel/time/timer_list.c   |  5 ++++-
>  3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
> index 2e4cb67f6e56..d969659cf688 100644
> --- a/include/linux/clockchips.h
> +++ b/include/linux/clockchips.h
> @@ -81,7 +81,8 @@ 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_dev_mode:	set dev mode function
> + * @set_mode:		set mode function (deprecated, use set_dev_mode instead)
>   * @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
> @@ -109,6 +110,8 @@ struct clock_event_device {
>  	unsigned long		retries;
> 
>  	void			(*broadcast)(const struct cpumask *mask);
> +	int			(*set_dev_mode)(enum clock_event_mode mode,
> +						struct clock_event_device *);
>  	void			(*set_mode)(enum clock_event_mode mode,
>  					    struct clock_event_device *);
>  	void			(*suspend)(struct clock_event_device *);
> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
> index 55449909f114..f7614041240e 100644
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -105,7 +105,16 @@ void clockevents_set_mode(struct clock_event_device *dev,
>  				 enum clock_event_mode mode)
>  {
>  	if (dev->mode != mode) {
> -		dev->set_mode(mode, dev);
> +		if (dev->set_dev_mode) {
> +			int ret = dev->set_dev_mode(mode, dev);
> +
> +			/* Currently available modes shouldn't fail */
> +			WARN_ONCE(ret, "Requested mode: %d, error: %d\n",
> +				  mode, ret);
> +		} else {
> +			dev->set_mode(mode, dev);
> +		}
> +
>  		dev->mode = mode;
> 
>  		/*
> @@ -448,8 +457,14 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
>  	if (dev->mode == CLOCK_EVT_MODE_ONESHOT)
>  		return clockevents_program_event(dev, dev->next_event, false);
> 
> -	if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
> -		dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
> +	if (dev->mode == CLOCK_EVT_MODE_PERIODIC) {
> +		/* Shouldn't fail while switching to PERIODIC MODE */
> +		if (dev->set_dev_mode)
> +			WARN_ON_ONCE(dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC,
> +						       dev));
> +		else
> +			dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
> +	}
> 
>  	return 0;
>  }
> diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
> index 61ed862cdd37..957a04951ef0 100644
> --- a/kernel/time/timer_list.c
> +++ b/kernel/time/timer_list.c
> @@ -229,7 +229,10 @@ print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu)
>  	SEQ_printf(m, "\n");
> 
>  	SEQ_printf(m, " set_mode:       ");
> -	print_name_offset(m, dev->set_mode);
> +	if (dev->set_dev_mode)
> +		print_name_offset(m, dev->set_dev_mode);
> +	else
> +		print_name_offset(m, dev->set_mode);
>  	SEQ_printf(m, "\n");
> 
>  	SEQ_printf(m, " event_handler:  ");
> 
Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Thomas Gleixner Jan. 23, 2015, 11:57 a.m. UTC | #2
On Tue, 9 Dec 2014, Kevin Hilman wrote:
> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
> index 55449909f114..f7614041240e 100644
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -105,7 +105,16 @@ void clockevents_set_mode(struct clock_event_device *dev,
>  				 enum clock_event_mode mode)
>  {
>  	if (dev->mode != mode) {
> -		dev->set_mode(mode, dev);
> +		if (dev->set_dev_mode) {
> +			int ret = dev->set_dev_mode(mode, dev);
> +
> +			/* Currently available modes shouldn't fail */
> +			WARN_ONCE(ret, "Requested mode: %d, error: %d\n",
> +				  mode, ret);
> +		} else {
> +			dev->set_mode(mode, dev);
> +		}
> +
>  		dev->mode = mode;
>  
>  		/*
> @@ -448,8 +457,14 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
>  	if (dev->mode == CLOCK_EVT_MODE_ONESHOT)
>  		return clockevents_program_event(dev, dev->next_event, false);
>  
> -	if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
> -		dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
> +	if (dev->mode == CLOCK_EVT_MODE_PERIODIC) {
> +		/* Shouldn't fail while switching to PERIODIC MODE */
> +		if (dev->set_dev_mode)
> +			WARN_ON_ONCE(dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC,
> +						       dev));
> +		else
> +			dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);

Please split that out into

static int __clockevents_set_mode(dev, mode)
{
	int ret = 0;

	if (dev->set_dev_mode) {
	        ret = set_dev_mode(mode, dev);
		WARN_ON_ONCE(ret);
	} else {
		dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
	}
	return ret;
}

Now looking at the conversion patch, it looks like we add lots of

    case X:
    case Y:
    	 return 0;
	 
to avoid the

    default:
	 return -ENOSYS;

After staring at that I'm less convinced about this approach. So lets
think about something different:

static int __clockevents_set_mode(dev, mode)
{
	/* Transition mode */
	if (dev->set_mode) {
	   if (mode > CLOCK_EVT_MODE_RESUME)
	         return -ENOSYS;
	   dev->set_mode(mode, dev);
	   return 0;
	}

	if (dev->features & CLOCK_EVT_FEAT_DUMMY)
	   return 0;

	switch (mode) {
	/*
	 * This is an internal state, which is guaranteed to
	 * go from SHUTDOWN to UNUSED. No driver interaction
	 * required.
	 */
	case CLOCK_EVT_MODE_UNUSED:
	     return 0;   

	case CLOCK_EVT_MODE_SHUTDOWN:
	     return dev->shutdown(dev);

	case CLOCK_EVT_MODE_PERIODIC:
	     /* Core internal bug */
	     if (!(dev->features & CLOCK_EVT_FEAT_PERIODIC))
	     	   return -ENOSYS;
	     return dev->setup_periodic(dev);

	case CLOCK_EVT_MODE_ONESHOT:
	     /* Core internal bug */
	     if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
	     	   return -ENOSYS;
	     return dev->setup_oneshot(dev);

	case CLOCK_EVT_MODE_RESUME:
	     return dev->setup_resume(dev);

     	default:
	     return -ENOSYS;
}

And do sanity checking on registration time:

static int ce_check(dev)
{
       if (dev->set_mode)
       	   return 0;

       if (dev->features & CLOCK_EVT_FEAT_DUMMY)
	   return 0;

       if (!dev->shutdown)
       	   return -EMORON;

       if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) &&
       	   !dev->setup_periodic)
       	   return -EMORON;

       if ((dev->features & CLOCK_EVT_FEAT_ONESHOT) &&
       	   !dev->setup_oneshot)
       	   return -EMORON;

       /* Optional callbacks */       
       if (!dev->setup_resume)
       	  dev->setup_resume = setup_noop();

       return 0;
}

That gives us a clear distinction of required and optional callbacks,
which will make error handling and recovery simpler as well.

Thanks,

	tglx
Viresh Kumar Jan. 25, 2015, 1:57 p.m. UTC | #3
Hi Thomas,

On 23 January 2015 at 17:27, Thomas Gleixner <tglx@linutronix.de> wrote:
> static int __clockevents_set_mode(dev, mode)
> {
>         /* Transition mode */
>         if (dev->set_mode) {
>            if (mode > CLOCK_EVT_MODE_RESUME)
>                  return -ENOSYS;
>            dev->set_mode(mode, dev);
>            return 0;
>         }
>
>         if (dev->features & CLOCK_EVT_FEAT_DUMMY)
>            return 0;
>
>         switch (mode) {
>         /*
>          * This is an internal state, which is guaranteed to
>          * go from SHUTDOWN to UNUSED. No driver interaction
>          * required.
>          */
>         case CLOCK_EVT_MODE_UNUSED:
>              return 0;
>
>         case CLOCK_EVT_MODE_SHUTDOWN:
>              return dev->shutdown(dev);
>
>         case CLOCK_EVT_MODE_PERIODIC:
>              /* Core internal bug */
>              if (!(dev->features & CLOCK_EVT_FEAT_PERIODIC))
>                    return -ENOSYS;
>              return dev->setup_periodic(dev);
>
>         case CLOCK_EVT_MODE_ONESHOT:
>              /* Core internal bug */
>              if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
>                    return -ENOSYS;
>              return dev->setup_oneshot(dev);
>
>         case CLOCK_EVT_MODE_RESUME:
>              return dev->setup_resume(dev);

Because setting to all these existing modes isn't allowed
to fail currently, shouldn't we make return type of all the new
callbacks as 'void' and return 0 from this routine ? That way, these
callbacks would stay consistent with the set_mode() callback..

>
>         default:
>              return -ENOSYS;
> }
>
> And do sanity checking on registration time:
>
> static int ce_check(dev)
> {
>        if (dev->set_mode)
>            return 0;
>
>        if (dev->features & CLOCK_EVT_FEAT_DUMMY)
>            return 0;
>
>        if (!dev->shutdown)
>            return -EMORON;
>
>        if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) &&
>            !dev->setup_periodic)
>            return -EMORON;
>
>        if ((dev->features & CLOCK_EVT_FEAT_ONESHOT) &&
>            !dev->setup_oneshot)
>            return -EMORON;
>
>        /* Optional callbacks */
>        if (!dev->setup_resume)
>           dev->setup_resume = setup_noop();
>
>        return 0;
> }
>
> That gives us a clear distinction of required and optional callbacks,
> which will make error handling and recovery simpler as well.

Now that we are looking forward to providing individual callbacks
per feature, I wanted to propose few more solutions to it, though I am
quite sure you would have already considered and rejected them.

I remember from your earlier mail that you didn't wanted to add
ONESHOT_STOPPED as a feature (as it isn't a feature really)
and that restricts us from doing this:

static int __clockevents_set_mode(dev, mode)
{
        if ((mode == CLOCK_EVT_MODE_ONESHOT_STOPPED) &&
            !(dev->features & CLOCK_EVT_FEAT_ONESHOT_STOPPED))
                return -ENOSYS;

        dev->set_mode(mode, dev);
        return 0;
}

But what about providing a separate callback only for ONESHOT_STOPPED
mode and use set_mode() for everything else ?

static int __clockevents_set_mode(dev, mode)
{
        if (mode == CLOCK_EVT_MODE_ONESHOT_STOPPED) {
                if (dev->stop_onshot)
                        return dev->stop_onshot(dev);
                else
                        return -ENOSYS;
        }

        dev->set_mode(mode, dev);
        return 0;
}

Thanks for looking into this thread again..

--
viresh
Thomas Gleixner Jan. 25, 2015, 3:03 p.m. UTC | #4
> >         case CLOCK_EVT_MODE_RESUME:
> >              return dev->setup_resume(dev);
> 
> Because setting to all these existing modes isn't allowed
> to fail currently, shouldn't we make return type of all the new
> callbacks as 'void' and return 0 from this routine ? That way, these
> callbacks would stay consistent with the set_mode() callback..

If we go to individual callbacks then we analyze of a case by case
basis which ones need a return value and which ones to not.
 
> > That gives us a clear distinction of required and optional callbacks,
> > which will make error handling and recovery simpler as well.
> 
> Now that we are looking forward to providing individual callbacks
> per feature, I wanted to propose few more solutions to it, though I am
> quite sure you would have already considered and rejected them.
> 
> I remember from your earlier mail that you didn't wanted to add
> ONESHOT_STOPPED as a feature (as it isn't a feature really)
> and that restricts us from doing this:
> 
> static int __clockevents_set_mode(dev, mode)
> {
>         if ((mode == CLOCK_EVT_MODE_ONESHOT_STOPPED) &&
>             !(dev->features & CLOCK_EVT_FEAT_ONESHOT_STOPPED))
>                 return -ENOSYS;
> 
>         dev->set_mode(mode, dev);
>         return 0;
> }
> 
> But what about providing a separate callback only for ONESHOT_STOPPED
> mode and use set_mode() for everything else ?

And then fixup all drivers which do not have a default clause in their
switch case and add MODE_ONESHOT_STOPPED? And half a year later we do
the same for the next mode. No way.

The current interface is suboptimal and we fix it proper. Period.

Thanks,

	tglx
diff mbox

Patch

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 2e4cb67f6e56..d969659cf688 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -81,7 +81,8 @@  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_dev_mode:	set dev mode function
+ * @set_mode:		set mode function (deprecated, use set_dev_mode instead)
  * @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
@@ -109,6 +110,8 @@  struct clock_event_device {
 	unsigned long		retries;
 
 	void			(*broadcast)(const struct cpumask *mask);
+	int			(*set_dev_mode)(enum clock_event_mode mode,
+						struct clock_event_device *);
 	void			(*set_mode)(enum clock_event_mode mode,
 					    struct clock_event_device *);
 	void			(*suspend)(struct clock_event_device *);
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 55449909f114..f7614041240e 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -105,7 +105,16 @@  void clockevents_set_mode(struct clock_event_device *dev,
 				 enum clock_event_mode mode)
 {
 	if (dev->mode != mode) {
-		dev->set_mode(mode, dev);
+		if (dev->set_dev_mode) {
+			int ret = dev->set_dev_mode(mode, dev);
+
+			/* Currently available modes shouldn't fail */
+			WARN_ONCE(ret, "Requested mode: %d, error: %d\n",
+				  mode, ret);
+		} else {
+			dev->set_mode(mode, dev);
+		}
+
 		dev->mode = mode;
 
 		/*
@@ -448,8 +457,14 @@  int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
 	if (dev->mode == CLOCK_EVT_MODE_ONESHOT)
 		return clockevents_program_event(dev, dev->next_event, false);
 
-	if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
-		dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
+	if (dev->mode == CLOCK_EVT_MODE_PERIODIC) {
+		/* Shouldn't fail while switching to PERIODIC MODE */
+		if (dev->set_dev_mode)
+			WARN_ON_ONCE(dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC,
+						       dev));
+		else
+			dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
+	}
 
 	return 0;
 }
diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index 61ed862cdd37..957a04951ef0 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -229,7 +229,10 @@  print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu)
 	SEQ_printf(m, "\n");
 
 	SEQ_printf(m, " set_mode:       ");
-	print_name_offset(m, dev->set_mode);
+	if (dev->set_dev_mode)
+		print_name_offset(m, dev->set_dev_mode);
+	else
+		print_name_offset(m, dev->set_mode);
 	SEQ_printf(m, "\n");
 
 	SEQ_printf(m, " event_handler:  ");