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@xxxxxxxxxx> 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
