Re: [PATCH v2] cpu_pm: Make notifier chain use a raw spinlock

[Valentin Schneider]

On 11/08/21 15:52, Sebastian Andrzej Siewior wrote:
> 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.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.

Noted, I'll pluck the warning out.

>> +/*
>> + * 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.
>
> ?
>

More generally I'd say the idle task is never preemptible (as in
preempt_count > 0 at all times), so any notification issued by the idle
task itself cannot block. The fact those are also issued in an IRQ-off
region just further cements that.

> …
>> @@ -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?
>

No objections from my end, I'll add that in v3 and see if anyone complains.

>> +	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