Re: i2c block reads > 32 bytes

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

Hi Wolfram,

Thanks for sharing your thoughts, much appreciated.

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

> On Jul 26, 2020, at 3:34 AM, Wolfram Sang <wsa@kernel.org> wrote:
> 
> Hi Daniel,
> 
>> Publicly available PMBus revisions appear to be based on SMBus 2.0,
>> but with relaxed constraints regarding block read/write length: 255
>> bytes, not 32. [1]
> 
> I missed that detail for PMBus...

I can relate. I’ve known the physical and link layer protocols for a while, 
but I’m only gradually learning about the  standards hierarchy and implied
limits these days, and then how they currently manifest in linux-i2c, specifically.

> I neither had hardware nor time for actually hacking on an
> implementation. However, I thought about how one could do it and I don't
> think the old limit is "set in stone". My idea for kernel space:
> 
> #define I2C_SMBUS20_BLOCK_MAX 32
> /* Next one only for a transition period */
> #define I2C_SMBUS_BLOCK_MAX I2C_SMBUS20_BLOCK_MAX
> 
> #define I2C_SMBUS30_BLOCK_MAX 255
> #define I2C_SMBUS_LATEST_BLOCK_MAX I2C_SMBUS30_BLOCK_MAX
> 
> And with that:
> 
> 1) Convert I2C_SMBUS_BLOCK_MAX to I2C_SMBUS20_BLOCK_MAX for all current
>   users in-tree to avoid regressions
> 
> 2) Update the I2C core to handle I2C_SMBUS_LATEST_BLOCK_MAX
> 
> 3) People can convert client drivers to I2C_SMBUS30_BLOCK_MAX
>   individually. So we ensure it has been properly tested.
> 
> I haven't fully dived into it but I'd think something similar can be
> done for userspace, too.

This gets interesting.

I haven’t spent time on strategies upgrading i2c_smbus_data before
now. Rather I2C_RDWR, but I’m starting to reconsider, based on what
you write.

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.

These comments are 15 years old now, but to me it seemed to anticipate that any
later evolution would naturally pick I2C_RDWR over I2C_SMBUS. That’s about
where my ’set in stone' came from. Anway, let’s scratch that.

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.

(Similarly, assuming that any adapter capable of transfers > 32 bytes
will implement master_xfer (mine does that) rather than smbus_xfer was
probably naive of me.)

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.

- 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'll return to I2C_RDWR below. Regarding I2C_SMBUS:

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

 - 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.

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

   (I could also see I2C_SMBUS_BLOCK_MAX remain 32 bytes in future,
    instead of adding the specific SMBUS20_ variant.
    It might be advantageous, but I haven’t investigated further).

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

Regarding userspace:

The part left open so far is that substituting I2C_SMBUS30_BLOCK_MAX
for I2C_SMBUS_BLOCK_MAX alone, whether conditional or fixed, renders
i2c_smbus_ioctl_data.data ambiguous. There is no way to see yet if
i2c_smbus_data.block is of size 34 bytes or 257.

Just trading ideas:

 * 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.)

 * 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.

 * 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?

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

>> Recap: the problem with the current i2c-core is that i2cdev_ioctl_rdwr
>> is passing msg[i].len in a way which makes it impossible for adapters
>> to execute block reads greater 32: kernel msg[i].len isn’t user
>> msg[i].len, but set to the number of extra bytes initially, so the
>> adapter driver is left with assurance that 32 bytes buffer space
>> available, not how much, if more. I suppose this is intentional.
> 
> I'd think we can extend that guarantee to 255 bytes when changing to the
> new max. Needs definately some more discussion with Jean, though. But
> also, if everything is correctly updated, you should be able to use the
> native i2c_smbus_* functions again. No need for rdwr detour.

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.

 - I currently don't see a problem with the ioctl interface.

 - But I seem to sense an issue with the master_xfer as presently
   implemented by adapters.

I proably should have been more careful to not use master_xfer and
I2C_RDWR synonymously in my original post.

Regarding I2C_RDWR: Afaict, user code typically sets:

  * extra_bytes = 1 (length only) or 2 (length + pec) or greater.
  * datasize = /* as needed according to slave / command specification */
  * msg[i].len = datasize + extra_bytes
  * msg[i].flags = I2C_M_RD | I2C_M_RECV_LEN
  * msg[i].buf = malloc(msg[i].len) or similar.
  . msg[i].buf[0] = extra_bytes

I assume there is no problem here: msg.len is u16 and so a datasize up
to 255 falls right into place. Just writing it down to point that out,
and for later reference (see below).

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. 

   msg.len == msg.buf[0] (== extra_bytes)

   In binary, the size of msg.buf is only guaranteed to be greater
   than I2C_SMBUS_BLOCK_MAX + extra_bytes. It might be greater, but
   that information isn't passed on.

 * 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.

There are a number of ways to expand this beyond
I2C_SMBUS_BLOCK_MAX=32, but some care seems to be needed.

Here is a relatively simple approach I'd suggest (not like I insist
it's the right one):

 * 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.

 * 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.

For example, if the slave supports large transfers, but the host adapter
never did, the driver shouldn’t use 255 here.
Or, same hardware limit, and reading a length prefix > 32 only owed to a
bit error due to line noise.

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.

>> Also, I suspect I’m not tellying anyone in this forum anything new.
>> Bear with me, I’ve made an attempt to find older discussions. But
>> didn’t see anything later than the exchange leading to the current
>> handling of I2C_M_RECV_LEN.
> 
> As said above, there was none. However, your timing is not bad. I am
> currently devleoping a "testunit" driver for IP cores which can also be
> an I2C client themselves. It is already on the todo-list to add test
> cases for I2C_M_RECV_LEN there, too, so it can be tested independently
> of specific hardware. Of course, it is very good if there are people to
> validate the results with devices actually needing that!

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

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

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

Kind regards, and thanks a lot for your time!
Daniel