From: Wolfram Sang <wsa@kernel.org>
To: Daniel Stodden <daniel.stodden@gmail.com>
Cc: Jean Delvare <jdelvare@suse.de>, linux-i2c@vger.kernel.org
Subject: Re: i2c block reads > 32 bytes
Date: Mon, 27 Jul 2020 22:53:38 +0200 [thread overview]
Message-ID: <20200727205338.GC18997@ninjato> (raw)
In-Reply-To: <F76BBB88-55D9-466F-8E39-730D18766917@gmail.com>
[-- 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 --]
prev parent reply other threads:[~2020-07-27 20:53 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-26 4:25 i2c block reads > 32 bytes Daniel Stodden
2020-07-26 10:34 ` Wolfram Sang
2020-07-26 23:21 ` Daniel Stodden
2020-07-27 8:37 ` Jean Delvare
2020-07-27 20:53 ` Wolfram Sang [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200727205338.GC18997@ninjato \
--to=wsa@kernel.org \
--cc=daniel.stodden@gmail.com \
--cc=jdelvare@suse.de \
--cc=linux-i2c@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).