hwmon: Add missing parentheses

I think this was intended? please review.

--------------------------->8-------------8<------------------------------ Add missing parentheses

Signed-off-by Roel Kluin <[email protected]>

---

drivers/hwmon/abituguru3.c +1 -1

index e52b388..fd98685 100644
@@ -761,7 +761,7 @@ static int abituguru3_read_increment_offset(struct
 abituguru3_data *data, for (i = 0; i < offset_count; i++)
 		if ((x = abituguru3_read(data, bank, offset + i, count,
 				buf + i * count)) != count)
-			return i * count + (i && (x < 0)) ? 0 : x;
+			return i * count + ((i && (x < 0)) ? 0 : x);
 
 	return i * count;
 }

Quoted 1 line · On Tuesday 17 February 2009 13:17:54 Roel Kluin wrote:

I think this was intended? please review.

This one's been there from the beginning.

The change looks right to me. (i * count) account the previous successes, add either zero or x depending on the kind of read failure (timeout/short read vs other I/O error). I'm not entirely sure why i must be >0 for accounting the short read.

Nothing calling abituguru3_read_increment_offset() checks for anything other than complete success. Every caller passes count=1, and offset_count>1, so both the new code and the old could never return a full count (offset_count*count) when an error has occured (old returns either 0 or x, x=count). Ergo, the change can't cause any regressions.

Acked-by Alistair John Strachan <[email protected]>

Jean, are you happy to take this?

Quoted 18 lines

--------------------------->8-------------8<------------------------------ Add missing parentheses

Signed-off-by Roel Kluin <[email protected]>

---

drivers/hwmon/abituguru3.c +1 -1

index e52b388..fd98685 100644
@@ -761,7 +761,7 @@ static int abituguru3_read_increment_offset(struct
 abituguru3_data *data, for (i = 0; i < offset_count; i++)
 		if ((x = abituguru3_read(data, bank, offset + i, count,
 				buf + i * count)) != count)
-			return i * count + (i && (x < 0)) ? 0 : x;
+			return i * count + ((i && (x < 0)) ? 0 : x);
 
 	return i * count;
 }

Quoted 8 lines · Alistair John Strachan

Quoted 1 line · On Tuesday 17 February 2009 13:17:54 Roel Kluin wrote:

I think this was intended? please review.

This one's been there from the beginning.

The change looks right to me. (i * count) account the previous successes, add either zero or x depending on the kind of read failure (timeout/short read vs other I/O error). I'm not entirely sure why i must be >0 for accounting the short read.

I can't make any sense of that "i &&" either. Hans?

Quoted 9 lines

Nothing calling abituguru3_read_increment_offset() checks for anything other than complete success. Every caller passes count=1, and offset_count>1, so both the new code and the old could never return a full count (offset_count*count) when an error has occured (old returns either 0 or x, x=count). Ergo, the change can't cause any regressions.

Acked-by Alistair John Strachan <[email protected]>

Jean, are you happy to take this?

Yes, but I'd like to understand why the "i &&" is there, and if there's no good reason we should drop it.

Quoted 16 lines

Quoted 16 lines

--------------------------->8-------------8<------------------------------ Add missing parentheses

Signed-off-by Roel Kluin <[email protected]>

---

drivers/hwmon/abituguru3.c +1 -1

index e52b388..fd98685 100644
@@ -761,7 +761,7 @@ static int abituguru3_read_increment_offset(struct
 abituguru3_data *data, for (i = 0; i < offset_count; i++)
 		if ((x = abituguru3_read(data, bank, offset + i, count,
 				buf + i * count)) != count)
-			return i * count + (i && (x < 0)) ? 0 : x;
+			return i * count + ((i && (x < 0)) ? 0 : x);
 

Quoted 2 lines

drivers/hwmon/abituguru3.c static int abituguru3_read_increment_offset(struct abituguru3_data *data, · line 767

Quoted 2 lines

drivers/hwmon/abituguru3.c +0 -0

 	return i * count;
 }

Quoted 9 lines · Jean Delvare

Quoted 6 lines · Alistair John Strachan

This one's been there from the beginning.

The change looks right to me. (i * count) account the previous successes, add either zero or x depending on the kind of read failure (timeout/short read vs other I/O error). I'm not entirely sure why i must be >0 for accounting the short read.

I can't make any sense of that "i &&" either. Hans?

This is in an error handling path, if some bytes were read but not count bytes, then we return i * count + x, where i * count is the number of bytes read successful so far and x is the number of bytes read for the offset we were reading from when we got the short read.

However if this is not just a short read, but a read which error-ed out then x < 0. If we then would return i * count + x, we would return less then what we've actually read. So in this case we want to return just i * count, which gets done in the form of i * count + 0.

There is one special case however if we get an error code in the form of x < 0, and we've not done any successfull reads (so i = 0), then the right thing to do is to pass the error code along. So the condition in which we want to add 0 to the result instead of x becomes (i && x < 0)

Hopes this helps.

Hi Hans,

Quoted 14 lines · Hans de Goede

Quoted 8 lines · Jean Delvare

Quoted 6 lines · Alistair John Strachan

This one's been there from the beginning.

The change looks right to me. (i * count) account the previous successes, add either zero or x depending on the kind of read failure (timeout/short read vs other I/O error). I'm not entirely sure why i must be >0 for accounting the short read.

I can't make any sense of that "i &&" either. Hans?

This is in an error handling path, if some bytes were read but not count bytes, then we return i * count + x, where i * count is the number of bytes read successful so far and x is the number of bytes read for the offset we were reading from when we got the short read.

Quoted 4 lines

However if this is not just a short read, but a read which error-ed out then x < 0. If we then would return i * count + x, we would return less then what we've actually read. So in this case we want to return just i * count, which gets done in the form of i * count + 0.

So far so good...

Quoted 6 lines

There is one special case however if we get an error code in the form of x < 0, and we've not done any successfull reads (so i = 0), then the right thing to do is to pass the error code along. So the condition in which we want to add 0 to the result instead of x becomes (i && x < 0)

Hopes this helps.

OK... I get the idea now. I'm glad you were still around to explain it because I would never have figured it out. And I don't really like the
logic: behaving differently if the error occurs during the first read or the later ones doesn't make a lot of sense to me. If the error is so important that it needs to be reported, then I say it should be reported later as well. Or seen the other way around, if it doesn't need to be reported on later reads, then why bother reporting it on the first read (rather than just returning 0)?

Roel's patch is correct and could be applied. That being said, I don't like that kind of tricky code that needs to be explained to you. I'd rather rewrite it in a simpler form which can be understood directly. Especially given the fact that all the callers care about is boolean success or failure. So I'd suggest the following code clean-up:

---
 drivers/hwmon/abituguru3.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/hwmon/abituguru3.c +5 -2

@@ -760,8 +760,11 @@ static int abituguru3_read_increment_off
 
 	for (i = 0; i < offset_count; i++)
 		if ((x = abituguru3_read(data, bank, offset + i, count,
-				buf + i * count)) != count)
-			return i * count + (i && (x < 0)) ? 0 : x;
+				buf + i * count)) != count) {
+			if (x < 0)
+				return x;
+			return i * count + x;
+		}
 
 	return i * count;
 }

Objections?

drivers/hwmon/abituguru3.c +5 -2

 	for (i = 0; i < offset_count; i++)
 		if ((x = abituguru3_read(data, bank, offset + i, count,
-				buf + i * count)) != count)
-			return i * count + (i && (x < 0)) ? 0 : x;
+				buf + i * count)) != count) {
+			if (x < 0)
+				return x;
+			return i * count + x;
+		}
 
 	return i * count;
 }

Objections?

(Sorry for the delay and probable mail corruption, I'm travelling)

[snip]

Quoted 13 lines

drivers/hwmon/abituguru3.c +5 -2

 	for (i = 0; i < offset_count; i++)
 		if ((x = abituguru3_read(data, bank, offset + i, count,
-				buf + i * count)) != count)
-			return i * count + (i && (x < 0)) ? 0 : x;
+				buf + i * count)) != count) {
+			if (x < 0)
+				return x;
+			return i * count + x;
+		}
 
 	return i * count;
 }

Quoted 1 line

Objections?

I have no objections to this patch.

However, if propagating errors up the stack is desirable, I think it would also be prudent to fix abituguru3_update_device() to pass the error back via ERR_PTR() or similar, instead of returning NULL. That way, show_value(), show_alarm() and abituguru3_probe() could return f.e. -EIO, generated by abituguru3_read().

This change is more involved; I can send a follow up for a later time-frame if desired.

(Sorry for the delay and probable mail corruption, I'm travelling)

[snip]

Quoted 10 lines

(quoted diff) +5 -2

 	for (i = 0; i < offset_count; i++)
 		if ((x = abituguru3_read(data, bank, offset + i, count,
-				buf + i * count)) != count)
-			return i * count + (i && (x < 0)) ? 0 : x;
+				buf + i * count)) != count) {
+			if (x < 0)
+				return x;
+			return i * count + x;
+		}

Quoted 4 lines

return i * count;
}

Objections?

I have no objections to this patch.

However, if propagating errors up the stack is desirable, I think it would also be prudent to fix abituguru3_update_device() to pass the error back via ERR_PTR() or similar, instead of returning NULL. That way, show_value(), show_alarm() and abituguru3_probe() could return f.e. -EIO, generated by abituguru3_read().

This change is more involved; I can send a follow up for a later time-frame if desired.