[PATCH] ARM: hw_breakpoint: Trap undef instruction exceptions on wake-up

[PATCH] ARM: hw_breakpoint: Trap undef instruction exceptions on wake-up

[Geert Uytterhoeven]

If power area D4, which contains the Coresight-ETM hardware block, is
powered down on R-Mobile A1 (r8a7740), the kernel crashes when
suspending from s2ram with:

    Internal error: Oops - undefined instruction: 0 [#1] ARM

This happens because dbg_cpu_pm_notify() calls reset_ctrl_regs(), which
can't access the debug registers as the debug module is powered down.

Protect the call to reset_ctrl_regs() by an undefined instruction hook,
like was done in commit 0d352e3d006c9589 ("ARM: hw_breakpoint: trap
undef instruction exceptions in reset_ctrl_regs") for another caller.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Question: Do we need the same protection in dbg_reset_notify()?
          I can't trigger that case, as r8a7740 has only one CPU core.
---
 arch/arm/kernel/hw_breakpoint.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 4d963fb66e3f0bce..f55f25d4e4de7674 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -1038,8 +1038,21 @@ static struct notifier_block dbg_reset_nb = {
 static int dbg_cpu_pm_notify(struct notifier_block *self, unsigned long action,
 			     void *v)
 {
-	if (action == CPU_PM_EXIT)
+	if (action == CPU_PM_EXIT) {
+		/*
+		 * We need to tread carefully here because DBGSWENABLE may be
+		 * driven low on this core and there isn't an architected way to
+		 * determine that.
+		 */
+		register_undef_hook(&debug_reg_hook);
+
+		/*
+		 * Reset the breakpoint resources. We assume that a halting
+		 * debugger will leave the world in a nice state for us.
+		 */
 		reset_ctrl_regs(NULL);
+		unregister_undef_hook(&debug_reg_hook);
+	}
 
 	return NOTIFY_OK;
 }
-- 
1.9.1

Re: [PATCH] ARM: hw_breakpoint: Trap undef instruction exceptions on wake-up

[Russell King - ARM Linux]

On Wed, Sep 17, 2014 at 01:57:16PM +0200, Geert Uytterhoeven wrote:
> If power area D4, which contains the Coresight-ETM hardware block, is
> powered down on R-Mobile A1 (r8a7740), the kernel crashes when
> suspending from s2ram with:
> 
>     Internal error: Oops - undefined instruction: 0 [#1] ARM
> 
> This happens because dbg_cpu_pm_notify() calls reset_ctrl_regs(), which
> can't access the debug registers as the debug module is powered down.
> 
> Protect the call to reset_ctrl_regs() by an undefined instruction hook,
> like was done in commit 0d352e3d006c9589 ("ARM: hw_breakpoint: trap
> undef instruction exceptions in reset_ctrl_regs") for another caller.

I'd prefer a better solution to this (such as tracking whether the ETM
block is powered down).  This feels very much like a hack than a good
solution.

Also, the undefined instruction hook was not supposed to be used in this
way - certainly *not* dynamically like this.  While the lookup of the
undef handler is done in a safe atomic way, calling the resulting
function is outside of the locks.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] ARM: hw_breakpoint: Trap undef instruction exceptions on wake-up

[Geert Uytterhoeven]

Hi Russell,

On Wed, Sep 17, 2014 at 2:18 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Sep 17, 2014 at 01:57:16PM +0200, Geert Uytterhoeven wrote:
>> If power area D4, which contains the Coresight-ETM hardware block, is
>> powered down on R-Mobile A1 (r8a7740), the kernel crashes when
>> suspending from s2ram with:
>>
>>     Internal error: Oops - undefined instruction: 0 [#1] ARM
>>
>> This happens because dbg_cpu_pm_notify() calls reset_ctrl_regs(), which
>> can't access the debug registers as the debug module is powered down.
>>
>> Protect the call to reset_ctrl_regs() by an undefined instruction hook,
>> like was done in commit 0d352e3d006c9589 ("ARM: hw_breakpoint: trap
>> undef instruction exceptions in reset_ctrl_regs") for another caller.
>
> I'd prefer a better solution to this (such as tracking whether the ETM
> block is powered down).  This feels very much like a hack than a good
> solution.

That seems to be something to fit into "[PATCH 00/11 v6] Coresight framework
and drivers" (https://lkml.org/lkml/2014/9/11/733)?

However, if I'm not mistaken, while that series introduces extensive handling
based on the presence of Coresight device nodes in DT, it looks like it doesn't
affect the current direct accesses to the debug registers in hw_breakpoint.c?

For now, do you consider it acceptable to introduce a global bool (defaulting
to true) to track whether accessing the debug registers is allowed?

> Also, the undefined instruction hook was not supposed to be used in this
> way - certainly *not* dynamically like this.  While the lookup of the
> undef handler is done in a safe atomic way, calling the resulting
> function is outside of the locks.

Right, that's definitely not optimal.
I guess this also applies to the existing (ab)users of debug_reg_hook?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds