media: staging/intel-ipu3: css - possible typo in array being assigned

media: staging/intel-ipu3: css - possible typo in array being assigned

[Colin King (gmail)]

[-- Attachment #1.1.1: Type: text/plain, Size: 2015 bytes --]

Hi,

I believe there maybe a typo in the name of an array being assigned, 
introduced in commit:

commit e11110a5b74487cfc99dad8a5744fe26782f5d9c
Author: Yong Zhi <yong.zhi@intel.com>
Date:   Thu Dec 6 20:03:33 2018 -0500

     media: staging/intel-ipu3: css: Compute and program ccs


The issue is as follows in function imgu_css_acc_process_lines in 
drivers/staging/media/ipu3/ipu3-css-params.c

                         if (pl_idx == 0)
                                 /* First process line */
                                 p_pl[pl_idx].lines = first_process_lines;
                         else if (pl_idx == num_of_sets - 1)
                                 /* Last in grid */
                                 p_pl[pl_idx].lines = 
last_process_lines_in_grid;
                         else if (pl_idx == num_of_process_lines - 1)
                                 /* After the grid */
                                 p_pl[pl_idx].lines = 
process_lines_after_grid;
                         else
                                 /* Inside the grid */
                                 p_pl[pl_idx].lines = process_lines;

                         if (p_tr) {
                                 p_pl[pl_idx].cfg_set = pl_cfg_set;
                                 pl_cfg_set = 1 - pl_cfg_set;
                         }
                         pl_idx++;


The non-null check 'if (p_tr)' seem suspect, should it be 'if (p_pl)' 
instead since p_pl is being assigned (this maybe a cut-n-paste issue 
from a previous hunk of code that does:

                         op_idx++;
                         if (p_tr) {
                                 p_tr[tr_idx].set_number = tr_set_num;
                                 tr_set_num = 1 - tr_set_num;
                         }
                         tr_idx++;

Also, if it is meant to be a check on p_pl then surely the assignments 
to p_pl[] also need a non-null check in the cascaded if/else checks too.

Colin

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 4901 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: media: staging/intel-ipu3: css - possible typo in array being assigned

[Dan Carpenter]

Yeah...  It probably should be the below patch.  The commit message would
say something like:

Subject: Fix copy and paste bug.

This code is copy and pasted from earlier in the function and the NULL
check should be if (p_pl) instead of if (p_tr).  Except that p_pl is
never NULL and the check can be removed.

The impact of this bug is that for several callers the
"p_pl[pl_idx].cfg_set = pl_cfg_set" value is never set.  (I wonder why
this didn't show up in testing?)"

regards,
dan carpenter

diff --git a/drivers/staging/media/ipu3/ipu3-css-params.c b/drivers/staging/media/ipu3/ipu3-css-params.c
index 2c48d57a3180..2d1c38119258 100644
--- a/drivers/staging/media/ipu3/ipu3-css-params.c
+++ b/drivers/staging/media/ipu3/ipu3-css-params.c
@@ -1617,10 +1617,9 @@ imgu_css_acc_process_lines(const struct process_lines *pl,
 				/* Inside the grid */
 				p_pl[pl_idx].lines = process_lines;
 
-			if (p_tr) {
-				p_pl[pl_idx].cfg_set = pl_cfg_set;
-				pl_cfg_set = 1 - pl_cfg_set;
-			}
+			p_pl[pl_idx].cfg_set = pl_cfg_set;
+			pl_cfg_set = 1 - pl_cfg_set;
+
 			pl_idx++;
 		}
 	}