smatch report: cx231xx: incorrect check in cx231xx_write_i2c_data()
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
Quoted 7 lines · Dan Carpenter
Hi,
I was doing an audit and I came across this.
drivers/media/video/cx231xx/cx231xx-core.c+1644cx231xx_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
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 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".
drivers/media/video/cx231xx/cx231xx-core.c +4 -4
@@ -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 */
RE: [patch] [media] cx231xxx: fix typo in saddr_len check
I am ok with the changes.
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".
drivers/media/video/cx231xx/cx231xx-core.c +4 -4
@@ -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."
**********************************************************************
---------------------------------------------------------------------