> On Jul 28, 2020, at 3:36 AM, Wolfram Sang <wsa@xxxxxxxxxx> 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
