| Message ID | 20181004162339.19493-2-nicolas.cavallari@green-communications.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/2] reboot: Make restart_handler_list a blocking notifier chain. | expand |
On Thu, Oct 04, 2018 at 06:23:38PM +0200, Nicolas Cavallari wrote: > Many users of restart_handlers are sleeping in their callbacks. Some > are doing infinite loops or calling driver code that may sleep or > perform operation on slow busses, like i2c. > > This is not allowed in an atomic notifier chain, which is what > restart_handler_list currently is, so use a blocking notifier chain > instead. This isn't going to work. For example, sysrq processing (which can happen in IRQ context) calls emergency_restart() for the reboot sysrq. That calls through to machine_restart(), which then calls do_kernel_restart(). If do_kernel_restart() sleeps, then we're trying to sleep in IRQ context, and that's a no no. I'm afraid you can't just add an irq enable and change the notifier list to be atomic - and, as you're making the change in generic code, this affects everyone, not just the architecture that happens to be in front of you (so if it were merged, you're likely to get a lot of bug reports!) It gets worse, because (eg) a panic() or IRQ can happen with any locks held - and if the I2C device's locks are held when one of those events happen, you have a deadlock situation (hence you won't reboot!) I suppose a good first step would be for us to have our own machine_emergency_restart() on ARM, to separate the atomic paths from the non-atomic paths, so that those who need to talk to an I2C, that can happen from the non-atomic path (which means things like /sbin/reboot will work) but other stuff (eg, restart on panic, sysrq, soft-watchdog) will fail. This issue as come up recently surrounding PMIC issues, but the discussions there appear to have completely dried up... However, my conclusion is that having an I2C driver deal with reboot is possible for the process-induced reboot cases, but it's never going to work reliably for the emergency case. If you want the emergency case to work, then you need to work out some way to talk on the I2C bus without involving any locks and with the I2C bus possibly mid-transfer - which is not an easy problem to solve.
On 04/10/2018 18:49, Russell King - ARM Linux wrote: > This isn't going to work. > > For example, sysrq processing (which can happen in IRQ context) calls > emergency_restart() for the reboot sysrq. That calls through to > machine_restart(), which then calls do_kernel_restart(). > > If do_kernel_restart() sleeps, then we're trying to sleep in IRQ > context, and that's a no no. I'm afraid you can't just add an irq > enable and change the notifier list to be atomic - and, as you're > making the change in generic code, this affects everyone, not just the > architecture that happens to be in front of you (so if it were merged, > you're likely to get a lot of bug reports!) I don't get it. Many implementations of machine_restart() sleeps or do an infinite loop. Almost half of the restart_handler users sleeps or do an infinite loop. I understand that sleeping in IRQ context is bad, but the kernel does it anyway. And it seems nobody have noticed any problem with this. The rn5t618 driver already does two I2C transfers in its restart handler. Haven't anyone reported any problem about it ? > It gets worse, because (eg) a panic() or IRQ can happen with any locks > held - and if the I2C device's locks are held when one of those events > happen, you have a deadlock situation (hence you won't reboot!) > > I suppose a good first step would be for us to have our own > machine_emergency_restart() on ARM, to separate the atomic paths > from the non-atomic paths, so that those who need to talk to an I2C, > that can happen from the non-atomic path (which means things like > /sbin/reboot will work) but other stuff (eg, restart on panic, sysrq, > soft-watchdog) will fail. That would fix my use case, but not the existing code ?
On Fri, Oct 05, 2018 at 10:27:48AM +0200, Nicolas Cavallari wrote: > On 04/10/2018 18:49, Russell King - ARM Linux wrote: > > This isn't going to work. > > > > For example, sysrq processing (which can happen in IRQ context) calls > > emergency_restart() for the reboot sysrq. That calls through to > > machine_restart(), which then calls do_kernel_restart(). > > > > If do_kernel_restart() sleeps, then we're trying to sleep in IRQ > > context, and that's a no no. I'm afraid you can't just add an irq > > enable and change the notifier list to be atomic - and, as you're > > making the change in generic code, this affects everyone, not just the > > architecture that happens to be in front of you (so if it were merged, > > you're likely to get a lot of bug reports!) > > I don't get it. > > Many implementations of machine_restart() sleeps or do an infinite loop. > Almost half of the restart_handler users sleeps or do an infinite loop. Infinite loops are not a problem when shutting down or rebooting - they're only "infinite" in the sense that control never returns but that is the case anyway when the restart or shutdown takes effect. > I understand that sleeping in IRQ context is bad, but the kernel does it > anyway. And it seems nobody have noticed any problem with this. That is incorrect - there are reports of this failing as I mentioned in my email. Also note that there is a big difference between sleeping in atomic context (iow, sleeping with spinlocks held, attempting to sleep with IRQs disabled) and sleeping in IRQ context (iow, sleeping in an interrupt handler). You can "work around" the former with your code, but in doing so you end up _breaking_ the latter case for every situation. I've pointed out some of the issues that make it unreliable in my initial email (quoted below). > > It gets worse, because (eg) a panic() or IRQ can happen with any locks > > held - and if the I2C device's locks are held when one of those events > > happen, you have a deadlock situation (hence you won't reboot!) > > > > I suppose a good first step would be for us to have our own > > machine_emergency_restart() on ARM, to separate the atomic paths > > from the non-atomic paths, so that those who need to talk to an I2C, > > that can happen from the non-atomic path (which means things like > > /sbin/reboot will work) but other stuff (eg, restart on panic, sysrq, > > soft-watchdog) will fail. > > That would fix my use case, but not the existing code ? There is no fix possible for a blocking I2C transfer in IRQ context, it will always be unreliable for the reasons I've explained above. All those existing cases where drivers are doing I2C transfers using the I2C host driver for power down or restart are broken and unreliable.
diff --git a/kernel/reboot.c b/kernel/reboot.c index 8fb44dec9ad7..f0ba0008dbde 100644 --- a/kernel/reboot.c +++ b/kernel/reboot.c @@ -135,7 +135,7 @@ EXPORT_SYMBOL(devm_register_reboot_notifier); * Notifier list for kernel code which wants to be called * to restart the system. */ -static ATOMIC_NOTIFIER_HEAD(restart_handler_list); +static BLOCKING_NOTIFIER_HEAD(restart_handler_list); /** * register_restart_handler - Register function to be called to reset @@ -172,12 +172,12 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list); * hardware is expected to register with low priority to ensure that * it only runs if no other means to restart the system is available. * - * Currently always returns zero, as atomic_notifier_chain_register() + * Currently always returns zero, as blocking_notifier_chain_register() * always returns zero. */ int register_restart_handler(struct notifier_block *nb) { - return atomic_notifier_chain_register(&restart_handler_list, nb); + return blocking_notifier_chain_register(&restart_handler_list, nb); } EXPORT_SYMBOL(register_restart_handler); @@ -192,7 +192,7 @@ EXPORT_SYMBOL(register_restart_handler); */ int unregister_restart_handler(struct notifier_block *nb) { - return atomic_notifier_chain_unregister(&restart_handler_list, nb); + return blocking_notifier_chain_unregister(&restart_handler_list, nb); } EXPORT_SYMBOL(unregister_restart_handler); @@ -209,7 +209,7 @@ EXPORT_SYMBOL(unregister_restart_handler); */ void do_kernel_restart(char *cmd) { - atomic_notifier_call_chain(&restart_handler_list, reboot_mode, cmd); + blocking_notifier_call_chain(&restart_handler_list, reboot_mode, cmd); } void migrate_to_reboot_cpu(void)
Many users of restart_handlers are sleeping in their callbacks. Some are doing infinite loops or calling driver code that may sleep or perform operation on slow busses, like i2c. This is not allowed in an atomic notifier chain, which is what restart_handler_list currently is, so use a blocking notifier chain instead. Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr> --- kernel/reboot.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)