Skip to contentSkip to conversation

clockevents: introduce ->set_dev_mode() and convert a few drivers

PATCH In review Kevin Hilman 4 participants 9 messages Dec 9, 2014 – Jan 25, 2015

Subsystems (5)
Broadcom BCM2711/BCM2835 Arm architecture Clocksource, clockevent drivers Broadcom BCM281XX/BCM11XXX/BCM216XX Arm architecture High-resolution timers, timer wheel, clockevents +1 more subsystems
lore
Share & tools
lore

Kevin Hilman

author
#0 lore raw

cover clockevents: introduce ->set_dev_mode() and convert a few drivers

From: Kevin Hilman <[email protected]>

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.

Rather than changing the current ->set_mode() and requiring all clockevent devices to change immedately, introduce a new mode setting method ->set_dev_mode() which returns 'int'.

In addition, migrate a few drivers over to the new method to demonstrate how the new method is to be used, and how to convert.

Proposal for new method originally suggested by Thomas Gleixner[1].

[1] https://lkml.org/lkml/2014/5/10/86

Viresh Kumar (2):
clockevents: introduce ->set_dev_mode() which can return error
clockevents: migrate some drivers to new ->set_dev_mode()

 drivers/clocksource/arm_arch_timer.c     | 46 +++++++++++++++++---------------
 drivers/clocksource/bcm2835_timer.c      | 10 +++----
 drivers/clocksource/bcm_kona_timer.c     | 15 ++++++++---
 drivers/clocksource/i8253.c              | 11 +++++---
 drivers/clocksource/time-armada-370-xp.c | 21 +++++++++++----
 include/linux/clockchips.h               |  5 +++-
 kernel/time/clockevents.c                | 21 ++++++++++++---
 kernel/time/timer_list.c                 |  5 +++-
 8 files changed, 91 insertions(+), 43 deletions(-)
Signature · 1 line
2.1.3
author
#1 lore raw

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

From: Viresh Kumar <[email protected]>

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 <[email protected]>
Reviewed-by Kevin Hilman <[email protected]>
[khilman: rework changelogs, minor formatting/checkpatch cleanups]
Signed-off-by Kevin Hilman <[email protected]>
---
 include/linux/clockchips.h |  5 ++++-
 kernel/time/clockevents.c  | 21 ++++++++++++++++++---
 kernel/time/timer_list.c   |  5 ++++-
 3 files changed, 26 insertions(+), 5 deletions(-)
include/linux/clockchips.h +4 -1
index 2e4cb67f6e56..d969659cf688 100644
@@ -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 *);
kernel/time/clockevents.c +18 -3
index 55449909f114..f7614041240e 100644
@@ -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;
 }
kernel/time/timer_list.c +4 -1
index 61ed862cdd37..957a04951ef0 100644
@@ -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:  ");
Signature · 1 line
2.1.3
Reviewed-by on v1 1/2
#2 lore raw

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

Quoted 95 lines · Kevin Hilman

From: Viresh Kumar <[email protected]>

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 <[email protected]>
Reviewed-by Kevin Hilman <[email protected]>
[khilman: rework changelogs, minor formatting/checkpatch cleanups]
Signed-off-by Kevin Hilman <[email protected]>
---
 include/linux/clockchips.h |  5 ++++-
 kernel/time/clockevents.c  | 21 ++++++++++++++++++---
 kernel/time/timer_list.c   |  5 ++++-
 3 files changed, 26 insertions(+), 5 deletions(-)
include/linux/clockchips.h +4 -1
index 2e4cb67f6e56..d969659cf688 100644
@@ -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 *);
kernel/time/clockevents.c +18 -3
index 55449909f114..f7614041240e 100644
@@ -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;
 }
kernel/time/timer_list.c +0 -0
index 61ed862cdd37..957a04951ef0 100644
@@ -229,7 +229,10 @@ print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu)
 	SEQ_printf(m, "\n");

kernel/time/timer_list.c print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu) · line 238

Quoted 10 lines
kernel/time/timer_list.c +4 -1
 
 	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 <[email protected]>
#3 lore raw

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

In reply to Kevin Hilman
Quoted 25 lines · Kevin Hilman
kernel/time/clockevents.c +10 -1
index 55449909f114..f7614041240e 100644
@@ -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);

kernel/time/clockevents.c int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) · line 466

Quoted 10 lines
kernel/time/clockevents.c +7 -2
 
-	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

#4 lore raw

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

Hi Thomas,

Quoted 31 lines · Thomas Gleixner

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);
Quoted 8 lines
        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..

Quoted 33 lines
        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;
}
Quoted 2 lines

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

#5 lore raw

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

Quoted 7 lines
Quoted 2 lines
        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.

Quoted 21 lines
Quoted 2 lines

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;
}
Quoted 2 lines

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

author
#6 lore raw

PATCH 2/2 clockevents: migrate some drivers to new ->set_dev_mode()

In reply to Kevin Hilman

From: Viresh Kumar <[email protected]>

Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.

This patch migrates few clockevent drivers to the new method to demonstrate how to convert to the new interface.

Drivers are modified to return -ENOSYS when requested to switch to unsupported mode and return 0 on success. This patch shouldn't result in any other functional change in drivers.

Most of the changes are automated with help of Coccinelle
(http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually. Some additional changes (like adding 'int ret = 0' and 'return ret') as well as whitespace fixups were also done manually after coccinelle.

A simplified version of the semantic patch is:

// <smpl>
@@
identifier m,c,setmode;
@@
-void
+int
setmode(enum clock_event_mode m, struct clock_event_device *c);

@@
identifier setmode;
@@
-void
+int
setmode(enum clock_event_mode, struct clock_event_device *);

@fixret@
identifier m,c,setmode;
@@
-void
+int
setmode(enum clock_event_mode m, struct clock_event_device *c)
{
...
+ return 0;
}

@depends on fixret@
identifier ced;
identifier fixret.setmode;
@@
... struct clock_event_device ced = {
...,
-.set_mode
+.set_dev_mode
= setmode,
};

@depends on fixret@
expression ced;
identifier fixret.setmode;
@@
- ced->set_mode
+ ced->set_dev_mode
= setmode

@depends on fixret@
identifier fixret.setmode;
@@
{
.
-set_mode
+set_dev_mode
= setmode
}

@depends on fixret@
expression ced;
identifier fixret.setmode;
@@
- ced.set_mode
+ ced.set_dev_mode
= setmode

// </smpl>

Signed-off-by Viresh Kumar <[email protected]>
Reviewed-by Kevin Hilman <[email protected]>
[khilman: rework changelog, minor formatting, checkpatch cleanups]
Signed-off-by Kevin Hilman <[email protected]>
---
 drivers/clocksource/arm_arch_timer.c     | 46 +++++++++++++++++---------------
 drivers/clocksource/bcm2835_timer.c      | 10 +++----
 drivers/clocksource/bcm_kona_timer.c     | 15 ++++++++---
 drivers/clocksource/i8253.c              | 11 +++++---
 drivers/clocksource/time-armada-370-xp.c | 21 +++++++++++----
 5 files changed, 65 insertions(+), 38 deletions(-)
drivers/clocksource/arm_arch_timer.c +25 -21
index 43005d4d3348..368302004076 100644
@@ -179,8 +179,8 @@ static irqreturn_t arch_timer_handler_virt_mem(int irq, void *dev_id)
 	return timer_handler(ARCH_TIMER_MEM_VIRT_ACCESS, evt);
 }
 
-static __always_inline void timer_set_mode(const int access, int mode,
-				  struct clock_event_device *clk)
+static __always_inline int timer_set_mode(const int access, int mode,
+					  struct clock_event_device *clk)
 {
 	unsigned long ctrl;
 	switch (mode) {

@@ -190,33 +190,37 @@ static __always_inline void timer_set_mode(const int access, int mode,
 		ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
 		arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
 		break;
-	default:
+	case CLOCK_EVT_MODE_ONESHOT:
+	case CLOCK_EVT_MODE_RESUME:
 		break;
+	default:
+		return -ENOSYS;
 	}
+	return 0;
 }
 
-static void arch_timer_set_mode_virt(enum clock_event_mode mode,
-				     struct clock_event_device *clk)
+static int arch_timer_set_mode_virt(enum clock_event_mode mode,
+				    struct clock_event_device *clk)
 {
-	timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode, clk);
+	return timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode, clk);
 }
 
-static void arch_timer_set_mode_phys(enum clock_event_mode mode,
-				     struct clock_event_device *clk)
+static int arch_timer_set_mode_phys(enum clock_event_mode mode,
+				    struct clock_event_device *clk)
 {
-	timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode, clk);
+	return timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode, clk);
 }
 
-static void arch_timer_set_mode_virt_mem(enum clock_event_mode mode,
-					 struct clock_event_device *clk)
+static int arch_timer_set_mode_virt_mem(enum clock_event_mode mode,
+					struct clock_event_device *clk)
 {
-	timer_set_mode(ARCH_TIMER_MEM_VIRT_ACCESS, mode, clk);
+	return timer_set_mode(ARCH_TIMER_MEM_VIRT_ACCESS, mode, clk);
 }
 
-static void arch_timer_set_mode_phys_mem(enum clock_event_mode mode,
-					 struct clock_event_device *clk)
+static int arch_timer_set_mode_phys_mem(enum clock_event_mode mode,
+					struct clock_event_device *clk)
 {
-	timer_set_mode(ARCH_TIMER_MEM_PHYS_ACCESS, mode, clk);
+	return timer_set_mode(ARCH_TIMER_MEM_PHYS_ACCESS, mode, clk);
 }
 
 static __always_inline void set_next_event(const int access, unsigned long evt,

@@ -271,11 +275,11 @@ static void __arch_timer_setup(unsigned type,
 		clk->cpumask = cpumask_of(smp_processor_id());
 		if (arch_timer_use_virtual) {
 			clk->irq = arch_timer_ppi[VIRT_PPI];
-			clk->set_mode = arch_timer_set_mode_virt;
+			clk->set_dev_mode = arch_timer_set_mode_virt;
 			clk->set_next_event = arch_timer_set_next_event_virt;
 		} else {
 			clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
-			clk->set_mode = arch_timer_set_mode_phys;
+			clk->set_dev_mode = arch_timer_set_mode_phys;
 			clk->set_next_event = arch_timer_set_next_event_phys;
 		}
 	} else {

@@ -284,17 +288,17 @@ static void __arch_timer_setup(unsigned type,
 		clk->rating = 400;
 		clk->cpumask = cpu_all_mask;
 		if (arch_timer_mem_use_virtual) {
-			clk->set_mode = arch_timer_set_mode_virt_mem;
+			clk->set_dev_mode = arch_timer_set_mode_virt_mem;
 			clk->set_next_event =
 				arch_timer_set_next_event_virt_mem;
 		} else {
-			clk->set_mode = arch_timer_set_mode_phys_mem;
+			clk->set_dev_mode = arch_timer_set_mode_phys_mem;
 			clk->set_next_event =
 				arch_timer_set_next_event_phys_mem;
 		}
 	}
 
-	clk->set_mode(CLOCK_EVT_MODE_SHUTDOWN, clk);
+	clk->set_dev_mode(CLOCK_EVT_MODE_SHUTDOWN, clk);
 
 	clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fffffff);
 }

@@ -497,7 +501,7 @@ static void arch_timer_stop(struct clock_event_device *clk)
 			disable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI]);
 	}
 
-	clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk);
+	clk->set_dev_mode(CLOCK_EVT_MODE_UNUSED, clk);
 }
 
 static int arch_timer_cpu_notify(struct notifier_block *self,
drivers/clocksource/bcm2835_timer.c +5 -5
index 26ed331b1aad..04ae14ca81ab 100644
@@ -54,8 +54,8 @@ static u64 notrace bcm2835_sched_read(void)
 	return readl_relaxed(system_clock);
 }
 
-static void bcm2835_time_set_mode(enum clock_event_mode mode,
-	struct clock_event_device *evt_dev)
+static int bcm2835_time_set_mode(enum clock_event_mode mode,
+				 struct clock_event_device *evt_dev)
 {
 	switch (mode) {
 	case CLOCK_EVT_MODE_ONESHOT:

@@ -64,9 +64,9 @@ static void bcm2835_time_set_mode(enum clock_event_mode mode,
 	case CLOCK_EVT_MODE_RESUME:
 		break;
 	default:
-		WARN(1, "%s: unhandled event mode %d\n", __func__, mode);
-		break;
+		return -ENOSYS;
 	}
+	return 0;
 }
 
 static int bcm2835_time_set_next_event(unsigned long event,

@@ -129,7 +129,7 @@ static void __init bcm2835_timer_init(struct device_node *node)
 	timer->evt.name = node->name;
 	timer->evt.rating = 300;
 	timer->evt.features = CLOCK_EVT_FEAT_ONESHOT;
-	timer->evt.set_mode = bcm2835_time_set_mode;
+	timer->evt.set_dev_mode = bcm2835_time_set_mode;
 	timer->evt.set_next_event = bcm2835_time_set_next_event;
 	timer->evt.cpumask = cpumask_of(0);
 	timer->act.name = node->name;
drivers/clocksource/bcm_kona_timer.c +11 -4
index 0595dc6c453e..807915e9e2f8 100644
@@ -128,25 +128,32 @@ static int kona_timer_set_next_event(unsigned long clc,
 	return 0;
 }
 
-static void kona_timer_set_mode(enum clock_event_mode mode,
-			     struct clock_event_device *unused)
+static int kona_timer_set_mode(enum clock_event_mode mode,
+			       struct clock_event_device *unused)
 {
+	int ret = 0;
+
 	switch (mode) {
 	case CLOCK_EVT_MODE_ONESHOT:
 		/* by default mode is one shot don't do any thing */
 		break;
+	default:
+		ret = -ENOSYS;
+		/* fall through so timer is disabled */
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
-	default:
+	case CLOCK_EVT_MODE_RESUME:
 		kona_timer_disable_and_clear(timers.tmr_regs);
+		break;
 	}
+	return ret;
 }
 
 static struct clock_event_device kona_clockevent_timer = {
 	.name = "timer 1",
 	.features = CLOCK_EVT_FEAT_ONESHOT,
 	.set_next_event = kona_timer_set_next_event,
-	.set_mode = kona_timer_set_mode
+	.set_dev_mode = kona_timer_set_mode
 };
 
 static void __init kona_timer_clockevents_init(void)
drivers/clocksource/i8253.c +8 -3
index 14ee3efcc404..ea28efbfe9cf 100644
@@ -105,9 +105,11 @@ int __init clocksource_i8253_init(void)
  *
  * This is also called after resume to bring the PIT into operation again.
  */
-static void init_pit_timer(enum clock_event_mode mode,
-			   struct clock_event_device *evt)
+static int init_pit_timer(enum clock_event_mode mode,
+			  struct clock_event_device *evt)
 {
+	int ret = 0;
+
 	raw_spin_lock(&i8253_lock);
 
 	switch (mode) {

@@ -136,8 +138,11 @@ static void init_pit_timer(enum clock_event_mode mode,
 	case CLOCK_EVT_MODE_RESUME:
 		/* Nothing to do here */
 		break;
+	default:
+		ret = -ENOSYS;
 	}
 	raw_spin_unlock(&i8253_lock);
+	return ret;
 }
 
 /*

@@ -162,7 +167,7 @@ static int pit_next_event(unsigned long delta, struct clock_event_device *evt)
 struct clock_event_device i8253_clockevent = {
 	.name		= "pit",
 	.features	= CLOCK_EVT_FEAT_PERIODIC,
-	.set_mode	= init_pit_timer,
+	.set_dev_mode	= init_pit_timer,
 	.set_next_event = pit_next_event,
 };
drivers/clocksource/time-armada-370-xp.c +16 -5
index 0451e62fac7a..4ea3d4c9d651 100644
@@ -120,12 +120,14 @@ armada_370_xp_clkevt_next_event(unsigned long delta,
 	return 0;
 }
 
-static void
+static int
 armada_370_xp_clkevt_mode(enum clock_event_mode mode,
 			  struct clock_event_device *dev)
 {
-	if (mode == CLOCK_EVT_MODE_PERIODIC) {
+	int ret = 0;
 
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
 		/*
 		 * Setup timer to fire at 1/HZ intervals.
 		 */

@@ -136,7 +138,14 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode,
 		 * Enable timer.
 		 */
 		local_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | enable_mask);
-	} else {
+		break;
+	default:
+		ret = -ENOSYS;
+		/* fall through so timer is disabled, ACK'd */
+	case CLOCK_EVT_MODE_ONESHOT:
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_RESUME:
 		/*
 		 * Disable timer.
 		 */

@@ -146,7 +155,9 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode,
 		 * ACK pending timer interrupt.
 		 */
 		writel(TIMER0_CLR_MASK, local_base + LCL_TIMER_EVENTS_STATUS);
+		break;
 	}
+	return ret;
 }
 
 static int armada_370_xp_clkevt_irq;

@@ -184,7 +195,7 @@ static int armada_370_xp_timer_setup(struct clock_event_device *evt)
 	evt->shift		= 32,
 	evt->rating		= 300,
 	evt->set_next_event	= armada_370_xp_clkevt_next_event,
-	evt->set_mode		= armada_370_xp_clkevt_mode,
+	evt->set_dev_mode	= armada_370_xp_clkevt_mode,
 	evt->irq		= armada_370_xp_clkevt_irq;
 	evt->cpumask		= cpumask_of(cpu);
 

@@ -196,7 +207,7 @@ static int armada_370_xp_timer_setup(struct clock_event_device *evt)
 
 static void armada_370_xp_timer_stop(struct clock_event_device *evt)
 {
-	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
+	evt->set_dev_mode(CLOCK_EVT_MODE_UNUSED, evt);
 	disable_percpu_irq(evt->irq);
 }
Signature · 1 line
2.1.3
author
#7 lore raw
In reply to Kevin Hilman

Thomas,

Gentle reminder ping...

Quoted 41 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

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.

Rather than changing the current ->set_mode() and requiring all clockevent devices to change immedately, introduce a new mode setting method ->set_dev_mode() which returns 'int'.

In addition, migrate a few drivers over to the new method to demonstrate how the new method is to be used, and how to convert.

Proposal for new method originally suggested by Thomas Gleixner[1].

[1] https://lkml.org/lkml/2014/5/10/86

Viresh Kumar (2):
clockevents: introduce ->set_dev_mode() which can return error
clockevents: migrate some drivers to new ->set_dev_mode()

 drivers/clocksource/arm_arch_timer.c     | 46 +++++++++++++++++---------------
 drivers/clocksource/bcm2835_timer.c      | 10 +++----
 drivers/clocksource/bcm_kona_timer.c     | 15 ++++++++---
 drivers/clocksource/i8253.c              | 11 +++++---
 drivers/clocksource/time-armada-370-xp.c | 21 +++++++++++----
 include/linux/clockchips.h               |  5 +++-
 kernel/time/clockevents.c                | 21 ++++++++++++---
 kernel/time/timer_list.c                 |  5 +++-
 8 files changed, 91 insertions(+), 43 deletions(-)

--
2.1.3

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

#8 lore raw
Quoted 3 lines · Kevin Hilman

Thomas,

Gentle reminder ping...

Crap. I knew that some stuff on that todo list which I deleted will haunt me.

Thanks for the reminder.

tglx