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

[Nishanth Menon <nm@ti.com>]

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