Found some errors and other oddities, largely by means of a static code analysis program
Dan Carpenter
dan.carpenter at oracle.com
Sat May 3 19:50:31 UTC 2014
This patch wasn't sent in the correct way. The subject should be:
[PATCH] staging: tidspbridge: silence some uninitialized variable warnings
On Sat, May 03, 2014 at 06:12:11PM +0200, Rickard Strandqvist wrote:
> Hi
>
> The static code analysis program called cppcheck.
> http://cppcheck.sourceforge.net/
>
> I found code that I think are bugs, or at least inappropriate or
> unnecessary code, in the files:
> drivers/staging/tidspbridge/pmgr/dspapi.c
> drivers/staging/tidspbridge/rmgr/node.c
>
>
> I have created a patch, and inluderat the error file generated by cppcheck.
>
> My goal was not to change any functionality, but it does not mean for
> example the unused variables can't mean that there are other
> problems/mistakes in the code. So a proper code review :)
>
> Is there anything else I can help with regarding the patch or
> cppcheck, do not hesitate to contact me.
> If you like this type of code analysis, it is possible to get more
> warnings, which are not as serious, but that may well indicate other
> mistakes.
>
This text should go in the patch description.
>
> Best regards
> Rickard Strandqvist
> From 0ef1cda18e05aa6d0b0ea745ce194f33d8f03973 Mon Sep 17 00:00:00 2001
> From: Rickard Strandqvist <rickard_strandqvist at spectrumdigital.se>
> Date: Wed, 30 Apr 2014 16:27:31 +0200
> Subject: [PATCH] Found same errors using a static code analysis program
> called cppcheck.
>
This block of text doesn't belong. Use git send-email.
There is no Signed-off-by line.
> ---
> drivers/staging/tidspbridge/pmgr/dspapi.c | 8 ++++----
> drivers/staging/tidspbridge/rmgr/node.c | 16 ++++------------
> 2 filer ändrade, 8 tillägg(+), 16 borttagningar(-)
>
> diff --git a/drivers/staging/tidspbridge/pmgr/dspapi.c b/drivers/staging/tidspbridge/pmgr/dspapi.c
> index b7d5c8c..4e12a5b 100644
> --- a/drivers/staging/tidspbridge/pmgr/dspapi.c
> +++ b/drivers/staging/tidspbridge/pmgr/dspapi.c
> @@ -340,7 +340,7 @@ int api_init_complete2(void)
> u32 mgrwrap_enum_node_info(union trapped_args *args, void *pr_ctxt)
> {
> u8 *pndb_props;
> - u32 num_nodes;
> + u32 num_nodes = 0;
> int status = 0;
> u32 size = args->args_mgr_enumnode_info.ndb_props_size;
>
This one is not a real bug. It is a false positive in cppcheck.
> @@ -372,7 +372,7 @@ u32 mgrwrap_enum_node_info(union trapped_args *args, void *pr_ctxt)
> u32 mgrwrap_enum_proc_info(union trapped_args *args, void *pr_ctxt)
> {
> u8 *processor_info;
> - u8 num_procs;
> + u8 num_procs = 0;
> int status = 0;
> u32 size = args->args_mgr_enumproc_info.processor_info_size;
>
Same thing.
> @@ -475,7 +475,7 @@ u32 mgrwrap_wait_for_bridge_events(union trapped_args *args, void *pr_ctxt)
> int status = 0;
> struct dsp_notification *anotifications[MAX_EVENTS];
> struct dsp_notification notifications[MAX_EVENTS];
> - u32 index, i;
> + u32 index = 0, i;
> u32 count = args->args_mgr_wait.count;
>
> if (count > MAX_EVENTS)
These all seem like false positives.
The code in tidspbridge is really garbage but if you follow it through
these are not real bugs.
I am not normally in favour of bogus initializations. Also I don't
normally think we should write code just to please a static checker.
In this case, what does it really *mean* to set num_procs to zero? Does
that help a human reader? It's not clear to me.
In the end, the better thing to do would be to fix tidspbridge to not
be an unreadable mass of spaghetti code. Then human readers and static
checkers will both be able to understand what's going on.
> diff --git a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
> index 9d3044a..9f0b511 100644
> --- a/drivers/staging/tidspbridge/rmgr/node.c
> +++ b/drivers/staging/tidspbridge/rmgr/node.c
> @@ -2361,8 +2361,7 @@ static void delete_node(struct node_object *hnode,
> struct node_taskargs task_arg_obj;
> #ifdef DSP_DMM_DEBUG
> struct dmm_object *dmm_mgr;
> - struct proc_object *p_proc_object =
> - (struct proc_object *)hnode->processor;
> + struct proc_object *p_proc_object;
> #endif
> int status;
> if (!hnode)
> @@ -2431,6 +2430,7 @@ static void delete_node(struct node_object *hnode,
> dsp_heap_res_addr,
> pr_ctxt);
> #ifdef DSP_DMM_DEBUG
> + p_proc_object = (struct proc_object *)hnode->processor;
> status = dmm_get_handle(p_proc_object, &dmm_mgr);
> if (dmm_mgr)
> dmm_mem_map_dump(dmm_mgr);
The patch description for this one should say something like
"dereferencing hnode before checking for NULL is bogus.". But actually
the check for NULL is not needed so probably the correct fix is to
remove the check. There are many bogus and inconsistent NULL checks in
this driver.
> @@ -2940,8 +2940,6 @@ static u32 ovly(void *priv_ref, u32 dsp_run_addr, u32 dsp_load_addr,
> struct node_object *hnode = (struct node_object *)priv_ref;
> struct node_mgr *hnode_mgr;
> u32 ul_bytes = 0;
> - u32 ul_size;
> - u32 ul_timeout;
> int status = 0;
> struct bridge_dev_context *hbridge_context;
> /* Function interface to Bridge driver*/
> @@ -2949,9 +2947,6 @@ static u32 ovly(void *priv_ref, u32 dsp_run_addr, u32 dsp_load_addr,
>
> hnode_mgr = hnode->node_mgr;
>
> - ul_size = ul_num_bytes / hnode_mgr->dsp_word_size;
> - ul_timeout = hnode->timeout;
> -
> /* Call new MemCopy function */
> intf_fxns = hnode_mgr->intf_fxns;
> status = dev_get_bridge_context(hnode_mgr->dev_obj, &hbridge_context);
Unused variables. This one is good. Btw, these should all be in
separate patches with a description and a quote of the cppcheck warning
message:
[patch 1/4] staging: tidspbridge: remove bogus NULL check
[patch 2/4] staging: tidspbridge: delete unused variables
etc.
> @@ -2982,21 +2977,18 @@ static u32 mem_write(void *priv_ref, u32 dsp_add, void *pbuf,
> struct node_object *hnode = (struct node_object *)priv_ref;
> struct node_mgr *hnode_mgr;
> u16 mem_sect_type;
> - u32 ul_timeout;
> - int status = 0;
> struct bridge_dev_context *hbridge_context;
> /* Function interface to Bridge driver */
> struct bridge_drv_interface *intf_fxns;
>
> hnode_mgr = hnode->node_mgr;
>
> - ul_timeout = hnode->timeout;
> mem_sect_type = (mem_space & DBLL_CODE) ? RMS_CODE : RMS_DATA;
>
> /* Call new MemWrite function */
> intf_fxns = hnode_mgr->intf_fxns;
> - status = dev_get_bridge_context(hnode_mgr->dev_obj, &hbridge_context);
> - status = (*intf_fxns->brd_mem_write) (hbridge_context, pbuf,
> + dev_get_bridge_context(hnode_mgr->dev_obj, &hbridge_context);
> + (*intf_fxns->brd_mem_write) (hbridge_context, pbuf,
> dsp_add, ul_num_bytes, mem_sect_type);
I feel like the code is buggy here. We should take "status" into
consideration. But I haven't looked at the context.
>
> return ul_num_bytes;
> --
> 1.7.10.4
>
> drivers/staging/tidspbridge/pmgr/dspapi.c : 1787] : (error) Uninitialized variable : mask
> drivers/staging/tidspbridge/pmgr/dspapi.c : 363] : (error) Uninitialized variable : num_nodes
> drivers/staging/tidspbridge/pmgr/dspapi.c : 396] : (error) Uninitialized variable : num_procs
> drivers/staging/tidspbridge/pmgr/dspapi.c : 503] : (error) Uninitialized variable : index
> drivers/staging/tidspbridge/rmgr/node.c : 2952] : (style) Variable 'ul_size' is assigned a value that is never used.
> drivers/staging/tidspbridge/rmgr/node.c : 2953] : (style) Variable 'ul_timeout' is assigned a value that is never used.
> drivers/staging/tidspbridge/rmgr/node.c : 2993] : (style) Variable 'ul_timeout' is assigned a value that is never used.
> drivers/staging/tidspbridge/rmgr/node.c : 2999] : (style) Variable 'status' is assigned a value that is never used.
> drivers/staging/tidspbridge/rmgr/node.c : 2365] -> [drivers/staging/tidspbridge/rmgr/node.c : 2368] : (warning) Possible null pointer dereference : hnode - otherwise it is redundant to check it against null.
These belong in the patch descriptions.
regards,
dan carpenter
More information about the devel
mailing list