From: mingo@kernel.org (Ingo Molnar)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] clockevents: Add (missing) default case for switch blocks
Date: Fri, 20 Feb 2015 15:04:32 +0100 [thread overview]
Message-ID: <20150220140432.GA31928@gmail.com> (raw)
In-Reply-To: <CAKohpokEV7EatQROgzQBWPRPFEn8OsMo5EHa+q1FMwBESwiU3w@mail.gmail.com>
* 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
WARNING: multiple messages have this Message-ID (diff)
From: Ingo Molnar <mingo@kernel.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>,
Linaro Kernel Mailman List <linaro-kernel@lists.linaro.org>,
Kevin Hilman <khilman@linaro.org>,
Preeti U Murthy <preeti@linux.vnet.ibm.com>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
Frederic Weisbecker <fweisbec@gmail.com>,
Linaro Networking <linaro-networking@linaro.org>,
Steven Miao <realmz6@gmail.com>, Mark Salter <msalter@redhat.com>,
Michal Simek <monstr@monstr.eu>,
Ralf Baechle <ralf@linux-mips.org>,
Ley Foon Tan <lftan@altera.com>, Jonas Bonn <jonas@southpole.se>,
"David S. Miller" <davem@davemloft.net>,
Jeff Dike <jdike@addtoit.com>, Guan Xuetao <gxt@mprc.pku.edu.cn>
Subject: Re: [PATCH] clockevents: Add (missing) default case for switch blocks
Date: Fri, 20 Feb 2015 15:04:32 +0100 [thread overview]
Message-ID: <20150220140432.GA31928@gmail.com> (raw)
In-Reply-To: <CAKohpokEV7EatQROgzQBWPRPFEn8OsMo5EHa+q1FMwBESwiU3w@mail.gmail.com>
* 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
next prev parent reply other threads:[~2015-02-20 14:04 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-20 6:32 [PATCH] clockevents: Add (missing) default case for switch blocks Viresh Kumar
2015-02-20 6:32 ` Viresh Kumar
2015-02-20 8:38 ` Ingo Molnar
2015-02-20 8:38 ` Ingo Molnar
2015-02-20 8:48 ` Peter Zijlstra
2015-02-20 8:48 ` Peter Zijlstra
2015-02-20 9:36 ` Ingo Molnar
2015-02-20 9:36 ` Ingo Molnar
2015-02-20 10:12 ` Viresh Kumar
2015-02-20 10:12 ` Viresh Kumar
2015-02-20 10:52 ` Ingo Molnar
2015-02-20 10:52 ` Ingo Molnar
2015-02-20 11:37 ` Peter Zijlstra
2015-02-20 11:37 ` Peter Zijlstra
2015-02-20 11:41 ` Ingo Molnar
2015-02-20 11:41 ` Ingo Molnar
2015-02-20 11:56 ` Viresh Kumar
2015-02-20 11:56 ` Viresh Kumar
2015-02-20 13:22 ` Ingo Molnar
2015-02-20 13:22 ` Ingo Molnar
2015-02-20 13:58 ` Viresh Kumar
2015-02-20 13:58 ` Viresh Kumar
2015-02-20 14:04 ` Ingo Molnar [this message]
2015-02-20 14:04 ` Ingo Molnar
2015-02-23 5:33 ` Viresh Kumar
2015-02-23 5:33 ` Viresh Kumar
2015-02-23 16:37 ` Ingo Molnar
2015-02-23 16:37 ` Ingo Molnar
2015-02-24 11:11 ` viresh kumar
2015-02-24 11:11 ` viresh kumar
2015-02-24 14:54 ` Ingo Molnar
2015-02-24 14:54 ` Ingo Molnar
2015-02-24 15:12 ` Viresh Kumar
2015-02-24 15:12 ` Viresh Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150220140432.GA31928@gmail.com \
--to=mingo@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.