linux-kernel - Re: [Outreachy kernel] [PATCH v4] staging: xgifb: correct the multiple line dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.20.1702261730350.2056@hadrien>
Date:   Sun, 26 Feb 2017 17:38:41 +0100 (CET)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     Arushi Singhal <arushisinghal19971997@...il.com>
cc:     arnaud.patard@...-net.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        outreachy-kernel@...glegroups.com
Subject: Re: [Outreachy kernel] [PATCH v4] staging: xgifb: correct the multiple
 line dereference

On Sun, 26 Feb 2017, Arushi Singhal wrote:

> Error was reported by checkpatch.pl as "Avoid multiple line
> dereference".And If there is boolean operator then it is
> fixed by Splitting line at boolean operator to satisfy coding
> style.

Please improve the commit message.  Capital letters should only appear at
the beginning of a sentence.  There should be a space after a period.

The current message also does not explain much about what you did.  The
key point in what you did would seem to be the addition of a new variable,
but that isn't mentioned at all.

>
> Signed-off-by: Arushi Singhal <arushisinghal19971997@...il.com>
> ---
> changes in v4
>   - changes done such that no other errors can generate.
>   - Improve the coding style.
>
>  drivers/staging/xgifb/XGI_main_26.c | 30 +++++++-----------------------
>  drivers/staging/xgifb/vb_setmode.c  | 14 +++++++-------
>  2 files changed, 14 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
> index 6930f7eb741b..825f7dcd2f49 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -878,30 +878,14 @@ static void XGIfb_post_setmode(struct xgifb_video_info *xgifb_info)
>  			}
>
>  			if ((filter >= 0) && (filter <= 7)) {
> +				const u8 *f = XGI_TV_filter[filter_tb].filter[filter];
> +
>  				pr_debug("FilterTable[%d]-%d: %*ph\n",
> -					 filter_tb, filter,
> -					 4, XGI_TV_filter[filter_tb].
> -						   filter[filter]);
> -				xgifb_reg_set(
> -					XGIPART2,
> -					0x35,
> -					(XGI_TV_filter[filter_tb].
> -						filter[filter][0]));
> -				xgifb_reg_set(
> -					XGIPART2,
> -					0x36,
> -					(XGI_TV_filter[filter_tb].
> -						filter[filter][1]));
> -				xgifb_reg_set(
> -					XGIPART2,
> -					0x37,
> -					(XGI_TV_filter[filter_tb].
> -						filter[filter][2]));
> -				xgifb_reg_set(
> -					XGIPART2,
> -					0x38,
> -					(XGI_TV_filter[filter_tb].
> -						filter[filter][3]));
> +					 filter_tb, filter, 4, f);
> +				xgifb_reg_set(XGIPART2, 0x35, f[0]);
> +			        xgifb_reg_set(XGIPART2, 0x36, f[1]);
> +			        xgifb_reg_set(XGIPART2, 0x37, f[2]);
> +				xgifb_reg_set(XGIPART2, 0x38, f[3]);

This part looks like it will now be much more readable.

>  			}
>  		}
>  	}
> diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
> index 7c7c8c8f1df3..1220d0cea87d 100644
> --- a/drivers/staging/xgifb/vb_setmode.c
> +++ b/drivers/staging/xgifb/vb_setmode.c
> @@ -221,8 +221,8 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
>
>  	for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
>  	       tempbx; (*i)--) {
> -		infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> -				Ext_InfoFlag;
> +		infoflag =
> +		XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;

Here the alignment is not very nice.  Maybe a new variable could also be
introduced.  But be careful with respect to the loop and the update on
(*i).

>  		if (infoflag & tempax)
>  			return 1;
>
> @@ -231,8 +231,8 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
>  	}
>
>  	for ((*i) = 0;; (*i)++) {
> -		infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> -				Ext_InfoFlag;
> +		infoflag =
> +		XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
>  		if (XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
>  				!= tempbx) {
>  			return 0;
> @@ -5092,8 +5092,8 @@ unsigned short XGI_GetRatePtrCRT2(struct xgi_hw_device_info *pXGIHWDE,
>
>  	i = 0;
>  	do {
> -		if (XGI330_RefIndex[RefreshRateTableIndex + i].
> -			ModeID != ModeNo)
> +		if (XGI330_RefIndex[RefreshRateTableIndex + i].ModeID
> +				!= ModeNo)

Can't you get != ModeNo int the line above?

>  			break;
>  		temp = XGI330_RefIndex[RefreshRateTableIndex + i].Ext_InfoFlag;
>  		temp &= ModeTypeMask;
> @@ -5484,7 +5484,7 @@ unsigned char XGISetModeNew(struct xgifb_video_info *xgifb_info,
>  				return 0;
>
>  		pVBInfo->ModeType = XGI330_EModeIDTable[ModeIdIndex].
> -						Ext_ModeFlag & ModeTypeMask;
> +				Ext_ModeFlag & ModeTypeMask;

This is not an improvement.  Try putting a newline after the =, and see if
the right hand side can all fit on one line that way.  The right hand side
of the assignment should be indented as compared to the left hand side,
though.

julia

>  		pVBInfo->SetFlag = 0;
>  		pVBInfo->VBInfo = DisableCRT2Display;
> --
> 2.11.0
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@...glegroups.com.
> To post to this group, send email to outreachy-kernel@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170226162633.GA20191%40arushi-HP-Pavilion-Notebook.
> For more options, visit https://groups.google.com/d/optout.
>