| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <510B8059.5040609@linaro.org>
Date: Fri, 01 Feb 2013 09:44:09 +0100
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Len Brown <lenb@...nel.org>
CC: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
Len Brown <len.brown@...el.com>
Subject: Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags
On 02/01/2013 05:11 AM, Len Brown wrote:
> From: Len Brown <len.brown@...el.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@...el.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists