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

[Daniel Stodden <daniel.stodden@gmail.com>]

> 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