diff mbox

ARM: hw_breakpoint: Enable debug powerdown only if system supports 'has_ossr'

Message ID 5148082A.3060503@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Santosh Shilimkar March 19, 2013, 6:39 a.m. UTC
On Monday 18 March 2013 10:36 PM, Will Deacon wrote:
> On Mon, Mar 18, 2013 at 03:46:28PM +0000, Santosh Shilimkar wrote:
>> On Monday 18 March 2013 08:37 PM, Will Deacon wrote:
>>> That really sucks :( Does this affect all OMAP-based boards?
>>>
>> All OMAP4 based boards..
> 
> Brilliant. Is there any way that the secure code can be fixed in future
> products?
> 
Nope. This can only be done with new silicon rev for GP devices which is
not going to happen. For secure devices, some secure patching is possible
but these are not development devices, so not much point.

>>>> +	/* Check if we have access to CPU debug features */
>>>> +	ARM_DBG_READ(c7, c14, 6, val);
>>>> +	if ((val & 0x1) == 0) {
>>>> +		pr_warn_once("CPU %d debug is unavailable\n", cpu);
>>>> +		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
>>>> +		return;
>>>> +	}
>>>
>>> There are a few of problems here:
>>>
>>> 	1. That is only checking for non-secure access, which precludes
>>> 	   running Linux in secure mode.
>>>
>> We can check bit 4 as well to take care linux running in secure mode.
> 
> It still doesn't help unless we know whether Linux is running secure or
> non-secure.
> 
ok.

>>> 	2. DBGAUTHSTATUS accesses are UNPREDICTABLE when the double-lock is
>>> 	   set for v7.1 processors.
>>>
>>> 	3. DBGAUTHSTATUS doesn't exist in ARMv6.
>>>
>> We cans skip the code for these versions like...
>> 	if (!ARM_DEBUG_ARCH_V7_ECP14 == get_debug_arch())
>> 		goto skip_dbgsts_read;
> 
> Sure, I was just pointing out that the code needs fixing for this.
> 
>>> 	4. CPUs without the security extensions have DBGAUTHSTATUS.NSE == 0
>>>
>> Which bit is that ? I don't find this bit in Cortex-A9 docs. With all
>> these checks, may be a separate function like 'is_debug_available()'
>> would be better.
> 
> NSE is bit 0 (the one you're checking).
> 
ok. So the subject patch might break those devices.

>>  
>>> 	5. Accessing DBGAUTHSTATUS requires DBGEN to be driven high.
>>> 	   Assuming that your pr_warn_once is emitted, this implies that
>>> 	   DBGEN is driven high from cold boot, yet the NSE bit is low,
>>> 	   implying that DBGEN is also low. That's contradictory, so I have
>>> 	   no idea what's going on...
>>>
>> Me neither. The warning is emitted at least.
> 
> Any chance you could follow up with your firmware/hardware guys about this
> please? I'd really like to understand how we end up in this state in case we
> can do something in the architecture to stop it from happening in future.
> 
I will check on this part and update you when I hear from them.

>>> Apart from that, it's fine!
>>>
>> If you agree, I can update patch accordingly.
>> BTW, do you have any better trick to take care of
>> above scenraio ?
> 
> Well, we could just add the warn_once prints but that doesn't stop debug
> from breaking after the first pm/suspend/hotplug operation.
> 
Probably we should drop the $subject patch approach and use warn_once
at least for current rc. Ofcourse it doesn't help to get working
hw_breakpoint support. Patch end of the email with warn_once.

Regards,
Santosh

From 6611d48eb5571e3e094c7a9c2479e652b37d35e3 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Tue, 19 Mar 2013 11:53:41 +0530
Subject: [PATCH] ARM: hw_breakpoints: Use warn_once to avoid debug message
 flood from reset_ctrl_regs()

CPU debug features like hardware break, watchpoints can be used only when
the debug mode is enabled and available. Unfortunately on OMAP4 based device,
after a CPU power cycle, the debug feature gets disabled which leads to
a flood of messages coming from reset_ctrl_regs() which gets called on
every CPU_PM_EXIT with CPUidle enabled.

So make use of warn_once() so that system is usable.

Thanks to Will for pointers and Lokesh for the analysis of the issue.

Cc: Will Deacon <Will.Deacon@arm.com>

Tested-by: Lokesh Vutla <lokeshvutla@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/kernel/hw_breakpoint.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Will Deacon March 19, 2013, 10:28 a.m. UTC | #1
On Tue, Mar 19, 2013 at 06:39:38AM +0000, Santosh Shilimkar wrote:
> On Monday 18 March 2013 10:36 PM, Will Deacon wrote:
> > Any chance you could follow up with your firmware/hardware guys about this
> > please? I'd really like to understand how we end up in this state in case we
> > can do something in the architecture to stop it from happening in future.
> > 
> I will check on this part and update you when I hear from them.

Ok, thanks.

Dietmar -- I seem to remember that you thought GDB did actually work with
hardware breakpoints on Pandaboard across low-power states. Can you
confirm/deny this please?

> > Well, we could just add the warn_once prints but that doesn't stop debug
> > from breaking after the first pm/suspend/hotplug operation.
> > 
> Probably we should drop the $subject patch approach and use warn_once
> at least for current rc. Ofcourse it doesn't help to get working
> hw_breakpoint support. Patch end of the email with warn_once.

Yeah, I'll merge that, but it's a real shame that this breaks hardware debug
on one of the few platforms where it was reported to work.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Santosh Shilimkar March 25, 2013, 9:11 a.m. UTC | #2
Will,

On Tuesday 19 March 2013 03:58 PM, Will Deacon wrote:
> On Tue, Mar 19, 2013 at 06:39:38AM +0000, Santosh Shilimkar wrote:
>> On Monday 18 March 2013 10:36 PM, Will Deacon wrote:

[..]

>>> Well, we could just add the warn_once prints but that doesn't stop debug
>>> from breaking after the first pm/suspend/hotplug operation.
>>>
>> Probably we should drop the $subject patch approach and use warn_once
>> at least for current rc. Ofcourse it doesn't help to get working
>> hw_breakpoint support. Patch end of the email with warn_once.
> 
> Yeah, I'll merge that, but it's a real shame that this breaks hardware debug
> on one of the few platforms where it was reported to work.
> 
Are you going to send the patch for 3.9-rcx ? As I said before without the
patch OMAP4 CPUILDE is unusable because of that debug noise and hence it
will be good to get that patch in

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Will Deacon March 25, 2013, 10:49 a.m. UTC | #3
On Mon, Mar 25, 2013 at 09:11:00AM +0000, Santosh Shilimkar wrote:
> Will,

Hi Santosh,

> Are you going to send the patch for 3.9-rcx ? As I said before without the
> patch OMAP4 CPUILDE is unusable because of that debug noise and hence it
> will be good to get that patch in

It's in Russell's tree, so should be queued for mainline fairly soon.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Santosh Shilimkar March 25, 2013, 10:55 a.m. UTC | #4
On Monday 25 March 2013 04:19 PM, Will Deacon wrote:
> On Mon, Mar 25, 2013 at 09:11:00AM +0000, Santosh Shilimkar wrote:
>> Will,
> 
> Hi Santosh,
> 
>> Are you going to send the patch for 3.9-rcx ? As I said before without the
>> patch OMAP4 CPUILDE is unusable because of that debug noise and hence it
>> will be good to get that patch in
> 
> It's in Russell's tree, so should be queued for mainline fairly soon.
> 
Thanks Will !!

Regards
Santosh

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dietmar Eggemann March 28, 2013, 11:59 a.m. UTC | #5
On 19/03/13 10:28, Will Deacon wrote:
> On Tue, Mar 19, 2013 at 06:39:38AM +0000, Santosh Shilimkar wrote:
>> On Monday 18 March 2013 10:36 PM, Will Deacon wrote:
>>> Any chance you could follow up with your firmware/hardware guys about this
>>> please? I'd really like to understand how we end up in this state in case we
>>> can do something in the architecture to stop it from happening in future.
>>>
>> I will check on this part and update you when I hear from them.
>
> Ok, thanks.
>
> Dietmar -- I seem to remember that you thought GDB did actually work with
> hardware breakpoints on Pandaboard across low-power states. Can you
> confirm/deny this please?

Sorry for the late response.

I did some testing on my Pandaboard trying to set hwbp from gdb.

System is a vanilla Linaro 13.02.
gdb is (GNU gdb (GDB) 7.5.1), build from tag gdb_7_5_1-2012-11-29-release

(gdb) hb *0x00008440
Hardware assisted breakpoint 2 at 0x8440: file test.c, line 14.
(gdb) c
Continuing.
Unexpected error setting breakpoint address: No such device.

With additional kernel logs:

[<c0011597>] (unwind_backtrace+0x1/0x92) from [<c0012603>]
(arch_validate_hwbkpt_settings+0x23/0x1d4)
[<c0012603>] (arch_validate_hwbkpt_settings+0x23/0x1d4) from
[<c0088aa9>] (register_perf_hw_breakpoint+0x19/0x30)
[<c0088aa9>] (register_perf_hw_breakpoint+0x19/0x30) from [<c0088ae9>]
(hw_breakpoint_event_init+0x29/0x48)
[<c0088ae9>] (hw_breakpoint_event_init+0x29/0x48) from [<c0086c81>]
(perf_init_event+0x79/0xcc)
[<c0086c81>] (perf_init_event+0x79/0xcc) from [<c0086e97>]
(perf_event_alloc+0x1c3/0x32c)
[<c0086e97>] (perf_event_alloc+0x1c3/0x32c) from [<c00871eb>]
(perf_event_create_kernel_counter+0x19/0xc6)
[<c00871eb>] (perf_event_create_kernel_counter+0x19/0xc6) from
[<c00885e9>] (register_user_hw_breakpoint+0x2d/0x38)
[<c00885e9>] (register_user_hw_breakpoint+0x2d/0x38) from [<c03dd723>]
(ptrace_hbp_create+0x53/0x5c)
[<c03dd723>] (ptrace_hbp_create+0x53/0x5c) from [<c000e0d7>]
(ptrace_sethbpregs+0x95/0x1a6)
[<c000e0d7>] (ptrace_sethbpregs+0x95/0x1a6) from [<c000e777>]
(arch_ptrace+0x3bf/0x3ec)
[<c000e777>] (arch_ptrace+0x3bf/0x3ec) from [<c002f621>]
(sys_ptrace+0x251/0x278)
[<c002f621>] (sys_ptrace+0x251/0x278) from [<c000c781>]
(ret_fast_syscall+0x1/0x52)
perf_event_create_kernel_counter:6721 ret=-19
ptrace_sethbpregs:557 ret=-19

The call to monitor_mode_enabled() in arch_validate_hwbkpt_settings()
fails since DSCR.15 MDBGen is not set:

monitor_mode_enabled:239 dscr=0x1030002

I don't get the warning "Failed to enable monitor mode on CPU" during
system start-up though.

Doing the same on my TC2 gives me:

monitor_mode_enabled:239 dscr=0x2008002

So I guess that setting hwbp from self-hosted debugger on Pandaboard is
not possible at all.

There is no CPUidle enabled on Linaro 13.02 Pandaboard.

-- Dietmar


>
>>> Well, we could just add the warn_once prints but that doesn't stop debug
>>> from breaking after the first pm/suspend/hotplug operation.
>>>
>> Probably we should drop the $subject patch approach and use warn_once
>> at least for current rc. Ofcourse it doesn't help to get working
>> hw_breakpoint support. Patch end of the email with warn_once.
>
> Yeah, I'll merge that, but it's a real shame that this breaks hardware debug
> on one of the few platforms where it was reported to work.
>
> Will
>

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 96093b7..5dc1aa6 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -966,7 +966,7 @@  static void reset_ctrl_regs(void *unused)
 	}
 
 	if (err) {
-		pr_warning("CPU %d debug is powered down!\n", cpu);
+		pr_warn_once("CPU %d debug is powered down!\n", cpu);
 		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
 		return;
 	}
@@ -987,7 +987,7 @@  clear_vcr:
 	isb();
 
 	if (cpumask_intersects(&debug_err_mask, cpumask_of(cpu))) {
-		pr_warning("CPU %d failed to disable vector catch\n", cpu);
+		pr_warn_once("CPU %d failed to disable vector catch\n", cpu);
 		return;
 	}
 
@@ -1007,7 +1007,7 @@  clear_vcr:
 	}
 
 	if (cpumask_intersects(&debug_err_mask, cpumask_of(cpu))) {
-		pr_warning("CPU %d failed to clear debug register pairs\n", cpu);
+		pr_warn_once("CPU %d failed to clear debug register pairs\n", cpu);
 		return;
 	}