linux-kernel - Re: [PATCH v7] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1488280264.25838.26.camel@perches.com>
Date:   Tue, 28 Feb 2017 03:11:04 -0800
From:   Joe Perches <joe@...ches.com>
To:     Arushi Singhal <arushisinghal19971997@...il.com>,
        arnaud.patard@...-net.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        outreachy-kernel@...glegroups.com
Subject: Re: [PATCH v7] staging: xgifb: correct the multiple line dereference

On Tue, 2017-02-28 at 16:15 +0530, Arushi Singhal wrote:
> Error reported by checkpatch.pl as "avoid multiple line dereference".
> Addition of new variables to make the code more readable.

You should probably split this into 2 patches, one for
each file.

What Julia said about finding a better/shorter identifier
than RefreshRateTableIndex is true but I still suggest
something like:

> diff --git 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;
> +

I suggest something like:
---
 drivers/staging/xgifb/vb_setmode.c | 41 ++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
index 7c7c8c8f1df3..daa3318aab41 100644
--- a/drivers/staging/xgifb/vb_setmode.c
+++ b/drivers/staging/xgifb/vb_setmode.c
@@ -172,10 +172,12 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
 		struct vb_device_info *pVBInfo)
 {
 	unsigned short tempax, tempbx, resinfo, modeflag, infoflag;
+	const struct XGI_Ext2Struct *ext2;
 
 	modeflag = XGI330_EModeIDTable[ModeIdIndex].Ext_ModeFlag;
 	resinfo = XGI330_EModeIDTable[ModeIdIndex].Ext_RESINFO;
-	tempbx = XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID;
+	ext2 = &XGI330_RefIndex[RefreshRateTableIndex];
+	tempbx = ext2[*i].ModeID;
 	tempax = 0;
 
 	if (pVBInfo->VBInfo & SetCRT2ToRAMDAC) {
@@ -219,10 +221,8 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
 			return 0;
 	}
 
-	for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
-	       tempbx; (*i)--) {
-		infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
-				Ext_InfoFlag;
+	for (; ext2[*i].ModeID == tempbx; (*i)--) {
+		infoflag = ext2[*i].Ext_InfoFlag;
 		if (infoflag & tempax)
 			return 1;
 
@@ -231,12 +231,9 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short ModeIdIndex,
 	}
 
 	for ((*i) = 0;; (*i)++) {
-		infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
-				Ext_InfoFlag;
-		if (XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
-				!= tempbx) {
+		infoflag = ext2[*i].Ext_InfoFlag;
+		if (ext2[*i].ModeID != tempbx)
 			return 0;
-		}
 
 		if (infoflag & tempax)
 			return 1;
@@ -5050,6 +5047,7 @@ unsigned short XGI_GetRatePtrCRT2(struct xgi_hw_device_info *pXGIHWDE,
 {
 	const u8 LCDARefreshIndex[] = {
 		0x00, 0x00, 0x03, 0x01, 0x01, 0x01, 0x01, 0x00 };
+	const struct XGI_Ext2Struct *ext2;
 
 	unsigned short RefreshRateTableIndex, i, index, temp;
 
@@ -5073,29 +5071,29 @@ unsigned short XGI_GetRatePtrCRT2(struct xgi_hw_device_info *pXGIHWDE,
 	}
 
 	RefreshRateTableIndex = XGI330_EModeIDTable[ModeIdIndex].REFindex;
-	ModeNo = XGI330_RefIndex[RefreshRateTableIndex].ModeID;
+	ext2 = &XGI330_RefIndex[RefreshRateTableIndex];
+	ModeNo = ext2->ModeID;
 	if (pXGIHWDE->jChipType >= XG20) { /* for XG20, XG21, XG27 */
-		if ((XGI330_RefIndex[RefreshRateTableIndex].XRes == 800) &&
-		    (XGI330_RefIndex[RefreshRateTableIndex].YRes == 600)) {
+		if ((ext2->XRes == 800) &&
+		    (ext2->YRes == 600)) {
 			index++;
 		}
 		/* do the similar adjustment like XGISearchCRT1Rate() */
-		if ((XGI330_RefIndex[RefreshRateTableIndex].XRes == 1024) &&
-		    (XGI330_RefIndex[RefreshRateTableIndex].YRes == 768)) {
+		if ((ext2->XRes == 1024) &&
+		    (ext2->YRes == 768)) {
 			index++;
 		}
-		if ((XGI330_RefIndex[RefreshRateTableIndex].XRes == 1280) &&
-		    (XGI330_RefIndex[RefreshRateTableIndex].YRes == 1024)) {
+		if ((ext2->XRes == 1280) &&
+		    (ext2->YRes == 1024)) {
 			index++;
 		}
 	}
 
 	i = 0;
 	do {
-		if (XGI330_RefIndex[RefreshRateTableIndex + i].
-			ModeID != ModeNo)
+		if (ext2[i].ModeID != ModeNo)
 			break;
-		temp = XGI330_RefIndex[RefreshRateTableIndex + i].Ext_InfoFlag;
+		temp = ext2[i].Ext_InfoFlag;
 		temp &= ModeTypeMask;
 		if (temp < pVBInfo->ModeType)
 			break;
@@ -5105,8 +5103,7 @@ unsigned short XGI_GetRatePtrCRT2(struct xgi_hw_device_info *pXGIHWDE,
 	} while (index != 0xFFFF);
 	if (!(pVBInfo->VBInfo & SetCRT2ToRAMDAC)) {
 		if (pVBInfo->VBInfo & SetInSlaveMode) {
-			temp = XGI330_RefIndex[RefreshRateTableIndex + i - 1].
-				Ext_InfoFlag;
+			temp = ext2[i - 1].Ext_InfoFlag;
 			if (temp & InterlaceMode)
 				i++;
 		}