[RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[daniel.stodden]

From: Daniel Stodden <dns@arista.com>

This is purely WIP, I haven't had a chance to run this myself. Posting
early just to attract some more commentary. But I'm hopeful to have
this running on my own hardware later today.

First revision aims to maintain smallest kernel + user code footprint,
while not breaking compatibility with old user binaries.

That said, I like the direction, but recognize it may not come out of
review as ideal. Maybe it's just small.

 * I suggest to just settle on '3' for new macro and type names
   (I2C_SMBUS3_*, i2c_smbus3_*)

 * Block size definitions maintain I2C_SMBUS_BLOCK_MAX (32). Only adds
   I2C_SMBUS3_BLOCK_MAX (255)

   - Means that drivers in drivers/i2c/busses/ default to their safe
     32B block limit without refactoring.

 * Does not fork i2c_smbus_data, or .block. Instead, new builds may
   adapt, while i2c-dev.c deals with legacy block byte counts as
   numeric limits only. It didn't seem to sacrifice readability.

I2C_SMBUS:

 * Add 3 new transaction types for
     - I2C_SMBUS_BLOCK_DATA,
     - I2C_SMBUS_BLOCK_PROC_CALL
     - I2C_SMBUS_I2C_BLOCK_DATA

 * These take the names of the old types, not a "3" in a new
   transfer name. Meaning new builds will just adapt.

 * Leaves out I2C_SMBUS_I2C_BLOCK_BROKEN, assuming it won't be
   missed.

 * Allocated bit 4 (I2C_SMBUS3_BLOCK=0x10), to simplify Smbus2
   compatibility: I2C_SMBUS_*BLOCK* = (<old type>|0x10)

   Therefore, i2cdev_ioctl_smbus() can

   1. Test for the bit

   2. Adjust copy_from_user block length accordingly.

   3. Proceed into i2c_smbus_xfer, with smbus3 limits established,
      and size likewise mapped to smbus3 transaction type names,
      for the remainder of the call.

   4. upon return from i2c_smbus_xfer, a user block size conflict
      will result in -EMSGSIZE.

I2C_RDWR:

 * No uapi changes.

 * Like i2c_smbus_xfer, kernel i2c_transfer calls assume safe smbus3
   block limits.

 * Like i2cdev_ioctl_smbus, i2cdev_ioctl_rdwr deals with all
   backward compatibility:

   - User block limits assuming 32 bytes are detected and translate
     to and from kernel block limits established at 255 bytes.

   - Upon return from i2c_transfer, a user block size conflict will
     result in -EMSGSIZE.

 * The most straightforward change to adjust kernel buffers it to

   - Memdup msgs[i].len user bytes, as before.
     This makes msgs[i].block[0] available early.

   - Only enforce
          msgs[i].blocks[0] + I2C_SMBUS_BLOCK_MAX
     byte sized user buffers, as used to be.

   - If smaller than I2C_SMBUS3_BLOCK_MAX, krealloc() kernel buffers to
          msgs[i].blocks[0] + I2C_SMBUS3_BLOCK_MAX
     bytes.

   Implies only a small penalty, and on compat calls only. New builds
   walk the preferred path as it used to be. It also results in a
   comparatively small and straightforward diff.
---
 drivers/i2c/i2c-dev.c    | 82 ++++++++++++++++++++++++++++++++++------
 include/uapi/linux/i2c.h | 17 ++++++---
 2 files changed, 81 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index da020acc9bbd..99f291f519b3 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -236,6 +236,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 		unsigned nmsgs, struct i2c_msg *msgs)
 {
 	u8 __user **data_ptrs;
+	u16 *user_len = NULL;
 	int i, res;
 
 	data_ptrs = kmalloc_array(nmsgs, sizeof(u8 __user *), GFP_KERNEL);
@@ -244,6 +245,12 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
+	user_len = kmalloc_array(nmsgs, sizeof(*user_len), GFP_KERNEL);
+	if (!user_len) {
+		res = -ENOMEM;
+		goto out;
+	}
+
 	res = 0;
 	for (i = 0; i < nmsgs; i++) {
 		/* Limit the size of the message to a sane amount */
@@ -262,16 +269,34 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 		msgs[i].flags |= I2C_M_DMA_SAFE;
 
 		/*
-		 * If the message length is received from the slave (similar
-		 * to SMBus block read), we must ensure that the buffer will
-		 * be large enough to cope with a message length of
-		 * I2C_SMBUS_BLOCK_MAX as this is the maximum underlying bus
-		 * drivers allow. The first byte in the buffer must be
-		 * pre-filled with the number of extra bytes, which must be
-		 * at least one to hold the message length, but can be
-		 * greater (for example to account for a checksum byte at
-		 * the end of the message.)
+		 * If the block length is received from the slave
+		 * (similar to SMBus block read), we ensure that
+		 *
+		 *  - the user buffer is large enough to hold a
+		 *    message length of I2C_SMBUS_BLOCK_MAX (32), as
+		 *    this is what any Smbus version allows.
+		 *
+		 *  - the kernel buffer is large enough to hold a
+		 *    message length of I2C_SMBUS3_BLOCK_MAX (255),
+		 *    which is what Smbus >= 3.0 allows.
+		 *
+		 * Kernel block lengths up to I2SMBUS3_BLOCK_MAX
+		 * ensure that drivers can always return up to 255
+		 * bytes safely.
+		 *
+		 * User block lengths up to only I2C_SMBUS_BLOCK_MAX
+		 * are supported for backward compatibility. If an
+		 * Smbus 3.0 slave produces a longer message than
+		 * userspace provides for, we truncate the user copy
+		 * and return -EMSGSIZE.
+		 *
+		 * The first byte in the user buffer must be
+		 * pre-filled with the number of extra bytes, at least
+		 * one to hold the message length, but can be greater
+		 * (for example to account for a checksum byte at the
+		 * end of the message.)
 		 */
+		user_len[i] = msgs[i].len;
 		if (msgs[i].flags & I2C_M_RECV_LEN) {
 			if (!(msgs[i].flags & I2C_M_RD) ||
 			    msgs[i].len < 1 || msgs[i].buf[0] < 1 ||
@@ -282,6 +307,21 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 				break;
 			}
 
+			if (msgs[i].len < msgs[i].buf[0] +
+					   I2C_SMBUS3_BLOCK_MAX) {
+				u8* buf =
+					krealloc(msgs[i].buf,
+						 msgs[i].buf[0] +
+						 I2C_SMBUS3_BLOCK_MAX,
+						 GFP_KERNEL);
+				if (!buf) {
+					i++;
+					res = -ENOMEM;
+					break;
+				}
+				msgs[i].buf = buf;
+			}
+
 			msgs[i].len = msgs[i].buf[0];
 		}
 	}
@@ -298,11 +338,15 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
 	while (i-- > 0) {
 		if (res >= 0 && (msgs[i].flags & I2C_M_RD)) {
 			if (copy_to_user(data_ptrs[i], msgs[i].buf,
-					 msgs[i].len))
+					 min(msgs[i].len, user_len[i])))
 				res = -EFAULT;
+			if (msgs[i].len > user_len[i])
+				res = res ? : -EMSGSIZE;
 		}
 		kfree(msgs[i].buf);
 	}
+out:
+	kfree(user_len);
 	kfree(data_ptrs);
 	kfree(msgs);
 	return res;
@@ -313,7 +357,19 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
 		union i2c_smbus_data __user *data)
 {
 	union i2c_smbus_data temp = {};
-	int datasize, res;
+	int block_max, datasize, res;
+
+	if (!(size & I2C_SMBUS3_BLOCK)) {
+		switch (size | I2C_SMBUS3_BLOCK) {
+		case I2C_SMBUS_BLOCK_DATA:
+		case I2C_SMBUS_BLOCK_PROC_CALL:
+		case I2C_SMBUS_I2C_BLOCK_DATA:
+			size |= I2C_SMBUS3_BLOCK;
+		}
+
+		block_max = I2C_SMBUS_BLOCK_MAX;
+	} else
+		block_max = I2C_SMBUS3_BLOCK_MAX;
 
 	if ((size != I2C_SMBUS_BYTE) &&
 	    (size != I2C_SMBUS_QUICK) &&
@@ -362,7 +418,7 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
 		 (size == I2C_SMBUS_PROC_CALL))
 		datasize = sizeof(data->word);
 	else /* size == smbus block, i2c block, or block proc. call */
-		datasize = sizeof(data->block);
+		datasize = block_max + 2;
 
 	if ((size == I2C_SMBUS_PROC_CALL) ||
 	    (size == I2C_SMBUS_BLOCK_PROC_CALL) ||
@@ -385,6 +441,8 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
 		     (read_write == I2C_SMBUS_READ))) {
 		if (copy_to_user(data, &temp, datasize))
 			return -EFAULT;
+		if (temp.block[0] > block_max)
+			return -EMSGSIZE;
 	}
 	return res;
 }
diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index f71a1751cacf..93f68de134c1 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -131,11 +131,13 @@ struct i2c_msg {
 /*
  * Data for SMBus Messages
  */
-#define I2C_SMBUS_BLOCK_MAX	32	/* As specified in SMBus standard */
+#define I2C_SMBUS_BLOCK_MAX	32	/* As specified in SMBus standard <= 3.0 */
+#define I2C_SMBUS3_BLOCK_MAX	255	/* As specified in SMBus 3.0 */
+
 union i2c_smbus_data {
 	__u8 byte;
 	__u16 word;
-	__u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
+	__u8 block[I2C_SMBUS3_BLOCK_MAX + 2]; /* block[0] is used for length */
 			       /* and one more for user-space compatibility */
 };
 
@@ -145,14 +147,17 @@ union i2c_smbus_data {
 
 /* SMBus transaction types (size parameter in the above functions)
    Note: these no longer correspond to the (arbitrary) PIIX4 internal codes! */
-#define I2C_SMBUS_QUICK		    0
+
+#define I2C_SMBUS3_BLOCK 0x10
+
+#define I2C_SMBUS_QUICK	    0
 #define I2C_SMBUS_BYTE		    1
 #define I2C_SMBUS_BYTE_DATA	    2
 #define I2C_SMBUS_WORD_DATA	    3
 #define I2C_SMBUS_PROC_CALL	    4
-#define I2C_SMBUS_BLOCK_DATA	    5
+#define I2C_SMBUS_BLOCK_DATA	    (5|I2C_SMBUS3_BLOCK)
 #define I2C_SMBUS_I2C_BLOCK_BROKEN  6
-#define I2C_SMBUS_BLOCK_PROC_CALL   7		/* SMBus 2.0 */
-#define I2C_SMBUS_I2C_BLOCK_DATA    8
+#define I2C_SMBUS_BLOCK_PROC_CALL   (7|I2C_SMBUS3_BLOCK) /* SMBus >= 2.0 */
+#define I2C_SMBUS_I2C_BLOCK_DATA    (8|I2C_SMBUS3_BLOCK)
 
 #endif /* _UAPI_LINUX_I2C_H */
-- 
2.20.1

Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Wolfram Sang]

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

Hi Daniel,

wow, that was fast! Thanks for the prototype.

>  * I suggest to just settle on '3' for new macro and type names
>    (I2C_SMBUS3_*, i2c_smbus3_*)

Yes, I agree.

> 
>  * Block size definitions maintain I2C_SMBUS_BLOCK_MAX (32). Only adds
>    I2C_SMBUS3_BLOCK_MAX (255)
> 
>    - Means that drivers in drivers/i2c/busses/ default to their safe
>      32B block limit without refactoring.

This is totally fine for this patch. However, I still think I will do
the renaming to I2C_SMBUS2_BLOCK_MAX in kernel space later. Just so
people will understand by looking at the code that this is an old limit
which can be removed if there is interest.

> -	__u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
> +	__u8 block[I2C_SMBUS3_BLOCK_MAX + 2]; /* block[0] is used for length */
>  			       /* and one more for user-space compatibility */

I thought about this, too, and wondered if this isn't a size regression
in userspace with every i2c_smbus_data getting 8 times the size? But
maybe it is worth if backwards compatibility is maintained in an
otherwise not so intrusive manner? Jean, what do you think?

Happy hacking everyone,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Daniel Stodden]

> On Jul 28, 2020, at 2:40 AM, Wolfram Sang <wsa@kernel.org> wrote:
> 
> Hi Daniel,
> 
> wow, that was fast! Thanks for the prototype.
> 
>> * I suggest to just settle on '3' for new macro and type names
>>   (I2C_SMBUS3_*, i2c_smbus3_*)
> 
> Yes, I agree.
> 
>> 
>> * Block size definitions maintain I2C_SMBUS_BLOCK_MAX (32). Only adds
>>   I2C_SMBUS3_BLOCK_MAX (255)
>> 
>>   - Means that drivers in drivers/i2c/busses/ default to their safe
>>     32B block limit without refactoring.
> 
> This is totally fine for this patch. However, I still think I will do
> the renaming to I2C_SMBUS2_BLOCK_MAX in kernel space later. Just so
> people will understand by looking at the code that this is an old limit
> which can be removed if there is interest.
> 
>> -	__u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
>> +	__u8 block[I2C_SMBUS3_BLOCK_MAX + 2]; /* block[0] is used for length */
>> 			       /* and one more for user-space compatibility */
> 
> I thought about this, too, and wondered if this isn't a size regression
> in userspace with every i2c_smbus_data getting 8 times the size? But
> maybe it is worth if backwards compatibility is maintained in an
> otherwise not so intrusive manner? Jean, what do you think?

Yep, exactly. It just made for a nice drop-in replacement for me to
focus on i2c-dev.c first.

A lot of clients will stack-allocate these. I suppose i2-tools doesn’t
load more than one at a time, but haven’t looked.

Retrospectively
- i2c_smbus_ioctl_data.data shouldn’t have been a pointer type, but is.
- i2c_smbus_data.block should have been pointer, but isn’t

And then the kernel would pass around an 32-bit-only i2c_smbus_data union -- by value.
Which would have been much leaner, and leave the right buffer choice
entirely to the client.

One could explore this in kernel space. Let me know how you’d like to experiment.
But in userspace that means we’re looking at a new call number. >:)

Cheers,
Daniel

Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Wolfram Sang]

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


> > I thought about this, too, and wondered if this isn't a size regression
> > in userspace with every i2c_smbus_data getting 8 times the size? But
> > maybe it is worth if backwards compatibility is maintained in an
> > otherwise not so intrusive manner? Jean, what do you think?
> 
> Yep, exactly. It just made for a nice drop-in replacement for me to
> focus on i2c-dev.c first.
> 
> A lot of clients will stack-allocate these. I suppose i2-tools doesn’t
> load more than one at a time, but haven’t looked.

I just checked all in-kernel users and i2c-tools. Never more than one.
Which is kind of expected because you usually re-use i2c_smbus_data and
move the buffer contents somewhere else.

> Retrospectively
> - i2c_smbus_ioctl_data.data shouldn’t have been a pointer type, but is.
> - i2c_smbus_data.block should have been pointer, but isn’t
> 
> And then the kernel would pass around an 32-bit-only i2c_smbus_data union -- by value.
> Which would have been much leaner, and leave the right buffer choice
> entirely to the client.
> 
> One could explore this in kernel space. Let me know how you’d like to experiment.

? I'd like to keep kernel space and user space in sync. Why should we
have a different i2c_smbus_data-variant in the kernel space? Or did I
get you wrong.

> But in userspace that means we’re looking at a new call number. >:)

No way! :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Daniel Stodden]

> On Jul 28, 2020, at 3:36 AM, Wolfram Sang <wsa@kernel.org> wrote:
> 
> 
>>> I thought about this, too, and wondered if this isn't a size regression
>>> in userspace with every i2c_smbus_data getting 8 times the size? But
>>> maybe it is worth if backwards compatibility is maintained in an
>>> otherwise not so intrusive manner? Jean, what do you think?
>> 
>> Yep, exactly. It just made for a nice drop-in replacement for me to
>> focus on i2c-dev.c first.
>> 
>> A lot of clients will stack-allocate these. I suppose i2-tools doesn’t
>> load more than one at a time, but haven’t looked.
> 
> I just checked all in-kernel users and i2c-tools. Never more than one.
> Which is kind of expected because you usually re-use i2c_smbus_data and
> move the buffer contents somewhere else.
> 
>> Retrospectively
>> - i2c_smbus_ioctl_data.data shouldn’t have been a pointer type, but is.
>> - i2c_smbus_data.block should have been pointer, but isn’t
>> 
>> And then the kernel would pass around an 32-bit-only i2c_smbus_data union -- by value.
>> Which would have been much leaner, and leave the right buffer choice
>> entirely to the client.
>> 
>> One could explore this in kernel space. Let me know how you’d like to experiment.
> 
> ? I'd like to keep kernel space and user space in sync. Why should we
> have a different i2c_smbus_data-variant in the kernel space? Or did I
> get you wrong.
> 
>> But in userspace that means we’re looking at a new call number. >:)
> 
> No way! :)

Here’s about what I meant.

Ok, it doesn’t really need a new call slot. Although, when
integrating into the original i2c_smbus_data and I2C_SMBUS, I’m not sure how
confusing this gets, to a casual reader not familiar with the history.

Daniel

diff --git a/include/uapi/linux/i2c-dev.h b/include/uapi/linux/i2c-dev.h
index 85f8047afcf2..38b1ae4d2448 100644
--- a/include/uapi/linux/i2c-dev.h
+++ b/include/uapi/linux/i2c-dev.h
@@ -58,7 +58,10 @@ struct i2c_smbus_ioctl_data {
        __u8 read_write;
        __u8 command;
        __u32 size;
-       union i2c_smbus_data __user *data;
+       union {
+               union i2c_smbus_data __user *data;
+               union i2c_smbus3_data __user u;
+       };
 };

 /* This is the structure as used in the I2C_RDWR ioctl call */
diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index f71a1751cacf..87ec2ea321ce 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -131,7 +131,9 @@ struct i2c_msg {
 /*
  * Data for SMBus Messages
  */
-#define I2C_SMBUS_BLOCK_MAX    32      /* As specified in SMBus standard */
+#define I2C_SMBUS_BLOCK_MAX    32      /* As specified in SMBus standard <= 3.0 */
+#define I2C_SMBUS3_BLOCK_MAX   255     /* As specified in SMBus 3.0 */
+
 union i2c_smbus_data {
        __u8 byte;
        __u16 word;
@@ -139,20 +141,36 @@ union i2c_smbus_data {
                               /* and one more for user-space compatibility */
 };

+typedef __u8 i2c_smbus_block[I2C_SMBUS_BLOCK_MAX + 2];
+typedef __u8 i2c_smbus3_block[I2C_SMBUS3_BLOCK_MAX + 2];
+
+union i2c_smbus3_data {
+       __u8 byte;
+       __u16 word;
+       i2c_smbus_block *block32;
+       i2c_smbus3_block *block255;
+};
+
 /* i2c_smbus_xfer read or write markers */
 #define I2C_SMBUS_READ 1
 #define I2C_SMBUS_WRITE        0

 /* SMBus transaction types (size parameter in the above functions)
    Note: these no longer correspond to the (arbitrary) PIIX4 internal codes! */
+
+#define I2C_SMBUS3_BLOCK 0x10
+
 #define I2C_SMBUS_QUICK                    0
 #define I2C_SMBUS_BYTE             1
 #define I2C_SMBUS_BYTE_DATA        2
 #define I2C_SMBUS_WORD_DATA        3
 #define I2C_SMBUS_PROC_CALL        4
 #define I2C_SMBUS_BLOCK_DATA       5
+#define I2C_SMBUS3_BLOCK_DATA      (5|I2C_SMBUS3_BLOCK)
 #define I2C_SMBUS_I2C_BLOCK_BROKEN  6
 #define I2C_SMBUS_BLOCK_PROC_CALL   7          /* SMBus 2.0 */
+#define I2C_SMBUS3_BLOCK_PROC_CALL  (7|I2C_SMBUS3_BLOCK)
 #define I2C_SMBUS_I2C_BLOCK_DATA    8
+#define I2C_SMBUS3_I2C_BLOCK_DATA   (8|I2C_SMBUS3_BLOCK)

 #endif /* _UAPI_LINUX_I2C_H */

— snip —

So:

 - ioctl_data.data would be inline. No more extra pointer derefs
   just to pass a .byte or .word

 - Likewise, kernel code gets to pass a leaner

       i2c_smbus_xfer( .., union i2c_smbus3_data data) {} etc.

   Note the lack of ‘*’ in ‘data’.

 - All clients get to choose between i2c_smbus_block or i2c_smbus3_block,
   depending on slave specs.

 - Similarly, code concerned about (stack) memory gets to pick 
   size = I2C_SMBUS_BLOCK_x vs I2C_SMBUS3_BLOCK_x.
   So we’d keep the old names in place, too.

 - But suffers terrible consequences when mixing I2C_SMBUS3_ transfers
   with i2c_smbus_data buffers.

   Really worth it? Sincerely, if 220-ish extra bytes aren’t
   a big deal, let’s step back and admit it adds not enough value.

 - Above won’t build yet, unless we’re okay with linux/i2c-dev.h 
   including linux/i2c.h.

Hth,
Daniel

Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Jean Delvare]

Hi Wolfram, Daniel,

On Tue, 28 Jul 2020 11:40:37 +0200, Wolfram Sang wrote:
> > -	__u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
> > +	__u8 block[I2C_SMBUS3_BLOCK_MAX + 2]; /* block[0] is used for length */
> >  			       /* and one more for user-space compatibility */  
> 
> I thought about this, too, and wondered if this isn't a size regression
> in userspace with every i2c_smbus_data getting 8 times the size? But
> maybe it is worth if backwards compatibility is maintained in an
> otherwise not so intrusive manner? Jean, what do you think?

In i2c-tools these are always allocated on the stack and one at a time.
Size isn't an issue in my opinion.

The only thing I'm truly concerned about here is compatibility. You
need to ensure that:

* User-space binaries that are around today (obviously compiled with
  the old versions of the kernel uapi headers) will work the same with
  kernels including your changes.

* User-space binaries compiled with the new kernel uapi headers will
  work with both old and new kernels.

For all transfer types. You will have to keep in mind that SMBus-style
and I2C-style read block transfers differ in who decides the length.
For I2C, it is the caller (I want to read N bytes from this location),
for SMBus it is the slave (I want to read a block from this location,
tell me how long it is). In both cases you need to ensure you do not
write beyond the buffer, no matter what, and return a proper error code
if it wouldn't fit.

The other compatibility type you need to care about is inside the
kernel itself: SMBus 2 and SMBus 3 controllers and devices may be mixed
up. Some (I expect most) SMBus 3 devices may be able to work with SMBus
2 controllers in "degraded" mode, in which case it will be the driver's
responsibility to check the capabilities of the controller and only use
transfer types that are supported. If such "degraded" mode is not
possible then the driver would simply refuse to bind. For such checks
to be possible, I would expect either one global I2C_FUNC_SMBUS* flag
to be added to denote SMBus 3 compliance, or possibly several flags for
finer grained control (seems overkill to me but you may have a
different opinion - also depends on what else SMBus 3 is introducing,
I admit I did not check). However I did not see that in your prototype.
Do you believe we can do without it? If so, please explain how.

-- 
Jean Delvare
SUSE L3 Support

Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Daniel Stodden]

Hi.

> On Jul 28, 2020, at 10:04 AM, Jean Delvare <jdelvare@suse.de> wrote:
> 
> Hi Wolfram, Daniel,
> 
> On Tue, 28 Jul 2020 11:40:37 +0200, Wolfram Sang wrote:
>>> -	__u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
>>> +	__u8 block[I2C_SMBUS3_BLOCK_MAX + 2]; /* block[0] is used for length */
>>> 			       /* and one more for user-space compatibility */  
>> 
>> I thought about this, too, and wondered if this isn't a size regression
>> in userspace with every i2c_smbus_data getting 8 times the size? But
>> maybe it is worth if backwards compatibility is maintained in an
>> otherwise not so intrusive manner? Jean, what do you think?
> 
> In i2c-tools these are always allocated on the stack and one at a time.
> Size isn't an issue in my opinion.

Cool. Meaning we may then leave it at a single i2c_smbus_data.block[257]
declaration in the headers, and i2c_smbus_data can be 255 bytes.

That doesn’t mean we shut the door on 32-byte buffers. After all, backward
compat requires we maintain support for those. Just that we need not
bother declaring or promoting dedicated small vs large transfer types
while noone wants one.

Code in need can still make its own. If we learn that there’s legitimate 
demand, we could add a i2c_smbus1_data later to sanction that.

> The only thing I'm truly concerned about here is compatibility. You
> need to ensure that:
> 
> * User-space binaries that are around today (obviously compiled with
>  the old versions of the kernel uapi headers) will work the same with
>  kernels including your changes.
> 
> * User-space binaries compiled with the new kernel uapi headers will
>  work with both old and new kernels.
> 
> For all transfer types. You will have to keep in mind that SMBus-style
> and I2C-style read block transfers differ in who decides the length.
> For I2C, it is the caller (I want to read N bytes from this location),
> for SMBus it is the slave (I want to read a block from this location,
> tell me how long it is). In both cases you need to ensure you do not
> write beyond the buffer, no matter what, and return a proper error code
> if it wouldn't fit.

Reverse compatibility would be a more interesting concern.

You’re saying you would need an actual universal binary?
This is indeed beyond only binary backward compatibility.

The headers can’t automate that for you, obviously. The macros would have
to be too use-case specific. 

It has to go into the library. But we can make sure we’re at least
not overcomplicating it.

But this can affect matters on how to name new vs old size macros.

For cases which are less interested in the reverse case, and not the
stack space increment either, making I2C_SMBUS_BLOCK_DATA the new smbus3
number was clearly the better choice.

It equates to source compatibility.

In contrast, if we need an I2C_SMBUS_BLOCK3_DATA instead, everything needs
a patch to move on.

Should we make that case slightly harder, to promote smbus3 defaults elsewhere?
For example, okay if an i2c library build has to compile conditionally, as in

#ifdef I2C_SMBUS_BLOCK3_DATA
# .. know that I2C_SMBUS_BLOCK_DATA means smbus3
#else
# .. know that I2C_SMBUS_BLOCK_DATA means smbus1
endif

to pull off the universal binary?

This would also mean we’re likely to add an I2C_FUNC_ to indicated smbus3
support at runtime. It’s probably what you already have in mind.

We’d probably still prefer to move all kernel/driver buffers to 255 bytes
unconditionally. However, I2C_FUNC_ presence normally indicates lower level
driver + hardware support, right? Not just a kernel upgrade. Which makes 
sense here, too.

I’m just pointing it out because if we want an I2C_FUNC_SMBUS3, and at the driver’s
discretion, not just indicating kernel + compatibility support presence,
and if i2c_utils uses that to pick safe size values, that again
means we’re yet again less likely to actually deprecate the old transfer numbers.
Some drivers will never be SMBUS3.

> The other compatibility type you need to care about is inside the
> kernel itself: SMBus 2 and SMBus 3 controllers and devices may be mixed
> up. Some (I expect most) SMBus 3 devices may be able to work with SMBus
> 2 controllers in "degraded" mode, in which case it will be the driver's
> responsibility to check the capabilities of the controller and only use
> transfer types that are supported.

Yes. This is what’s already happening. It’s the only workaround I’m aware
of to get PMBus devices working with what current kernels provide.

It’s basically why I’m here :)

Let’s say your have a device assuming smbus3 limits. If you have a legacy
smbus_xfer()-based controller you’re pretty much out of luck.

But if you know what you’ve wired together, you’ll have a master_xfer()
implementation with no such limits for fixed length reads 
(I2C_M_RD, but not I2C_M_RECV_LEN). Here is an example to execute a
PMBus command producing unknown length L > 32:

 1. Do a read_byte() on on that command. It will stop the transfer
    after the length field, but at least you know L now.

 2. Re-invoke the command, this time through I2C_RDWR,
    with a fixed message length (1 + L [ + pec]) to get the full transfer
    through. 32B is only for I2C_RECV_LEN.

It’s not pretty, but saves the day. Assume there is plenty of code around
which does such things.

> If such "degraded" mode is not
> possible then the driver would simply refuse to bind.

I’d prefer if we keep things as permissive as possible.

For above reasons: not to fence off workarounds which used to be possible.

We’re not anticipating the invention of smbus3 here. 
We’re not paving the way to get there. We’re not setting a standard.
We’re only trying to top getting in the of what already happened.

It seems perfecty fine to pass an smbus3 transfer from an smbus3 device
found under an smbus2 adapter, just by truncating the data.

The people who open(‘/dev/i2c-%d’) are often not the type which
roots for autoconfiguration or so-helpful warnings spamming their console.

Intuitively, I’d expect the above quiet truncation could be what several 
32B-controller designers may have chosen to do. E.g. when 
anticipating bit errors in the length prefix observed by their hardware 
block-read accellerator?

At least as far as kernel space is concerned, I’d leave it there.

I2c libraries may differ, to some degree. Users unhappy with a strict-mode
library then at least keep their chance to bypass the library and
grow their own ioctls().

> For such checks
> to be possible, I would expect either one global I2C_FUNC_SMBUS* flag
> to be added to denote SMBus 3 compliance, or possibly several flags for
> finer grained control (seems overkill to me but you may have a
> different opinion - also depends on what else SMBus 3 is introducing,
> I admit I did not check). However I did not see that in your prototype.
> Do you believe we can do without it? If so, please explain how.

I think in any case, we can make kernel-side buffers smbus3-sized
unconditionally.

Even if we had an I2C_FUNC_SMBUS3 to signal actual support. So far,
everyone confirms not doing so offers only minor savings.
But for a quite realistic bug scenario where drivers and clients
getting semantics wrong will honor that with occasional stack corruption.

If it sounds important: I recognize one could make the krealloc()
in i2cdev_ioctl_rdwr in the original patch dependent on an
I2C_FUNC_SMBUS3 from the driver, if we want to avoid it. But it would
be an oddball variant.

User buffers can be 32B or 255B, we just require the command to match,
because I2C_SMBUS has currently no other way to disambiguate message
length limits.

I2C_FUNC_SMBUS3 is nice to have. I’m not against it, I’m for it.
For i2c-tools and all clients, it can be valuable to confirm conflicts.

Still, one can keep it indicative only, and not synthesize faults 
because of a perceived client/adapter/device feature mismatch. 

We only don’t want to corrupt data. So far we don’t seem to need an
I2C_FUNC_SMBUS3 to accommodate that.

I’m not even sure if my new -EMSGSIZE condition in i2c-dev.c is such
a good idea.

If noone else thinks it’s critical, maybe we should drop that too.
(But the current code makes sure at least all (truncated) transfer
 gets copied_to before before returning it. So libraries may choose
 to ignore this particular ret val and pass the partial result on
 instead.)

Legitimately naive clients trying to avoid smbus1-vs-3 conditionals,
will look at block[0] anyway, vs their buffer length, to figure out that
something may not work as they though it would. At least their stacks
shall be fine. :)

Daniel

> -- 
> Jean Delvare
> SUSE L3 Support

Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Wolfram Sang]

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

Hi Daniel, hi Jean,

some more bits from me.

> Cool. Meaning we may then leave it at a single i2c_smbus_data.block[257]
> declaration in the headers, and i2c_smbus_data can be 255 bytes.

I think we all agree on this one.

> That doesn’t mean we shut the door on 32-byte buffers. After all, backward
> compat requires we maintain support for those. Just that we need not
> bother declaring or promoting dedicated small vs large transfer types
> while noone wants one.

That's how I see it as well.

> This would also mean we’re likely to add an I2C_FUNC_ to indicated smbus3
> support at runtime. It’s probably what you already have in mind.

I suggest I2C_FUNC_SMBUS3_BLOCKSIZE. I have seen the datasheet of one
client device where you could set a bit to select how many bytes should
be sent by the device in a block read (32 or 255). So, you could set
this bit according to the presence of the above FUNC. Also, I suggest to
do it finegrained, because SMBus3 also specifies 32- and 64-bit
transfers. If those are ever implemented, they should also get seperate
FUNC bits. All of them can then be ORed into I2C_FUNC_SMBUS3 if needed.

> We’d probably still prefer to move all kernel/driver buffers to 255 bytes
> unconditionally. However, I2C_FUNC_ presence normally indicates lower level
> driver + hardware support, right? Not just a kernel upgrade. Which makes 
> sense here, too.

Yes. I am all for using 257 byte buffers in-kernel only. a) defensive
programming b) for most I2C controllers only emulating SMBus, it will be
super easy to support the new block size. I expect it to become it kinda
default soon (for bus master drivers, that is).

> I’m just pointing it out because if we want an I2C_FUNC_SMBUS3, and at
> the driver’s discretion, not just indicating kernel + compatibility
> support presence, and if i2c_utils uses that to pick safe size values,
> that again means we’re yet again less likely to actually deprecate the
> old transfer numbers. Some drivers will never be SMBUS3.

Yes, they will stay around. When I said "deprecated", I didn't mean it
in the way of "will be removed" but more in "please use the new ones".

> I2C_FUNC_SMBUS3 is nice to have. I’m not against it, I’m for it.
> For i2c-tools and all clients, it can be valuable to confirm conflicts.
> 
> Still, one can keep it indicative only, and not synthesize faults 
> because of a perceived client/adapter/device feature mismatch. 

That's always been the case for I2C_FUNC_*. It helps people not to shoot
in their feet, but when ignored things happen.

> I’m not even sure if my new -EMSGSIZE condition in i2c-dev.c is such
> a good idea.

I'd need to see this in a new patch to comment on it. For me, it is
really easier to talk over code. But we are not right there yet, or?

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Wolfram Sang]

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


>  * Allocated bit 4 (I2C_SMBUS3_BLOCK=0x10), to simplify Smbus2
>    compatibility: I2C_SMBUS_*BLOCK* = (<old type>|0x10)

I think the code becomes easier to understand, if we use new transfer
types a bit more explicitly. Also, I am not sure of the extra bit
because it is not clearly visible that types >= 16 and <= 31 will have a
special meaning. We could do like this if we sacrifice one number for
an unused BROKEN with 255 byte:

-#define I2C_SMBUS_BLOCK_DATA	    5
+#define I2C_SMBUS2_BLOCK_DATA	    5 /* 32 byte only, deprecated */
-#define I2C_SMBUS_I2C_BLOCK_BROKEN  6
+#define I2C_SMBUS2_I2C_BLOCK_BROKEN  6 /* 32 byte only, deprecated */
-#define I2C_SMBUS_BLOCK_PROC_CALL   7		/* SMBus 2.0 */
+#define I2C_SMBUS2_BLOCK_PROC_CALL   7		/* SMBus 2.0, 32 byte only, deprecated */
-#define I2C_SMBUS_I2C_BLOCK_DATA    8
+#define I2C_SMBUS2_I2C_BLOCK_DATA    8 /* 32 byte only, deprecated */

+#define I2C_SMBUS_BLOCK_DATA		9
+#define I2C_SMBUS_I2C_BLOCK_BROKEN	10 /* FIXME: probably say "don't use" here
+#define I2C_SMBUS_BLOCK_PROC_CALL	11 /* SMBus >= 2.0 */
+#define I2C_SMBUS_I2C_BLOCK_DATA	12
>  

> +	user_len = kmalloc_array(nmsgs, sizeof(*user_len), GFP_KERNEL);
> +	if (!user_len) {
> +		res = -ENOMEM;
> +		goto out;
> +	}

Maybe on stack? I2C_RDWR_IOCTL_MAX_MSGS will ensure this will stay at a
sane value.

> @@ -313,7 +357,19 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
>  		union i2c_smbus_data __user *data)
>  {
>  	union i2c_smbus_data temp = {};
> -	int datasize, res;
> +	int block_max, datasize, res;
> +

'size' is really a misleading name :(

+	if (size <= I2C_SMBUS2_I2C_BLOCK_DATA) {
+		if (size >= I2C_SMBUS2_BLOCK_DATA)
+			size += I2C_SMBUS_BLOCK_DATA - I2C_SMBUS2_BLOCK_DATA;
+		block_max = I2C_SMBUS_BLOCK_MAX;
+	} else {
+		block_max = I2C_SMBUS3_BLOCK_MAX;
+	}

Would this work, too?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Daniel Stodden]

> On Jul 28, 2020, at 4:16 AM, Wolfram Sang <wsa@kernel.org> wrote:
> 
> 
>> * Allocated bit 4 (I2C_SMBUS3_BLOCK=0x10), to simplify Smbus2
>>   compatibility: I2C_SMBUS_*BLOCK* = (<old type>|0x10)
> 
> I think the code becomes easier to understand, if we use new transfer
> types a bit more explicitly. Also, I am not sure of the extra bit
> because it is not clearly visible that types >= 16 and <= 31 will have a
> special meaning. We could do like this if we sacrifice one number for
> an unused BROKEN with 255 byte:
> 
> -#define I2C_SMBUS_BLOCK_DATA	    5
> +#define I2C_SMBUS2_BLOCK_DATA	    5 /* 32 byte only, deprecated */
> -#define I2C_SMBUS_I2C_BLOCK_BROKEN  6
> +#define I2C_SMBUS2_I2C_BLOCK_BROKEN  6 /* 32 byte only, deprecated */
> -#define I2C_SMBUS_BLOCK_PROC_CALL   7		/* SMBus 2.0 */
> +#define I2C_SMBUS2_BLOCK_PROC_CALL   7		/* SMBus 2.0, 32 byte only, deprecated */
> -#define I2C_SMBUS_I2C_BLOCK_DATA    8
> +#define I2C_SMBUS2_I2C_BLOCK_DATA    8 /* 32 byte only, deprecated */
> 
> +#define I2C_SMBUS_BLOCK_DATA		9
> +#define I2C_SMBUS_I2C_BLOCK_BROKEN	10 /* FIXME: probably say "don't use" here
> +#define I2C_SMBUS_BLOCK_PROC_CALL	11 /* SMBus >= 2.0 */
> +#define I2C_SMBUS_I2C_BLOCK_DATA	12
>> 
> 
>> +	user_len = kmalloc_array(nmsgs, sizeof(*user_len), GFP_KERNEL);
>> +	if (!user_len) {
>> +		res = -ENOMEM;
>> +		goto out;
>> +	}
> 
> Maybe on stack? I2C_RDWR_IOCTL_MAX_MSGS will ensure this will stay at a
> sane value.
> 
>> @@ -313,7 +357,19 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
>> 		union i2c_smbus_data __user *data)
>> {
>> 	union i2c_smbus_data temp = {};
>> -	int datasize, res;
>> +	int block_max, datasize, res;
>> +
> 
> 'size' is really a misleading name :(

Yep. :/

> +	if (size <= I2C_SMBUS2_I2C_BLOCK_DATA) {
> +		if (size >= I2C_SMBUS2_BLOCK_DATA)
> +			size += I2C_SMBUS_BLOCK_DATA - I2C_SMBUS2_BLOCK_DATA;
> +		block_max = I2C_SMBUS_BLOCK_MAX;
> +	} else {
> +		block_max = I2C_SMBUS3_BLOCK_MAX;
> +	}
> 
> Would this work, too?

“3” ;)

But I get what you mean.

I’m not too passionate about the bit flip. Adding relative offsets would work for me too.

In fact, if we just want to keep a full switch (size) {} and map {9, 10, 11} to {5, 7, 8},
(i.e. no dummy-broken), my world wouldn’t collapse yet.

Daniel

Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

[Daniel Stodden]

> On Jul 28, 2020, at 4:40 AM, Daniel Stodden <daniel.stodden@gmail.com> wrote:
> 
> “3” ;)

Sorry, I got confused. You’re suggesting to label the old calls “2” and
the new ones take the original name.

That’s fine! But we need to figure out what we really require, and which
protocol to prefer.

 * If the new block size can be too large for small clients with Smbus < 3
   slaves, we’re not going to deprecate those.

   But phasing out 32 byte transfers for good would make a simpler header.

 * If some clients need the old calls and make a new i2c_smbusX_data,
   then those aren’t /* deprecated */ any more.

   With the above scheme, we're making i2c_smbus_data size 255 and
   and rename the old type to smbus2.

Btw: I think I was confused because would have called those i2c_smbus1_ then.

Best,
Daniel