iio: tsl4531: fix error handling in tsl4531_check_id()

PATCH Superseded Dan Carpenter 4 participants 9 messages Aug 13, 2015 – Aug 31, 2015

Subsystems (2)

Kernel janitors IIO

lore

Share & tools

lore

Dan Carpenter

author

2015-08-13 20:21 UTC #0 lore raw

PATCH 1/1 iio: tsl4531: fix error handling in tsl4531_check_id()

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 <[email protected]>

drivers/iio/light/tsl4531.c +1 -1

index 2697918..730fd2b 100644
@@ -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:

Dan Murphy

2015-08-13 21:03 UTC #1 lore raw

Dan

Quoted 10 lines · 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 <[email protected]>
drivers/iio/light/tsl4531.c +0 -0
index 2697918..730fd2b 100644

drivers/iio/light/tsl4531.c static int tsl4531_check_id(struct i2c_client *client) · line 157

Quoted 8 lines

drivers/iio/light/tsl4531.c +1 -1

@@ -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;
    }

    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

Signature · 2 lines

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

Jonathan Cameron

maintainer

2015-08-15 14:36 UTC #2 lore raw

Quoted 35 lines · Dan Murphy

Dan
Quoted 10 lines · 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 <[email protected]>
drivers/iio/light/tsl4531.c +0 -0
index 2697918..730fd2b 100644
drivers/iio/light/tsl4531.c static int tsl4531_check_id(struct i2c_client *client) · line 157
Quoted 8 lines
drivers/iio/light/tsl4531.c +1 -1
@@ -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

Quoted 9 lines

    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

Jonathan Cameron

maintainer

2015-08-15 14:35 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 4 lines · 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 <[email protected]>

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

Quoted 19 lines

drivers/iio/light/tsl4531.c +1 -2
index 2697918..730fd2b 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2015-08-17 14:38 UTC #4 lore raw

PATCH v2 1/1 iio: tsl4531: fix error handling in tsl4531_check_id()

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 <[email protected]>

---
v2: slightly different fix

drivers/iio/light/tsl4531.c +4 -1

index 2697918..fca0cb0 100644
@@ -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

Acked-by on v2 1/1

2015-08-17 14:51 UTC #5 lore raw

Quoted 5 lines · 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 <[email protected]>

Acked-by Peter Meerwald <[email protected]>

Quoted 20 lines

---
v2: slightly different fix

drivers/iio/light/tsl4531.c +4 -1

index 2697918..fca0cb0 100644
@@ -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;
 	}

Signature · 2 lines

Peter Meerwald
+43-664-2444418 (mobile)

Dan Murphy

2015-08-17 15:05 UTC #6 lore raw

In reply to Dan Carpenter

Dan

Quoted 14 lines · 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 <[email protected]>
---
v2: slightly different fix
drivers/iio/light/tsl4531.c +0 -0
index 2697918..fca0cb0 100644
@@ -180,7 +180,10 @@ static int tsl4531_probe(struct i2c_client *client,
 	data->client = client;

drivers/iio/light/tsl4531.c static int tsl4531_probe(struct i2c_client *client, · line 189

Quoted 10 lines

drivers/iio/light/tsl4531.c +4 -1

 	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

Signature · 2 lines

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

Dan Carpenter

author

2015-08-18 09:16 UTC #7 lore raw

PATCH v3 1/1 iio: tsl4531: fix error handling in tsl4531_check_id()

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 <[email protected]>

---
v3: slightly different fix again
v2: different fix

drivers/iio/light/tsl4531.c +5 -4

index 2697918..cf94ec7 100644
@@ -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,

Jonathan Cameron

maintainer Applied

2015-08-31 15:30 UTC #8 lore raw

Quoted 9 lines · 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 <[email protected]>

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

Quoted 34 lines

---
v3: slightly different fix again
v2: different fix

drivers/iio/light/tsl4531.c +5 -4

index 2697918..cf94ec7 100644
@@ -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,