[PATCH] ARM: Temporary fix for broken arch reboot

[PATCH] ARM: Temporary fix for broken arch reboot

[Shilimkar, Santosh]

With commit 3d3f78d752bf, reboot seems to broken on ARM
machines. CPU dies while doing flush_pmd_entry() as part of
setup_mm_for_reboot()

I know this is not the fix but intention is to report the
issue and also provide temporary fix till it get fixed correctly

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reported-by: Anand Gadiyar <gadiyar@ti.com>
---
Tested with OMAP4 device.

 arch/arm/kernel/process.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index e76fcaa..ac370b2 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -247,7 +247,6 @@ void machine_power_off(void)
 
 void machine_restart(char *cmd)
 {
-	machine_shutdown();
 	arm_pm_restart(reboot_mode, cmd);
 }
 
-- 
1.6.0.4

[PATCH] ARM: Temporary fix for broken arch reboot

[Russell King - ARM Linux]

On Tue, Nov 09, 2010 at 06:40:39PM +0530, Shilimkar, Santosh wrote:
> With commit 3d3f78d752bf, reboot seems to broken on ARM
> machines. CPU dies while doing flush_pmd_entry() as part of
> setup_mm_for_reboot()
> 
> I know this is not the fix but intention is to report the
> issue and also provide temporary fix till it get fixed correctly

So you're now rebooting with the secondary CPUs still running.  I guess
that the secondary CPUs end up crashing and don't restart.

I think more the question is why the CP15 cache clean/flush is hanging
with the other CPUs taken down.  All the other CPUs will be doing is
sitting in a loop doing nothing.

Catalin?

[PATCH] ARM: Temporary fix for broken arch reboot

[Shilimkar, Santosh]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, November 09, 2010 6:49 PM
> To: Shilimkar, Santosh; Catalin Marinas
> Cc: linux-arm-kernel at lists.infradead.org; Gadiyar, Anand
> Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> 
> On Tue, Nov 09, 2010 at 06:40:39PM +0530, Shilimkar, Santosh wrote:
> > With commit 3d3f78d752bf, reboot seems to broken on ARM
> > machines. CPU dies while doing flush_pmd_entry() as part of
> > setup_mm_for_reboot()
> >
> > I know this is not the fix but intention is to report the
> > issue and also provide temporary fix till it get fixed correctly
> 
> So you're now rebooting with the secondary CPUs still running.  I guess
> that the secondary CPUs end up crashing and don't restart.
> 
This behaviour depends on the reset mechanism implemented. On OMAP,
we have a way to do a reset on all the CPUs with single register bit,
like a global warm/cold reset. 

> I think more the question is why the CP15 cache clean/flush is hanging
> with the other CPUs taken down.  All the other CPUs will be doing is
> sitting in a loop doing nothing.
> 
> Catalin?

[PATCH] ARM: Temporary fix for broken arch reboot

[Catalin Marinas]

On Tue, 2010-11-09 at 13:18 +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 09, 2010 at 06:40:39PM +0530, Shilimkar, Santosh wrote:
> > With commit 3d3f78d752bf, reboot seems to broken on ARM
> > machines. CPU dies while doing flush_pmd_entry() as part of
> > setup_mm_for_reboot()

What do you mean by 'dies'? Can you still connect with a debugger or it
got to some weird state?

> > I know this is not the fix but intention is to report the
> > issue and also provide temporary fix till it get fixed correctly
> 
> So you're now rebooting with the secondary CPUs still running.  I guess
> that the secondary CPUs end up crashing and don't restart.
> 
> I think more the question is why the CP15 cache clean/flush is hanging
> with the other CPUs taken down.  All the other CPUs will be doing is
> sitting in a loop doing nothing.

I can't think of anything. Did the other CPUs print 'stopping'?

Catalin

[PATCH] ARM: Temporary fix for broken arch reboot

[Anand Gadiyar]

Catalin Marinas wrote:
> On Tue, 2010-11-09 at 13:18 +0000, Russell King - ARM Linux wrote:
> > On Tue, Nov 09, 2010 at 06:40:39PM +0530, Shilimkar, Santosh wrote:
> > > With commit 3d3f78d752bf, reboot seems to broken on ARM
> > > machines. CPU dies while doing flush_pmd_entry() as part of
> > > setup_mm_for_reboot()
>
> What do you mean by 'dies'? Can you still connect with a debugger or it
> got to some weird state?
>

I didn't try connecting with a debugger - will try this.

> > > I know this is not the fix but intention is to report the
> > > issue and also provide temporary fix till it get fixed correctly
> >
> > So you're now rebooting with the secondary CPUs still running.  I guess
> > that the secondary CPUs end up crashing and don't restart.
> >
> > I think more the question is why the CP15 cache clean/flush is hanging
> > with the other CPUs taken down.  All the other CPUs will be doing is
> > sitting in a loop doing nothing.
>
> I can't think of anything. Did the other CPUs print 'stopping'?
>

I'm fairly certain the other CPUs did not print 'stopping'. Will double
check.

- Anand

[PATCH] ARM: Temporary fix for broken arch reboot

[Shilimkar, Santosh]

> -----Original Message-----
> From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> Sent: Tuesday, November 09, 2010 10:08 PM
> To: Russell King - ARM Linux
> Cc: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org; Gadiyar,
> Anand
> Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> 
> On Tue, 2010-11-09 at 13:18 +0000, Russell King - ARM Linux wrote:
> > On Tue, Nov 09, 2010 at 06:40:39PM +0530, Shilimkar, Santosh wrote:
> > > With commit 3d3f78d752bf, reboot seems to broken on ARM
> > > machines. CPU dies while doing flush_pmd_entry() as part of
> > > setup_mm_for_reboot()
> 
> What do you mean by 'dies'? Can you still connect with a debugger or it
> got to some weird state?
> 
It goes to some weird state. Basically the emulation connection dies,
and debugger gets disconnected.

> > > I know this is not the fix but intention is to report the
> > > issue and also provide temporary fix till it get fixed correctly
> >
> > So you're now rebooting with the secondary CPUs still running.  I guess
> > that the secondary CPUs end up crashing and don't restart.
> >
> > I think more the question is why the CP15 cache clean/flush is hanging
> > with the other CPUs taken down.  All the other CPUs will be doing is
> > sitting in a loop doing nothing.
> 
> I can't think of anything. Did the other CPUs print 'stopping'?
No it doesn't not print anything.

[PATCH] ARM: Temporary fix for broken arch reboot

[Russell King - ARM Linux]

On Wed, Nov 10, 2010 at 11:25:21AM +0530, Shilimkar, Santosh wrote:
> > -----Original Message-----
> > From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> > Sent: Tuesday, November 09, 2010 10:08 PM
> > To: Russell King - ARM Linux
> > Cc: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org; Gadiyar,
> > Anand
> > Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> > 
> > On Tue, 2010-11-09 at 13:18 +0000, Russell King - ARM Linux wrote:
> > > On Tue, Nov 09, 2010 at 06:40:39PM +0530, Shilimkar, Santosh wrote:
> > > > With commit 3d3f78d752bf, reboot seems to broken on ARM
> > > > machines. CPU dies while doing flush_pmd_entry() as part of
> > > > setup_mm_for_reboot()
> > 
> > What do you mean by 'dies'? Can you still connect with a debugger or it
> > got to some weird state?
> > 
> It goes to some weird state. Basically the emulation connection dies,
> and debugger gets disconnected.
> 
> > > > I know this is not the fix but intention is to report the
> > > > issue and also provide temporary fix till it get fixed correctly
> > >
> > > So you're now rebooting with the secondary CPUs still running.  I guess
> > > that the secondary CPUs end up crashing and don't restart.
> > >
> > > I think more the question is why the CP15 cache clean/flush is hanging
> > > with the other CPUs taken down.  All the other CPUs will be doing is
> > > sitting in a loop doing nothing.
> > 
> > I can't think of anything. Did the other CPUs print 'stopping'?
> No it doesn't not print anything.

The processing of the IPI is asynchronous to the CPU which is rebooting
continuing - which means that if there is some kind of bus lockup, you
won't get anything from any of the CPUs.

You might be able to get something from the other CPUs by inserting a
mdelay(100) after the call to machine_shutdown(), to give the other CPUs
time to respond.

[PATCH] ARM: Temporary fix for broken arch reboot

[Catalin Marinas]

On Wed, 2010-11-10 at 10:06 +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 10, 2010 at 11:25:21AM +0530, Shilimkar, Santosh wrote:
> > > -----Original Message-----
> > > From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> > > Sent: Tuesday, November 09, 2010 10:08 PM
> > > To: Russell King - ARM Linux
> > > Cc: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org; Gadiyar,
> > > Anand
> > > Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> > >
> > > On Tue, 2010-11-09 at 13:18 +0000, Russell King - ARM Linux wrote:
> > > > On Tue, Nov 09, 2010 at 06:40:39PM +0530, Shilimkar, Santosh wrote:
> > > > > With commit 3d3f78d752bf, reboot seems to broken on ARM
> > > > > machines. CPU dies while doing flush_pmd_entry() as part of
> > > > > setup_mm_for_reboot()
> > >
> > > What do you mean by 'dies'? Can you still connect with a debugger or it
> > > got to some weird state?
> > >
> > It goes to some weird state. Basically the emulation connection dies,
> > and debugger gets disconnected.
> >
> > > > > I know this is not the fix but intention is to report the
> > > > > issue and also provide temporary fix till it get fixed correctly
> > > >
> > > > So you're now rebooting with the secondary CPUs still running.  I guess
> > > > that the secondary CPUs end up crashing and don't restart.
> > > >
> > > > I think more the question is why the CP15 cache clean/flush is hanging
> > > > with the other CPUs taken down.  All the other CPUs will be doing is
> > > > sitting in a loop doing nothing.
> > >
> > > I can't think of anything. Did the other CPUs print 'stopping'?
> > No it doesn't not print anything.
> 
> The processing of the IPI is asynchronous to the CPU which is rebooting
> continuing - which means that if there is some kind of bus lockup, you
> won't get anything from any of the CPUs.

The printing only happens for SYSTEM_BOOTING or SYSTEM_RUNNING. I
suspect in this case we have SYSTEM_RESTARTING and the condition in
ipi_cpu_stop() is false, therefore no printing. It may be worth putting
some printks outside the 'if' to see whether the secondary CPUs get
there.

I also wonder what happens when you stop a secondary CPU without
clearing the SMP/nAMP bit first. The SCU may still consider it as part
of the inner coherency domain. We were clearing this in some ARM tests
for CPU hotplug but mainly to improve the SCU speed.

BTW, does setup_mm_for_reboot works if PHYS_OFFSET > PAGE_OFFSET?

Catalin

[PATCH] ARM: Temporary fix for broken arch reboot

[Shilimkar, Santosh]

> -----Original Message-----
> From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> Sent: Wednesday, November 10, 2010 7:35 PM
> To: Russell King - ARM Linux
> Cc: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org; Gadiyar,
> Anand
> Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> 
> On Wed, 2010-11-10 at 10:06 +0000, Russell King - ARM Linux wrote:
> > On Wed, Nov 10, 2010 at 11:25:21AM +0530, Shilimkar, Santosh wrote:
> > > > -----Original Message-----
> > > > From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> > > > Sent: Tuesday, November 09, 2010 10:08 PM
> > > > To: Russell King - ARM Linux
> > > > Cc: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org;
> Gadiyar,
> > > > Anand
> > > > Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> > > >
> > > > On Tue, 2010-11-09 at 13:18 +0000, Russell King - ARM Linux wrote:
> > > > > On Tue, Nov 09, 2010 at 06:40:39PM +0530, Shilimkar, Santosh
> wrote:
> > > > > > With commit 3d3f78d752bf, reboot seems to broken on ARM
> > > > > > machines. CPU dies while doing flush_pmd_entry() as part of
> > > > > > setup_mm_for_reboot()
> > > >
> > > > What do you mean by 'dies'? Can you still connect with a debugger or
> it
> > > > got to some weird state?
> > > >
> > > It goes to some weird state. Basically the emulation connection dies,
> > > and debugger gets disconnected.
> > >
> > > > > > I know this is not the fix but intention is to report the
> > > > > > issue and also provide temporary fix till it get fixed correctly
> > > > >
> > > > > So you're now rebooting with the secondary CPUs still running.  I
> guess
> > > > > that the secondary CPUs end up crashing and don't restart.
> > > > >
> > > > > I think more the question is why the CP15 cache clean/flush is
> hanging
> > > > > with the other CPUs taken down.  All the other CPUs will be doing
> is
> > > > > sitting in a loop doing nothing.
> > > >
> > > > I can't think of anything. Did the other CPUs print 'stopping'?
> > > No it doesn't not print anything.
> >
> > The processing of the IPI is asynchronous to the CPU which is rebooting
> > continuing - which means that if there is some kind of bus lockup, you
> > won't get anything from any of the CPUs.
> 
> The printing only happens for SYSTEM_BOOTING or SYSTEM_RUNNING. I
> suspect in this case we have SYSTEM_RESTARTING and the condition in
> ipi_cpu_stop() is false, therefore no printing. It may be worth putting
> some printks outside the 'if' to see whether the secondary CPUs get
> there.
> 
While doing some experiments on this issue, one interesting
observation I made. Looks like there is race between two
Cores which makes system behave badly in reboot path.

Just adding a delay in the ipi_cpu_stop() makes the reboot work
as well

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 8c19595..f7dadbf 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -526,6 +526,8 @@ static void ipi_cpu_stop(unsigned int cpu)
                spin_unlock(&stop_lock);
        }

+       udelay(500);
+
        set_cpu_online(cpu, false);

        local_fiq_disable();

Regards,
Santosh

[PATCH] ARM: Temporary fix for broken arch reboot

[Russell King - ARM Linux]

On Wed, Nov 10, 2010 at 09:13:21PM +0530, Shilimkar, Santosh wrote:
> 
> 
> > -----Original Message-----
> > From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> > Sent: Wednesday, November 10, 2010 7:35 PM
> > To: Russell King - ARM Linux
> > Cc: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org; Gadiyar,
> > Anand
> > Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> > 
> > The printing only happens for SYSTEM_BOOTING or SYSTEM_RUNNING. I
> > suspect in this case we have SYSTEM_RESTARTING and the condition in
> > ipi_cpu_stop() is false, therefore no printing. It may be worth putting
> > some printks outside the 'if' to see whether the secondary CPUs get
> > there.
> > 
> While doing some experiments on this issue, one interesting
> observation I made. Looks like there is race between two
> Cores which makes system behave badly in reboot path.
> 
> Just adding a delay in the ipi_cpu_stop() makes the reboot work
> as well

That could just be that it's taking longer than 500us for the reboot CPU
to get through setup_mm_for_reboot().

[PATCH] ARM: Temporary fix for broken arch reboot

[Shilimkar, Santosh]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Wednesday, November 10, 2010 9:16 PM
> To: Shilimkar, Santosh
> Cc: Catalin Marinas; linux-arm-kernel at lists.infradead.org; Gadiyar, Anand
> Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> 
> On Wed, Nov 10, 2010 at 09:13:21PM +0530, Shilimkar, Santosh wrote:
> >
> >
> > > -----Original Message-----
> > > From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> > > Sent: Wednesday, November 10, 2010 7:35 PM
> > > To: Russell King - ARM Linux
> > > Cc: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org; Gadiyar,
> > > Anand
> > > Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> > >
> > > The printing only happens for SYSTEM_BOOTING or SYSTEM_RUNNING. I
> > > suspect in this case we have SYSTEM_RESTARTING and the condition in
> > > ipi_cpu_stop() is false, therefore no printing. It may be worth
> putting
> > > some printks outside the 'if' to see whether the secondary CPUs get
> > > there.
> > >
> > While doing some experiments on this issue, one interesting
> > observation I made. Looks like there is race between two
> > Cores which makes system behave badly in reboot path.
> >
> > Just adding a delay in the ipi_cpu_stop() makes the reboot work
> > as well
> 
> That could just be that it's taking longer than 500us for the reboot CPU
> to get through setup_mm_for_reboot().

It not seems to be the case otherwise below patch should have worked.
Right ? Did I miss your point ?

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index e76fcaa..c3ae1d6 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -103,6 +103,8 @@ void arm_machine_restart(char mode, const char *cmd)
         * soft boot works.
         */
        setup_mm_for_reboot(mode);
+
+       machine_shutdown();

        /* Clean and invalidate caches */
        flush_cache_all();
@@ -247,7 +249,6 @@ void machine_power_off(void)

 void machine_restart(char *cmd)
 {
-       machine_shutdown();
        arm_pm_restart(reboot_mode, cmd);
 }

[PATCH] ARM: Temporary fix for broken arch reboot

[Russell King - ARM Linux]

On Fri, Nov 12, 2010 at 04:01:25PM +0530, Shilimkar, Santosh wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > Sent: Wednesday, November 10, 2010 9:16 PM
> > To: Shilimkar, Santosh
> > Cc: Catalin Marinas; linux-arm-kernel at lists.infradead.org; Gadiyar, Anand
> > Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> > 
> > On Wed, Nov 10, 2010 at 09:13:21PM +0530, Shilimkar, Santosh wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Catalin Marinas [mailto:catalin.marinas at arm.com]
> > > > Sent: Wednesday, November 10, 2010 7:35 PM
> > > > To: Russell King - ARM Linux
> > > > Cc: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org; Gadiyar,
> > > > Anand
> > > > Subject: Re: [PATCH] ARM: Temporary fix for broken arch reboot
> > > >
> > > > The printing only happens for SYSTEM_BOOTING or SYSTEM_RUNNING. I
> > > > suspect in this case we have SYSTEM_RESTARTING and the condition in
> > > > ipi_cpu_stop() is false, therefore no printing. It may be worth
> > putting
> > > > some printks outside the 'if' to see whether the secondary CPUs get
> > > > there.
> > > >
> > > While doing some experiments on this issue, one interesting
> > > observation I made. Looks like there is race between two
> > > Cores which makes system behave badly in reboot path.
> > >
> > > Just adding a delay in the ipi_cpu_stop() makes the reboot work
> > > as well
> > 
> > That could just be that it's taking longer than 500us for the reboot CPU
> > to get through setup_mm_for_reboot().
> 
> It not seems to be the case otherwise below patch should have worked.
> Right ? Did I miss your point ?

Yes you did.  Maybe all this code completes before your 500us in the
secondary CPUs complete.  So the delay in the secondary CPU path proves
nothing.

A delay in the reboot CPU path ensures that the other CPUs have time to
react to the stop IPI, so we know what state the system is in.