From 91048ce4227868000bfe624707e42274ef2198aa Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Wed, 19 Apr 2023 17:39:41 +0300 Subject: intel_idle: use pr_info() instead of printk() Substitute 'printk()' with 'pr_info()', because 'intel_idle' already uses 'pr_debug()', so using 'pr_info()' will be more consistent. In addition to this, this patch addresses the following checkpatch.pl warning: WARNING: printk() should include KERN_ facility level Signed-off-by: Artem Bityutskiy Reviewed-by: Zhang Rui Signed-off-by: Rafael J. Wysocki --- drivers/idle/intel_idle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/idle/intel_idle.c') diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index 938c17f25d94b..726a361da4222 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -1895,7 +1895,7 @@ static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv) drv->states[drv->state_count] = cpuidle_state_table[cstate]; if ((cpuidle_state_table[cstate].flags & CPUIDLE_FLAG_IRQ_ENABLE) || force_irq_on) { - printk("intel_idle: forced intel_idle_irq for state %d\n", cstate); + pr_info("forced intel_idle_irq for state %d\n", cstate); drv->states[drv->state_count].enter = intel_idle_irq; } -- cgit v1.2.3 From a78032e94bf16e9c53238cd83c82095b4a251d4b Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Thu, 20 Apr 2023 09:47:18 +0300 Subject: intel_idle: clean up intel_idle_init_cstates_icpu() The intel_idle_init_cstates_icpu() function includes a loop that iterates over every C-state. Inside the loop, the same C-state data is referenced 2 ways: 1. as cpuidle_state_table[cstate] 2. as drv->states[drv->state_count] (but it is a copy of #1, not the same object). Make the code be more consistent and easier to read by using only the 2nd way. So the code structure would be as follows: 1. Use cpuidle_state_table[cstate] 2. Copy cpuidle_state_table[cstate] to drv->states[drv->state_count] 3. Use only drv->states[drv->state_count] from this point. Note, this change introduces a checkpatch.pl warning (too long line), but it will be addressed in the next patch. Signed-off-by: Artem Bityutskiy Reviewed-by: Zhang Rui Signed-off-by: Rafael J. Wysocki --- drivers/idle/intel_idle.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'drivers/idle/intel_idle.c') diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index 726a361da4222..190410fc9ce5f 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -1894,24 +1894,24 @@ static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv) /* Structure copy. */ drv->states[drv->state_count] = cpuidle_state_table[cstate]; - if ((cpuidle_state_table[cstate].flags & CPUIDLE_FLAG_IRQ_ENABLE) || force_irq_on) { + if ((drv->states[drv->state_count].flags & CPUIDLE_FLAG_IRQ_ENABLE) || force_irq_on) { pr_info("forced intel_idle_irq for state %d\n", cstate); drv->states[drv->state_count].enter = intel_idle_irq; } if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) && - cpuidle_state_table[cstate].flags & CPUIDLE_FLAG_IBRS) { - WARN_ON_ONCE(cpuidle_state_table[cstate].flags & CPUIDLE_FLAG_IRQ_ENABLE); + drv->states[drv->state_count].flags & CPUIDLE_FLAG_IBRS) { + WARN_ON_ONCE(drv->states[drv->state_count].flags & CPUIDLE_FLAG_IRQ_ENABLE); drv->states[drv->state_count].enter = intel_idle_ibrs; } - if (cpuidle_state_table[cstate].flags & CPUIDLE_FLAG_INIT_XSTATE) + if (drv->states[drv->state_count].flags & CPUIDLE_FLAG_INIT_XSTATE) drv->states[drv->state_count].enter = intel_idle_xstate; if ((disabled_states_mask & BIT(drv->state_count)) || ((icpu->use_acpi || force_use_acpi) && intel_idle_off_by_default(mwait_hint) && - !(cpuidle_state_table[cstate].flags & CPUIDLE_FLAG_ALWAYS_ENABLE))) + !(drv->states[drv->state_count].flags & CPUIDLE_FLAG_ALWAYS_ENABLE))) drv->states[drv->state_count].flags |= CPUIDLE_FLAG_OFF; if (intel_idle_state_needs_timer_stop(&drv->states[drv->state_count])) -- cgit v1.2.3 From 1abffbd827668b3b44c34b168ccb5ee2866714a8 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Wed, 19 Apr 2023 17:39:43 +0300 Subject: intel_idle: further intel_idle_init_cstates_icpu() cleanup Introduce a temporary 'state' variable for referencing the currently processed C-state in the intel_idle_init_cstates_icpu() function. This makes code lines shorter and easier to read. Signed-off-by: Artem Bityutskiy Reviewed-by: Zhang Rui Signed-off-by: Rafael J. Wysocki --- drivers/idle/intel_idle.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) (limited to 'drivers/idle/intel_idle.c') diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index 190410fc9ce5f..73ddb1d8cfcf8 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -1871,6 +1871,7 @@ static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv) } for (cstate = 0; cstate < CPUIDLE_STATE_MAX; ++cstate) { + struct cpuidle_state *state; unsigned int mwait_hint; if (intel_idle_max_cstate_reached(cstate)) @@ -1893,29 +1894,30 @@ static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv) /* Structure copy. */ drv->states[drv->state_count] = cpuidle_state_table[cstate]; + state = &drv->states[drv->state_count]; - if ((drv->states[drv->state_count].flags & CPUIDLE_FLAG_IRQ_ENABLE) || force_irq_on) { + if ((state->flags & CPUIDLE_FLAG_IRQ_ENABLE) || force_irq_on) { pr_info("forced intel_idle_irq for state %d\n", cstate); - drv->states[drv->state_count].enter = intel_idle_irq; + state->enter = intel_idle_irq; } if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) && - drv->states[drv->state_count].flags & CPUIDLE_FLAG_IBRS) { - WARN_ON_ONCE(drv->states[drv->state_count].flags & CPUIDLE_FLAG_IRQ_ENABLE); - drv->states[drv->state_count].enter = intel_idle_ibrs; + state->flags & CPUIDLE_FLAG_IBRS) { + WARN_ON_ONCE(state->flags & CPUIDLE_FLAG_IRQ_ENABLE); + state->enter = intel_idle_ibrs; } - if (drv->states[drv->state_count].flags & CPUIDLE_FLAG_INIT_XSTATE) - drv->states[drv->state_count].enter = intel_idle_xstate; + if (state->flags & CPUIDLE_FLAG_INIT_XSTATE) + state->enter = intel_idle_xstate; if ((disabled_states_mask & BIT(drv->state_count)) || ((icpu->use_acpi || force_use_acpi) && intel_idle_off_by_default(mwait_hint) && - !(drv->states[drv->state_count].flags & CPUIDLE_FLAG_ALWAYS_ENABLE))) - drv->states[drv->state_count].flags |= CPUIDLE_FLAG_OFF; + !(state->flags & CPUIDLE_FLAG_ALWAYS_ENABLE))) + state->flags |= CPUIDLE_FLAG_OFF; - if (intel_idle_state_needs_timer_stop(&drv->states[drv->state_count])) - drv->states[drv->state_count].flags |= CPUIDLE_FLAG_TIMER_STOP; + if (intel_idle_state_needs_timer_stop(state)) + state->flags |= CPUIDLE_FLAG_TIMER_STOP; drv->state_count++; } -- cgit v1.2.3 From 00433eae1771d355f20c152ddc5976baedb427e2 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Wed, 19 Apr 2023 17:39:44 +0300 Subject: intel_idle: improve C-state flags handling robustness The following C-state flags are currently mutually-exclusive and should not be combined: * IRQ_ENABLE * IBRS * XSTATE There is a warning for the situation when the IRQ_ENABLE flag is combined with the IBRS flag, but no warnings for other combinations. This is inconsistent and prone to errors. Improve the situation by adding warnings for all the unexpected combinations. Add a couple of helpful commentaries too. Signed-off-by: Artem Bityutskiy Reviewed-by: Zhang Rui Signed-off-by: Rafael J. Wysocki --- drivers/idle/intel_idle.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) (limited to 'drivers/idle/intel_idle.c') diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index 73ddb1d8cfcf8..1de36df15d5a9 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -1896,20 +1896,28 @@ static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv) drv->states[drv->state_count] = cpuidle_state_table[cstate]; state = &drv->states[drv->state_count]; - if ((state->flags & CPUIDLE_FLAG_IRQ_ENABLE) || force_irq_on) { - pr_info("forced intel_idle_irq for state %d\n", cstate); - state->enter = intel_idle_irq; - } - - if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) && - state->flags & CPUIDLE_FLAG_IBRS) { + if (state->flags & CPUIDLE_FLAG_INIT_XSTATE) { + /* + * Combining with XSTATE with IBRS or IRQ_ENABLE flags + * is not currently supported but this driver. + */ + WARN_ON_ONCE(state->flags & CPUIDLE_FLAG_IBRS); + WARN_ON_ONCE(state->flags & CPUIDLE_FLAG_IRQ_ENABLE); + state->enter = intel_idle_xstate; + } else if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) && + state->flags & CPUIDLE_FLAG_IBRS) { + /* + * IBRS mitigation requires that C-states are entered + * with interrupts disabled. + */ WARN_ON_ONCE(state->flags & CPUIDLE_FLAG_IRQ_ENABLE); state->enter = intel_idle_ibrs; + } else if ((state->flags & CPUIDLE_FLAG_IRQ_ENABLE) || + force_irq_on) { + pr_info("forced intel_idle_irq for state %d\n", cstate); + state->enter = intel_idle_irq; } - if (state->flags & CPUIDLE_FLAG_INIT_XSTATE) - state->enter = intel_idle_xstate; - if ((disabled_states_mask & BIT(drv->state_count)) || ((icpu->use_acpi || force_use_acpi) && intel_idle_off_by_default(mwait_hint) && -- cgit v1.2.3 From db1ae0c99950502d10e09d0c57ed1e16cd854b20 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Wed, 19 Apr 2023 17:39:45 +0300 Subject: intel_idle: fix confusing message By default, all non-POLL C-states are entered with interrupts disabled. There are 2 ways to make 'intel_idle' enter C-states with interrupts enabled: 1. Mark the C-state with the CPUIDLE_FLAG_IRQ_ENABLE flag. 2. Use the force_irq_on module parameter. The former is the "proper" way of doing it, it is per-C-state and per-platform. The latter is for debugging purposes only. The problem is that intel_idle prints the "forced intel_idle_irq" message in both cases, even though the former case does not needed this message, because nothing is forced there. This patch addresses the problem. Signed-off-by: Artem Bityutskiy Reviewed-by: Zhang Rui Signed-off-by: Rafael J. Wysocki --- drivers/idle/intel_idle.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'drivers/idle/intel_idle.c') diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index 1de36df15d5a9..bff0d17aeda43 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -1912,8 +1912,9 @@ static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv) */ WARN_ON_ONCE(state->flags & CPUIDLE_FLAG_IRQ_ENABLE); state->enter = intel_idle_ibrs; - } else if ((state->flags & CPUIDLE_FLAG_IRQ_ENABLE) || - force_irq_on) { + } else if (state->flags & CPUIDLE_FLAG_IRQ_ENABLE) { + state->enter = intel_idle_irq; + } else if (force_irq_on) { pr_info("forced intel_idle_irq for state %d\n", cstate); state->enter = intel_idle_irq; } -- cgit v1.2.3 From 4152379a701ad66b6c4e1c9fc93704d993e49615 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Thu, 20 Apr 2023 09:47:21 +0300 Subject: intel_idle: do not sprinkle module parameter definitions around This is a cleanup which improves code consistency. Move the force_irq_on module parameter variable and definition to the same place where we have variables and definitions for other module parameters. Signed-off-by: Artem Bityutskiy Reviewed-by: Zhang Rui Signed-off-by: Rafael J. Wysocki --- drivers/idle/intel_idle.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'drivers/idle/intel_idle.c') diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index bff0d17aeda43..35bd284f7763c 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -68,6 +68,7 @@ static struct cpuidle_driver intel_idle_driver = { static int max_cstate = CPUIDLE_STATE_MAX - 1; static unsigned int disabled_states_mask; static unsigned int preferred_states_mask; +static bool force_irq_on __read_mostly; static struct cpuidle_device __percpu *intel_idle_cpuidle_devices; @@ -1838,9 +1839,6 @@ static bool __init intel_idle_verify_cstate(unsigned int mwait_hint) return true; } -static bool force_irq_on __read_mostly; -module_param(force_irq_on, bool, 0444); - static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv) { int cstate; @@ -2157,3 +2155,9 @@ MODULE_PARM_DESC(states_off, "Mask of disabled idle states"); */ module_param_named(preferred_cstates, preferred_states_mask, uint, 0444); MODULE_PARM_DESC(preferred_cstates, "Mask of preferred idle states"); +/* + * Debugging option that forces the driver to enter all C-states with + * interrupts enabled. Does not apply to C-states with + * 'CPUIDLE_FLAG_INIT_XSTATE' and 'CPUIDLE_FLAG_IBRS' flags. + */ +module_param(force_irq_on, bool, 0444); -- cgit v1.2.3 From bd4468295e7a335319c10affdf594b56f1e6c0a4 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Wed, 19 Apr 2023 17:39:47 +0300 Subject: intel_idle: mark few variables as __read_mostly The intention is to clean up the code and make it look a bit more consistent. Mark all unitialized module parameter variables as __read_mostly, not just one of them. The other parameters are read-mostly too. Signed-off-by: Artem Bityutskiy Signed-off-by: Rafael J. Wysocki --- drivers/idle/intel_idle.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/idle/intel_idle.c') diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index 35bd284f7763c..aa2d19db2b1d9 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -66,8 +66,8 @@ static struct cpuidle_driver intel_idle_driver = { }; /* intel_idle.max_cstate=0 disables driver */ static int max_cstate = CPUIDLE_STATE_MAX - 1; -static unsigned int disabled_states_mask; -static unsigned int preferred_states_mask; +static unsigned int disabled_states_mask __read_mostly; +static unsigned int preferred_states_mask __read_mostly; static bool force_irq_on __read_mostly; static struct cpuidle_device __percpu *intel_idle_cpuidle_devices; -- cgit v1.2.3