| Message ID | 1392774729-3235-2-git-send-email-sebastian.capella@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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.
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
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.
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 --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) { }