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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 12 Jul 2014 11:20:19 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Wolfram Sang <wsa@...-dreams.de>,
	Randy Dunlap <rdunlap@...radead.org>,
	linux-i2c@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] i2c: stub: Add support for SMBus block commands

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@...e.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@...e.de>
Cc: Guenter Roeck <linux@...ck-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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/