Re: [PATCH v2 01/47] kernel: Add support for poweroff handler call chain

[Guenter Roeck <linux@roeck-us.net>]

On 10/22/2014 01:02 AM, Philippe Rétornaz wrote:
> Le 21/10/2014 15:29, Guenter Roeck a écrit :
>> On 10/20/2014 11:46 PM, Philippe Rétornaz wrote:
>>> Hello
>>>
>>> [...]
>>>> - Use raw notifiers protected by spinlocks instead of atomic notifiers
>>> [...]
>>>
>>>> +/**
>>>> + *    do_kernel_power_off - Execute kernel poweroff handler call chain
>>>> + *
>>>> + *    Calls functions registered with register_power_off_handler.
>>>> + *
>>>> + *    Expected to be called from machine_power_off as last step of
>>>> + *    the poweroff sequence.
>>>> + *
>>>> + *    Powers off the system immediately if a poweroff handler function
>>>> + *    has been registered. Otherwise does nothing.
>>>> + */
>>>> +void do_kernel_power_off(void)
>>>> +{
>>>> +    spin_lock(&power_off_handler_lock);
>>>> +    raw_notifier_call_chain(&power_off_handler_list, 0, NULL);
>>>> +    spin_unlock(&power_off_handler_lock);
>>>> +}
>>>
>>> I don't get it. You are still in atomic context inside the poweroff
>>> callback
>>> since you lock it with a spinlock.
> [...]
>>>
>>> Why not using the blocking_notifier_* family ?
>>> It will lock with a read-write semaphore under which you can sleep.
>>>
>>> For instance, twl4030_power_off will sleep, since it is doing I2C access.
>>> So you cannot call it in atomic context.
>>>
>>
>> Learning something new all the time. I assumed that spin_lock (unlike
>> spin_lock_irqsafe) would not run in atomic context.
>>
>> I did not want to use a sleeping lock because I am not sure if that
>> works for all architectures; some disable (local) interrupts before
>> calling the function (eg arm and arm64), and I don't want to change
>> that semantics.
>
> You're right and it even disable all others CPUs (if any).
> I don't understand why it needs to disable local interrupts since the
> code path to pm_power_off is simply doing:
>
> syscall -> migrate to reboot cpu -> disable local interrupt -> disable others cpu -> pm_power_off().
>
> I don't understand why we cannot re-enable interrupts right before pm_power_off().
> And it looks like that all pm_power_off callbacks which can sleep are broken.
> I still wonder how i2c communication can works without local interrupts ... it looks
> like somebody is re-enabling them (or the code was never run)
>
Good question. Or the code was never run under arm, or the drivers have a polling
fallback if interrupts are disabled.

>> I have another idea how to get there without changing the lock situation
>> while executing the call chain - just set a flag indicating that it is
>> running and execute it without lock. Would that work ?
>
> I don't think inventing a new locking mechanism is a good solution.
> We need first to know for sure if we can sleep or not in pm_power_off.
> If yes then we need to re-enable local interrupts and we can use a mutex.
> If not then the atomic notifier is fine and a lots of drivers are wrong.
>

I had another look into the kernel, checking all the callers of machine_power_off().
In some cases, the function is called directly from interrupt handlers, meaning
there is no guarantee that interrupts are enabled to start with when it is called.
So there is more to fix if the semantics of pm_power_off requires interrupts
to be enabled.

My current solution is to use a spinlock with disabled interrupts during
poweroff handler registration and unregistration, and no lock at all while
executing the call chain. That is not perfect, but it retains the current
situation and thus guarantees that existing code works as before. This
disconnects the sleep vs. no sleep problem from the current patch series,
and we can solve that problem separately.

Thanks,
Guenter