On 2021-08-11 14:14:05 [+0100], Valentin Schneider wrote:
> Booting a recent PREEMPT_RT kernel (v5.14-rc5-rt8 with the previous version
> of this fix reverted) on my arm4 Juno leads to the idle task blocking on a
> sleeping spinlock down some notifier path:
>
> [ 5.163034] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:35
> [ 5.163042] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 0, name: swapper/1
> [ 5.163049] 1 lock held by swapper/1/0:
> [ 5.163053] #0: ffff8000120950e8 (cpu_pm_notifier_chain.lock){+.+.}-{2:2}, at: atomic_notifier_call_chain_robust (kernel/notifier.c:186)
> [ 5.163133] Preemption disabled at:
> [ 5.163136] rt_mutex_slowunlock (kernel/locking/rtmutex.c:1242)
> [ 5.163148] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.14.0-rc5-rt8-00001-ga7cd9160688d #202
> [ 5.163158] Hardware name: ARM Juno development board (r0) (DT)
> [ 5.163162] Call trace:
> [ 5.163165] dump_backtrace (arch/arm64/kernel/stacktrace.c:161)
> [ 5.163177] show_stack (arch/arm64/kernel/stacktrace.c:217)
> [ 5.163187] dump_stack_lvl (lib/dump_stack.c:106)
> [ 5.163195] dump_stack (lib/dump_stack.c:113)
> [ 5.163202] ___might_sleep (kernel/sched/core.c:9286)
> [ 5.163210] rt_spin_lock (kernel/locking/rtmutex.c:1668 (discriminator 4) kernel/locking/spinlock_rt.c:30 (discriminator 4) kernel/locking/spinlock_rt.c:36 (discriminator 4) kernel/locking/spinlock_rt.c:44 (discriminator 4))
> [ 5.163216] atomic_notifier_call_chain_robust (kernel/notifier.c:186)
> [ 5.163225] cpu_pm_notify_robust (kernel/cpu_pm.c:39)
> [ 5.163233] cpu_pm_enter (kernel/cpu_pm.c:94)
> [ 5.163239] psci_enter_idle_state (drivers/cpuidle/cpuidle-psci.c:53 drivers/cpuidle/cpuidle-psci.c:154)
> [ 5.163250] cpuidle_enter_state (drivers/cpuidle/cpuidle.c:238)
> [ 5.163258] cpuidle_enter (drivers/cpuidle/cpuidle.c:353)
> [ 5.163266] call_cpuidle (kernel/sched/idle.c:159)
> [ 5.163272] do_idle (kernel/sched/idle.c:243 kernel/sched/idle.c:306)
> [ 5.163277] cpu_startup_entry (kernel/sched/idle.c:402 (discriminator 1))
> [ 5.163285] secondary_start_kernel (arch/arm64/kernel/smp.c:266)
> [ 5.163294] __secondary_switched (arch/arm64/kernel/head.S:661)
I would shrink that part above. The important part is that the CPU-idle
code runs with disabled interrupts. Then cpu_pm_notify_robust() invokes
the notifier which requires to acquire the spinlock_t. On PREEMPT_RT the
spinlock_t becomes a sleeping spinlock and must not be acquired with
disabled interrupts.
> Making *all* atomic_notifiers use a raw_spinlock is too big of a hammer, as
> only notifications issued by the idle task are problematic.
>
> Special-case cpu_pm_notifier_chain by kludging a raw_notifier and
> raw_spinlock together, matching the atomic_notifier behavior with a
> raw_spinlock.
>
> Fixes: 70d932985757 ("notifier: Fix broken error handling pattern")
> Signed-off-by: Valentin Schneider <valentin.schneider@xxxxxxx>
…
> kernel/cpu_pm.c | 49 +++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
> index f7e1d0eccdbc..707b8ace9fc7 100644
> --- a/kernel/cpu_pm.c
> +++ b/kernel/cpu_pm.c
…
> #include <linux/spinlock.h>
> #include <linux/syscore_ops.h>
>
> -static ATOMIC_NOTIFIER_HEAD(cpu_pm_notifier_chain);
> +/*
> + * atomic_notifiers use a regular spinlock, but notifications for this chain
> + * will be issued by the idle task which cannot block.
Maybe + a few details and make it more explicit
* atomic_notifiers use a spinlock_t, but notifications for this chain
* will be issued by the idle task with disabled interrupts which cannot
* block on PREEMPT_RT.
?
…
> @@ -33,10 +45,13 @@ static int cpu_pm_notify(enum cpu_pm_event event)
>
> static int cpu_pm_notify_robust(enum cpu_pm_event event_up, enum cpu_pm_event event_down)
> {
> + unsigned long flags;
> int ret;
>
> rcu_irq_enter_irqson();
> - ret = atomic_notifier_call_chain_robust(&cpu_pm_notifier_chain, event_up, event_down, NULL);
could we get rid of atomic_notifier_call_chain_robust() now that we have
zero users?
> + raw_spin_lock_irqsave(&cpu_pm_notifier.lock, flags);
> + ret = raw_notifier_call_chain_robust(&cpu_pm_notifier.chain, event_up, event_down, NULL);
> + raw_spin_unlock_irqrestore(&cpu_pm_notifier.lock, flags);
> rcu_irq_exit_irqson();
>
> return notifier_to_errno(ret);
Sebastian
