Message-ID: <5148082A.3060503@ti.com>
Date:	Tue, 19 Mar 2013 12:09:38 +0530
From:	Santosh Shilimkar <santosh.shilimkar@...com>
To:	Will Deacon <will.deacon@....com>
CC:	Will Deacon <will@...ldeacon.co.uk>,
	Lokesh Vutla <lokeshvutla@...com>,
	Dietmar Eggemann <Dietmar.Eggemann@....com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>
Subject: Re: [PATCH] ARM: hw_breakpoint: Enable debug powerdown only if system
 supports 'has_ossr'

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@...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@....com>

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

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;
 	}
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists