On Thu, Feb 13, 2020 at 11:00:26PM +0100, Rafael J. Wysocki wrote: > From: "Rafael J. Wysocki" <rafael.j.wyso...@intel.com> > > Move the definitions of intel_idle() and intel_idle_s2idle() before > the definitions of cpuidle_state structures referring to them to > avoid having to use additional declarations of them (and drop those > declarations). > > No functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com> > --- > drivers/idle/intel_idle.c | 154 > ++++++++++++++++++++++------------------------ > 1 file changed, 75 insertions(+), 79 deletions(-) > > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > index 5adc058c705d..e0332d567735 100644 > --- a/drivers/idle/intel_idle.c > +++ b/drivers/idle/intel_idle.c [] > +/** > + * intel_idle - Ask the processor to enter the given idle state. > + * @dev: cpuidle device of the target CPU. > + * @drv: cpuidle driver (assumed to point to intel_idle_driver). > + * @index: Target idle state index. > + * > + * Use the MWAIT instruction to notify the processor that the CPU > represented by > + * @dev is idle and it can try to enter the idle state corresponding to > @index. > + * > + * If the local APIC timer is not known to be reliable in the target idle > state, > + * enable one-shot tick broadcasting for the target CPU before executing > MWAIT. > + * > + * Optionally call leave_mm() for the target CPU upfront to avoid wakeups > due to > + * flushing user TLBs. > + * > + * Must be called under local_irq_disable(). > + */ > +static __cpuidle int intel_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + struct cpuidle_state *state = &drv->states[index]; > + unsigned long eax = flg2MWAIT(state->flags); > + unsigned long ecx = 1; /* break on interrupt flag */ > + bool uninitialized_var(tick);
This will generate an UBSAN warning because Clang could poison all uninitialized stack variables to 0xAA due to CONFIG_INIT_STACK_ALL=y, so one issue is that, bool uninitialized_var(x); would always broken on Clang like this, [ 92.140611] UBSAN: invalid-load in drivers/idle/intel_idle.c:135:7 [ 92.143111] load of value 170 is not a valid value for type 'bool' (aka '_Bool') [ 92.145657] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-rc6-next-20200522+ #3 [ 92.147424] Hardware name: HP ProLiant BL660c Gen9, BIOS I38 10/17/2018 [ 92.149869] Call Trace: [ 92.149869] dump_stack+0x10b/0x17f [ 92.149869] __ubsan_handle_load_invalid_value+0xd2/0x110 [ 92.149869] intel_idle+0x54/0xf0 [ 92.156202] cpuidle_enter_state+0x120/0x4f0 [ 92.156202] cpuidle_enter+0x5b/0xa0 [ 92.156202] call_cpuidle+0x25/0x50 [ 92.156202] do_idle+0x1eb/0x2c0 [ 92.156202] cpu_startup_entry+0x25/0x30 [ 92.156202] rest_init+0x26f/0x280 [ 92.156202] arch_call_rest_init+0x17/0x1e [ 92.156202] start_kernel+0x598/0x633 [ 92.156202] x86_64_start_reservations+0x24/0x26 [ 92.156202] x86_64_start_kernel+0x116/0x1c1 [ 92.156202] secondary_startup_64+0xb6/0xc0 However, I am wondering if it is correct to let "tick" uninitialized to begin with. If this condition is true, !static_cpu_has(X86_FEATURE_ARAT) && lapic_timer_always_reliable Then, we could in the final branch to use the uninitialized value. if (!static_cpu_has(X86_FEATURE_ARAT) && tick) Isn't that possible? > + int cpu = smp_processor_id(); > + > + /* > + * leave_mm() to avoid costly and often unnecessary wakeups > + * for flushing the user TLB's associated with the active mm. > + */ > + if (state->flags & CPUIDLE_FLAG_TLB_FLUSHED) > + leave_mm(cpu); > + > + if (!static_cpu_has(X86_FEATURE_ARAT) && !lapic_timer_always_reliable) { > + /* > + * Switch over to one-shot tick broadcast if the target C-state > + * is deeper than C1. > + */ > + if ((eax >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) { > + tick = true; > + tick_broadcast_enter(); > + } else { > + tick = false; > + } > + } > + > + mwait_idle_with_hints(eax, ecx); > + > + if (!static_cpu_has(X86_FEATURE_ARAT) && tick) > + tick_broadcast_exit(); > + > + return index; > +}