[PATCH] powerpc/pseries/cpuidle: Fix preempt warning

[PATCH] powerpc/pseries/cpuidle: Fix preempt warning

[Breno Leitao]

When booting a pseries kernel with PREEMPT enabled, it dumps the following
warning:

   BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1
   caller is pseries_processor_idle_init+0x5c/0x22c
   CPU: 13 PID: 1 Comm: swapper/0 Not tainted 4.20.0-rc3-00090-g12201a0128bc-dirty #828
   Call Trace:
   [c000000429437ab0] [c0000000009c8878] dump_stack+0xec/0x164 (unreliable)
   [c000000429437b00] [c0000000005f2f24] check_preemption_disabled+0x154/0x160
   [c000000429437b90] [c000000000cab8e8] pseries_processor_idle_init+0x5c/0x22c
   [c000000429437c10] [c000000000010ed4] do_one_initcall+0x64/0x300
   [c000000429437ce0] [c000000000c54500] kernel_init_freeable+0x3f0/0x500
   [c000000429437db0] [c0000000000112dc] kernel_init+0x2c/0x160
   [c000000429437e20] [c00000000000c1d0] ret_from_kernel_thread+0x5c/0x6c

This happens because the code calls get_lppaca() which calls get_paca() and
it checks if preemption is disabled through check_preemption_disabled().

Preemption should be disabled because the per CPU variable may make no
sense if there is a preemption (and a CPU switch) after it reads the per
CPU data and when it is used.

At this device driver specifically, it is not a problem, because this code
just need to have access to one lppaca struct, and it does not matter if it
is the current per CPU lppaca struct or not (i.e. when there is a
preemption and a CPU migration).

That said, the most appropriated fix seems to be related to avoiding the
debug_smp_processor_id() call at get_paca(), instead of calling
preempt_disable() before get_paca().

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/cpuidle/cpuidle-pseries.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 9e56bc411061..63f70e2c1f13 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -247,7 +247,12 @@ static int pseries_idle_probe(void)
 		return -ENODEV;
 
 	if (firmware_has_feature(FW_FEATURE_SPLPAR)) {
-		if (lppaca_shared_proc(get_lppaca())) {
+		/* Using local_paca instead of get_lppaca() since
+		 * preemption is not disabled, and it is not required in
+		 * fact, since lppaca_ptr does not need to be the value
+		 * associated to the current CPU, it could be from any CPU.
+		 */
+		if (lppaca_shared_proc(local_paca->lppaca_ptr)) {
 			cpuidle_state_table = shared_states;
 			max_idle_state = ARRAY_SIZE(shared_states);
 		} else {
-- 
2.19.0

Re: powerpc/pseries/cpuidle: Fix preempt warning

[Michael Ellerman]

On Fri, 2018-11-23 at 16:30:11 UTC, Breno Leitao wrote:
> When booting a pseries kernel with PREEMPT enabled, it dumps the following
> warning:
> 
>    BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1
>    caller is pseries_processor_idle_init+0x5c/0x22c
>    CPU: 13 PID: 1 Comm: swapper/0 Not tainted 4.20.0-rc3-00090-g12201a0128bc-dirty #828
>    Call Trace:
>    [c000000429437ab0] [c0000000009c8878] dump_stack+0xec/0x164 (unreliable)
>    [c000000429437b00] [c0000000005f2f24] check_preemption_disabled+0x154/0x160
>    [c000000429437b90] [c000000000cab8e8] pseries_processor_idle_init+0x5c/0x22c
>    [c000000429437c10] [c000000000010ed4] do_one_initcall+0x64/0x300
>    [c000000429437ce0] [c000000000c54500] kernel_init_freeable+0x3f0/0x500
>    [c000000429437db0] [c0000000000112dc] kernel_init+0x2c/0x160
>    [c000000429437e20] [c00000000000c1d0] ret_from_kernel_thread+0x5c/0x6c
> 
> This happens because the code calls get_lppaca() which calls get_paca() and
> it checks if preemption is disabled through check_preemption_disabled().
> 
> Preemption should be disabled because the per CPU variable may make no
> sense if there is a preemption (and a CPU switch) after it reads the per
> CPU data and when it is used.
> 
> At this device driver specifically, it is not a problem, because this code
> just need to have access to one lppaca struct, and it does not matter if it
> is the current per CPU lppaca struct or not (i.e. when there is a
> preemption and a CPU migration).
> 
> That said, the most appropriated fix seems to be related to avoiding the
> debug_smp_processor_id() call at get_paca(), instead of calling
> preempt_disable() before get_paca().
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/2b038cbc5fcf12a7ee1cc9bfd5da1e

cheers