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:
signature.asc
Description: PGP signature
