Skip to contentSkip to conversation

smatch report: cx231xx: incorrect check in cx231xx_write_i2c_data()

PATCH In review Dan Carpenter 3 participants 5 messages Dec 23, 2010 1 message not in the archive Media (V4L/DVB)

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hi,

I was doing an audit and I came across this.

drivers/media/video/cx231xx/cx231xx-core.c +1644 cx231xx_write_i2c_data(14)
warn: 'saddr_len' is non-zero. Did you mean 'saddr'

  1642          if (saddr_len == 0)
  1643                  saddr = 0;
  1644          else if (saddr_len == 0)
  1645                  saddr &= 0xff;

We check "saddr_len == 0" twice which doesn't make sense. I'm not sure what the correct fix is though. It's been this way since the driver was merged.

regards,
dan carpenter

#1 lore raw
Quoted 7 lines · Dan Carpenter

Hi,

I was doing an audit and I came across this.

drivers/media/video/cx231xx/cx231xx-core.c +1644 cx231xx_write_i2c_data(14)
warn: 'saddr_len' is non-zero. Did you mean 'saddr'

Quoted 7 lines
  1642          if (saddr_len == 0)
  1643                  saddr = 0;
  1644          else if (saddr_len == 0)
  1645                  saddr &= 0xff;

We check "saddr_len == 0" twice which doesn't make sense. I'm not sure what the correct fix is though.

Given that "saddr" probably means "sub-address", that "saddr_len" probably means "sub-address length", that saddr is only a 16 bit value, and this switch in cx231xx_send_usb_command():

        /* set index value */
        switch (saddr_len) {
        case 0:
                ven_req.wIndex = 0;     /* need check */
                break;
        case 1:
                ven_req.wIndex = (req_data->saddr_dat & 0xff);
                break;
        case 2:
                ven_req.wIndex = req_data->saddr_dat;
                break;
        }

and noting that "req_data->saddr_dat" holds what was "saddr"; the if statement, and the many others like it, should probably be:

	if (saddr_len == 0)
		saddr = 0;
	else if (saddr_len == 1)       <----- == 1
		saddr &= 0xff;

Regards,
Andy

Quoted 5 lines

It's been this way since the driver was
merged.

regards,
dan carpenter

author
#2 lore raw
Quoted 2 lines · Andy Walls

and noting that "req_data->saddr_dat" holds what was "saddr"; the if statement, and the many others like it, [...]

The others I see are:
cx231xx_write_i2c_data()
cx231xx_read_i2c_data()
cx231xx_write_i2c_master()
cx231xx_read_i2c_master()

regards,
dan carpenter

author
#4 lore raw

PATCH 1/1 cx231xxx: fix typo in saddr_len check

In reply to a message not in the archive

The original code compared "saddr_len" with zero twice in a nonsensical way. I asked the list, and Andy Walls and Sri Deevi say that the second check should be if "saddr_len == 1".

Signed-off-by Dan Carpenter <[email protected]>
drivers/media/video/cx231xx/cx231xx-core.c +4 -4
index 44d124c..7d62d58 100644
@@ -1515,7 +1515,7 @@ int cx231xx_read_i2c_master(struct cx231xx *dev, u8 dev_addr, u16 saddr,
 
 	if (saddr_len == 0)
 		saddr = 0;
-	else if (saddr_len == 0)
+	else if (saddr_len == 1)
 		saddr &= 0xff;
 
 	/* prepare xfer_data struct */

@@ -1566,7 +1566,7 @@ int cx231xx_write_i2c_master(struct cx231xx *dev, u8 dev_addr, u16 saddr,
 
 	if (saddr_len == 0)
 		saddr = 0;
-	else if (saddr_len == 0)
+	else if (saddr_len == 1)
 		saddr &= 0xff;
 
 	/* prepare xfer_data struct */

@@ -1600,7 +1600,7 @@ int cx231xx_read_i2c_data(struct cx231xx *dev, u8 dev_addr, u16 saddr,
 
 	if (saddr_len == 0)
 		saddr = 0;
-	else if (saddr_len == 0)
+	else if (saddr_len == 1)
 		saddr &= 0xff;
 
 	/* prepare xfer_data struct */

@@ -1641,7 +1641,7 @@ int cx231xx_write_i2c_data(struct cx231xx *dev, u8 dev_addr, u16 saddr,
 
 	if (saddr_len == 0)
 		saddr = 0;
-	else if (saddr_len == 0)
+	else if (saddr_len == 1)
 		saddr &= 0xff;
 
 	/* prepare xfer_data struct */
#5 lore raw

RE: [patch] [media] cx231xxx: fix typo in saddr_len check

I am ok with the changes.

Signed-off-by Srinivasa Deevi <[email protected]>

Sri

-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Thursday, December 23, 2010 11:39 AM
To: Sri Deevi
Cc: 'Andy Walls'; [email protected]; [email protected]
Subject: [patch] [media] cx231xxx: fix typo in saddr_len check

The original code compared "saddr_len" with zero twice in a nonsensical way. I asked the list, and Andy Walls and Sri Deevi say that the second check should be if "saddr_len == 1".

Signed-off-by Dan Carpenter <[email protected]>
drivers/media/video/cx231xx/cx231xx-core.c +4 -4
index 44d124c..7d62d58 100644
@@ -1515,7 +1515,7 @@ int cx231xx_read_i2c_master(struct cx231xx *dev, u8 dev_addr, u16 saddr,
 
 	if (saddr_len == 0)
 		saddr = 0;
-	else if (saddr_len == 0)
+	else if (saddr_len == 1)
 		saddr &= 0xff;
 
 	/* prepare xfer_data struct */

@@ -1566,7 +1566,7 @@ int cx231xx_write_i2c_master(struct cx231xx *dev, u8 dev_addr, u16 saddr,
 
 	if (saddr_len == 0)
 		saddr = 0;
-	else if (saddr_len == 0)
+	else if (saddr_len == 1)
 		saddr &= 0xff;
 
 	/* prepare xfer_data struct */

@@ -1600,7 +1600,7 @@ int cx231xx_read_i2c_data(struct cx231xx *dev, u8 dev_addr, u16 saddr,
 
 	if (saddr_len == 0)
 		saddr = 0;
-	else if (saddr_len == 0)
+	else if (saddr_len == 1)
 		saddr &= 0xff;
 
 	/* prepare xfer_data struct */

@@ -1641,7 +1641,7 @@ int cx231xx_write_i2c_data(struct cx231xx *dev, u8 dev_addr, u16 saddr,
 
 	if (saddr_len == 0)
 		saddr = 0;
-	else if (saddr_len == 0)
+	else if (saddr_len == 1)
 		saddr &= 0xff;
 
 	/* prepare xfer_data struct */

Conexant E-mail Firewall (Conexant.Com) made the following annotations
---------------------------------------------------------------------
********************** Legal Disclaimer ****************************

"This email may contain confidential and privileged material for the sole use of the intended recipient. Any unauthorized review, use or distribution by others is strictly prohibited. If you have received the message in error, please advise the sender by reply email and delete the message. Thank you."

**********************************************************************

---------------------------------------------------------------------