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

[Viresh Kumar]

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/