[PATCHv2] DSPBRIDGE: Include missing info in MMU Fault debugging trace

[PATCHv2] DSPBRIDGE: Include missing info in MMU Fault debugging trace

[Ramos Falcon, Ernesto]

Resending this patch rebased to the last changes included in dspbridge.
---

Some execution info is missing at the beginning of the
trace buffer printout. This fix is to include this information
according to the buffer contents description and
also couple of cosmetic changes to make the traces more
readable.

Signed-off-by: Ernesto Ramos <ernesto@ti.com>
---
 drivers/dsp/bridge/core/io_sm.c |   44 +++++++++++++++++++++++++++++++-------
 1 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/dsp/bridge/core/io_sm.c b/drivers/dsp/bridge/core/io_sm.c
index d6c1a98..d11bd9f 100644
--- a/drivers/dsp/bridge/core/io_sm.c
+++ b/drivers/dsp/bridge/core/io_sm.c
@@ -1949,9 +1949,9 @@ dsp_status print_dsp_trace_buffer(struct bridge_dev_context *hbridge_context)
 		if (DSP_FAILED(status))
 			goto func_end;
 		/* Pack and do newline conversion */
-		pr_info("%s: DSP Trace Buffer Begin:\n"
+		pr_info("DSP Trace Buffer Begin:\n"
 			"=======================\n%s\n",
-			__func__, psz_buf);
+			psz_buf);
 
 
 		/* convert to offset */
@@ -2058,6 +2058,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
 		u32 size;
 	} mmu_fault_dbg_info;
 	u32 *buffer;
+	u32 *buffer_beg;
 	u32 *buffer_end;
 	u32 exc_type;
 	u32 i;
@@ -2067,6 +2068,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
 	const char *dsp_regs[] = {"EFR", "IERR", "ITSR", "NTSR",
 				"IRP", "NRP", "AMR", "SSR",
 				"ILC", "RILC", "IER", "CSR"};
+	const char *exec_ctxt[] = {"Task", "SWI", "HWI", "Unknown"};
 	struct bridge_drv_interface *intf_fxns;
 	struct dev_object *dev_object = bridge_context->hdev_obj;
 
@@ -2136,6 +2138,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
 			total_size = MAX_MMU_DBGBUFF;
 
 		buffer = kzalloc(total_size, GFP_ATOMIC);
+		buffer_beg = buffer;
 		buffer_end =  buffer + total_size / 4;
 
 		if (!buffer) {
@@ -2155,7 +2158,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
 			goto func_end;
 		}
 
-		pr_err("Aproximate Crash Position:\n");
+		pr_err("\nAproximate Crash Position:\n");
 		pr_err("--------------------------\n");
 
 		exc_type = buffer[3];
@@ -2171,7 +2174,27 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
 		else
 			pr_err("0x%-8x [Unable to match to a symbol.]\n", i);
 
-		pr_err("Execution Info:\n");
+		buffer += 4;
+
+		pr_err("\nExecution Info:\n");
+		pr_err("---------------\n");
+
+		if (*buffer < ARRAY_SIZE(exec_ctxt)) {
+			pr_err("Execution context \t%s\n",
+				exec_ctxt[*buffer++]);
+		} else {
+			pr_err("Execution context corrupt\n");
+			kfree(buffer_beg);
+			return -EFAULT;
+		}
+		pr_err("Task Handle\t\t0x%x\n", *buffer++);
+		pr_err("Stack Pointer\t\t0x%x\n", *buffer++);
+		pr_err("Stack Top\t\t0x%x\n", *buffer++);
+		pr_err("Stack Bottom\t\t0x%x\n", *buffer++);
+		pr_err("Stack Size\t\t0x%x\n", *buffer++);
+		pr_err("Stack Size In Use\t0x%x\n", *buffer++);
+
+		pr_err("\nCPU Registers\n");
 		pr_err("---------------\n");
 
 		for (i = 0; i < 32; i++) {
@@ -2205,16 +2228,21 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
 			if (i == 4 || i == 6 || i == 8)
 				pr_err("B%d 0x%-8x [Function Argument %d]\n",
 							i, *buffer++, i-2);
-			else if (i == 15)
+			else if (i == 14)
 				pr_err("B14 0x%-8x [Data Page Pointer]\n",
 								*buffer++);
 			else
 				pr_err("B%d 0x%x\n", i, *buffer++);
 		}
 
+		pr_err("\n");
+
 		for (i = 0; i < ARRAY_SIZE(dsp_regs); i++)
 			pr_err("%s 0x%x\n", dsp_regs[i], *buffer++);
 
+		pr_err("\nStack:\n");
+		pr_err("------\n");
+
 		for (i = 0; buffer < buffer_end; i++, buffer++) {
 			if ((*buffer > 0x01000000) && (node_find_addr(node_mgr,
 				*buffer , 0x600, &offset_output, name) ==
@@ -2225,7 +2253,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
 			else
 				pr_err("[%d] 0x%x\n", i, *buffer);
 		}
-		kfree(buffer - total_size / 4);
+		kfree(buffer_beg);
 	}
 func_end:
 	return status;
@@ -2290,8 +2318,8 @@ void dump_dl_modules(struct bridge_dev_context *bridge_context)
 	pr_debug("%s: dll_module_header 0x%x %d\n", __func__, module_dsp_addr,
 								module_size);
 
-	pr_err("%s: \nDynamically Loaded Modules:\n"
-		"---------------------------\n", __func__);
+	pr_err("\nDynamically Loaded Modules:\n"
+		"---------------------------\n");
 
 	/* For each dll_module structure in the list... */
 	while (module_size) {
-- 
1.5.4.5

Re: [PATCHv2] DSPBRIDGE: Include missing info in MMU Fault debugging trace

[Nishanth Menon]

Ramos Falcon, Ernesto had written, on 05/13/2010 10:40 AM, the following:
> Resending this patch rebased to the last changes included in dspbridge.
> ---
> 
> Some execution info is missing at the beginning of the
> trace buffer printout. This fix is to include this information
> according to the buffer contents description and
> also couple of cosmetic changes to make the traces more
> readable.
> 
> Signed-off-by: Ernesto Ramos <ernesto@ti.com>
> ---
>  drivers/dsp/bridge/core/io_sm.c |   44 +++++++++++++++++++++++++++++++-------
>  1 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dsp/bridge/core/io_sm.c b/drivers/dsp/bridge/core/io_sm.c
> index d6c1a98..d11bd9f 100644
> --- a/drivers/dsp/bridge/core/io_sm.c
> +++ b/drivers/dsp/bridge/core/io_sm.c
> @@ -1949,9 +1949,9 @@ dsp_status print_dsp_trace_buffer(struct bridge_dev_context *hbridge_context)
>  		if (DSP_FAILED(status))
>  			goto func_end;
>  		/* Pack and do newline conversion */
> -		pr_info("%s: DSP Trace Buffer Begin:\n"
> +		pr_info("DSP Trace Buffer Begin:\n"
>  			"=======================\n%s\n",
> -			__func__, psz_buf);
> +			psz_buf);
>  
>  
>  		/* convert to offset */
> @@ -2058,6 +2058,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
>  		u32 size;
>  	} mmu_fault_dbg_info;
>  	u32 *buffer;
> +	u32 *buffer_beg;
>  	u32 *buffer_end;
>  	u32 exc_type;
>  	u32 i;
> @@ -2067,6 +2068,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
>  	const char *dsp_regs[] = {"EFR", "IERR", "ITSR", "NTSR",
>  				"IRP", "NRP", "AMR", "SSR",
>  				"ILC", "RILC", "IER", "CSR"};
> +	const char *exec_ctxt[] = {"Task", "SWI", "HWI", "Unknown"};
>  	struct bridge_drv_interface *intf_fxns;
>  	struct dev_object *dev_object = bridge_context->hdev_obj;
>  
> @@ -2136,6 +2138,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
>  			total_size = MAX_MMU_DBGBUFF;
>  
>  		buffer = kzalloc(total_size, GFP_ATOMIC);
> +		buffer_beg = buffer;
>  		buffer_end =  buffer + total_size / 4;
>  
>  		if (!buffer) {
> @@ -2155,7 +2158,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
>  			goto func_end;
>  		}
>  
> -		pr_err("Aproximate Crash Position:\n");
> +		pr_err("\nAproximate Crash Position:\n");
>  		pr_err("--------------------------\n");
>  
>  		exc_type = buffer[3];
> @@ -2171,7 +2174,27 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
>  		else
>  			pr_err("0x%-8x [Unable to match to a symbol.]\n", i);
>  
> -		pr_err("Execution Info:\n");
> +		buffer += 4;
> +
> +		pr_err("\nExecution Info:\n");
> +		pr_err("---------------\n");
> +
> +		if (*buffer < ARRAY_SIZE(exec_ctxt)) {
> +			pr_err("Execution context \t%s\n",
> +				exec_ctxt[*buffer++]);
> +		} else {
> +			pr_err("Execution context corrupt\n");
> +			kfree(buffer_beg);
> +			return -EFAULT;
> +		}
> +		pr_err("Task Handle\t\t0x%x\n", *buffer++);
> +		pr_err("Stack Pointer\t\t0x%x\n", *buffer++);
> +		pr_err("Stack Top\t\t0x%x\n", *buffer++);
> +		pr_err("Stack Bottom\t\t0x%x\n", *buffer++);
> +		pr_err("Stack Size\t\t0x%x\n", *buffer++);
> +		pr_err("Stack Size In Use\t0x%x\n", *buffer++);
> +
How about casting buffer to a structure and using it?
the use of buffer++ etc is a nightmare as can be seen here..

> +		pr_err("\nCPU Registers\n");
>  		pr_err("---------------\n");
btw, linux allows you to use string in a single pr_err.. e.g.
pr_err("\n CPU Registers\n"
        "----------------\n");

it is a bit easier on the system without going thru two function call 
overhead

>  
>  		for (i = 0; i < 32; i++) {
> @@ -2205,16 +2228,21 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
>  			if (i == 4 || i == 6 || i == 8)
>  				pr_err("B%d 0x%-8x [Function Argument %d]\n",
>  							i, *buffer++, i-2);
> -			else if (i == 15)
> +			else if (i == 14)
>  				pr_err("B14 0x%-8x [Data Page Pointer]\n",
>  								*buffer++);
>  			else
>  				pr_err("B%d 0x%x\n", i, *buffer++);
>  		}
>  
> +		pr_err("\n");
> +
>  		for (i = 0; i < ARRAY_SIZE(dsp_regs); i++)
>  			pr_err("%s 0x%x\n", dsp_regs[i], *buffer++);
>  
> +		pr_err("\nStack:\n");
> +		pr_err("------\n");
> +
>  		for (i = 0; buffer < buffer_end; i++, buffer++) {
>  			if ((*buffer > 0x01000000) && (node_find_addr(node_mgr,
>  				*buffer , 0x600, &offset_output, name) ==
> @@ -2225,7 +2253,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context *bridge_context)
>  			else
>  				pr_err("[%d] 0x%x\n", i, *buffer);
>  		}
> -		kfree(buffer - total_size / 4);
> +		kfree(buffer_beg);
>  	}
>  func_end:
>  	return status;
> @@ -2290,8 +2318,8 @@ void dump_dl_modules(struct bridge_dev_context *bridge_context)
>  	pr_debug("%s: dll_module_header 0x%x %d\n", __func__, module_dsp_addr,
>  								module_size);
>  
> -	pr_err("%s: \nDynamically Loaded Modules:\n"
> -		"---------------------------\n", __func__);
> +	pr_err("\nDynamically Loaded Modules:\n"
> +		"---------------------------\n");
>  
>  	/* For each dll_module structure in the list... */
>  	while (module_size) {


-- 
Regards,
Nishanth Menon

RE: [PATCHv2] DSPBRIDGE: Include missing info in MMU Fault debugging trace

[Ramos Falcon, Ernesto]

>-----Original Message-----
>From: Menon, Nishanth
>Sent: Thursday, May 13, 2010 10:49 AM
>To: Ramos Falcon, Ernesto
>Cc: linux-omap@vger.kernel.org; Ameya Palande; Contreras Felipe (Nokia-
>D/Helsinki)
>Subject: Re: [PATCHv2] DSPBRIDGE: Include missing info in MMU Fault
>debugging trace
>
>Ramos Falcon, Ernesto had written, on 05/13/2010 10:40 AM, the following:
>> Resending this patch rebased to the last changes included in dspbridge.
>> ---
>>
>> Some execution info is missing at the beginning of the
>> trace buffer printout. This fix is to include this information
>> according to the buffer contents description and
>> also couple of cosmetic changes to make the traces more
>> readable.
>>
>> Signed-off-by: Ernesto Ramos <ernesto@ti.com>
>> ---
>>  drivers/dsp/bridge/core/io_sm.c |   44 +++++++++++++++++++++++++++++++--
>-----
>>  1 files changed, 36 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/dsp/bridge/core/io_sm.c
>b/drivers/dsp/bridge/core/io_sm.c
>> index d6c1a98..d11bd9f 100644
>> --- a/drivers/dsp/bridge/core/io_sm.c
>> +++ b/drivers/dsp/bridge/core/io_sm.c
>> @@ -1949,9 +1949,9 @@ dsp_status print_dsp_trace_buffer(struct
>bridge_dev_context *hbridge_context)
>>  		if (DSP_FAILED(status))
>>  			goto func_end;
>>  		/* Pack and do newline conversion */
>> -		pr_info("%s: DSP Trace Buffer Begin:\n"
>> +		pr_info("DSP Trace Buffer Begin:\n"
>>  			"=======================\n%s\n",
>> -			__func__, psz_buf);
>> +			psz_buf);
>>
>>
>>  		/* convert to offset */
>> @@ -2058,6 +2058,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context
>*bridge_context)
>>  		u32 size;
>>  	} mmu_fault_dbg_info;
>>  	u32 *buffer;
>> +	u32 *buffer_beg;
>>  	u32 *buffer_end;
>>  	u32 exc_type;
>>  	u32 i;
>> @@ -2067,6 +2068,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context
>*bridge_context)
>>  	const char *dsp_regs[] = {"EFR", "IERR", "ITSR", "NTSR",
>>  				"IRP", "NRP", "AMR", "SSR",
>>  				"ILC", "RILC", "IER", "CSR"};
>> +	const char *exec_ctxt[] = {"Task", "SWI", "HWI", "Unknown"};
>>  	struct bridge_drv_interface *intf_fxns;
>>  	struct dev_object *dev_object = bridge_context->hdev_obj;
>>
>> @@ -2136,6 +2138,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context
>*bridge_context)
>>  			total_size = MAX_MMU_DBGBUFF;
>>
>>  		buffer = kzalloc(total_size, GFP_ATOMIC);
>> +		buffer_beg = buffer;
>>  		buffer_end =  buffer + total_size / 4;
>>
>>  		if (!buffer) {
>> @@ -2155,7 +2158,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context
>*bridge_context)
>>  			goto func_end;
>>  		}
>>
>> -		pr_err("Aproximate Crash Position:\n");
>> +		pr_err("\nAproximate Crash Position:\n");
>>  		pr_err("--------------------------\n");
>>
>>  		exc_type = buffer[3];
>> @@ -2171,7 +2174,27 @@ dsp_status dump_dsp_stack(struct
>bridge_dev_context *bridge_context)
>>  		else
>>  			pr_err("0x%-8x [Unable to match to a symbol.]\n", i);
>>
>> -		pr_err("Execution Info:\n");
>> +		buffer += 4;
>> +
>> +		pr_err("\nExecution Info:\n");
>> +		pr_err("---------------\n");
>> +
>> +		if (*buffer < ARRAY_SIZE(exec_ctxt)) {
>> +			pr_err("Execution context \t%s\n",
>> +				exec_ctxt[*buffer++]);
>> +		} else {
>> +			pr_err("Execution context corrupt\n");
>> +			kfree(buffer_beg);
>> +			return -EFAULT;
>> +		}
>> +		pr_err("Task Handle\t\t0x%x\n", *buffer++);
>> +		pr_err("Stack Pointer\t\t0x%x\n", *buffer++);
>> +		pr_err("Stack Top\t\t0x%x\n", *buffer++);
>> +		pr_err("Stack Bottom\t\t0x%x\n", *buffer++);
>> +		pr_err("Stack Size\t\t0x%x\n", *buffer++);
>> +		pr_err("Stack Size In Use\t0x%x\n", *buffer++);
>> +
>How about casting buffer to a structure and using it?
>the use of buffer++ etc is a nightmare as can be seen here..
>

I don't see the need to use a structure for this set of data if anyway I need to use buffer++ for the rest of the buffer data. Furthermore, if I declare a structure I need to initialize it using buffer as a reference.
I don't think is such a nightmare.

>> +		pr_err("\nCPU Registers\n");
>>  		pr_err("---------------\n");
>btw, linux allows you to use string in a single pr_err.. e.g.
>pr_err("\n CPU Registers\n"
>        "----------------\n");
>
>it is a bit easier on the system without going thru two function call
>overhead
>
OK. I'll change that.

>>
>>  		for (i = 0; i < 32; i++) {
>> @@ -2205,16 +2228,21 @@ dsp_status dump_dsp_stack(struct
>bridge_dev_context *bridge_context)
>>  			if (i == 4 || i == 6 || i == 8)
>>  				pr_err("B%d 0x%-8x [Function Argument %d]\n",
>>  							i, *buffer++, i-2);
>> -			else if (i == 15)
>> +			else if (i == 14)
>>  				pr_err("B14 0x%-8x [Data Page Pointer]\n",
>>  								*buffer++);
>>  			else
>>  				pr_err("B%d 0x%x\n", i, *buffer++);
>>  		}
>>
>> +		pr_err("\n");
>> +
>>  		for (i = 0; i < ARRAY_SIZE(dsp_regs); i++)
>>  			pr_err("%s 0x%x\n", dsp_regs[i], *buffer++);
>>
>> +		pr_err("\nStack:\n");
>> +		pr_err("------\n");
>> +
>>  		for (i = 0; buffer < buffer_end; i++, buffer++) {
>>  			if ((*buffer > 0x01000000) && (node_find_addr(node_mgr,
>>  				*buffer , 0x600, &offset_output, name) ==
>> @@ -2225,7 +2253,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context
>*bridge_context)
>>  			else
>>  				pr_err("[%d] 0x%x\n", i, *buffer);
>>  		}
>> -		kfree(buffer - total_size / 4);
>> +		kfree(buffer_beg);
>>  	}
>>  func_end:
>>  	return status;
>> @@ -2290,8 +2318,8 @@ void dump_dl_modules(struct bridge_dev_context
>*bridge_context)
>>  	pr_debug("%s: dll_module_header 0x%x %d\n", __func__,
>module_dsp_addr,
>>  								module_size);
>>
>> -	pr_err("%s: \nDynamically Loaded Modules:\n"
>> -		"---------------------------\n", __func__);
>> +	pr_err("\nDynamically Loaded Modules:\n"
>> +		"---------------------------\n");
>>
>>  	/* For each dll_module structure in the list... */
>>  	while (module_size) {
>
>
>--
>Regards,
>Nishanth Menon

RE: [PATCHv2] DSPBRIDGE: Include missing info in MMU Fault debugging trace

[Kanigeri, Hari]

Ernesto

> @@ -2136,6 +2138,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context
> *bridge_context)
>  			total_size = MAX_MMU_DBGBUFF;
> 
>  		buffer = kzalloc(total_size, GFP_ATOMIC);
> +		buffer_beg = buffer;
>  		buffer_end =  buffer + total_size / 4;
> 
>  		if (!buffer) {

-- Shouldn't this check be present before the above 2 statements ?

>  		for (i = 0; buffer < buffer_end; i++, buffer++) {
>  			if ((*buffer > 0x01000000) && (node_find_addr(node_mgr,

-- Is 0x01000000 MAX_MMU_DBGBUFF ? 

Thank you,
Best regards,
Hari

RE: [PATCHv2] DSPBRIDGE: Include missing info in MMU Fault debugging trace

[Ramos Falcon, Ernesto]

Hi,

>-----Original Message-----
>From: Kanigeri, Hari
>Sent: Thursday, May 13, 2010 10:55 AM
>To: Ramos Falcon, Ernesto; linux-omap@vger.kernel.org
>Cc: Ameya Palande; Contreras Felipe (Nokia-D/Helsinki)
>Subject: RE: [PATCHv2] DSPBRIDGE: Include missing info in MMU Fault
>debugging trace
>
>Ernesto
>
>> @@ -2136,6 +2138,7 @@ dsp_status dump_dsp_stack(struct bridge_dev_context
>> *bridge_context)
>>  			total_size = MAX_MMU_DBGBUFF;
>>
>>  		buffer = kzalloc(total_size, GFP_ATOMIC);
>> +		buffer_beg = buffer;
>>  		buffer_end =  buffer + total_size / 4;
>>
>>  		if (!buffer) {
>
>-- Shouldn't this check be present before the above 2 statements ?
>

I think in this case does not affect those two assignments could be avoid it, so OK.

>>  		for (i = 0; buffer < buffer_end; i++, buffer++) {
>>  			if ((*buffer > 0x01000000) && (node_find_addr(node_mgr,
>
>-- Is 0x01000000 MAX_MMU_DBGBUFF ?

OK, I'll change.
>
>Thank you,
>Best regards,
>Hari