| Message ID | CAKohpokmCof2P8Un+i5eOS3BQhH_pMNATH4xkbbhw5TqJFEy9Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Viresh Kumar <viresh.kumar@linaro.org> wrote: > 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 ? > > 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, What is 'unused' - not initialized yet? > + CLOCK_EVT_DEV_MODE_SHUTDOWN, > + CLOCK_EVT_DEV_MODE_PERIODIC, > + CLOCK_EVT_DEV_MODE_ONESHOT, > + CLOCK_EVT_DEV_MODE_RESUME, What is 'resume' mode? > + CLOCK_EVT_DEV_MODE_ONESHOT_STOPPED, /* This would be the new > mode which I will add later */ What does this mode express? So for state machines it's important to document the states and the transitions between them very clearly - please start with that. > +}; Also, this should not be in a generic header, it should be somewhere internal in kernel/time/. > 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, Ingo
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.
* Viresh Kumar <viresh.kumar@linaro.org> wrote: > 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. 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. 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 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. > >> + 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.. Yeah. > 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? Thanks, Ingo
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.
* Viresh Kumar <viresh.kumar@linaro.org> wrote: > 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. Ok, could we rename it to something like DETACHED? 'UNUSED' really gives me the wrong impression - it's what we do for unused fields, unused ABI enumertion constants, etc. > > 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.. Well, how does the diffstat look like? But if the conversion was scripted (i.e. is relatively secure from typos) then that's still fine IMO. Peter? Thanks, Ingo
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; }