Skip to contentSkip to conversation

i2c-core: return -Errno, not -1

PATCH New David Brownell 2 participants 6 messages May 3, 2008 – May 17, 2008 5 messages not in the archive

Subsystems (2)
I2C subsystem host drivers I2C
lore
Share & tools
lore

David Brownell

author
#0 lore raw

PATCH 1/1 i2c-core: return -Errno, not -1

More updates to the I2C stack's fault reporting: make the core stop returning "-1" (usually "-EPERM") for all faults. Instead, pass lower level fault code up the stack, or return some appropriate errno.

This patch happens to touch almost exclusively SMBus calls.

Signed-off-by David Brownell <[email protected]>

---
Combine this and the previous i2c_adapter patch, and I suspect that well over half the causes of bogus "-EPERM" reports are now gone ... including in particular those reported by i2c-dev to userspace when it issues SMBus calls "for portability".

 drivers/i2c/i2c-core.c |   76 +++++++++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 33 deletions(-)
drivers/i2c/i2c-core.c +43 -33
@@ -1113,7 +1113,7 @@ int i2c_probe(struct i2c_adapter *adapte
 
 		dev_warn(&adapter->dev, "SMBus Quick command not supported, "
 			 "can't probe for chips\n");
-		return -1;
+		return -EOPNOTSUPP;
 	}
 
 	/* Probe entries are done second, and are not affected by ignore

@@ -1305,7 +1305,7 @@ static int i2c_smbus_check_pec(u8 cpec, 
 	if (rpec != cpec) {
 		pr_debug("i2c-core: Bad PEC 0x%02x vs. 0x%02x\n",
 			rpec, cpec);
-		return -1;
+		return -EPROTO;
 	}
 	return 0;
 }

@@ -1320,11 +1320,12 @@ EXPORT_SYMBOL(i2c_smbus_write_quick);
 s32 i2c_smbus_read_byte(struct i2c_client *client)
 {
 	union i2c_smbus_data data;
-	if (i2c_smbus_xfer(client->adapter,client->addr,client->flags,
-	                   I2C_SMBUS_READ,0,I2C_SMBUS_BYTE, &data))
-		return -1;
-	else
-		return data.byte;
+	int status;
+
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_READ, 0,
+			I2C_SMBUS_BYTE, &data);
+	return (status < 0) ? status : data.byte;
 }
 EXPORT_SYMBOL(i2c_smbus_read_byte);
 

@@ -1338,11 +1339,12 @@ EXPORT_SYMBOL(i2c_smbus_write_byte);
 s32 i2c_smbus_read_byte_data(struct i2c_client *client, u8 command)
 {
 	union i2c_smbus_data data;
-	if (i2c_smbus_xfer(client->adapter,client->addr,client->flags,
-	                   I2C_SMBUS_READ,command, I2C_SMBUS_BYTE_DATA,&data))
-		return -1;
-	else
-		return data.byte;
+	int status;
+
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_READ, command,
+			I2C_SMBUS_BYTE_DATA, &data);
+	return (status < 0) ? status : data.byte;
 }
 EXPORT_SYMBOL(i2c_smbus_read_byte_data);
 

@@ -1359,11 +1361,12 @@ EXPORT_SYMBOL(i2c_smbus_write_byte_data)
 s32 i2c_smbus_read_word_data(struct i2c_client *client, u8 command)
 {
 	union i2c_smbus_data data;
-	if (i2c_smbus_xfer(client->adapter,client->addr,client->flags,
-	                   I2C_SMBUS_READ,command, I2C_SMBUS_WORD_DATA, &data))
-		return -1;
-	else
-		return data.word;
+	int status;
+
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_READ, command,
+			I2C_SMBUS_WORD_DATA,  &data);
+	return (status < 0) ? status : data.word;
 }
 EXPORT_SYMBOL(i2c_smbus_read_word_data);
 

@@ -1397,11 +1400,13 @@ s32 i2c_smbus_read_block_data(struct i2c
 			      u8 *values)
 {
 	union i2c_smbus_data data;
+	int status;
 
-	if (i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-	                   I2C_SMBUS_READ, command,
-	                   I2C_SMBUS_BLOCK_DATA, &data))
-		return -1;
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_READ, command,
+			I2C_SMBUS_BLOCK_DATA, &data);
+	if (status)
+		return status;
 
 	memcpy(values, &data.block[1], data.block[0]);
 	return data.block[0];

@@ -1428,14 +1433,16 @@ s32 i2c_smbus_read_i2c_block_data(struct
 				  u8 length, u8 *values)
 {
 	union i2c_smbus_data data;
+	int status;
 
 	if (length > I2C_SMBUS_BLOCK_MAX)
 		length = I2C_SMBUS_BLOCK_MAX;
 	data.block[0] = length;
-	if (i2c_smbus_xfer(client->adapter,client->addr,client->flags,
-	                      I2C_SMBUS_READ,command,
-	                      I2C_SMBUS_I2C_BLOCK_DATA,&data))
-		return -1;
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_READ, command,
+			I2C_SMBUS_I2C_BLOCK_DATA, &data);
+	if (status < 0)
+		return status;
 
 	memcpy(values, &data.block[1], data.block[0]);
 	return data.block[0];

@@ -1476,6 +1483,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 	                        };
 	int i;
 	u8 partial_pec = 0;
+	int status;
 
 	msgbuf0[0] = command;
 	switch(size) {

@@ -1528,7 +1536,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 				dev_err(&adapter->dev, "smbus_access called with "
 				       "invalid block write size (%d)\n",
 				       data->block[0]);
-				return -1;
+				return -EMSGSIZE;
 			}
 			for (i = 1; i < msg[0].len; i++)
 				msgbuf0[i] = data->block[i-1];

@@ -1541,7 +1549,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 			dev_err(&adapter->dev, "%s called with invalid "
 				"block proc call size (%d)\n", __func__,
 				data->block[0]);
-			return -1;
+			return -EMSGSIZE;
 		}
 		msg[0].len = data->block[0] + 2;
 		for (i = 1; i < msg[0].len; i++)

@@ -1559,7 +1567,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 				dev_err(&adapter->dev, "i2c_smbus_xfer_emulated called with "
 				       "invalid block write size (%d)\n",
 				       data->block[0]);
-				return -1;
+				return -EMSGSIZE;
 			}
 			for (i = 1; i <= data->block[0]; i++)
 				msgbuf0[i] = data->block[i];

@@ -1568,7 +1576,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 	default:
 		dev_err(&adapter->dev, "smbus_access called with invalid size (%d)\n",
 		       size);
-		return -1;
+		return -EOPNOTSUPP;
 	}
 
 	i = ((flags & I2C_CLIENT_PEC) && size != I2C_SMBUS_QUICK

@@ -1586,13 +1594,15 @@ static s32 i2c_smbus_xfer_emulated(struc
 			msg[num-1].len++;
 	}
 
-	if (i2c_transfer(adapter, msg, num) < 0)
-		return -1;
+	status = i2c_transfer(adapter, msg, num);
+	if (status < 0)
+		return status;
 
 	/* Check PEC if last message is a read */
 	if (i && (msg[num-1].flags & I2C_M_RD)) {
-		if (i2c_smbus_check_pec(partial_pec, &msg[num-1]) < 0)
-			return -1;
+		status = i2c_smbus_check_pec(partial_pec, &msg[num-1]);
+		if (status < 0)
+			return status;
 	}
 
 	if (read_write == I2C_SMBUS_READ)

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): 200805031306.16698.david-b-yBeKhBN/[email protected]

author
#2 lore raw

PATCH 1/1 i2c-core: return -Errno, not -1

More updates to the I2C stack's fault reporting: make the core stop returning "-1" (usually "-EPERM") for all faults. Instead, pass lower level fault code up the stack, or return some appropriate errno.

This patch happens to touch almost exclusively SMBus calls.

Signed-off-by David Brownell <[email protected]>

---
This is an updated version of:

	http://marc.info/?l=i2c&m=120984528415259&w=2

with some return codes updated to address feedback from the similar patch for the x86 I2C/SMBus adapters.

 drivers/i2c/i2c-core.c |   78 +++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 34 deletions(-)
drivers/i2c/i2c-core.c +44 -34
@@ -981,7 +981,7 @@ int i2c_transfer(struct i2c_adapter * ad
 		return ret;
 	} else {
 		dev_dbg(&adap->dev, "I2C level transfers not supported\n");
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 	}
 }
 EXPORT_SYMBOL(i2c_transfer);

@@ -1113,7 +1113,7 @@ int i2c_probe(struct i2c_adapter *adapte
 
 		dev_warn(&adapter->dev, "SMBus Quick command not supported, "
 			 "can't probe for chips\n");
-		return -1;
+		return -EOPNOTSUPP;
 	}
 
 	/* Probe entries are done second, and are not affected by ignore

@@ -1305,7 +1305,7 @@ static int i2c_smbus_check_pec(u8 cpec, 
 	if (rpec != cpec) {
 		pr_debug("i2c-core: Bad PEC 0x%02x vs. 0x%02x\n",
 			rpec, cpec);
-		return -1;
+		return -EBADMSG;
 	}
 	return 0;
 }

@@ -1320,11 +1320,12 @@ EXPORT_SYMBOL(i2c_smbus_write_quick);
 s32 i2c_smbus_read_byte(struct i2c_client *client)
 {
 	union i2c_smbus_data data;
-	if (i2c_smbus_xfer(client->adapter,client->addr,client->flags,
-	                   I2C_SMBUS_READ,0,I2C_SMBUS_BYTE, &data))
-		return -1;
-	else
-		return data.byte;
+	int status;
+
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_READ, 0,
+			I2C_SMBUS_BYTE, &data);
+	return (status < 0) ? status : data.byte;
 }
 EXPORT_SYMBOL(i2c_smbus_read_byte);
 

@@ -1338,11 +1339,12 @@ EXPORT_SYMBOL(i2c_smbus_write_byte);
 s32 i2c_smbus_read_byte_data(struct i2c_client *client, u8 command)
 {
 	union i2c_smbus_data data;
-	if (i2c_smbus_xfer(client->adapter,client->addr,client->flags,
-	                   I2C_SMBUS_READ,command, I2C_SMBUS_BYTE_DATA,&data))
-		return -1;
-	else
-		return data.byte;
+	int status;
+
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_READ, command,
+			I2C_SMBUS_BYTE_DATA, &data);
+	return (status < 0) ? status : data.byte;
 }
 EXPORT_SYMBOL(i2c_smbus_read_byte_data);
 

@@ -1359,11 +1361,12 @@ EXPORT_SYMBOL(i2c_smbus_write_byte_data)
 s32 i2c_smbus_read_word_data(struct i2c_client *client, u8 command)
 {
 	union i2c_smbus_data data;
-	if (i2c_smbus_xfer(client->adapter,client->addr,client->flags,
-	                   I2C_SMBUS_READ,command, I2C_SMBUS_WORD_DATA, &data))
-		return -1;
-	else
-		return data.word;
+	int status;
+
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_READ, command,
+			I2C_SMBUS_WORD_DATA,  &data);
+	return (status < 0) ? status : data.word;
 }
 EXPORT_SYMBOL(i2c_smbus_read_word_data);
 

@@ -1397,11 +1400,13 @@ s32 i2c_smbus_read_block_data(struct i2c
 			      u8 *values)
 {
 	union i2c_smbus_data data;
+	int status;
 
-	if (i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-	                   I2C_SMBUS_READ, command,
-	                   I2C_SMBUS_BLOCK_DATA, &data))
-		return -1;
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_READ, command,
+			I2C_SMBUS_BLOCK_DATA, &data);
+	if (status)
+		return status;
 
 	memcpy(values, &data.block[1], data.block[0]);
 	return data.block[0];

@@ -1428,14 +1433,16 @@ s32 i2c_smbus_read_i2c_block_data(struct
 				  u8 length, u8 *values)
 {
 	union i2c_smbus_data data;
+	int status;
 
 	if (length > I2C_SMBUS_BLOCK_MAX)
 		length = I2C_SMBUS_BLOCK_MAX;
 	data.block[0] = length;
-	if (i2c_smbus_xfer(client->adapter,client->addr,client->flags,
-	                      I2C_SMBUS_READ,command,
-	                      I2C_SMBUS_I2C_BLOCK_DATA,&data))
-		return -1;
+	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_READ, command,
+			I2C_SMBUS_I2C_BLOCK_DATA, &data);
+	if (status < 0)
+		return status;
 
 	memcpy(values, &data.block[1], data.block[0]);
 	return data.block[0];

@@ -1476,6 +1483,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 	                        };
 	int i;
 	u8 partial_pec = 0;
+	int status;
 
 	msgbuf0[0] = command;
 	switch(size) {

@@ -1528,7 +1536,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 				dev_err(&adapter->dev, "smbus_access called with "
 				       "invalid block write size (%d)\n",
 				       data->block[0]);
-				return -1;
+				return -EINVAL;
 			}
 			for (i = 1; i < msg[0].len; i++)
 				msgbuf0[i] = data->block[i-1];

@@ -1541,7 +1549,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 			dev_err(&adapter->dev, "%s called with invalid "
 				"block proc call size (%d)\n", __func__,
 				data->block[0]);
-			return -1;
+			return -EINVAL;
 		}
 		msg[0].len = data->block[0] + 2;
 		for (i = 1; i < msg[0].len; i++)

@@ -1559,7 +1567,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 				dev_err(&adapter->dev, "i2c_smbus_xfer_emulated called with "
 				       "invalid block write size (%d)\n",
 				       data->block[0]);
-				return -1;
+				return -EINVAL;
 			}
 			for (i = 1; i <= data->block[0]; i++)
 				msgbuf0[i] = data->block[i];

@@ -1568,7 +1576,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 	default:
 		dev_err(&adapter->dev, "smbus_access called with invalid size (%d)\n",
 		       size);
-		return -1;
+		return -EOPNOTSUPP;
 	}
 
 	i = ((flags & I2C_CLIENT_PEC) && size != I2C_SMBUS_QUICK

@@ -1586,13 +1594,15 @@ static s32 i2c_smbus_xfer_emulated(struc
 			msg[num-1].len++;
 	}
 
-	if (i2c_transfer(adapter, msg, num) < 0)
-		return -1;
+	status = i2c_transfer(adapter, msg, num);
+	if (status < 0)
+		return status;
 
 	/* Check PEC if last message is a read */
 	if (i && (msg[num-1].flags & I2C_M_RD)) {
-		if (i2c_smbus_check_pec(partial_pec, &msg[num-1]) < 0)
-			return -1;
+		status = i2c_smbus_check_pec(partial_pec, &msg[num-1]);
+		if (status < 0)
+			return status;
 	}
 
 	if (read_write == I2C_SMBUS_READ)

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): 200805111516.08453.david-b-yBeKhBN/[email protected]

#4 lore raw

Hi David,

Quoted 7 lines · David Brownell

More updates to the I2C stack's fault reporting: make the core stop returning "-1" (usually "-EPERM") for all faults. Instead, pass lower level fault code up the stack, or return some appropriate errno.

This patch happens to touch almost exclusively SMBus calls.

Signed-off-by David Brownell <[email protected]>
Quoted 9 lines

---
This is an updated version of:

	http://marc.info/?l=i2c&m=120984528415259&w=2

with some return codes updated to address feedback from the similar patch for the x86 I2C/SMBus adapters.

 drivers/i2c/i2c-core.c |   78 +++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

Looks very good, with just one suggested change:

drivers/i2c/i2c-core.c static s32 i2c_smbus_xfer_emulated(struc · line 1580

Quoted 10 lines
drivers/i2c/i2c-core.c +1 -1
(...)

@@ -1568,7 +1576,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 	default:
 		dev_err(&adapter->dev, "smbus_access called with invalid size (%d)\n",
 		       size);
-		return -1;
+		return -EOPNOTSUPP;
 	}

I'd rather use -EINVAL here. This case is a bit different from what we did in the bus drivers, because here the function handles all transaction types that exist. So if we reach the default case, it means that the caller passed an invalid parameter. -EOPNOTSUPP is not fundamentally incorrect but it's a little less precise.

In theory, in bus drivers we should separate between invalid transaction types and unsupported transaction types, returning -EINVAL for the former and -EOPNOTSUPP only for the latter. We do not simply because it makes little sense to make the drivers bigger just to report finer-grained error codes, especially when these error paths are almost never reached, and also because it's convenient to be able to grep for I2C_SMBUS_* transaction type constants to find out which drivers support a given transaction type.

BTW, feel free to adjust the debugging message above, as you did in some bus drivers already, to clearly say that we're speaking about a transaction type and not a "size".

All the rest looks OK to me from a functional point of view. As far as style is concerned, please keep the alignment on opening parenthesis when the original code did that. i2c-core uses this style consistently and I like it.

Thanks,

Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

#6 lore raw
Quoted 22 lines · Jean Delvare

Hi David,

Quoted 7 lines · David Brownell

More updates to the I2C stack's fault reporting: make the core stop returning "-1" (usually "-EPERM") for all faults. Instead, pass lower level fault code up the stack, or return some appropriate errno.

This patch happens to touch almost exclusively SMBus calls.

Signed-off-by David Brownell <[email protected]>
Quoted 9 lines

---
This is an updated version of:

	http://marc.info/?l=i2c&m=120984528415259&w=2

with some return codes updated to address feedback from the similar patch for the x86 I2C/SMBus adapters.

 drivers/i2c/i2c-core.c |   78 +++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

Looks very good, with just one suggested change:

Quoted 12 lines

drivers/i2c/i2c-core.c static s32 i2c_smbus_xfer_emulated(struc · line 1580

Quoted 10 lines
drivers/i2c/i2c-core.c +1 -1
(...)

@@ -1568,7 +1576,7 @@ static s32 i2c_smbus_xfer_emulated(struc
 	default:
 		dev_err(&adapter->dev, "smbus_access called with invalid size (%d)\n",
 		       size);
-		return -1;
+		return -EOPNOTSUPP;
 	}

I'd rather use -EINVAL here. (...)

Hmh, scratch this. Thinking about it some more (night helps) -EOPNOTSUPP is consistent with what we did for the bus drivers after all, and it also anticipates addition of new transaction types to <linux/i2c.h> or removal of support for some transaction types from i2c_smbus_xfer_emulated() (which might as well happen, if you look carefully you'll see that there are no in-kernel users of transaction types I2C_SMBUS_PROC_CALL and I2C_SMBUS_BLOCK_PROC_CALL so we might decide to remove thir support from i2c_smbus_xfer_emulated someday.)

Quoted 9 lines

(..)
BTW, feel free to adjust the debugging message above, as you did in some bus drivers already, to clearly say that we're speaking about a transaction type and not a "size".

All the rest looks OK to me from a functional point of view. As far as style is concerned, please keep the alignment on opening parenthesis when the original code did that. i2c-core uses this style consistently and I like it.

I'll do all this myself now, no need to resend.

Thanks,

Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#8 lore raw
Quoted 7 lines · Jean Delvare
Quoted 7 lines
Quoted 6 lines

    default:
            dev_err(&adapter->dev, "smbus_access called with invalid size (%d)\n", size);
-           return -1;
+           return -EOPNOTSUPP;
    }

Quoted 7 lines
Quoted 1 line

I'd rather use -EINVAL here. (...)

Hmh, scratch this. Thinking about it some more (night helps) -EOPNOTSUPP is consistent with what we did for the bus drivers after all, and it also anticipates addition of new transaction types to <linux/i2c.h> or removal of support for some transaction types from i2c_smbus_xfer_emulated() ..

Right.

Quoted 4 lines
Quoted 4 lines

(..)
BTW, feel free to adjust the debugging message above, as you did in some bus drivers already, to clearly say that we're speaking about a transaction type and not a "size".

I must have missed a few. Those updates weren't initially on a list of things-to-change, but I guess some of the issues were egregious enough to make me add them -- after that particular confusion percolated long enough. The third or fourth time through a piece of code, it's a bit easier to notice that sort of confusion. (Probably worth someone's time to switch *ALL* drivers from calling that a "size". I think that bit of history needs to be edited out of existence...)

Quoted 6 lines
Quoted 4 lines

All the rest looks OK to me from a functional point of view. As far as style is concerned, please keep the alignment on opening parenthesis when the original code did that. i2c-core uses this style consistently and I like it.

I'll do all this myself now, no need to resend.

Good. In general, when it's down to the last set of small comments like this, it's probably easier all around if you do stuff like that (rather than writing it up in email, having someone like me both read it and make time to act on it, then have you re-review). That's fairly common maintainer practice.

- Dave

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): 200805170653.19424.david-b-yBeKhBN/[email protected]

#10 lore raw
Quoted 8 lines · David Brownell
Quoted 1 line · Jean Delvare

I'll do all this myself now, no need to resend.

Good. In general, when it's down to the last set of small comments like this, it's probably easier all around if you do stuff like that (rather than writing it up in email, having someone like me both read it and make time to act on it, then have you re-review). That's fairly common maintainer practice.

Well, the patch has your name on it, so I think it's only fair to give you a chance to nack if I am going to change it before I push it upstream. But if you don't care, I can skip this step on future patches, no problem.

Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c