[patch] iio: tsl4531: fix error handling in tsl4531_check_id()

[patch] iio: tsl4531: fix error handling in tsl4531_check_id()

[Dan Carpenter]

The caller expect us to return zero if the ID doesn't match.  Negative
error codes are treated as success.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/iio/light/tsl4531.c b/drivers/iio/light/tsl4531.c
index 2697918..730fd2b 100644
--- a/drivers/iio/light/tsl4531.c
+++ b/drivers/iio/light/tsl4531.c
@@ -151,7 +151,7 @@ static int tsl4531_check_id(struct i2c_client *client)
 {
 	int ret = i2c_smbus_read_byte_data(client, TSL4531_ID);
 	if (ret < 0)
-		return ret;
+		return 0;
 
 	switch (ret >> TSL4531_ID_SHIFT) {
 	case TSL45311_ID:

Re: [patch] iio: tsl4531: fix error handling in tsl4531_check_id()

[Dan Murphy]

Dan

On 08/13/2015 03:21 PM, Dan Carpenter wrote:
> The caller expect us to return zero if the ID doesn't match.  Negative
> error codes are treated as success.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/iio/light/tsl4531.c b/drivers/iio/light/tsl4531.c
> index 2697918..730fd2b 100644
> --- a/drivers/iio/light/tsl4531.c
> +++ b/drivers/iio/light/tsl4531.c
> @@ -151,7 +151,7 @@ static int tsl4531_check_id(struct i2c_client *client)
>  {
>  	int ret = i2c_smbus_read_byte_data(client, TSL4531_ID);
>  	if (ret < 0)
> -		return ret;
> +		return 0;
>  
>  	switch (ret >> TSL4531_ID_SHIFT) {
>  	case TSL45311_ID:

Would this not be better to change the logic?

Zero return is generally true and non-zero is false.

Then this statement in the probe then can go from

    if (!tsl4531_check_id(client)) {
        dev_err(&client->dev, "no TSL4531 sensor\n");
        return -ENODEV;
    }

to

    if (tsl4531_check_id(client)) {
        dev_err(&client->dev, "no TSL4531 sensor\n");
        return -ENODEV;
    }

And if then return ret can be honored in probe.

Dan

-- 
------------------
Dan Murphy

Re: [patch] iio: tsl4531: fix error handling in tsl4531_check_id()

[Jonathan Cameron]

On 13/08/15 21:21, Dan Carpenter wrote:
> The caller expect us to return zero if the ID doesn't match.  Negative
> error codes are treated as success.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Whilst this does indeed fix the issue, I think I'd prefer changing the call
site to deal with the error properly. Could return -ENODEV directly
from the function to make the code cleaner.  Not quite the minimal fix
but only slightly worse and avoids reworking it again to pass the
error up correctly as it should do (after the fix is in place).

Jonathan
> 
> diff --git a/drivers/iio/light/tsl4531.c b/drivers/iio/light/tsl4531.c
> index 2697918..730fd2b 100644
> --- a/drivers/iio/light/tsl4531.c
> +++ b/drivers/iio/light/tsl4531.c
> @@ -151,7 +151,7 @@ static int tsl4531_check_id(struct i2c_client *client)
>  {
>  	int ret = i2c_smbus_read_byte_data(client, TSL4531_ID);
>  	if (ret < 0)
> -		return ret;
> +		return 0;
>  
>  	switch (ret >> TSL4531_ID_SHIFT) {
>  	case TSL45311_ID:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch] iio: tsl4531: fix error handling in tsl4531_check_id()

[Jonathan Cameron]

On 13/08/15 22:03, Dan Murphy wrote:
> Dan
> 
> On 08/13/2015 03:21 PM, Dan Carpenter wrote:
>> The caller expect us to return zero if the ID doesn't match.  Negative
>> error codes are treated as success.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/drivers/iio/light/tsl4531.c b/drivers/iio/light/tsl4531.c
>> index 2697918..730fd2b 100644
>> --- a/drivers/iio/light/tsl4531.c
>> +++ b/drivers/iio/light/tsl4531.c
>> @@ -151,7 +151,7 @@ static int tsl4531_check_id(struct i2c_client *client)
>>  {
>>  	int ret = i2c_smbus_read_byte_data(client, TSL4531_ID);
>>  	if (ret < 0)
>> -		return ret;
>> +		return 0;
>>  
>>  	switch (ret >> TSL4531_ID_SHIFT) {
>>  	case TSL45311_ID:
> 
> Would this not be better to change the logic?
> 
> Zero return is generally true and non-zero is false.
> 
> Then this statement in the probe then can go from
> 
>     if (!tsl4531_check_id(client)) {
>         dev_err(&client->dev, "no TSL4531 sensor\n");
>         return -ENODEV;
>     }
> 
> to
> 
>     if (tsl4531_check_id(client)) {
>         dev_err(&client->dev, "no TSL4531 sensor\n");
>         return -ENODEV;
>     }
> 
> And if then return ret can be honored in probe.
> 
> Dan
> 
Oops, missed this email.  I'd prefer to take it further and spit
out 0 for success as you suggest, but also do the error code
generation for no device in the check_id function, thus
allowing read failures to be passed up as well.

Jonathan

[patch v2] iio: tsl4531: fix error handling in tsl4531_check_id()

[Dan Carpenter]

tsl4531_check_id() returns 1 on "found", 0 on "not found" and negative
if there is an error.  The bug here is that the error handling, treats
errors as "found".

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: slightly different fix

diff --git a/drivers/iio/light/tsl4531.c b/drivers/iio/light/tsl4531.c
index 2697918..fca0cb0 100644
--- a/drivers/iio/light/tsl4531.c
+++ b/drivers/iio/light/tsl4531.c
@@ -180,7 +180,10 @@ static int tsl4531_probe(struct i2c_client *client,
 	data->client = client;
 	mutex_init(&data->lock);
 
-	if (!tsl4531_check_id(client)) {
+	ret = tsl4531_check_id(client);
+	if (ret < 0)
+		return ret;
+	if (ret == 0) {
 		dev_err(&client->dev, "no TSL4531 sensor\n");
 		return -ENODEV;
 	}

Re: [patch v2] iio: tsl4531: fix error handling in tsl4531_check_id()

[Peter Meerwald]

On Mon, 17 Aug 2015, Dan Carpenter wrote:

> tsl4531_check_id() returns 1 on "found", 0 on "not found" and negative
> if there is an error.  The bug here is that the error handling, treats
> errors as "found".
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Peter Meerwald <pmeerw@pmeerw.net>

> ---
> v2: slightly different fix
> 
> diff --git a/drivers/iio/light/tsl4531.c b/drivers/iio/light/tsl4531.c
> index 2697918..fca0cb0 100644
> --- a/drivers/iio/light/tsl4531.c
> +++ b/drivers/iio/light/tsl4531.c
> @@ -180,7 +180,10 @@ static int tsl4531_probe(struct i2c_client *client,
>  	data->client = client;
>  	mutex_init(&data->lock);
>  
> -	if (!tsl4531_check_id(client)) {
> +	ret = tsl4531_check_id(client);
> +	if (ret < 0)
> +		return ret;
> +	if (ret == 0) {
>  		dev_err(&client->dev, "no TSL4531 sensor\n");
>  		return -ENODEV;
>  	}
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [patch v2] iio: tsl4531: fix error handling in tsl4531_check_id()

[Dan Murphy]

Dan

On 08/17/2015 09:38 AM, Dan Carpenter wrote:
> tsl4531_check_id() returns 1 on "found", 0 on "not found" and negative
> if there is an error.  The bug here is that the error handling, treats
> errors as "found".
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: slightly different fix
>
> diff --git a/drivers/iio/light/tsl4531.c b/drivers/iio/light/tsl4531.c
> index 2697918..fca0cb0 100644
> --- a/drivers/iio/light/tsl4531.c
> +++ b/drivers/iio/light/tsl4531.c
> @@ -180,7 +180,10 @@ static int tsl4531_probe(struct i2c_client *client,
>  	data->client = client;
>  	mutex_init(&data->lock);
>  
> -	if (!tsl4531_check_id(client)) {
> +	ret = tsl4531_check_id(client);
> +	if (ret < 0)
> +		return ret;
> +	if (ret == 0) {
>  		dev_err(&client->dev, "no TSL4531 sensor\n");
>  		return -ENODEV;
>  	}

Almost but not quite.  The suggestion from Jon and myself was to return 0 on device found and
non-zero on an error or not found.

In the check_id call I would probably do something like

    switch (ret >> TSL4531_ID_SHIFT) {
    case TSL45311_ID:
    case TSL45313_ID:
    case TSL45315_ID:
    case TSL45317_ID:
        return 0;
    default:
        return -ENODEV;
    }

You could probably just drop the default case all together and just have the call return -ENODEV

Then in probe you just need to do

if (tsl4531_check_id(client)) {
    return ret;

This way you will return the exact error.

Dan

-- 
------------------
Dan Murphy

[patch v3] iio: tsl4531: fix error handling in tsl4531_check_id()

[Dan Carpenter]

The tsl4531_check_id() function returned 1 on "found" and 0 on "not
found" and negative error codes on failure.  This was non-standard and
bug prone.  The caller treated all non-zero values including error codes
as "found".

This patch fixes it by changing the tsl4531_check_id() to return zero on
success or a negative error code, and updates the caller.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v3: slightly different fix again
v2: different fix

diff --git a/drivers/iio/light/tsl4531.c b/drivers/iio/light/tsl4531.c
index 2697918..cf94ec7 100644
--- a/drivers/iio/light/tsl4531.c
+++ b/drivers/iio/light/tsl4531.c
@@ -158,9 +158,9 @@ static int tsl4531_check_id(struct i2c_client *client)
 	case TSL45313_ID:
 	case TSL45315_ID:
 	case TSL45317_ID:
-		return 1;
-	default:
 		return 0;
+	default:
+		return -ENODEV;
 	}
 }
 
@@ -180,9 +180,10 @@ static int tsl4531_probe(struct i2c_client *client,
 	data->client = client;
 	mutex_init(&data->lock);
 
-	if (!tsl4531_check_id(client)) {
+	ret = tsl4531_check_id(client);
+	if (ret) {
 		dev_err(&client->dev, "no TSL4531 sensor\n");
-		return -ENODEV;
+		return ret;
 	}
 
 	ret = i2c_smbus_write_byte_data(data->client, TSL4531_CONTROL,

Re: [patch v3] iio: tsl4531: fix error handling in tsl4531_check_id()

[Jonathan Cameron]

On 18/08/15 10:16, Dan Carpenter wrote:
> The tsl4531_check_id() function returned 1 on "found" and 0 on "not
> found" and negative error codes on failure.  This was non-standard and
> bug prone.  The caller treated all non-zero values including error codes
> as "found".
> 
> This patch fixes it by changing the tsl4531_check_id() to return zero on
> success or a negative error code, and updates the caller.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied to the fixes-togreg branch of iio.git.

Given it's just a slightly different approach to the v2 that
Peter acked, I pulled that ack in here as well.

Thanks,

Jonathan
> ---
> v3: slightly different fix again
> v2: different fix
> 
> diff --git a/drivers/iio/light/tsl4531.c b/drivers/iio/light/tsl4531.c
> index 2697918..cf94ec7 100644
> --- a/drivers/iio/light/tsl4531.c
> +++ b/drivers/iio/light/tsl4531.c
> @@ -158,9 +158,9 @@ static int tsl4531_check_id(struct i2c_client *client)
>  	case TSL45313_ID:
>  	case TSL45315_ID:
>  	case TSL45317_ID:
> -		return 1;
> -	default:
>  		return 0;
> +	default:
> +		return -ENODEV;
>  	}
>  }
>  
> @@ -180,9 +180,10 @@ static int tsl4531_probe(struct i2c_client *client,
>  	data->client = client;
>  	mutex_init(&data->lock);
>  
> -	if (!tsl4531_check_id(client)) {
> +	ret = tsl4531_check_id(client);
> +	if (ret) {
>  		dev_err(&client->dev, "no TSL4531 sensor\n");
> -		return -ENODEV;
> +		return ret;
>  	}
>  
>  	ret = i2c_smbus_write_byte_data(data->client, TSL4531_CONTROL,
>