diff mbox

[RFC,v1,1/3] ARM: Add irq disabled version of soft_restart.

Message ID 1392774729-3235-2-git-send-email-sebastian.capella@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sebastian Capella Feb. 19, 2014, 1:52 a.m. UTC
From: Russ Dill <Russ.Dill@ti.com>

This adds the ability to run soft_restart with local_irq/fiq_disable
already called. This is helpful for the hibernation code paths.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Robin Holt <holt@sgi.com>
---
 arch/arm/include/asm/system_misc.h |    1 +
 arch/arm/kernel/process.c          |   15 ++++++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

Comments

Russell King - ARM Linux Feb. 22, 2014, 10:26 a.m. UTC | #1
On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> From: Russ Dill <Russ.Dill@ti.com>
> 
> This adds the ability to run soft_restart with local_irq/fiq_disable
> already called. This is helpful for the hibernation code paths.

I'd rather keep this simple.  There's no problem with calling soft_restart
with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
there should be harmless.
Sebastian Capella Feb. 24, 2014, 11:13 p.m. UTC | #2
Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> > From: Russ Dill <Russ.Dill@ti.com>
> > 
> > This adds the ability to run soft_restart with local_irq/fiq_disable
> > already called. This is helpful for the hibernation code paths.
> 
> I'd rather keep this simple.  There's no problem with calling soft_restart
> with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
> there should be harmless.

Hi Russell,

I'm observing a data abort loop when I replace this call:

In the local_irq_disable, it ends up calling trace_hardirqs_off
(CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
trace_hardirqs_off_caller which checks lockdep_recursion in the
current task, but we've switched to a temporary stack with the
call_with_stack, and get_current is returning NULL.  This
triggers a data abort, which calls trace_hardirqs_off
again and so on.

Do you have any suggestions here?

Thanks,

Sebastian
Sebastian Capella Feb. 25, 2014, 12:22 a.m. UTC | #3
Quoting Sebastian Capella (2014-02-24 15:13:45)
> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
> > On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> > > From: Russ Dill <Russ.Dill@ti.com>
> > > 
> > > This adds the ability to run soft_restart with local_irq/fiq_disable
> > > already called. This is helpful for the hibernation code paths.
> > 
> > I'd rather keep this simple.  There's no problem with calling soft_restart
> > with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
> > there should be harmless.
> 
> Hi Russell,
> 
> I'm observing a data abort loop when I replace this call:
> 
> In the local_irq_disable, it ends up calling trace_hardirqs_off
> (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
> trace_hardirqs_off_caller which checks lockdep_recursion in the
> current task, but we've switched to a temporary stack with the
> call_with_stack, and get_current is returning NULL.  This
> triggers a data abort, which calls trace_hardirqs_off
> again and so on.
> 
> Do you have any suggestions here?

I'll go ahead and send v2, leaving the soft_restart_noirq in place for
now since it's already past midnight in Surrey.

Thanks!

Sebastian
Russ Dill Feb. 25, 2014, 7:56 a.m. UTC | #4
On 02/24/2014 03:13 PM, Sebastian Capella wrote:
> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
>> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
>>> From: Russ Dill <Russ.Dill@ti.com>
>>>
>>> This adds the ability to run soft_restart with local_irq/fiq_disable
>>> already called. This is helpful for the hibernation code paths.
>>
>> I'd rather keep this simple.  There's no problem with calling soft_restart
>> with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
>> there should be harmless.
> 
> Hi Russell,
> 
> I'm observing a data abort loop when I replace this call:
> 
> In the local_irq_disable, it ends up calling trace_hardirqs_off
> (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
> trace_hardirqs_off_caller which checks lockdep_recursion in the
> current task, but we've switched to a temporary stack with the
> call_with_stack, and get_current is returning NULL.  This
> triggers a data abort, which calls trace_hardirqs_off
> again and so on.
> 
> Do you have any suggestions here?
> 
> Thanks,
> 
> Sebastian
> 

So the alternative is to have a version of the call that calls a special
no trace version of local_irq_disable()/local_fiq_disable(). Which would
be preferable? Having a noirq version of soft_restart seems much simpler
to me.
Thomas Gleixner Feb. 25, 2014, 10:27 a.m. UTC | #5
On Mon, 24 Feb 2014, Russ Dill wrote:
> On 02/24/2014 03:13 PM, Sebastian Capella wrote:
> > Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
> >> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> >>> From: Russ Dill <Russ.Dill@ti.com>
> >>>
> >>> This adds the ability to run soft_restart with local_irq/fiq_disable
> >>> already called. This is helpful for the hibernation code paths.
> >>
> >> I'd rather keep this simple.  There's no problem with calling soft_restart
> >> with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
> >> there should be harmless.
> > 
> > Hi Russell,
> > 
> > I'm observing a data abort loop when I replace this call:
> > 
> > In the local_irq_disable, it ends up calling trace_hardirqs_off
> > (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
> > trace_hardirqs_off_caller which checks lockdep_recursion in the
> > current task, but we've switched to a temporary stack with the
> > call_with_stack, and get_current is returning NULL.  This
> > triggers a data abort, which calls trace_hardirqs_off
> > again and so on.
> > 
> > Do you have any suggestions here?
> > 
> > Thanks,
> > 
> > Sebastian
> > 
> 
> So the alternative is to have a version of the call that calls a special
> no trace version of local_irq_disable()/local_fiq_disable(). Which would
> be preferable? Having a noirq version of soft_restart seems much simpler
> to me.

If you want escape the tracer and in that case you really want it
being on a different stack, use raw_local_irq_* which are not traced.

Thanks,

	tglx
Russ Dill Feb. 25, 2014, 5:15 p.m. UTC | #6
On 02/25/2014 02:27 AM, Thomas Gleixner wrote:
> On Mon, 24 Feb 2014, Russ Dill wrote:
>> On 02/24/2014 03:13 PM, Sebastian Capella wrote:
>>> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
>>>> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella
>>>> wrote:
>>>>> From: Russ Dill <Russ.Dill@ti.com>
>>>>> 
>>>>> This adds the ability to run soft_restart with
>>>>> local_irq/fiq_disable already called. This is helpful for
>>>>> the hibernation code paths.
>>>> 
>>>> I'd rather keep this simple.  There's no problem with calling
>>>> soft_restart with interrupts already disabled.
>>>> local_irq_disable()/local_fiq_disable() there should be
>>>> harmless.
>>> 
>>> Hi Russell,
>>> 
>>> I'm observing a data abort loop when I replace this call:
>>> 
>>> In the local_irq_disable, it ends up calling
>>> trace_hardirqs_off (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled),
>>> which calls trace_hardirqs_off_caller which checks
>>> lockdep_recursion in the current task, but we've switched to a
>>> temporary stack with the call_with_stack, and get_current is
>>> returning NULL.  This triggers a data abort, which calls
>>> trace_hardirqs_off again and so on.
>>> 
>>> Do you have any suggestions here?
>>> 
>>> Thanks,
>>> 
>>> Sebastian
>>> 
>> 
>> So the alternative is to have a version of the call that calls a
>> special no trace version of
>> local_irq_disable()/local_fiq_disable(). Which would be
>> preferable? Having a noirq version of soft_restart seems much
>> simpler to me.
> 
> If you want escape the tracer and in that case you really want it 
> being on a different stack, use raw_local_irq_* which are not
> traced.

So it might make sense to change soft_restart to use the
raw_local_irq_* calls.
Sebastian Capella Feb. 25, 2014, 11:24 p.m. UTC | #7
Quoting Russ Dill (2014-02-25 09:15:33)
> On 02/25/2014 02:27 AM, Thomas Gleixner wrote:
> > If you want escape the tracer and in that case you really want it 
> > being on a different stack, use raw_local_irq_* which are not
> > traced.
> 
> So it might make sense to change soft_restart to use the
> raw_local_irq_* calls.

I've added this change.  So far it's working well.

Thanks!

Sebastian
diff mbox

Patch

diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
index a3d61ad..eca8dd4 100644
--- a/arch/arm/include/asm/system_misc.h
+++ b/arch/arm/include/asm/system_misc.h
@@ -10,6 +10,7 @@ 
 
 extern void cpu_init(void);
 
+void soft_restart_noirq(unsigned long);
 void soft_restart(unsigned long);
 extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 extern void (*arm_pm_idle)(void);
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 92f7b15..685bc4a 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -95,14 +95,10 @@  static void __soft_restart(void *addr)
 	BUG();
 }
 
-void soft_restart(unsigned long addr)
+void soft_restart_noirq(unsigned long addr)
 {
 	u64 *stack = soft_restart_stack + ARRAY_SIZE(soft_restart_stack);
 
-	/* Disable interrupts first */
-	local_irq_disable();
-	local_fiq_disable();
-
 	/* Disable the L2 if we're the last man standing. */
 	if (num_online_cpus() == 1)
 		outer_disable();
@@ -114,6 +110,15 @@  void soft_restart(unsigned long addr)
 	BUG();
 }
 
+void soft_restart(unsigned long addr)
+{
+	/* Disable interrupts first */
+	local_irq_disable();
+	local_fiq_disable();
+
+	soft_restart_noirq(addr);
+}
+
 static void null_restart(enum reboot_mode reboot_mode, const char *cmd)
 {
 }