On 29 May 2014 15:28, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all MIPS specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
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 drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
@@
[snip]
diff --git a/arch/mips/kernel/cevt-bcm1480.c b/arch/mips/kernel/cevt-bcm1480.c index 7976457..007fdb1 100644 --- a/arch/mips/kernel/cevt-bcm1480.c +++ b/arch/mips/kernel/cevt-bcm1480.c @@ -40,7 +40,7 @@
- The general purpose timer ticks at 1MHz independent if
- the rest of the system
*/ -static void sibyte_set_mode(enum clock_event_mode mode, +static int sibyte_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned int cpu = smp_processor_id(); @@ -63,10 +63,13 @@ static void sibyte_set_mode(enum clock_event_mode mode, __raw_writeq(0, cfg); break;
case CLOCK_EVT_MODE_UNUSED: /* shuddup gcc */Why is this comment removed?
I thought it was added there because it didn't had a default case and these empty 'cases' are there to make sure compiler doesn't throw warnings at them.
Because we have a default case now, these prints should go.
case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME:
;
break;default:return -ENOSYS; }return 0;}
static int sibyte_next_event(unsigned long delta, struct clock_event_device *cd) @@ -130,7 +133,7 @@ void sb1480_clockevent_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = sibyte_next_event;
cd->set_mode = sibyte_set_mode;
cd->set_dev_mode = sibyte_set_mode; clockevents_register_device(cd); bcm1480_mask_irq(cpu, irq);diff --git a/arch/mips/kernel/cevt-ds1287.c b/arch/mips/kernel/cevt-ds1287.c index ff1f01b..f86642a 100644 --- a/arch/mips/kernel/cevt-ds1287.c +++ b/arch/mips/kernel/cevt-ds1287.c @@ -59,7 +59,7 @@ static int ds1287_set_next_event(unsigned long delta, return -EINVAL; }
-static void ds1287_set_mode(enum clock_event_mode mode, +static int ds1287_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u8 val; @@ -72,14 +72,20 @@ static void ds1287_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_PERIODIC: val |= RTC_PIE; break;
default:
case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_SHUTDOWN:case CLOCK_EVT_MODE_RESUME: val &= ~RTC_PIE; break;default:spin_unlock(&rtc_lock);return -ENOSYS; } CMOS_WRITE(val, RTC_REG_B);What about this ^^ in default case ? In default case, they would fall through and execute this instruction. After this patch, they won't. Would this be ok?
They shouldn't execute this for any unsupported modes, like ONESHOT_STOPPED as we are handling all available modes in the switch block now.
Earlier default meant: UNUSED/SHUTDOWN/RESUME probably.
diff --git a/arch/mips/ralink/cevt-rt3352.c b/arch/mips/ralink/cevt-rt3352.c index 24bf057..c92b211 100644 --- a/arch/mips/ralink/cevt-rt3352.c +++ b/arch/mips/ralink/cevt-rt3352.c @@ -36,7 +36,7 @@ struct systick_device { int freq_scale; };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt);
static int systick_next_event(unsigned long delta, @@ -76,7 +76,7 @@ static struct systick_device systick = { .rating = 310, .features = CLOCK_EVT_FEAT_ONESHOT, .set_next_event = systick_next_event,
.set_mode = systick_set_clock_mode,
.set_dev_mode = systick_set_clock_mode, .event_handler = systick_event_handler, },}; @@ -87,7 +87,7 @@ static struct irqaction systick_irqaction = { .dev_id = &systick.dev, };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct systick_device *sdev; @@ -110,10 +110,13 @@ static void systick_set_clock_mode(enum clock_event_mode mode, iowrite32(0, systick.membase + SYSTICK_CONFIG); break;
default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);
case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_RESUME: break;default:return -ENOSYS; }This is another scenario I am confused about. We have a pr_err() already in the default case. Why do we skip adding the pr_err() in the case of MODE_UNUSED and MODE_RESUME and in the default case ? Only then would it be consistent with the earlier scenario right? I am saying something like the below:
default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);
case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_RESUME:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name); break;default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);return -ENOSYS; }
These modes aren't optional and so we don't return any error from them. And so doing a pr_err wouldn't be right anymore.
return 0;}
static void __init ralink_systick_init(struct device_node *np) diff --git a/arch/mips/sgi-ip27/ip27-timer.c b/arch/mips/sgi-ip27/ip27-timer.c index 1d97eab..5d5e90c 100644 --- a/arch/mips/sgi-ip27/ip27-timer.c +++ b/arch/mips/sgi-ip27/ip27-timer.c @@ -63,10 +63,20 @@ static int rt_next_event(unsigned long delta, struct clock_event_device *evt) return LOCAL_HUB_L(PI_RT_COUNT) >= cnt ? -ETIME : 0; }
-static void rt_set_mode(enum clock_event_mode mode, +static int rt_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) {
switch (mode) {case CLOCK_EVT_MODE_ONESHOT:case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_SHUTDOWN:case CLOCK_EVT_MODE_RESUME: /* Nothing to do ... */break;default:return -ENOSYS;}return 0;}
unsigned int rt_timer_irq; @@ -123,7 +133,7 @@ void hub_rt_clock_event_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = rt_next_event;
cd->set_mode = rt_set_mode;
cd->set_dev_mode = rt_set_mode; clockevents_register_device(cd);}
diff --git a/arch/mips/sni/time.c b/arch/mips/sni/time.c index cf8ec56..ac666e5 100644 --- a/arch/mips/sni/time.c +++ b/arch/mips/sni/time.c @@ -14,7 +14,7 @@ #define SNI_COUNTER2_DIV 64 #define SNI_COUNTER0_DIV ((SNI_CLOCK_TICK_RATE / SNI_COUNTER2_DIV) / HZ)
-static void a20r_set_mode(enum clock_event_mode mode, +static int a20r_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -34,13 +34,14 @@ static void a20r_set_mode(enum clock_event_mode mode, wmb();
break;
case CLOCK_EVT_MODE_ONESHOT:Why is this line deleted?
ONESHOT isn't supported by this driver, they just had a empty case here.