From: Dan Carpenter <dan.carpenter@oracle.com>
To: Pratik Jain <pratik.jain0509@gmail.com>
Cc: devel@driverdev.osuosl.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, arnaud.patard@rtp-net.org
Subject: Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
Date: Tue, 20 Mar 2018 15:03:44 +0300 [thread overview]
Message-ID: <20180320120344.fyhw4iry4xrtxjpu@mwanda> (raw)
In-Reply-To: <20180320083549.6795-1-pratik.jain0509@gmail.com>
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@gmail.com>
> ---
> 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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2018-03-20 12:03 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-20 8:35 [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function Pratik Jain
2018-03-20 11:20 ` Dan Carpenter
2018-03-20 11:41 ` Pratik Jain
2018-03-20 11:54 ` Pratik Jain
2018-03-20 12:03 ` Dan Carpenter [this message]
2018-03-20 14:02 ` Pratik Jain
2018-03-21 9:24 ` Dan Carpenter
2018-03-21 9:50 ` Pratik Jain
2018-03-21 10:05 ` Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2018-03-19 15:56 Pratik Jain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180320120344.fyhw4iry4xrtxjpu@mwanda \
--to=dan.carpenter@oracle.com \
--cc=arnaud.patard@rtp-net.org \
--cc=devel@driverdev.osuosl.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pratik.jain0509@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox