[PATCH v2] i2c: stub: Add support for SMBus block commands

[PATCH v2] i2c: stub: Add support for SMBus block commands

[Guenter Roeck]

SMBus block commands are different to I2C block commands since
the returned data is not normally accessible with byte or word
commands on other command offsets. Add linked list of 'block'
commands to support those commands.

Access mechanism is quite simple: Block commands must be written
before they can be read. The first write selects the block length.
Subsequent writes can be partial. Block read commands always return
the number of bytes selected with the first write.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
v2: Make new functionality only available on request via functionality
    module parameter
    Add more details about SMBus block mode support to documentation
    Use correct sizeof() variable in devm_kzalloc
    Use stub_find_block() only in SMBus block command itself.
    Store first word of block data in chip->words[].
    When writing block data and the written data is longer than
    the first write, bail out with debug message indicating the reason
    for the error.

 Documentation/i2c/i2c-stub |  9 ++++-
 drivers/i2c/i2c-stub.c     | 99 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 101 insertions(+), 7 deletions(-)

diff --git a/Documentation/i2c/i2c-stub b/Documentation/i2c/i2c-stub
index fa4b669..531510b 100644
--- a/Documentation/i2c/i2c-stub
+++ b/Documentation/i2c/i2c-stub
@@ -2,9 +2,9 @@ MODULE: i2c-stub
 
 DESCRIPTION:
 
-This module is a very simple fake I2C/SMBus driver.  It implements five
+This module is a very simple fake I2C/SMBus driver.  It implements six
 types of SMBus commands: write quick, (r/w) byte, (r/w) byte data, (r/w)
-word data, and (r/w) I2C block data.
+word data, (r/w) I2C block data, and (r/w) SMBus block data.
 
 You need to provide chip addresses as a module parameter when loading this
 driver, which will then only react to SMBus commands to these addresses.
@@ -19,6 +19,11 @@ A pointer register with auto-increment is implemented for all byte
 operations.  This allows for continuous byte reads like those supported by
 EEPROMs, among others.
 
+SMBus block commands must be written to configure an SMBus command for
+SMBus block operations. The first SMBus block write selects the block length.
+Subsequent writes can be partial. Block read commands always return
+the number of bytes selected with the first write.
+
 The typical use-case is like this:
 	1. load this module
 	2. use i2cset (from the i2c-tools project) to pre-load some data
diff --git a/drivers/i2c/i2c-stub.c b/drivers/i2c/i2c-stub.c
index 77e4849..4fbe565 100644
--- a/drivers/i2c/i2c-stub.c
+++ b/drivers/i2c/i2c-stub.c
@@ -27,29 +27,64 @@
 #include <linux/slab.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
+#include <linux/list.h>
 
 #define MAX_CHIPS 10
-#define STUB_FUNC (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | \
-		   I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | \
-		   I2C_FUNC_SMBUS_I2C_BLOCK)
+#define STUB_FUNC_DEFAULT \
+		(I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | \
+		 I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | \
+		 I2C_FUNC_SMBUS_I2C_BLOCK)
+
+#define STUB_FUNC_ALL \
+		(STUB_FUNC_DEFAULT | I2C_FUNC_SMBUS_BLOCK_DATA)
 
 static unsigned short chip_addr[MAX_CHIPS];
 module_param_array(chip_addr, ushort, NULL, S_IRUGO);
 MODULE_PARM_DESC(chip_addr,
 		 "Chip addresses (up to 10, between 0x03 and 0x77)");
 
-static unsigned long functionality = STUB_FUNC;
+static unsigned long functionality = STUB_FUNC_DEFAULT;
 module_param(functionality, ulong, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(functionality, "Override functionality bitfield");
 
+struct smbus_block_data {
+	struct list_head node;
+	u8 command;
+	u8 len;
+	u8 block[I2C_SMBUS_BLOCK_MAX];
+};
+
 struct stub_chip {
 	u8 pointer;
 	u16 words[256];		/* Byte operations use the LSB as per SMBus
 				   specification */
+	struct list_head smbus_blocks;
 };
 
 static struct stub_chip *stub_chips;
 
+static struct smbus_block_data *stub_find_block(struct device *dev,
+						struct stub_chip *chip,
+						u8 command, bool create)
+{
+	struct smbus_block_data *b, *rb = NULL;
+
+	list_for_each_entry(b, &chip->smbus_blocks, node) {
+		if (b->command == command) {
+			rb = b;
+			break;
+		}
+	}
+	if (rb == NULL && create) {
+		rb = devm_kzalloc(dev, sizeof(*rb), GFP_KERNEL);
+		if (rb == NULL)
+			return rb;
+		rb->command = command;
+		list_add(&rb->node, &chip->smbus_blocks);
+	}
+	return rb;
+}
+
 /* Return negative errno on error. */
 static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags,
 	char read_write, u8 command, int size, union i2c_smbus_data *data)
@@ -57,6 +92,7 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags,
 	s32 ret;
 	int i, len;
 	struct stub_chip *chip = NULL;
+	struct smbus_block_data *b;
 
 	/* Search for the right chip */
 	for (i = 0; i < MAX_CHIPS && chip_addr[i]; i++) {
@@ -148,6 +184,57 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags,
 		ret = 0;
 		break;
 
+	case I2C_SMBUS_BLOCK_DATA:
+		b = stub_find_block(&adap->dev, chip, command, false);
+		if (read_write == I2C_SMBUS_WRITE) {
+			len = data->block[0];
+			if (len == 0 || len > I2C_SMBUS_BLOCK_MAX) {
+				ret = -EINVAL;
+				break;
+			}
+			if (b && len > b->len) {
+				dev_dbg(&adap->dev,
+					"Attempt to write more data (%d) than with initial SMBus block write (%d)\n",
+					len, b->len);
+				ret = -EINVAL;
+				break;
+			}
+			if (b == NULL) {
+				b = stub_find_block(&adap->dev, chip, command,
+						    true);
+				if (b == NULL) {
+					ret = -ENOMEM;
+					break;
+				}
+				/* First write sets block length */
+				b->len = len;
+			}
+			for (i = 0; i < len; i++)
+				b->block[i] = data->block[i + 1];
+			/* update for byte and word commands */
+			chip->words[command] = (b->block[0] << 8) | b->len;
+			dev_dbg(&adap->dev,
+				"smbus block data - addr 0x%02x, wrote %d bytes at 0x%02x.\n",
+				addr, len, command);
+		} else {
+			if (b == NULL) {
+				dev_dbg(&adap->dev,
+					"SMBus block read command without prior block write not supported\n");
+				ret = -EOPNOTSUPP;
+				break;
+			}
+			len = b->len;
+			data->block[0] = len;
+			for (i = 0; i < len; i++)
+				data->block[i + 1] = b->block[i];
+			dev_dbg(&adap->dev,
+				"smbus block data - addr 0x%02x, read  %d bytes at 0x%02x.\n",
+				addr, len, command);
+		}
+
+		ret = 0;
+		break;
+
 	default:
 		dev_dbg(&adap->dev, "Unsupported I2C/SMBus command\n");
 		ret = -EOPNOTSUPP;
@@ -159,7 +246,7 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags,
 
 static u32 stub_func(struct i2c_adapter *adapter)
 {
-	return STUB_FUNC & functionality;
+	return STUB_FUNC_ALL & functionality;
 }
 
 static const struct i2c_algorithm smbus_algorithm = {
@@ -199,6 +286,8 @@ static int __init i2c_stub_init(void)
 		pr_err("i2c-stub: Out of memory\n");
 		return -ENOMEM;
 	}
+	for (i--; i >= 0; i--)
+		INIT_LIST_HEAD(&stub_chips[i].smbus_blocks);
 
 	ret = i2c_add_adapter(&stub_adapter);
 	if (ret)
-- 
1.9.1

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Jean Delvare]

Hi Guenter,

On Mon,  7 Jul 2014 07:23:03 -0700, Guenter Roeck wrote:
> SMBus block commands are different to I2C block commands since
> the returned data is not normally accessible with byte or word
> commands on other command offsets. Add linked list of 'block'
> commands to support those commands.
> 
> Access mechanism is quite simple: Block commands must be written
> before they can be read. The first write selects the block length.
> Subsequent writes can be partial. Block read commands always return
> the number of bytes selected with the first write.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Make new functionality only available on request via functionality
>     module parameter
>     Add more details about SMBus block mode support to documentation
>     Use correct sizeof() variable in devm_kzalloc
>     Use stub_find_block() only in SMBus block command itself.
>     Store first word of block data in chip->words[].
>     When writing block data and the written data is longer than
>     the first write, bail out with debug message indicating the reason
>     for the error.

Looks good, thanks for the quick update.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

Just one thing I have been thinking about while reviewing the updated
code... You decided to make the first SMBus block write select the
maximum block length, and you always use that for SMBus block reads.
However you accept partial writes. The fact that the order in which
writes are performed has an effect on which writes are accepted is
somewhat unexpected.

Wouldn't it make more sense to accept all SMBus block writes,
regardless of the size (as long as it is within the limits of the SMBus
standard, of course)? Then the only thing left to decide is whether
SMBus block reads use the maximum size or the size of the most recent
SMBus block write.

I suspect this would mimic the behavior of real chips better. What do
you think?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Guenter Roeck]

Hi Jean,

On 07/08/2014 12:54 PM, Jean Delvare wrote:
> Hi Guenter,
>
> On Mon,  7 Jul 2014 07:23:03 -0700, Guenter Roeck wrote:
>> SMBus block commands are different to I2C block commands since
>> the returned data is not normally accessible with byte or word
>> commands on other command offsets. Add linked list of 'block'
>> commands to support those commands.
>>
>> Access mechanism is quite simple: Block commands must be written
>> before they can be read. The first write selects the block length.
>> Subsequent writes can be partial. Block read commands always return
>> the number of bytes selected with the first write.
>>
>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>> ---
>> v2: Make new functionality only available on request via functionality
>>      module parameter
>>      Add more details about SMBus block mode support to documentation
>>      Use correct sizeof() variable in devm_kzalloc
>>      Use stub_find_block() only in SMBus block command itself.
>>      Store first word of block data in chip->words[].
>>      When writing block data and the written data is longer than
>>      the first write, bail out with debug message indicating the reason
>>      for the error.
>
> Looks good, thanks for the quick update.
>
> Reviewed-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
>
> Just one thing I have been thinking about while reviewing the updated
> code... You decided to make the first SMBus block write select the
> maximum block length, and you always use that for SMBus block reads.
> However you accept partial writes. The fact that the order in which
> writes are performed has an effect on which writes are accepted is
> somewhat unexpected.
>
> Wouldn't it make more sense to accept all SMBus block writes,
> regardless of the size (as long as it is within the limits of the SMBus
> standard, of course)? Then the only thing left to decide is whether
> SMBus block reads use the maximum size or the size of the most recent
> SMBus block write.
>
> I suspect this would mimic the behavior of real chips better. What do
> you think?
>

Not really sure what the expected behavior is. My original code
accepted all writes and returned the most recent write, including
the most recent write length. I thought this was untypical, and that
it would be more typical for the chip to return a fixed length.
But ultimately I don't really know, and I am fine either way.

Guenter

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Jean Delvare]

Hi Guenter,

On Tue, 08 Jul 2014 13:05:41 -0700, Guenter Roeck wrote:
> On 07/08/2014 12:54 PM, Jean Delvare wrote:
> > Just one thing I have been thinking about while reviewing the updated
> > code... You decided to make the first SMBus block write select the
> > maximum block length, and you always use that for SMBus block reads.
> > However you accept partial writes. The fact that the order in which
> > writes are performed has an effect on which writes are accepted is
> > somewhat unexpected.
> >
> > Wouldn't it make more sense to accept all SMBus block writes,
> > regardless of the size (as long as it is within the limits of the SMBus
> > standard, of course)? Then the only thing left to decide is whether
> > SMBus block reads use the maximum size or the size of the most recent
> > SMBus block write.
> >
> > I suspect this would mimic the behavior of real chips better. What do
> > you think?
> 
> Not really sure what the expected behavior is. My original code
> accepted all writes and returned the most recent write, including
> the most recent write length. I thought this was untypical, and that
> it would be more typical for the chip to return a fixed length.
> But ultimately I don't really know, and I am fine either way.

I agree that different chips may behave differently and it is not
possible for i2c-stub to please everyone. However I do not think that
the current implementation mimics any actual chip behavior. So we might
as well switch to something more simple and more likely to please at
least one device driver:

From: Jean Delvare <jdelvare@suse.de>
Subject: i2c-stub: Allow the increasing SMBus block write length

This is no good reason to not allow SMBus block writes longer than the
first one was. Lift this limitation, this makes the code more simple.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Guenter Roeck <linux@roeck-us.net>
---
 Documentation/i2c/i2c-stub |    5 ++---
 drivers/i2c/i2c-stub.c     |   12 +++---------
 2 files changed, 5 insertions(+), 12 deletions(-)

--- linux-3.16-rc4.orig/Documentation/i2c/i2c-stub	2014-07-12 09:41:26.508195718 +0200
+++ linux-3.16-rc4/Documentation/i2c/i2c-stub	2014-07-12 10:40:05.064578130 +0200
@@ -20,9 +20,8 @@ operations.  This allows for continuous
 EEPROMs, among others.
 
 SMBus block commands must be written to configure an SMBus command for
-SMBus block operations. The first SMBus block write selects the block length.
-Subsequent writes can be partial. Block read commands always return
-the number of bytes selected with the first write.
+SMBus block operations. Writes can be partial. Block read commands always
+return the number of bytes selected with the largest write so far.
 
 The typical use-case is like this:
 	1. load this module
--- linux-3.16-rc4.orig/drivers/i2c/i2c-stub.c	2014-07-12 09:41:26.508195718 +0200
+++ linux-3.16-rc4/drivers/i2c/i2c-stub.c	2014-07-12 11:00:41.472813787 +0200
@@ -254,13 +254,6 @@ static s32 stub_xfer(struct i2c_adapter
 				ret = -EINVAL;
 				break;
 			}
-			if (b && len > b->len) {
-				dev_dbg(&adap->dev,
-					"Attempt to write more data (%d) than with initial SMBus block write (%d)\n",
-					len, b->len);
-				ret = -EINVAL;
-				break;
-			}
 			if (b == NULL) {
 				b = stub_find_block(&adap->dev, chip, command,
 						    true);
@@ -268,9 +261,10 @@ static s32 stub_xfer(struct i2c_adapter
 					ret = -ENOMEM;
 					break;
 				}
-				/* First write sets block length */
-				b->len = len;
 			}
+			/* Largest write sets read block length */
+			if (len > b->len)
+				b->len = len;
 			for (i = 0; i < len; i++)
 				b->block[i] = data->block[i + 1];
 			/* update for byte and word commands */

Would that work for you?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Guenter Roeck]

On 07/12/2014 02:20 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Tue, 08 Jul 2014 13:05:41 -0700, Guenter Roeck wrote:
>> On 07/08/2014 12:54 PM, Jean Delvare wrote:
>>> Just one thing I have been thinking about while reviewing the updated
>>> code... You decided to make the first SMBus block write select the
>>> maximum block length, and you always use that for SMBus block reads.
>>> However you accept partial writes. The fact that the order in which
>>> writes are performed has an effect on which writes are accepted is
>>> somewhat unexpected.
>>>
>>> Wouldn't it make more sense to accept all SMBus block writes,
>>> regardless of the size (as long as it is within the limits of the SMBus
>>> standard, of course)? Then the only thing left to decide is whether
>>> SMBus block reads use the maximum size or the size of the most recent
>>> SMBus block write.
>>>
>>> I suspect this would mimic the behavior of real chips better. What do
>>> you think?
>>
>> Not really sure what the expected behavior is. My original code
>> accepted all writes and returned the most recent write, including
>> the most recent write length. I thought this was untypical, and that
>> it would be more typical for the chip to return a fixed length.
>> But ultimately I don't really know, and I am fine either way.
>
> I agree that different chips may behave differently and it is not
> possible for i2c-stub to please everyone. However I do not think that
> the current implementation mimics any actual chip behavior. So we might
> as well switch to something more simple and more likely to please at
> least one device driver:
>
> From: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
> Subject: i2c-stub: Allow the increasing SMBus block write length
>
> This is no good reason to not allow SMBus block writes longer than the
> first one was. Lift this limitation, this makes the code more simple.
>
> Signed-off-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
> Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> ---
>   Documentation/i2c/i2c-stub |    5 ++---
>   drivers/i2c/i2c-stub.c     |   12 +++---------
>   2 files changed, 5 insertions(+), 12 deletions(-)
>
> --- linux-3.16-rc4.orig/Documentation/i2c/i2c-stub	2014-07-12 09:41:26.508195718 +0200
> +++ linux-3.16-rc4/Documentation/i2c/i2c-stub	2014-07-12 10:40:05.064578130 +0200
> @@ -20,9 +20,8 @@ operations.  This allows for continuous
>   EEPROMs, among others.
>
>   SMBus block commands must be written to configure an SMBus command for
> -SMBus block operations. The first SMBus block write selects the block length.
> -Subsequent writes can be partial. Block read commands always return
> -the number of bytes selected with the first write.
> +SMBus block operations. Writes can be partial. Block read commands always
> +return the number of bytes selected with the largest write so far.
>
>   The typical use-case is like this:
>   	1. load this module
> --- linux-3.16-rc4.orig/drivers/i2c/i2c-stub.c	2014-07-12 09:41:26.508195718 +0200
> +++ linux-3.16-rc4/drivers/i2c/i2c-stub.c	2014-07-12 11:00:41.472813787 +0200
> @@ -254,13 +254,6 @@ static s32 stub_xfer(struct i2c_adapter
>   				ret = -EINVAL;
>   				break;
>   			}
> -			if (b && len > b->len) {
> -				dev_dbg(&adap->dev,
> -					"Attempt to write more data (%d) than with initial SMBus block write (%d)\n",
> -					len, b->len);
> -				ret = -EINVAL;
> -				break;
> -			}
>   			if (b == NULL) {
>   				b = stub_find_block(&adap->dev, chip, command,
>   						    true);
> @@ -268,9 +261,10 @@ static s32 stub_xfer(struct i2c_adapter
>   					ret = -ENOMEM;
>   					break;
>   				}
> -				/* First write sets block length */
> -				b->len = len;
>   			}
> +			/* Largest write sets read block length */
> +			if (len > b->len)
> +				b->len = len;
>   			for (i = 0; i < len; i++)
>   				b->block[i] = data->block[i + 1];
>   			/* update for byte and word commands */
>
> Would that work for you?
>

Yes, sure, that works fine.

Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

Thanks,
Guenter

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Guenter Roeck]

Hi Jean,

On 07/12/2014 02:20 AM, Jean Delvare wrote:
> Hi Guenter,
>

Something else:

Any idea how we could inject errors ? Error path testing would be quite useful.

Thanks,
Guenter

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Jean Delvare]

Hi Guenter,

On Sat, 12 Jul 2014 08:05:49 -0700, Guenter Roeck wrote:
> Any idea how we could inject errors ? Error path testing would be quite useful.

Good idea. This should probably be done with a sysfs attribute so that
it can be turned on and off as desired. Off by default, of course. Some
other subsystems already support error injection, you could check how
they are doing it, do that we do not diverge needlessly.

Do you think there is any value in failing with different error codes,
or just -EIO is enough?

Do you think it should fail all the time when error injection is
enabled, or is there a value in having only a certain % of commands
fail?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Guenter Roeck]

On 07/13/2014 12:21 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Sat, 12 Jul 2014 08:05:49 -0700, Guenter Roeck wrote:
>> Any idea how we could inject errors ? Error path testing would be quite useful.
>
> Good idea. This should probably be done with a sysfs attribute so that
> it can be turned on and off as desired. Off by default, of course. Some
> other subsystems already support error injection, you could check how
> they are doing it, do that we do not diverge needlessly.
>
> Do you think there is any value in failing with different error codes,
> or just -EIO is enough?
>
How about writing the error code to return into the attribute ?
Write anything negative, and it is returned as error. Write 0,
and the driver works as normal.

> Do you think it should fail all the time when error injection is
> enabled, or is there a value in having only a certain % of commands
> fail?
>
For my purposes I would want it to fail reliably. We could add some fanciness,
though: Provide a second attribute which specifies how many operations should
pass before the first failure.

Thanks,
Guenter

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Jean Delvare]

On Sun, 13 Jul 2014 08:04:54 -0700, Guenter Roeck wrote:
> On 07/13/2014 12:21 AM, Jean Delvare wrote:
> > Hi Guenter,
> >
> > On Sat, 12 Jul 2014 08:05:49 -0700, Guenter Roeck wrote:
> >> Any idea how we could inject errors ? Error path testing would be quite useful.
> >
> > Good idea. This should probably be done with a sysfs attribute so that
> > it can be turned on and off as desired. Off by default, of course. Some
> > other subsystems already support error injection, you could check how
> > they are doing it, do that we do not diverge needlessly.
> >
> > Do you think there is any value in failing with different error codes,
> > or just -EIO is enough?
>
> How about writing the error code to return into the attribute ?
> Write anything negative, and it is returned as error. Write 0,
> and the driver works as normal.

This is smart, I like it :)

> > Do you think it should fail all the time when error injection is
> > enabled, or is there a value in having only a certain % of commands
> > fail?
>
> For my purposes I would want it to fail reliably. We could add some fanciness,
> though: Provide a second attribute which specifies how many operations should
> pass before the first failure.

Let's start simple and just implement what you need.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Guenter Roeck]

On 07/13/2014 08:13 AM, Jean Delvare wrote:
> On Sun, 13 Jul 2014 08:04:54 -0700, Guenter Roeck wrote:
>> On 07/13/2014 12:21 AM, Jean Delvare wrote:
>>> Hi Guenter,
>>>
>>> On Sat, 12 Jul 2014 08:05:49 -0700, Guenter Roeck wrote:
>>>> Any idea how we could inject errors ? Error path testing would be quite useful.
>>>
>>> Good idea. This should probably be done with a sysfs attribute so that
>>> it can be turned on and off as desired. Off by default, of course. Some
>>> other subsystems already support error injection, you could check how
>>> they are doing it, do that we do not diverge needlessly.
>>>
>>> Do you think there is any value in failing with different error codes,
>>> or just -EIO is enough?
>>
>> How about writing the error code to return into the attribute ?
>> Write anything negative, and it is returned as error. Write 0,
>> and the driver works as normal.
>
> This is smart, I like it :)
>
>>> Do you think it should fail all the time when error injection is
>>> enabled, or is there a value in having only a certain % of commands
>>> fail?
>>
>> For my purposes I would want it to fail reliably. We could add some fanciness,
>> though: Provide a second attribute which specifies how many operations should
>> pass before the first failure.
>
> Let's start simple and just implement what you need.
>

I would actually benefit from both. The ability to return an error unconditionally
lets me test the first error path. The ability to return an error starting with the
n-th transfer lets me test the n-th error path.

Guenter

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Sanford Rockowitz]

Forgive me for jumping in.   I'm a noob at I2C.   But I have built a 
couple substantial error injection frameworks over the years, one for a 
mainframe DBMS and one for Java's checked exceptions.   So while I have 
nothing useful to say about how to inject exceptions here, I have 
thought a lot about use cases.

Failing all the time is the necessary first step.  It allows for testing 
error paths without manually inserting a failure and recompiling, and 
makes it possible to build unit tests.

Failing randomly (or pseudo-randomly) is important for testing the 
overall recovery mechanism, particularly where you have an inherently 
unreliable subsystem like networks or I2C.   By changing the failure 
rate you can explore, for example, at what point the failure rate of the 
lower level system becomes so great that it makes the upper level system 
unreliable.

The one use case I would add, and it may be outside the scope here, is 
data errors.  I've been using the DDC protocol over I2C to communicate 
with monitors.  The DDC Get Capabilities request entails multiple 
write/read exchanges, with responses of up to 37 bytes each.   Most of 
the time this works ok, but I have one monitor that produces a high 
volume of data errors (double bytes or missing bytes).   This is only 
detected by examining the data itself (fixed fields and checksum).

Sanford



On 07/13/2014 08:46 AM, Guenter Roeck wrote:
> On 07/13/2014 08:13 AM, Jean Delvare wrote:
>> On Sun, 13 Jul 2014 08:04:54 -0700, Guenter Roeck wrote:
>>> On 07/13/2014 12:21 AM, Jean Delvare wrote:
>>>> Hi Guenter,
>>>>
>>>> On Sat, 12 Jul 2014 08:05:49 -0700, Guenter Roeck wrote:
>>>>> Any idea how we could inject errors ? Error path testing would be 
>>>>> quite useful.
>>>>
>>>> Good idea. This should probably be done with a sysfs attribute so that
>>>> it can be turned on and off as desired. Off by default, of course. 
>>>> Some
>>>> other subsystems already support error injection, you could check how
>>>> they are doing it, do that we do not diverge needlessly.
>>>>
>>>> Do you think there is any value in failing with different error codes,
>>>> or just -EIO is enough?
>>>
>>> How about writing the error code to return into the attribute ?
>>> Write anything negative, and it is returned as error. Write 0,
>>> and the driver works as normal.
>>
>> This is smart, I like it :)
>>
>>>> Do you think it should fail all the time when error injection is
>>>> enabled, or is there a value in having only a certain % of commands
>>>> fail?
>>>
>>> For my purposes I would want it to fail reliably. We could add some 
>>> fanciness,
>>> though: Provide a second attribute which specifies how many 
>>> operations should
>>> pass before the first failure.
>>
>> Let's start simple and just implement what you need.
>>
>
> I would actually benefit from both. The ability to return an error 
> unconditionally
> lets me test the first error path. The ability to return an error 
> starting with the
> n-th transfer lets me test the n-th error path.
>
> Guenter
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]

On Mon, Jul 07, 2014 at 07:23:03AM -0700, Guenter Roeck wrote:
> SMBus block commands are different to I2C block commands since
> the returned data is not normally accessible with byte or word
> commands on other command offsets. Add linked list of 'block'
> commands to support those commands.
> 
> Access mechanism is quite simple: Block commands must be written
> before they can be read. The first write selects the block length.
> Subsequent writes can be partial. Block read commands always return
> the number of bytes selected with the first write.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Make new functionality only available on request via functionality
>     module parameter
>     Add more details about SMBus block mode support to documentation
>     Use correct sizeof() variable in devm_kzalloc
>     Use stub_find_block() only in SMBus block command itself.
>     Store first word of block data in chip->words[].
>     When writing block data and the written data is longer than
>     the first write, bail out with debug message indicating the reason
>     for the error.

Thanks for doing this and thanks to Jean for the thorough review.

The thing I miss is the documentation that SMBUS_BLOCK must explicitly
be activated and the description how to do it. A comment in the driver,
above STUB_FUNC_* defines might not hurt as well.

Also, I'd prefer to squash Jean's "increasing block length" patch into
this one if both of you are fine with that?

Regards,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

[Jean Delvare]

On Thu, 17 Jul 2014 15:21:35 +0200, Wolfram Sang wrote:
> On Mon, Jul 07, 2014 at 07:23:03AM -0700, Guenter Roeck wrote:
> > SMBus block commands are different to I2C block commands since
> > the returned data is not normally accessible with byte or word
> > commands on other command offsets. Add linked list of 'block'
> > commands to support those commands.
> > 
> > Access mechanism is quite simple: Block commands must be written
> > before they can be read. The first write selects the block length.
> > Subsequent writes can be partial. Block read commands always return
> > the number of bytes selected with the first write.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > v2: Make new functionality only available on request via functionality
> >     module parameter
> >     Add more details about SMBus block mode support to documentation
> >     Use correct sizeof() variable in devm_kzalloc
> >     Use stub_find_block() only in SMBus block command itself.
> >     Store first word of block data in chip->words[].
> >     When writing block data and the written data is longer than
> >     the first write, bail out with debug message indicating the reason
> >     for the error.
> 
> Thanks for doing this and thanks to Jean for the thorough review.

You're welcome.

> The thing I miss is the documentation that SMBUS_BLOCK must explicitly
> be activated and the description how to do it. A comment in the driver,
> above STUB_FUNC_* defines might not hurt as well.

Good point.

> Also, I'd prefer to squash Jean's "increasing block length" patch into
> this one if both of you are fine with that?

Yes, I would be perfectly fine with that.

Guenter, can you please send an updated patch?

-- 
Jean Delvare
SUSE L3 Support

[PATCH v3] i2c: stub: Add support for SMBus block commands

[Guenter Roeck]

SMBus block commands are different to I2C block commands since
the returned data is not normally accessible with byte or word
commands on other command offsets. Add linked list of 'block'
commands to support those commands.

Access mechanism is quite simple: Block commands must be written
before they can be read. Subsequent writes can be partial. Block
read commands always return the number of bytes associated with
the longest previous write.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
v3: Document that SMBus block commands are by default disabled and how
    to enable them.
    Merge Jean's "increasing block length" patch into this one.
v2: Make new functionality only available on request via functionality
    module parameter.
    Add more details about SMBus block mode support to documentation.
    Use correct sizeof() variable in devm_kzalloc.
    Use stub_find_block() only in SMBus block command itself.
    Store first word of block data in chip->words[].
    When writing block data and the written data is longer than
    the first write, bail out with debug message indicating the reason
    for the error.

 Documentation/i2c/i2c-stub | 12 +++++-
 drivers/i2c/i2c-stub.c     | 99 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/Documentation/i2c/i2c-stub b/Documentation/i2c/i2c-stub
index fa4b669..a0fe7a0 100644
--- a/Documentation/i2c/i2c-stub
+++ b/Documentation/i2c/i2c-stub
@@ -2,9 +2,9 @@ MODULE: i2c-stub
 
 DESCRIPTION:
 
-This module is a very simple fake I2C/SMBus driver.  It implements five
+This module is a very simple fake I2C/SMBus driver.  It implements six
 types of SMBus commands: write quick, (r/w) byte, (r/w) byte data, (r/w)
-word data, and (r/w) I2C block data.
+word data, (r/w) I2C block data, and (r/w) SMBus block data.
 
 You need to provide chip addresses as a module parameter when loading this
 driver, which will then only react to SMBus commands to these addresses.
@@ -19,6 +19,14 @@ A pointer register with auto-increment is implemented for all byte
 operations.  This allows for continuous byte reads like those supported by
 EEPROMs, among others.
 
+SMBus block command support is disabled by default, and must be enabled
+explicitly by setting the respective bits (0x03000000) in the functionality
+module parameter.
+
+SMBus block commands must be written to configure an SMBus command for
+SMBus block operations. Writes can be partial. Block read commands always
+return the number of bytes selected with the largest write so far.
+
 The typical use-case is like this:
 	1. load this module
 	2. use i2cset (from the i2c-tools project) to pre-load some data
diff --git a/drivers/i2c/i2c-stub.c b/drivers/i2c/i2c-stub.c
index 77e4849..e0bb465 100644
--- a/drivers/i2c/i2c-stub.c
+++ b/drivers/i2c/i2c-stub.c
@@ -27,29 +27,70 @@
 #include <linux/slab.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
+#include <linux/list.h>
 
 #define MAX_CHIPS 10
-#define STUB_FUNC (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | \
-		   I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | \
-		   I2C_FUNC_SMBUS_I2C_BLOCK)
+
+/*
+ * Support for I2C_FUNC_SMBUS_BLOCK_DATA is disabled by default and must
+ * be enabled explicitly by setting the I2C_FUNC_SMBUS_BLOCK_DATA bits
+ * in the 'functionality' module parameter.
+ */
+#define STUB_FUNC_DEFAULT \
+		(I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | \
+		 I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | \
+		 I2C_FUNC_SMBUS_I2C_BLOCK)
+
+#define STUB_FUNC_ALL \
+		(STUB_FUNC_DEFAULT | I2C_FUNC_SMBUS_BLOCK_DATA)
 
 static unsigned short chip_addr[MAX_CHIPS];
 module_param_array(chip_addr, ushort, NULL, S_IRUGO);
 MODULE_PARM_DESC(chip_addr,
 		 "Chip addresses (up to 10, between 0x03 and 0x77)");
 
-static unsigned long functionality = STUB_FUNC;
+static unsigned long functionality = STUB_FUNC_DEFAULT;
 module_param(functionality, ulong, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(functionality, "Override functionality bitfield");
 
+struct smbus_block_data {
+	struct list_head node;
+	u8 command;
+	u8 len;
+	u8 block[I2C_SMBUS_BLOCK_MAX];
+};
+
 struct stub_chip {
 	u8 pointer;
 	u16 words[256];		/* Byte operations use the LSB as per SMBus
 				   specification */
+	struct list_head smbus_blocks;
 };
 
 static struct stub_chip *stub_chips;
 
+static struct smbus_block_data *stub_find_block(struct device *dev,
+						struct stub_chip *chip,
+						u8 command, bool create)
+{
+	struct smbus_block_data *b, *rb = NULL;
+
+	list_for_each_entry(b, &chip->smbus_blocks, node) {
+		if (b->command == command) {
+			rb = b;
+			break;
+		}
+	}
+	if (rb == NULL && create) {
+		rb = devm_kzalloc(dev, sizeof(*rb), GFP_KERNEL);
+		if (rb == NULL)
+			return rb;
+		rb->command = command;
+		list_add(&rb->node, &chip->smbus_blocks);
+	}
+	return rb;
+}
+
 /* Return negative errno on error. */
 static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags,
 	char read_write, u8 command, int size, union i2c_smbus_data *data)
@@ -57,6 +98,7 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags,
 	s32 ret;
 	int i, len;
 	struct stub_chip *chip = NULL;
+	struct smbus_block_data *b;
 
 	/* Search for the right chip */
 	for (i = 0; i < MAX_CHIPS && chip_addr[i]; i++) {
@@ -148,6 +190,51 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags,
 		ret = 0;
 		break;
 
+	case I2C_SMBUS_BLOCK_DATA:
+		b = stub_find_block(&adap->dev, chip, command, false);
+		if (read_write == I2C_SMBUS_WRITE) {
+			len = data->block[0];
+			if (len == 0 || len > I2C_SMBUS_BLOCK_MAX) {
+				ret = -EINVAL;
+				break;
+			}
+			if (b == NULL) {
+				b = stub_find_block(&adap->dev, chip, command,
+						    true);
+				if (b == NULL) {
+					ret = -ENOMEM;
+					break;
+				}
+			}
+			/* Largest write sets read block length */
+			if (len > b->len)
+				b->len = len;
+			for (i = 0; i < len; i++)
+				b->block[i] = data->block[i + 1];
+			/* update for byte and word commands */
+			chip->words[command] = (b->block[0] << 8) | b->len;
+			dev_dbg(&adap->dev,
+				"smbus block data - addr 0x%02x, wrote %d bytes at 0x%02x.\n",
+				addr, len, command);
+		} else {
+			if (b == NULL) {
+				dev_dbg(&adap->dev,
+					"SMBus block read command without prior block write not supported\n");
+				ret = -EOPNOTSUPP;
+				break;
+			}
+			len = b->len;
+			data->block[0] = len;
+			for (i = 0; i < len; i++)
+				data->block[i + 1] = b->block[i];
+			dev_dbg(&adap->dev,
+				"smbus block data - addr 0x%02x, read  %d bytes at 0x%02x.\n",
+				addr, len, command);
+		}
+
+		ret = 0;
+		break;
+
 	default:
 		dev_dbg(&adap->dev, "Unsupported I2C/SMBus command\n");
 		ret = -EOPNOTSUPP;
@@ -159,7 +246,7 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags,
 
 static u32 stub_func(struct i2c_adapter *adapter)
 {
-	return STUB_FUNC & functionality;
+	return STUB_FUNC_ALL & functionality;
 }
 
 static const struct i2c_algorithm smbus_algorithm = {
@@ -199,6 +286,8 @@ static int __init i2c_stub_init(void)
 		pr_err("i2c-stub: Out of memory\n");
 		return -ENOMEM;
 	}
+	for (i--; i >= 0; i--)
+		INIT_LIST_HEAD(&stub_chips[i].smbus_blocks);
 
 	ret = i2c_add_adapter(&stub_adapter);
 	if (ret)
-- 
1.9.1

Re: [PATCH v3] i2c: stub: Add support for SMBus block commands

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

On Thu, Jul 17, 2014 at 09:56:03AM -0700, Guenter Roeck wrote:
> SMBus block commands are different to I2C block commands since
> the returned data is not normally accessible with byte or word
> commands on other command offsets. Add linked list of 'block'
> commands to support those commands.
> 
> Access mechanism is quite simple: Block commands must be written
> before they can be read. Subsequent writes can be partial. Block
> read commands always return the number of bytes associated with
> the longest previous write.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Applied to for-next, thanks! I took the liberty to add Jean's Rev-by tag
from V2.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]