i2c block reads > 32 bytes

i2c block reads > 32 bytes

[Daniel Stodden]

Hi.

I’ve been looking at a PMBus chip lately.

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]

Contrasting I2C_SMBUS_BLOCK_MAX=32, obviously. Similarly, Smbus 3.1 appears to have removed the 32-byte limit 
for block read, block write, and block-write block-read process call. [2]

Now, I'm working with a proprietary i2c adapter. The circuit would likewise support transfer sizes way beyond 32 bytes.
And the platform I’m working on has a TI UCD90320 power sequencer, which is using PMBus limits

There are workarounds in place, but I don’t find them very attractive, compared to a more supportive I2C_RDWR
ioctl. I’m fully aware that I2C_SMBUS_BLOCK_MAX=32 is basically set in stone. But I could imagine I2C_RDWR growing 
to support newer Smbus protocols. My question would be whether this has been considered already.

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.

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.

Thanks,
Daniel

[1] https://pmbus.org/Assets/PDFS/Public/PMBus_Specification_Part_II_Rev_1-1_20070205.pdf
[2] http://smbus.org/specs/SMBus_3_1_20180319.pdf
[3] https://marc.info/?l=linux-i2c&m=133361075928680&w=2

Re: i2c block reads > 32 bytes

[Wolfram Sang]

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

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

> Contrasting I2C_SMBUS_BLOCK_MAX=32, obviously. Similarly, Smbus 3.1 appears to have removed the 32-byte limit 
> for block read, block write, and block-write block-read process call. [2]

... but I am aware of this change since SMBus 3.0 specs came out. As you
already discovered, you are the first one to ask for support for it.

> There are workarounds in place, but I don’t find them very attractive,
> compared to a more supportive I2C_RDWR ioctl. I’m fully aware that
> I2C_SMBUS_BLOCK_MAX=32 is basically set in stone. But I could imagine
> I2C_RDWR growing to support newer Smbus protocols. My question would
> be whether this has been considered already.

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.

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

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

Kind regards,

   Wolfram


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

Re: i2c block reads > 32 bytes

[Daniel Stodden]

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

Re: i2c block reads > 32 bytes

[Jean Delvare]

Hi Daniel,

On Sun, 26 Jul 2020 16:21:28 -0700, Daniel Stodden wrote:
> 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.

This "it will never change" was written before SMBus 3.0 came to
existence. The point of this commit was to clarify that there was only
one block size limit at the time and it came from the SMBus
specification. There is no limit at the I2C level so
I2C_SMBUS_I2C_BLOCK_MAX was a nonsense in the first place.

This says nothing about the inability to change I2C_SMBUS_BLOCK_MAX if
a future SMBus specification needs that. Keeping compatibility in mind,
of course.

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

Correct. Actually I2C_RDWR is for I2C-level transactions, not
SMBus-level, and as such the reason why I2C_SMBUS_BLOCK_MAX appears in
that code path is not obvious and should be investigated. It might be a
shortcut that was considered acceptable due to SMBus being the only
user of I2C_M_RD in practice at that time (this is definitely no longer
true).

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

Yes it was ;-) master_xfer is for I2C-level transfers, smbus_xfer for
SMBus-level transfers. It's not only a matter of maximum block size,
SMBus is really a subset of what I2C allows in all dimensions, and
SMBus-only controllers can't implement master_xfer (if they did then
the clients would assume they have access to *all* of I2C, and would be
deeply disappointed then they realize it isn't the case).

The rule that transfers > 32 bytes implied I2C held as long as the
32-byte limit was set by SMBus. Now that SMBus raised that limit, this
is no longer the case.

If there currently are code paths that go through master_xfer and DO
enforce the 32-byte limit, I would call that a bug, which should be
investigated and fixed independently of adding support for larger block
sizes to the SMBus code paths. The latter is new from SMBus 3.x, while
the former should have been working from the beginning.

> (...)
>  * Alternatively, I could see room for a union i2c_smbus30_data, to
>    leave the classic i2c_smbus_data alone.

That was my idea as well. i2c_smbus_data is part of the user-space API
and as such I don't think incompatible changes can even be considered.

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

"size" in this context has always been a misnomer, what is really
encoded in that field is an SMBus transfer type without its "direction"
(the whole transfer type being expressed by the combination of
"read_write" and "size"). While having a 32-bit field to store that
information was clearly too much, I would refrain from abusing this
field for other purposes unless alternatives are worse.

Instinctively I would rather define new I2C_SMBUS3_BLOCK_DATA and
I2C_SMBUS3_BLOCK_PROC_CALL if that solves the problem.

Note that we have room left to introduce I2C_M_* and/or I2C_FUNC_*
flags to indicate the use of, or compatibility with, the new maximum
block size. Hopefully this should ensure a smooth transition and
cohabitation between current and future hardware and drivers.

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

i2c-tools is maintained by me with the help of Wolfram. Note that I am
not subscribed to linux-i2c at the moment so please make sure you Cc me
in every thread dealing with this issue.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: i2c block reads > 32 bytes

[Wolfram Sang]

[-- 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 --]