Re: i2c block reads > 32 bytes

[Wolfram Sang <wsa@kernel.org>]

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

Hi Daniel,

> Before reading on: sorry in advance for a very long reply.
> If you want to split the conversation into sub-items, let me know.

We will see if that makes sense. In general, I am happy that you also go
into details already!

> With ’set in stone’ I was just echoing what I found old posts, which
> seemed to acknowledge the difficulties:
> 
> E.g. "this will never change“ in
> https://marc.info/?l=git-commits-head&m=113053689014136&w=2.

Same as Jean, I read this as "I2C_SMBUS_I2C_BLOCK_MAX will never be
different from I2C_SMBUS_BLOCK_MAX". This is why the former could go.
The latter is a different thing.

> The level of compatbility offered through
> i2c_smbus_xfer_emulated is probably essential anywhere (kernel + user
> clients). So leaving out I2C_SMBUS and relying only on I2C_RDWR seems
> incomplete.

Yes.

> Let me first summarize, there seem to be two areas of work then:
> 
> - I2C_SMBUS should support block transfers up to 255 bytes.
> 
>   One concern here is how to guarantee that growing i2c_smbus_data
>   does not break the binary interface, both in user and kernel space.

Exactly.

> - I2C_RDWR should support block transfers up to 255 bytes. 
> 
>   The ioctl interface looks robust enough (to me), since msg[i].len
>   may carry the difference without changing semantics.
> 
>   But the master_xfer interface, while employting the same i2c_msg
>   type, is different from the ioctl one, and it currently doesn't
>   accommodate block length > 32 bytes.
> 
>   (Maybe I should rename this item to "master_xfer should …”. Just
>   leaving it there in case I2C_RDWR needs work I didn’t see yet.)

I fully agree to Jean's comments here.

>  - I agree that I2C_SMBUS20_BLOCK_MAX and I2C_SMBUS30_BLOCK_MAX, and
>    conditional compilation, may pave the way.

No conditional compilation, please. One kernel should support both. But
two defines to not mix up things.

> 
>  - Intuitively, it sees a dedicated I2C_SMBUS_LATEST_BLOCK_MAX is too much.
> 
>    Simply because 255 designates the 1-byte length prefix limit. Smbus
>    would need word-sized prefixes to further expand, at which point
>    both the link layer protocol and the kernel ABI would have to
>    evolve significantly beyond what we're currently looking at.
> 
>    This is as redundant as “I2C_SMBUS_I2C_BLOCK_MAX” used to be.

I agree.

>  - Unless we see a value in conditional compilation in the headers,
>    should I2C_SMBUS_BLOCK_MAX just get removed?

Interesting idea. I am far more familiar with kernel space than user
space. In kernel space, I would rename I2C_SMBUS_BLOCK_MAX to
I2C_SMBUS2_BLOCK_MAX. And then probably remove the old
I2C_SMBUS_BLOCK_MAX altogether because we now utilize the whole 8 bit
size field. But we can't do this in userspace.

So, when next rc1 is out, I will send out conversion patches to all
users of I2C_SMBUS_BLOCK_MAX and convert them. Except for the I2C core
parts which expose to userspace. On this, we can work in parallel.

>  - I understand the kernel clients may be satisfied with the
>    above. User space is what's primarily on my mind.

Sounds like we make a good team then :)

>  * Would we compile i2c_smbus_data conditionally in the header? 
>    I.e. let userspace
>    do things like -DI2C_SMBUS_BLOCK_MAX=I2C_SMBUS30_BLOCK_MAX?
> 
>    (Not saying it's my preferred way, if avoidable. Just putting it here.)

Nope. Nothing conditional, please.

>  * Alternatively, I could see room for a union i2c_smbus30_data, to
>    leave the classic i2c_smbus_data alone.
> 
>    This in turn would render i2c_smbus_ioctl_data.data as a union pointer-type.

Sounds like the safest path to me.


>  * Semantics for i2c_smbus_ioctl_data.size have always been strictly
>    defined: valid range is currently 0..8.
> 
>    Would we consider allocating bit positions greater
>    size[0:3] do communicate a user block size of
>    I2C_SMBUS30_BLOCK_MAX, next to the given transaction type?
> 
>    #define I2C_SMBUS_QUICK	    0
>    [..]
>    #define I2C_SMBUS_I2C_BLOCK_DATA    8
>    #define I2C_SMBUS30_BLOCKSIZE    (1<<31)
> 
>    or similar?

Let's try the other solution first.

>  * Btw, I thought the transition to 255 bytes was Smbus 3.1, not Smbus
>    3.0 (?). Should these be named I2C_SMBUS31 instead?

Nope, it was SMBus 3.0 which introduced it:
http://smbus.org/specs/SMBus_3_0_20141220.pdf

Sidenote: Length can now be 0 as well which wasn't the case before (at
least for PROC_CALL).

> So here goes my current state of mind about I2C_RDWR.
> 
>  - Foremost: agreed, userspace talking to Smbus (or PMBus, or any
>    other variant) should stay encouraged to employ SMBUS calls, not
>    I2C_RDWR based calls.
> 
>  - Nonetheless, the I2C_RDWR ioctl and I2C_M_RECV_LEN exist, and
>    should naturally evolve to accommodate transfers length > 32, too.

Yes.

> Now, regarding i2c_transfer, aka master_xfer: presently, there is a transform in
> i2cdev_ioctl_rdwr, resulting in
> 
>  * i2c_msg as passed to i2cdev_ioctl_rdwr
> vs
>  * i2c_msg as passed to i2c_transfer
> 
> having different semantics:
> 
> https://github.com/torvalds/linux/blob/v5.8-rc7/drivers/i2c/i2c-dev.c#L285:
> 
> 		msgs[i].buf = memdup_user(data_ptrs[i], msgs[i].len);
> 
>                 [..]
> 
> 		if (msgs[i].flags & I2C_M_RECV_LEN) {
>                         [..]
> 
> 			msgs[i].len = msgs[i].buf[0];
>                 }
> 
> Iow, msg[i].len in master_xfer will only hold <extra_bytes>, not
> sizeof(buf). To a bus adapter, this means:
> 
>  * The actual size of msg.buf is not known. 

I never had a device with I2C_M_RECV_LEN, so I never noticed this code
in i2c-dev. Your arguments make sense to me, so I wonder why the above
mechanism was implemented like this. I believe there is a reason, but
for today I don't have the bandwidth to dive into it. That all
withstanding, I think it is fixable without hurting userspace.

>  * The way drivers handle i2c_msg output is:
> 
>    1. read the length prefix into msgs[i].buf[0]
>    2. ensure msgs[i].buf[0] <= I2C_SMBUS_BLOCK_MAX
>    3. msg[i].len += msg[i].buf[0]
> 
> And the latter is what gets copied back to userspace. This seems to be
> a very common construct I see in most adapter implementations in i2c/busses.

Yes.

>  * Replace above msgs[i].buf = memdup_user(.., msg[i].len) with
>    allocating a buffer sized *at least* 255 + extra_bytes, then
>    copying msg.len bytes from user.
> 
>  * Hardware + drivers wishing to support transfers > 32 bytes can then
>    always rely on I2C_SMBUS30_BLOCK_MAX (255) bytes to be available.
> 
>    The above msg[i].len handling algo remains in place.
>    The only thing changing is the buffer size guarantee alone.
> 
>  * On the return path from i2c_transfer(), a user-msg[i].len too small
>    to contain the result might EFAULT.
> 
>    Unless a different errno seems seems better suited, to avoid
>    ambiguities. EMSGSIZE?
> 
>    In any case, the philosophy would be that user msg lengths
>    would be handled in i2c-core, not at all by drivers, beyond an option
>    to adopt larger buffers.
> 

Probably a good idea, but I need to dive into some more to give a fully
qualified comment hre.

>  * For safety, either leave I2C_SMBUS_BLOCK_MAX at 32 by default,
>    when building busses/*. Or
>    initially substitute all uses with I2C_SMBUS20_BLOCK_MAX.
> 
>    I suppose it’s the latter, unless we avoid I2C_SMBUS20_BLOCK_MAX
>    altogether, and stay with I2C_SMBUS_BLOCK_MAX=32 everywhere.
>  
> The last point is because 32B-only capable drivers should avoid
> breaking hardware semantics in some pathologic cases. Until they
> get reviewed, each individually.

Yes, the only mass-conversion I'd suggest is from I2C_SMBUS_BLOCK_MAX to
I2C_SMBUS20_BLOCK_MAX which is a plain rename. If a bus master driver
wants to support 255 byte transfers, then it needs an individually
crafted and tested patch. It will be trivial for most drivers, still.

> If the above sounds good enough to get started, I could volunteer to
> make a prototype change. But I'd rather hear other opinions first.

I can prepare an initial branch for us to work on.

> Yes, I’d anticipate this should see a bunch of reviews from all kinds of people.
> Kernel, user, etc.

I am glad that Jean (former I2C maintainer and with lots of distro
experience) already commented.

> As a first goal, we should find a point where anyone feels the most important
> requirements are at least gathered.

I think kernel side is pretty good, and we have an idea which path to
tackle for userspace, too. Or?

> Is i2c-tools maintaned by roughly the same folks reading linux-i2c?
> Who else cares most, besides kernel clients?

Yes, Jean is the maintainer and I have write access there, too. We are a
good bunch already, and maybe others will join as well.

Thanks and happy hacking,

   Wolfram


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