[PATCH] ARM: hw_breakpoint: Clear breakpoints before enabling monitor mode

[PATCH] ARM: hw_breakpoint: Clear breakpoints before enabling monitor mode

[Stephen Boyd]

The reset value of the BCR, BVR, WCR, and WVR registers are all
UNKNOWN on ARMv7. Unfortunately, reset_ctrl_regs() clears these
registers *after* enabling monitor mode, not before, and so some
implementations may experience UNPREDICTABLE behavior if the
reset values of these registers are non-zero. Clear the
breakpoints before enabling monitor mode so that we don't
experience boot hangs/loops due to breakpoints being enabled
out of reset.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/kernel/hw_breakpoint.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index ba386bd..9eac571 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -216,6 +216,20 @@ static int get_num_brps(void)
 	return core_has_mismatch_brps() ? brps - 1 : brps;
 }
 
+/* Determine if halting mode is enabled */
+static int halting_mode_enabled(void)
+{
+	u32 dscr;
+
+	ARM_DBG_READ(c1, 0, dscr);
+
+	if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN,
+		"halting debug mode enabled. Unable to access hardware resources.\n")) {
+		return -EPERM;
+	}
+	return 0;
+}
+
 /*
  * In order to access the breakpoint/watchpoint control registers,
  * we must be running in debug monitor mode. Unfortunately, we can
@@ -225,16 +239,13 @@ static int get_num_brps(void)
 static int enable_monitor_mode(void)
 {
 	u32 dscr;
-	int ret = 0;
+	int ret;
 
 	ARM_DBG_READ(c1, 0, dscr);
 
 	/* Ensure that halting mode is disabled. */
-	if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN,
-		"halting debug mode enabled. Unable to access hardware resources.\n")) {
-		ret = -EPERM;
+	if ((ret = halting_mode_enabled()))
 		goto out;
-	}
 
 	/* If monitor mode is already enabled, just return. */
 	if (dscr & ARM_DSCR_MDBGEN)
@@ -935,7 +946,7 @@ static void reset_ctrl_regs(void *unused)
 	isb();
 
 reset_regs:
-	if (enable_monitor_mode())
+	if (halting_mode_enabled())
 		return;
 
 	/* We must also reset any reserved registers. */
@@ -949,6 +960,7 @@ reset_regs:
 		write_wb_reg(ARM_BASE_WCR + i, 0UL);
 		write_wb_reg(ARM_BASE_WVR + i, 0UL);
 	}
+	enable_monitor_mode();
 }
 
 static int __cpuinit dbg_reset_notify(struct notifier_block *self,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] ARM: hw_breakpoint: Clear breakpoints before enabling monitor mode

[Will Deacon]

Hi Stephen,

On Thu, Sep 20, 2012 at 05:57:40PM +0100, Stephen Boyd wrote:
> The reset value of the BCR, BVR, WCR, and WVR registers are all
> UNKNOWN on ARMv7. Unfortunately, reset_ctrl_regs() clears these
> registers *after* enabling monitor mode, not before, and so some
> implementations may experience UNPREDICTABLE behavior if the
> reset values of these registers are non-zero. Clear the
> breakpoints before enabling monitor mode so that we don't
> experience boot hangs/loops due to breakpoints being enabled
> out of reset.
> 
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Aha, thanks for the patch. We should definitely zero these registers before
enabling monitor mode. However...

> +/* Determine if halting mode is enabled */
> +static int halting_mode_enabled(void)
> +{
> +	u32 dscr;
> +
> +	ARM_DBG_READ(c1, 0, dscr);
> +
> +	if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN,
> +		"halting debug mode enabled. Unable to access hardware resources.\n")) {
> +		return -EPERM;
> +	}
> +	return 0;
> +}

...it looks like debug arch 7.1 defines this bit as UNKNOWN when the OS lock
is clear, so we probably shouldn't be reading it at all. I'll pour myself a
stiff drink and start reading the debug arch docs to work out what on Earth
we should do.

Stay tuned.

Will

[PATCH] ARM: hw_breakpoint: Clear breakpoints before enabling monitor mode

[Will Deacon]

On Thu, Sep 20, 2012 at 06:35:56PM +0100, Will Deacon wrote:
> On Thu, Sep 20, 2012 at 05:57:40PM +0100, Stephen Boyd wrote:
> > +/* Determine if halting mode is enabled */
> > +static int halting_mode_enabled(void)
> > +{
> > +	u32 dscr;
> > +
> > +	ARM_DBG_READ(c1, 0, dscr);
> > +
> > +	if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN,
> > +		"halting debug mode enabled. Unable to access hardware resources.\n")) {
> > +		return -EPERM;
> > +	}
> > +	return 0;
> > +}
> 
> ...it looks like debug arch 7.1 defines this bit as UNKNOWN when the OS lock
> is clear, so we probably shouldn't be reading it at all. I'll pour myself a
> stiff drink and start reading the debug arch docs to work out what on Earth
> we should do.
> 
> Stay tuned.

Ok, I've pushed a bunch of patches to my hw-breakpoint branch (head commit
55cb726797c7). I'll post them to the list after the merge window, but please
do take them for a spin if you get a chance.

Will

[PATCH] ARM: hw_breakpoint: Clear breakpoints before enabling monitor mode

[Stephen Boyd]

On 09/24/12 10:19, Will Deacon wrote:
> On Thu, Sep 20, 2012 at 06:35:56PM +0100, Will Deacon wrote:
>> On Thu, Sep 20, 2012 at 05:57:40PM +0100, Stephen Boyd wrote:
>>> +/* Determine if halting mode is enabled */
>>> +static int halting_mode_enabled(void)
>>> +{
>>> +	u32 dscr;
>>> +
>>> +	ARM_DBG_READ(c1, 0, dscr);
>>> +
>>> +	if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN,
>>> +		"halting debug mode enabled. Unable to access hardware resources.\n")) {
>>> +		return -EPERM;
>>> +	}
>>> +	return 0;
>>> +}
>> ...it looks like debug arch 7.1 defines this bit as UNKNOWN when the OS lock
>> is clear, so we probably shouldn't be reading it at all. I'll pour myself a
>> stiff drink and start reading the debug arch docs to work out what on Earth
>> we should do.
>>
>> Stay tuned.
> Ok, I've pushed a bunch of patches to my hw-breakpoint branch (head commit
> 55cb726797c7). I'll post them to the list after the merge window, but please
> do take them for a spin if you get a chance.
>

Sure, I'll try them later today. I would say just send them out so
people can add tested and reviewed tags. Otherwise we should all start
planning week long vacations every 7 to 8 weeks to coincide with the
merge window.

Also, it would be nice if we could fix this in 3.7 or even 3.6. Booting
on an MSM8660 is very fragile right now and this patch fixes it for me.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] ARM: hw_breakpoint: Clear breakpoints before enabling monitor mode

[Will Deacon]

On Mon, Sep 24, 2012 at 07:04:56PM +0100, Stephen Boyd wrote:
> On 09/24/12 10:19, Will Deacon wrote:
> > Ok, I've pushed a bunch of patches to my hw-breakpoint branch (head commit
> > 55cb726797c7). I'll post them to the list after the merge window, but please
> > do take them for a spin if you get a chance.
> >
> 
> Sure, I'll try them later today. I would say just send them out so
> people can add tested and reviewed tags. Otherwise we should all start
> planning week long vacations every 7 to 8 weeks to coincide with the
> merge window.

I like the idea of that! However, it's more that most people are worrying
about getting their trees into shape rather than reviewing new code at
the moment, so I suspect that posting it now will go largely un-noticed
and I don't think this is a critical fix (see below).

> Also, it would be nice if we could fix this in 3.7 or even 3.6. Booting
> on an MSM8660 is very fragile right now and this patch fixes it for me.

Whilst I appreciate that it solves your problem, I'm *really* wary about
changing this stuff without giving it a thorough airing beforehand. The
debug reset path is extremely error-prone and its behaviour is tangled
up with the state of various external signals into the core, meaning that
different platforms with the same CPU can take different paths at runtime.

I can definitely CC stable once we're happy with this, but I'd rather
avoid rushing the patches in before 3.8.

Cheers,

Will

[PATCH] ARM: hw_breakpoint: Clear breakpoints before enabling monitor mode

[Stephen Boyd]

On 09/24/12 10:19, Will Deacon wrote:
> Ok, I've pushed a bunch of patches to my hw-breakpoint branch (head commit
> 55cb726797c7). I'll post them to the list after the merge window, but please
> do take them for a spin if you get a chance.
>

Forgot to reply here. Took them for a spin last week with that commit.
No more boot hangs but I can't say much else beyond that. When you post
to the list I'll add my tested-by.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] ARM: hw_breakpoint: Clear breakpoints before enabling monitor mode

[Will Deacon]

On Tue, Oct 02, 2012 at 01:34:28AM +0100, Stephen Boyd wrote:
> On 09/24/12 10:19, Will Deacon wrote:
> > Ok, I've pushed a bunch of patches to my hw-breakpoint branch (head commit
> > 55cb726797c7). I'll post them to the list after the merge window, but please
> > do take them for a spin if you get a chance.
> >
> 
> Forgot to reply here. Took them for a spin last week with that commit.
> No more boot hangs but I can't say much else beyond that. When you post
> to the list I'll add my tested-by.

Thanks Stephen. I've also got some patches for OS save/restore of the debug
registers using the various hardware locking and readout mechanisms, so that
debug state can be persisted across low-power states.

Do any of the Qualcomm chips (v7 as opposed to v7.1) implement the save/restore
registers?

Will

[PATCH] ARM: hw_breakpoint: Clear breakpoints before enabling monitor mode

[Stephen Boyd]

On 10/02/12 02:13, Will Deacon wrote:
>
> Thanks Stephen. I've also got some patches for OS save/restore of the debug
> registers using the various hardware locking and readout mechanisms, so that
> debug state can be persisted across low-power states.
>
> Do any of the Qualcomm chips (v7 as opposed to v7.1) implement the save/restore
> registers?

Yes, we have save and restore registers.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation