Messages in this thread |  | | Date | Wed, 22 Oct 2014 10:02:57 +0200 | From | Philippe Rétornaz <> | Subject | Re: [PATCH v2 01/47] kernel: Add support for poweroff handler call chain |
| |
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)
> 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.
Thanks,
Philippe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |