Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function

[Dan Carpenter]

On Tue, Mar 20, 2018 at 02:05:49PM +0530, Pratik Jain wrote:
> Refactored the function `XGIfb_search_refresh_rate` by removing a level
> of `if...else` block nesting. Removed unnecessary parantheses.
> 
> Signed-off-by: Pratik Jain <pratik.jain0509@xxxxxxxxx>
> ---
>  drivers/staging/xgifb/XGI_main_26.c | 63 +++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
> index 10107de0119a..ef9a726cd35d 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -544,41 +544,44 @@ static u8 XGIfb_search_refresh_rate(struct xgifb_video_info *xgifb_info,
>  	yres = XGIbios_mode[xgifb_info->mode_idx].yres;
>  
>  	xgifb_info->rate_idx = 0;
> -	while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
> -		if ((XGIfb_vrate[i].xres == xres) &&
> -		    (XGIfb_vrate[i].yres == yres)) {
> -			if (XGIfb_vrate[i].refresh == rate) {
> +	

There is a stray tab here.  You didn't run checkpatch.pl.

> +	// Skip values with less xres

Linus likes this comment style, but I would prefer normal comments,
please.

> +	while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres < xres)
> +		++i;
> +

I have reviewed the code, and I still find the single loop more
readable.

> +	while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
> +		if (XGIfb_vrate[i].yres != yres) {
> +			++i;
> +			continue;
> +		}

I would like a change that did:

		if ((XGIfb_vrate[i].xres != xres) ||
		    (XGIfb_vrate[i].yres != yres)) {
			i++;
			continue;
		}

so we could pull everything in one tab.

> +		if (XGIfb_vrate[i].refresh == rate) {
> +			xgifb_info->rate_idx = XGIfb_vrate[i].idx;
> +			break;
> +		} else if (XGIfb_vrate[i].refresh > rate) {
> +			if (XGIfb_vrate[i].refresh - rate <= 3) {
> +				pr_debug("Adjusting rate from %d up to %d\n",
> +					rate, XGIfb_vrate[i].refresh);
>  				xgifb_info->rate_idx = XGIfb_vrate[i].idx;
> -				break;
> -			} else if (XGIfb_vrate[i].refresh > rate) {
> -				if ((XGIfb_vrate[i].refresh - rate) <= 3) {
> -					pr_debug("Adjusting rate from %d up to %d\n",
> -						 rate, XGIfb_vrate[i].refresh);
> -					xgifb_info->rate_idx =
> -						XGIfb_vrate[i].idx;
> -					xgifb_info->refresh_rate =
> -						XGIfb_vrate[i].refresh;
> -				} else if (((rate - XGIfb_vrate[i - 1].refresh)
> -						<= 2) && (XGIfb_vrate[i].idx
> -						!= 1)) {
> -					pr_debug("Adjusting rate from %d down to %d\n",
> -						 rate,
> -						 XGIfb_vrate[i - 1].refresh);
> -					xgifb_info->rate_idx =
> -						XGIfb_vrate[i - 1].idx;
> -					xgifb_info->refresh_rate =
> -						XGIfb_vrate[i - 1].refresh;
> -				}
> -				break;
> -			} else if ((rate - XGIfb_vrate[i].refresh) <= 2) {
> +				xgifb_info->refresh_rate =
> +					XGIfb_vrate[i].refresh;
> +			} else if ((rate - XGIfb_vrate[i - 1].refresh <= 2)
> +					&& (XGIfb_vrate[i].idx != 1)) {
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This bug is there in the original code, and not something that you
introduced but the second part of the if condition is to ensure that
we didn't do an array underflow in the first part of the if statement.

These days that can trigger a kasan warning, I believe.

The conditions should be swapped around to avoid the read before the
start of the array altogether.  And, in fact, it should be written like
this to make it easier for static analysis tools:

			} else if (i != 0 &&
				   rate - XGIfb_vrate[i - 1].refresh <= 2) {

regards,
dan carpenter