Re: [PATCH] clockevents: Add (missing) default case for switch blocks

From: Ingo Molnar
Date: Fri Feb 20 2015 - 03:38:53 EST



* Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:

> Many clockevent drivers are using a switch block for handling modes in their
> ->set_mode() callback. Some of these do not have a 'default' case and adding a
> new mode in the 'enum clock_event_mode', starts giving following warnings for
> these platforms about unhandled modes (e.g. XXX).
>
> warning: enumeration value âXXXâ not handled in switch [-Wswitch]
>
> This patch adds default cases for them.
>
> In order to keep things simple, add following to the switch blocks:
>
> default:
> break;
>
> This can lead to different behavior for individual cases.
>
> 1) Some of the drivers don't have any special stuff in their ->set_mode()
> callback before or after the switch blocks. And so this default case would
> simply return for them without any updates to the clockevent device.
>
> 2) But in some cases, the clockevent device is disabled/stopped as soon as we
> enter the ->set_mode() callback and is enabled within the switch block or
> after it. And the clockevent device *may* stay disabled for default case.

So this whole approach looks fragile for several reasons:

- 'mode setting' callbacks are just bad by design
because they mix several functions into the same entry
point, complicating the handler functions
unnecessarily. We should reduce complexity, not expand
on it.

- now by adding 'default' you hide from drivers the
ability to easily discover whether it has been updated
to some new core clockevents mode setting feature or
not.

So NAK: the real solution would be to eliminate 'mode
setting' altogether: treat it as a legacy, introduce
properly separated callbacks and function pointer callback
driver templates, and let drivers fill in (or not)
individual entries. Clockevents core can mandate certain
entries to be filled in, or allow NULL with some default
behavior.

The ->set_mode() approach allows none of this. It is
ioctl()-alike interface design, and that is a singularly
bad design for the aforementioned reasons.

So to give a quick example what I have in mind, I looked up
a random clockevents driver callback from your patch
(arch/arm/mach-mmp/time.c):

static void timer_set_mode(enum clock_event_mode mode,
struct clock_event_device *dev)
{
unsigned long flags;

local_irq_save(flags);
switch (mode) {
case CLOCK_EVT_MODE_ONESHOT:
case CLOCK_EVT_MODE_UNUSED:
case CLOCK_EVT_MODE_SHUTDOWN:
/* disable the matching interrupt */
__raw_writel(0x00, mmp_timer_base + TMR_IER(0));
break;
case CLOCK_EVT_MODE_RESUME:
case CLOCK_EVT_MODE_PERIODIC:
break;
default:
break;
}
local_irq_restore(flags);
}

Instead of that I'd let the driver define just a single
function:

static void mmp_timer_disable(struct clock_event_device *dev __unused)
{
unsigned long flags;

local_irq_save(flags);
__raw_writel(0x00, mmp_timer_base + TMR_IER(0));
local_irq_restore(flags);
}


static struct clockevents_driver mmp_clockevents {
.clock_event__oneshot = mmp_timer_disable,
.clock_event__shutdown = mmp_timer_disable,

/* clock_event__resume and __periodic are NULL */
};

See how much cleaner and easier to read it all is?

Note how the NULL entries express a 'don't do anything'
default in a natural fashion. It's also future proof: if a
new callback is added to 'struct clockevents_driver' then
it's filled in with NULL and the clockevents core may
define a default behavior, without having to touch the
driver.

It's also _faster_: no function call needed and there's a
pointless local_irq_save()/restore() call optimized out as
well.

So let's clean up the clockevents mode setting design,
okay?

Thanks,

Ingo
--
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/