So i got this ARM board with a rtc controlled by i2c that can also cut
power. I want to use it to reboot, because it will also cut power to
clumsy USB and MMC devices.
So the natural thing to do would be to just register a restart_handler
that will kindly do i2c (saw at least a driver doing it), or use
arm_pm_restart and be done with it.
Well that what I though. Most of my naive attempts were met with
occasional failures and now my understanding is that I do not
understand why anything works in the first place.
So the first thing I saw while doing i2c in a restart_handler are
kernel splats about sleeping in a RCU critical section.
Since i2c is slow, most i2c controllers either sleep until the i2c
transfer is complete or they wait for an interrupt. restart_handler
is an atomic notifier chain, so sleeping in there should be bad.
So I looked at drivers to see what they do and it seems many of them
sleep inside their restart_handler callbacks. Some even have infinite
loops. And there is the rn5t618 driver which does i2c (as well as a
small mdelay(1)), which is used in some meson8 boards, whose i2c
controller wait for completion of interrupts. So I'm wondering why am I
the only one with these problems.
So i cobbled a patch that turns restart_handler into a notifier call
chain that can block. It removes the splat, but reboot still
occasionally fails.
By the time we get to arm_pm_restart() or do_machine_restart(),
interrupts are disabled and only one processor is running.
The first thing i did was to enable interrupts on the remaining
processor, so that i2c could work, but it turns out my i2c controller
does not use interrupts, so that didn't change anything. Still, i would
consider this to be desirable anyway.
The remaining problems turns out to be timers that never fires. sysrq Q
shows:
active timers:
#0: <e5d80165> , hrtimer_wakeup , S:01
# expires at 349641289373-349641389373 nsecs [in -1000130841786 to -1000130741786 nsecs]
#1: <b06a222c> , tick_sched_timer , S:01
# expires at 349650000000-349650000000 nsecs [in -1000122131159 to -1000122131159 nsecs]
#2: sched_clock_timer , sched_clock_poll , S:01
# expires at 715827882841-715827882841 nsecs [in -633944248318 to -633944248318 nsecs]
with no matching .next_timer in any clockevent device.
I think it is because when we call send_smp_stop(), we don't unregister the
twd clockevent of the affected CPU and we may still allocate timers on
it.
I noticed that the CPU hotplug code handles this fine, so my current
workaround is to enable CPU hotplug in the kernel and manually
deactivate CPUs in the init system before rebooting
So did I missing something ? What would be the correct to fix all this ?
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 <[email protected]>
---
kernel/reboot.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
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)
--
2.19.0
Some system power controller are behind bus like i2c, whose
controller driver requires interrupts to work.
Signed-off-by: Nicolas Cavallari <[email protected]>
---
arch/arm/kernel/reboot.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/kernel/reboot.c b/arch/arm/kernel/reboot.c
index 3b2aa9a9fe26..2773068c0d67 100644
--- a/arch/arm/kernel/reboot.c
+++ b/arch/arm/kernel/reboot.c
@@ -140,6 +140,7 @@ void machine_restart(char *cmd)
{
local_irq_disable();
smp_send_stop();
+ local_irq_enable();
if (arm_pm_restart)
arm_pm_restart(reboot_mode, cmd);
--
2.19.0
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.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
On Thu, Oct 04, 2018 at 06:23:37PM +0200, Nicolas Cavallari wrote:
> So i got this ARM board with a rtc controlled by i2c that can also cut
> power. I want to use it to reboot, because it will also cut power to
> clumsy USB and MMC devices.
> So the natural thing to do would be to just register a restart_handler
> that will kindly do i2c (saw at least a driver doing it), or use
> arm_pm_restart and be done with it.
>
> Well that what I though. Most of my naive attempts were met with
> occasional failures and now my understanding is that I do not
> understand why anything works in the first place.
Hi Nicolas
I doubt you can use the platform i2c driver.
If you look at how this is done for sending commands over a UART to a
micro-controller, you will see the reset driver hijack the UART
device. They poke registers with the require baud rate settings, and
then poke registers with the bytes to send, polling for completion of
each byte if needed.
To get this working reliably, i suspect you are going to need to do
the same with your i2c bus controller. Directly control it, stop any
in progress transfers, setup a new transfer, and hope you can do it
polled IO, not DMA.
Andrew
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.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up