diff mbox

clockevents: Add (missing) default case for switch blocks

Message ID CAKohpokmCof2P8Un+i5eOS3BQhH_pMNATH4xkbbhw5TqJFEy9Q@mail.gmail.com
State New
Headers show

Commit Message

Viresh Kumar Feb. 20, 2015, 11:56 a.m. UTC
On 20 February 2015 at 17:11, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Peter Zijlstra <peterz@infradead.org> wrote:

>> Maybe we should break that enum into two; one for devices
>> and one for the core interface and avoid the problem that
>> way.
>
> Yeah, that would do the trick.

Thanks for your suggestions. Just to confirm (before I spam lists with
patches), is
this somewhat similar to what you are looking for ?


@@ -111,29 +115,29 @@ static int __clockevents_set_mode(struct
clock_event_device *dev,

        /* Transition with new mode-specific callbacks */
        switch (mode) {
-       case CLOCK_EVT_MODE_UNUSED:
+       case CLOCK_EVT_DEV_MODE_UNUSED:
                /*
                 * This is an internal state, which is guaranteed to go from
                 * SHUTDOWN to UNUSED. No driver interaction required.
                 */
                return 0;

-       case CLOCK_EVT_MODE_SHUTDOWN:
+       case CLOCK_EVT_DEV_MODE_SHUTDOWN:
                return dev->set_mode_shutdown(dev);

-       case CLOCK_EVT_MODE_PERIODIC:
+       case CLOCK_EVT_DEV_MODE_PERIODIC:
                /* Core internal bug */
                if (!(dev->features & CLOCK_EVT_FEAT_PERIODIC))
                        return -ENOSYS;
                return dev->set_mode_periodic(dev);

-       case CLOCK_EVT_MODE_ONESHOT:
+       case CLOCK_EVT_DEV_MODE_ONESHOT:
                /* Core internal bug */
                if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
                        return -ENOSYS;
                return dev->set_mode_oneshot(dev);

-       case CLOCK_EVT_MODE_RESUME:
+       case CLOCK_EVT_DEV_MODE_RESUME:
                /* Optional callback */
                if (dev->set_mode_resume)
                        return dev->set_mode_resume(dev);



Ofcourse, we also need to replace 'clock_event_mode' with 'clock_event_dev_mode'
and 'CLOCK_EVT_MODE_*' with 'CLOCK_EVT_DEV_MODE_*' in all core code..

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Viresh Kumar Feb. 20, 2015, 1:58 p.m. UTC | #1
On 20 February 2015 at 18:52, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> +       CLOCK_EVT_DEV_MODE_UNUSED = 0,
>
> What is 'unused' - not initialized yet?

Unused. Initially all clockevent devices are supposed to be in this
mode but later if another device replaces an existing one, the existing
one is put into this mode.

>> +       CLOCK_EVT_DEV_MODE_SHUTDOWN,
>> +       CLOCK_EVT_DEV_MODE_PERIODIC,
>> +       CLOCK_EVT_DEV_MODE_ONESHOT,
>> +       CLOCK_EVT_DEV_MODE_RESUME,
>
> What is 'resume' mode?

Introduced with: 18de5bc4c1f1 ("clockevents: fix resume logic") and is only
called during system resume to resume the clockevent devices before resuming
the tick. Only few implementations do meaningful stuff here.

>> +       CLOCK_EVT_DEV_MODE_ONESHOT_STOPPED,  /* This would be the new
>> mode which I will add later */
>
> What does this mode express?

I have added it here to show how things would look like eventually,
but it wouldn't
be present in the patch which splits the enum into two parts..

Its only important for NOHZ_FULL (IDLE ? Maybe). When we decide that the tick
(LOWRES) or hrtimer interrupt (HIGHRES) isn't required for indefinite
period of time
(i.e. no timers/hrtimers are present to serve), we skip reprogramming
the clockevent
device. But its already reprogrammed from the tick-handler and so will
fire atleast
once again.

The case is worst for implementations where the underlying hardware doesn't have
support for ONESHOT mode. And so they emulate ONESHOT over PERIODIC.
And in those cases, these spurious interrupts come at a rate last programmed for
the clockevent device. And that is mostly tick-rate..

> So for state machines it's important to document the states
> and the transitions between them very clearly - please
> start with that.

Sure.

> Also, this should not be in a generic header, it should be
> somewhere internal in kernel/time/.

Right. But there are some excellent drivers which are comparing things against
dev->mode (i.e. enum clock_event_dev_mode now..). I need to fix them as well
first to push this into some internal header.

>> Ofcourse, we also need to replace 'clock_event_mode' with
>> 'clock_event_dev_mode' and 'CLOCK_EVT_MODE_*' with
>> 'CLOCK_EVT_DEV_MODE_*' in all core code..
>
> Yes.

Thanks for your suggestions.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Viresh Kumar Feb. 23, 2015, 5:33 a.m. UTC | #2
On 20 February 2015 at 19:34, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Viresh Kumar <viresh.kumar@linaro.org> wrote:

>> Unused. Initially all clockevent devices are supposed to
>> be in this mode but later if another device replaces an
>> existing one, the existing one is put into this mode.
>
> I'd suggest to rename it to MODE_INIT - at first glance it
> gave me the impression that it's some sort of API
> placeholder - i.e. an unused flag or so.

Sorry, if I wasn't able to clarify this earlier. The impression you got
at first glance is correct. And it should be UNUSED only instead of
INIT. Its not about if the the device is initialized or not, but if it is used
or not. And that's why there is no callback for this in the new per-mode
callbacks.

> Also, I'd suggest to rename all 'modes' to true state
> machine naming: STATE_INITIALIZED, STATE_SHUT_DOWN,
> STATE_PERIODIC, STATE_RESUMED, etc.: if these are enums for

I thought so initially and it looked like the diff will be huge as all the
variables for the enum, i.e. 'mode', need to be renamed to 'state'..

But, if you are okay with it then I would be happy to do that..

> states and not state transition names, see my later
> questions:
>
>> >> +       CLOCK_EVT_DEV_MODE_SHUTDOWN,
>> >> +       CLOCK_EVT_DEV_MODE_PERIODIC,
>> >> +       CLOCK_EVT_DEV_MODE_ONESHOT,
>> >> +       CLOCK_EVT_DEV_MODE_RESUME,
>> >
>> > What is 'resume' mode?
>>
>> Introduced with: 18de5bc4c1f1 ("clockevents: fix resume
>> logic") and is only called during system resume to resume
>> the clockevent devices before resuming the tick. Only few
>> implementations do meaningful stuff here.
>
> So is it a state that a clockevents device reaches, or a
> state transition? The two purposes seem to be mixed up in
> the nomenclature.

Where all other modes are states, 'resume' probably represents
transition. It is immediately followed by a transition to ONESHOT or
PERIODIC mode and so it is just a preliminary step before moving to
final states during resume. We *may* not need to keep this in the
states enum..

>> Its only important for NOHZ_FULL (IDLE ? Maybe). When we
>> decide that the tick (LOWRES) or hrtimer interrupt
>> (HIGHRES) isn't required for indefinite period of time
>> (i.e. no timers/hrtimers are present to serve), we skip
>> reprogramming the clockevent device. But its already
>> reprogrammed from the tick-handler and so will fire
>> atleast once again.
>
> So this new 'mode' appears to be a true state of the
> device?

Yes.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 59af26b54d15..80b669cb836d 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -32,17 +32,24 @@  enum clock_event_nofitiers {
 struct clock_event_device;
 struct module;

-/* Clock event mode commands */
+/* Clock event mode commands for legacy ->set_mode(): OBSOLETE */
 enum clock_event_mode {
        CLOCK_EVT_MODE_UNUSED = 0,
        CLOCK_EVT_MODE_SHUTDOWN,
        CLOCK_EVT_MODE_PERIODIC,
        CLOCK_EVT_MODE_ONESHOT,
        CLOCK_EVT_MODE_RESUME,
-
-       /* Legacy ->set_mode() callback doesn't support below modes */
 };

+/* Clock event modes, only for core's internal use */
+enum clock_event_dev_mode {
+       CLOCK_EVT_DEV_MODE_UNUSED = 0,
+       CLOCK_EVT_DEV_MODE_SHUTDOWN,
+       CLOCK_EVT_DEV_MODE_PERIODIC,
+       CLOCK_EVT_DEV_MODE_ONESHOT,
+       CLOCK_EVT_DEV_MODE_RESUME,
+       CLOCK_EVT_DEV_MODE_ONESHOT_STOPPED,  /* This would be the new
mode which I will add later */
+};
+
 /*
  * Clock event features
  */
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 489642b08d64..16555d3db94d 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -95,14 +95,18 @@  u64 clockevent_delta2ns(unsigned long latch,
struct clock_event_device *evt)
 EXPORT_SYMBOL_GPL(clockevent_delta2ns);

 static int __clockevents_set_mode(struct clock_event_device *dev,
-                                 enum clock_event_mode mode)
+                                 enum clock_event_dev_mode mode)
 {
        /* Transition with legacy set_mode() callback */
        if (dev->set_mode) {
                /* Legacy callback doesn't support new modes */
-               if (mode > CLOCK_EVT_MODE_RESUME)
+               if (mode > CLOCK_EVT_DEV_MODE_RESUME)
                        return -ENOSYS;
-               dev->set_mode(mode, dev);
+               /*
+                * 'clock_event_dev_mode' and 'clock_event_mode' have 1-to-1
+                * mapping until *_RESUME, and so a simple cast will work.
+                */
+               dev->set_mode((enum clock_event_mode)mode, dev);
                return 0;
        }