On 20 February 2015 at 15:06, Ingo Molnar mingo@kernel.org wrote:
- Peter Zijlstra peterz@infradead.org 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