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

From: Viresh Kumar
Date: Fri Feb 20 2015 - 05:12:47 EST


On 20 February 2015 at 15:06, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
>> > 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 this patch was a follow on from bd624d75db21
>> ("clockevents: Introduce mode specific callbacks").
>>
>> That patch changes the set_mode() interface; and provides
>> per mode functions.

Thanks Peter ..

> So why is a 'default' mode needed then? It makes the
> addition of new modes to the legacy handler easier, which
> looks backwards.

The requirement was to add another mode ONESHOT_STOPPED [1],
to be supported only by the new per-mode callbacks..

We have got a clear check in core with the patch Peter mentioned above,
which doesn't let us call legacy ->set_mode() for the newer modes.

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;
}


The intention of adding a 'default' case (which is already present in most
of clockevents drivers) here was to make compiler happy and that's
why the commit logs mentioned this:

"
The rationale behind only adding a 'break' was that the default case *will
never* be hit during execution of code.
"

>> New (and updated) drivers should not use ->set_mode()
>> anymore, but it was felt that we do not want to go do
>> flag day changes.
>
> I fully agree that we don't want flag day changes, but make
> it really apparent that it's an obsolete interface:
>
> - rename it to set_mode_obsolete()
>
> - try to convert as many of the easy cases as possible -
> the overwhelming majority of mode setting functions
> look reasonably simple.

Yes, we can do that..

> - get rid of the mode enum in the core, and rename the
> mode bits to CLOCK_EVT_MODE_OBSOLETE_XXX.

> So I'm confused: if we are using proper callbacks (like my
> example outlined) , why is a 'mode enum' needed at all?

The enum has two uses today:

- pass mode to the legacy ->set_mode() callback, which isn't
required for the new callbacks.

- flag for clockevent core's internal state machine, which it would
still require. For example, it checks new-mode != old-mode before
changing the mode..

I believe the enum is still required for the state machine, even with
new per-mode callbacks.

--
viresh

[1] https://lkml.org/lkml/2014/5/9/508
--
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/