[RESEND PATCH] media: staging: atomisp: rework reading the id and revision values

[RESEND PATCH] media: staging: atomisp: rework reading the id and revision values

[trix]

From: Tom Rix <trix@redhat.com>

Clang static analysis reports this representative issue
atomisp-ov2722.c:920:3: warning: 3rd function call
  argument is an uninitialized value
  dev_err(&client->dev, "sensor_id_high = 0x%x\n", high);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

high and low are only set when ov2722_read_reg() is successful.
Reporting the high value when there is an error is not
meaningful.  The later read for low is not checked.  high
and low are or-ed together and checked against a non zero
value.

Remove the unneeded error reporting for high.  Initialize
high and low to 0 and use the id check to determine if
the reads were successful

The later read for revision is not checked.  If it
fails the old high value will be used and the revision
will be misreported.

Since the revision is only reported and not checked or
stored it is not necessary to return if the read with
successful.  This makes the ret variable unnecessary
so remove it.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 .../media/atomisp/i2c/atomisp-ov2722.c        | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
index da98094d7094..d5d099ac1b70 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
@@ -906,22 +906,17 @@ static int ov2722_get_fmt(struct v4l2_subdev *sd,
 static int ov2722_detect(struct i2c_client *client)
 {
 	struct i2c_adapter *adapter = client->adapter;
-	u16 high, low;
-	int ret;
+	u16 high = 0, low = 0;
 	u16 id;
 	u8 revision;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
 		return -ENODEV;
 
-	ret = ov2722_read_reg(client, OV2722_8BIT,
-			      OV2722_SC_CMMN_CHIP_ID_H, &high);
-	if (ret) {
-		dev_err(&client->dev, "sensor_id_high = 0x%x\n", high);
-		return -ENODEV;
-	}
-	ret = ov2722_read_reg(client, OV2722_8BIT,
-			      OV2722_SC_CMMN_CHIP_ID_L, &low);
+	ov2722_read_reg(client, OV2722_8BIT,
+			OV2722_SC_CMMN_CHIP_ID_H, &high);
+	ov2722_read_reg(client, OV2722_8BIT,
+			OV2722_SC_CMMN_CHIP_ID_L, &low);
 	id = (high << 8) | low;
 
 	if ((id != OV2722_ID) && (id != OV2720_ID)) {
@@ -929,8 +924,9 @@ static int ov2722_detect(struct i2c_client *client)
 		return -ENODEV;
 	}
 
-	ret = ov2722_read_reg(client, OV2722_8BIT,
-			      OV2722_SC_CMMN_SUB_ID, &high);
+	high = 0;
+	ov2722_read_reg(client, OV2722_8BIT,
+			OV2722_SC_CMMN_SUB_ID, &high);
 	revision = (u8)high & 0x0f;
 
 	dev_dbg(&client->dev, "sensor_revision = 0x%x\n", revision);
-- 
2.26.3

Re: [RESEND PATCH] media: staging: atomisp: rework reading the id and revision values

[Sakari Ailus]

Hi Tom,

On Tue, Mar 29, 2022 at 04:25:26PM -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> Clang static analysis reports this representative issue
> atomisp-ov2722.c:920:3: warning: 3rd function call
>   argument is an uninitialized value
>   dev_err(&client->dev, "sensor_id_high = 0x%x\n", high);
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> high and low are only set when ov2722_read_reg() is successful.
> Reporting the high value when there is an error is not
> meaningful.  The later read for low is not checked.  high
> and low are or-ed together and checked against a non zero
> value.
> 
> Remove the unneeded error reporting for high.  Initialize
> high and low to 0 and use the id check to determine if
> the reads were successful
> 
> The later read for revision is not checked.  If it
> fails the old high value will be used and the revision
> will be misreported.
> 
> Since the revision is only reported and not checked or
> stored it is not necessary to return if the read with
> successful.  This makes the ret variable unnecessary
> so remove it.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  .../media/atomisp/i2c/atomisp-ov2722.c        | 20 ++++++++-----------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> index da98094d7094..d5d099ac1b70 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> @@ -906,22 +906,17 @@ static int ov2722_get_fmt(struct v4l2_subdev *sd,
>  static int ov2722_detect(struct i2c_client *client)
>  {
>  	struct i2c_adapter *adapter = client->adapter;
> -	u16 high, low;
> -	int ret;
> +	u16 high = 0, low = 0;
>  	u16 id;
>  	u8 revision;
>  
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
>  		return -ENODEV;
>  
> -	ret = ov2722_read_reg(client, OV2722_8BIT,
> -			      OV2722_SC_CMMN_CHIP_ID_H, &high);
> -	if (ret) {
> -		dev_err(&client->dev, "sensor_id_high = 0x%x\n", high);
> -		return -ENODEV;
> -	}
> -	ret = ov2722_read_reg(client, OV2722_8BIT,
> -			      OV2722_SC_CMMN_CHIP_ID_L, &low);
> +	ov2722_read_reg(client, OV2722_8BIT,
> +			OV2722_SC_CMMN_CHIP_ID_H, &high);
> +	ov2722_read_reg(client, OV2722_8BIT,
> +			OV2722_SC_CMMN_CHIP_ID_L, &low);

You should check the return value as it's an entirely different problem not
being able to access the device than reading back wrong identification
information.

>  	id = (high << 8) | low;
>  
>  	if ((id != OV2722_ID) && (id != OV2720_ID)) {
> @@ -929,8 +924,9 @@ static int ov2722_detect(struct i2c_client *client)
>  		return -ENODEV;
>  	}
>  
> -	ret = ov2722_read_reg(client, OV2722_8BIT,
> -			      OV2722_SC_CMMN_SUB_ID, &high);
> +	high = 0;
> +	ov2722_read_reg(client, OV2722_8BIT,
> +			OV2722_SC_CMMN_SUB_ID, &high);
>  	revision = (u8)high & 0x0f;
>  
>  	dev_dbg(&client->dev, "sensor_revision = 0x%x\n", revision);

This will also mean failure to read the revision is reported as revision 0.

These were the problems that pre-existed the patch, so if you're willing to
address these, please send v2 (would be nice). I can also merge this.

-- 
Kind regards,

Sakari Ailus