| Message ID | 20140712112019.618d8a03@endymion.delvare |
|---|---|
| State | Superseded |
| Headers | show |
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@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? > Yes, sure, that works fine. Reviewed-by: Guenter Roeck <linux@roeck-us.net> Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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 */