From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
Len Brown <len.brown@intel.com>
Subject: Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags
Date: Fri, 01 Feb 2013 09:44:09 +0100 [thread overview]
Message-ID: <510B8059.5040609@linaro.org> (raw)
In-Reply-To: <9a38405338d7464c852c4524465f84f8a2ac22fb.1359691799.git.len.brown@intel.com>
On 02/01/2013 05:11 AM, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
>
> The commit, 4202735e8ab6ecfb0381631a0d0b58fefe0bd4e2
> (cpuidle: Split cpuidle_state structure and move per-cpu statistics fields)
> observed that the MWAIT flags for Cn on every processor to date were the
> same, and created get_driver_data() to supply them.
>
> Unfortunately, that assumption is false, going forward.
> So here we restore the MWAIT flags to the cpuidle_state table.
> However, instead restoring the old "driver_data" field,
> we put the flags into the existing "flags" field,
> where they probalby should have lived all along.
Removing the driver_data is a good thing but I am not sure it is the
case by moving the MWAIT flags to the cpuidle_state's flags field. We
are mixing arch specific flags with a generic code.
This is prone to errors because new flags could appear for the cpuidle
core code and could collide with the arch specific flags.
Wouldn't make sense to add a private field in the struct cpuidle_state
structure to let the driver/arch specific to store whatever is needed ?
struct cpuidle_state {
...
unsigned long private;
...
}
> This patch does not change any operation.
>
> This patch removes 1 of the 3 users of cpuidle_state_usage.driver_data.
> Perhaps some day we'll get rid of the other 2.
+1 :)
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
> drivers/idle/intel_idle.c | 75 ++++++++++++++++-------------------------------
> 1 file changed, 26 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 2df9414..8331753 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -109,6 +109,16 @@ static struct cpuidle_state *cpuidle_state_table;
> #define CPUIDLE_FLAG_TLB_FLUSHED 0x10000
>
> /*
> + * MWAIT takes an 8-bit "hint" in EAX "suggesting"
> + * the C-state (top nibble) and sub-state (bottom nibble)
> + * 0x00 means "MWAIT(C1)", 0x10 means "MWAIT(C2)" etc.
> + *
> + * We store the hint at the top of our "flags" for each state.
> + */
> +#define flags_2_MWAIT_EAX(flags) (((flags) >> 24) & 0xFF)
> +#define MWAIT_EAX_2_flags(eax) ((eax & 0xFF) << 24)
> +
> +/*
> * States are indexed by the cstate number,
> * which is also the index into the MWAIT hint array.
> * Thus C0 is a dummy.
> @@ -118,21 +128,21 @@ static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = {
> { /* MWAIT C1 */
> .name = "C1-NHM",
> .desc = "MWAIT 0x00",
> - .flags = CPUIDLE_FLAG_TIME_VALID,
> + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID,
> .exit_latency = 3,
> .target_residency = 6,
> .enter = &intel_idle },
> { /* MWAIT C2 */
> .name = "C3-NHM",
> .desc = "MWAIT 0x10",
> - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> .exit_latency = 20,
> .target_residency = 80,
> .enter = &intel_idle },
> { /* MWAIT C3 */
> .name = "C6-NHM",
> .desc = "MWAIT 0x20",
> - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> + .flags = MWAIT_EAX_2_flags(0x20) |CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> .exit_latency = 200,
> .target_residency = 800,
> .enter = &intel_idle },
> @@ -143,28 +153,28 @@ static struct cpuidle_state snb_cstates[MWAIT_MAX_NUM_CSTATES] = {
> { /* MWAIT C1 */
> .name = "C1-SNB",
> .desc = "MWAIT 0x00",
> - .flags = CPUIDLE_FLAG_TIME_VALID,
> + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID,
> .exit_latency = 1,
> .target_residency = 1,
> .enter = &intel_idle },
> { /* MWAIT C2 */
> .name = "C3-SNB",
> .desc = "MWAIT 0x10",
> - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> .exit_latency = 80,
> .target_residency = 211,
> .enter = &intel_idle },
> { /* MWAIT C3 */
> .name = "C6-SNB",
> .desc = "MWAIT 0x20",
> - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> + .flags = MWAIT_EAX_2_flags(0x20) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> .exit_latency = 104,
> .target_residency = 345,
> .enter = &intel_idle },
> { /* MWAIT C4 */
> .name = "C7-SNB",
> .desc = "MWAIT 0x30",
> - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> + .flags = MWAIT_EAX_2_flags(0x30) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> .exit_latency = 109,
> .target_residency = 345,
> .enter = &intel_idle },
> @@ -175,28 +185,28 @@ static struct cpuidle_state ivb_cstates[MWAIT_MAX_NUM_CSTATES] = {
> { /* MWAIT C1 */
> .name = "C1-IVB",
> .desc = "MWAIT 0x00",
> - .flags = CPUIDLE_FLAG_TIME_VALID,
> + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID,
> .exit_latency = 1,
> .target_residency = 1,
> .enter = &intel_idle },
> { /* MWAIT C2 */
> .name = "C3-IVB",
> .desc = "MWAIT 0x10",
> - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> .exit_latency = 59,
> .target_residency = 156,
> .enter = &intel_idle },
> { /* MWAIT C3 */
> .name = "C6-IVB",
> .desc = "MWAIT 0x20",
> - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> + .flags = MWAIT_EAX_2_flags(0x20) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> .exit_latency = 80,
> .target_residency = 300,
> .enter = &intel_idle },
> { /* MWAIT C4 */
> .name = "C7-IVB",
> .desc = "MWAIT 0x30",
> - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> + .flags = MWAIT_EAX_2_flags(0x30) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> .exit_latency = 87,
> .target_residency = 300,
> .enter = &intel_idle },
> @@ -207,14 +217,14 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
> { /* MWAIT C1 */
> .name = "C1-ATM",
> .desc = "MWAIT 0x00",
> - .flags = CPUIDLE_FLAG_TIME_VALID,
> + .flags = MWAIT_EAX_2_flags(0x00) | CPUIDLE_FLAG_TIME_VALID,
> .exit_latency = 1,
> .target_residency = 4,
> .enter = &intel_idle },
> { /* MWAIT C2 */
> .name = "C2-ATM",
> .desc = "MWAIT 0x10",
> - .flags = CPUIDLE_FLAG_TIME_VALID,
> + .flags = MWAIT_EAX_2_flags(0x10) | CPUIDLE_FLAG_TIME_VALID,
> .exit_latency = 20,
> .target_residency = 80,
> .enter = &intel_idle },
> @@ -222,7 +232,7 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
> { /* MWAIT C4 */
> .name = "C4-ATM",
> .desc = "MWAIT 0x30",
> - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> + .flags = MWAIT_EAX_2_flags(0x30) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> .exit_latency = 100,
> .target_residency = 400,
> .enter = &intel_idle },
> @@ -230,41 +240,12 @@ static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
> { /* MWAIT C6 */
> .name = "C6-ATM",
> .desc = "MWAIT 0x52",
> - .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> + .flags = MWAIT_EAX_2_flags(0x52) | CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TLB_FLUSHED,
> .exit_latency = 140,
> .target_residency = 560,
> .enter = &intel_idle },
> };
>
> -static long get_driver_data(int cstate)
> -{
> - int driver_data;
> - switch (cstate) {
> -
> - case 1: /* MWAIT C1 */
> - driver_data = 0x00;
> - break;
> - case 2: /* MWAIT C2 */
> - driver_data = 0x10;
> - break;
> - case 3: /* MWAIT C3 */
> - driver_data = 0x20;
> - break;
> - case 4: /* MWAIT C4 */
> - driver_data = 0x30;
> - break;
> - case 5: /* MWAIT C5 */
> - driver_data = 0x40;
> - break;
> - case 6: /* MWAIT C6 */
> - driver_data = 0x52;
> - break;
> - default:
> - driver_data = 0x00;
> - }
> - return driver_data;
> -}
> -
> /**
> * intel_idle
> * @dev: cpuidle_device
> @@ -278,8 +259,7 @@ static int intel_idle(struct cpuidle_device *dev,
> {
> unsigned long ecx = 1; /* break on interrupt flag */
> struct cpuidle_state *state = &drv->states[index];
> - struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
> - unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
> + unsigned long eax = flags_2_MWAIT_EAX(state->flags);
> unsigned int cstate;
> int cpu = smp_processor_id();
>
> @@ -558,9 +538,6 @@ static int intel_idle_cpu_init(int cpu)
> if (cpuidle_state_table[cstate].enter == NULL)
> continue;
>
> - dev->states_usage[dev->state_count].driver_data =
> - (void *)get_driver_data(cstate);
> -
> dev->state_count += 1;
> }
>
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
next prev parent reply other threads:[~2013-02-01 8:44 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-01 4:11 intel_idle and turbostat patches supporting Haswell Len Brown
2013-02-01 4:11 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Len Brown
2013-02-01 4:11 ` [PATCH 2/4] tools/power turbostat: support HSW Len Brown
2013-02-01 4:11 ` [PATCH 3/4] tools/power turbostat: decode MSR_IA32_POWER_CTL Len Brown
2013-02-01 4:11 ` [PATCH 4/4] intel_idle: initial HSW support Len Brown
2013-02-01 8:44 ` Daniel Lezcano [this message]
2013-02-01 18:40 ` [PATCH 1/4] intel_idle: stop using driver_data for static flags Len Brown
2013-02-01 22:16 ` Daniel Lezcano
2013-02-02 2:16 ` Len Brown
[not found] ` <510C7708.4080707-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
2013-02-02 9:44 ` Daniel Lezcano
2013-02-02 9:44 ` Daniel Lezcano
2013-02-02 20:18 ` Len Brown
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=510B8059.5040609@linaro.org \
--to=daniel.lezcano@linaro.org \
--cc=len.brown@intel.com \
--cc=lenb@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.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.