[PATCH] i2c-dev: relax ban on I2C_M_RECV_LEN

[PATCH] i2c-dev: relax ban on I2C_M_RECV_LEN

[Douglas Gilbert]

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

The I2C_M_RECV_LEN flag indicates that the length of
an I2C response is in the first byte read. So the maximum
size of the read buffer using this option is 256 bytes.

Currently the i2c-dev driver returns EINVAL when an
attempt is made to use ioctl(I2C_RDWR) with the
I2C_M_RECV_LEN flag set. That is overly paranoid:

ChangeLog:
   - allow I2C_M_RECV_LEN flag in the i2c-dev driver as
     long as the associated buffer length can cope with
     the worst case size (which is 256 bytes).

Doug Gilbert
[Please cc responses to me]

[-- Attachment #2: i2c-dev_mrecv_len1.patch --]
[-- Type: text/x-patch, Size: 634 bytes --]

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 57a45ce8..b8f226a0 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -270,9 +270,10 @@ static noinline int i2cdev_ioctl_rdrw(struct i2c_client *client,
 	res = 0;
 	for (i = 0; i < rdwr_arg.nmsgs; i++) {
 		/* Limit the size of the message to a sane amount;
-		 * and don't let length change either. */
+		 * with I2C_M_RECV_LEN require worst case len. */
 		if ((rdwr_pa[i].len > 8192) ||
-		    (rdwr_pa[i].flags & I2C_M_RECV_LEN)) {
+		    ((rdwr_pa[i].flags & I2C_M_RECV_LEN) &&
+		     (rdwr_pa[i].len < 256))) {
 			res = -EINVAL;
 			break;
 		}

Re: [PATCH] i2c-dev: relax ban on I2C_M_RECV_LEN

[Jean Delvare]

Hi Doug,

Sorry for the late reply, I wanted to make sure I remembered all the
I2C_M_RECV_LEN logic before replying.

On Sat, 04 Feb 2012 16:42:29 -0500, Douglas Gilbert wrote:
> The I2C_M_RECV_LEN flag indicates that the length of
> an I2C response is in the first byte read. So the maximum
> size of the read buffer using this option is 256 bytes.
> 
> Currently the i2c-dev driver returns EINVAL when an
> attempt is made to use ioctl(I2C_RDWR) with the
> I2C_M_RECV_LEN flag set. That is overly paranoid:

No, this is playing it safe, in the absence of use case and complete
review of all involved code paths.

> ChangeLog:
>    - allow I2C_M_RECV_LEN flag in the i2c-dev driver as
>      long as the associated buffer length can cope with
>      the worst case size (which is 256 bytes).

This means that you expect user-space to provide a 256 byte message
buffer when passing the I2C_M_RECV_LEN flag. Underlying bus drivers
OTOH expect len to be set to 1 when I2C_M_RECV_LEN is used (and they
add the received block length to that to come up with the actual used
length.) I don't think this is documented formally anywhere, but reading
the code of function i2c_smbus_xfer_emulated() in i2c-core will show you
the calling conventions and expectations. Function readbytes() in
i2c-algo-bit is also worth reading. So your patch is not correct.

To be honest, I think I recall being the one designing things that way
but I can't remember why I did so. Some git and mailing list digging
might be needed. Might be related to the support of SMBus PEC but I'm
not sure.

Unfortunately there is only i2c_msg.len available to pass length
information, so it isn't possible to dissociate the buffer size from
the used size. It happens to be the same in most cases so it has never
been a problem in practice. Only for the I2C_M_RECV_LEN case it would
be useful to distinguish between both, and presumably SMBus PEC too.

It has never been a problem so far because only the SMBus layer is
using I2C_M_RECV_LEN and PEC, and there we know that the block size
cannot exceed I2C_SMBUS_BLOCK_MAX == 32. Every bus driver can (and
should) enforce that, and buffers are always large enough to contain 32
bytes, by design.

Passing I2C_M_RECV_LEN at the I2C (not SMBus) level wouldn't work
safely, not even in the kernel. The only non-SMBus implementation is in
i2c-algo-bit as far as I know, and it enforces the I2C_SMBUS_BLOCK_MAX
limit. So it should be pretty clear that flag I2C_M_RECV_LEN was
introduced for and designed with SMBus in mind. Using it for I2C
messaging just doesn't work, thus the ban in i2c-dev.

This makes me wonder how you did test your patch, as I can't see how it
would work with the upstream driver code. But more importantly, can you
please explain what you are trying to achieve in the first place?
Receiving block length as the first data byte is an SMBus thing,
traditionally non-SMBus devices don't do that. And for SMBus devices
through i2c-dev, you'd be using ioctl I2C_SMBUS not I2C_RDWR.

If you have a legitimate use case for I2C_M_RECV_LEN, then we can
discuss it, but it will take a lot more than your 2-line patch to get
it right.

-- 
Jean Delvare

Re: [PATCH] i2c-dev: relax ban on I2C_M_RECV_LEN

[Douglas Gilbert]

On 12-02-20 10:29 AM, Jean Delvare wrote:
> Hi Doug,
>
> Sorry for the late reply, I wanted to make sure I remembered all the
> I2C_M_RECV_LEN logic before replying.
>
> On Sat, 04 Feb 2012 16:42:29 -0500, Douglas Gilbert wrote:
>> The I2C_M_RECV_LEN flag indicates that the length of
>> an I2C response is in the first byte read. So the maximum
>> size of the read buffer using this option is 256 bytes.
>>
>> Currently the i2c-dev driver returns EINVAL when an
>> attempt is made to use ioctl(I2C_RDWR) with the
>> I2C_M_RECV_LEN flag set. That is overly paranoid:
>
> No, this is playing it safe, in the absence of use case and complete
> review of all involved code paths.
>
>> ChangeLog:
>>     - allow I2C_M_RECV_LEN flag in the i2c-dev driver as
>>       long as the associated buffer length can cope with
>>       the worst case size (which is 256 bytes).
>
> This means that you expect user-space to provide a 256 byte message
> buffer when passing the I2C_M_RECV_LEN flag. Underlying bus drivers
> OTOH expect len to be set to 1 when I2C_M_RECV_LEN is used (and they
> add the received block length to that to come up with the actual used
> length.) I don't think this is documented formally anywhere, but reading
> the code of function i2c_smbus_xfer_emulated() in i2c-core will show you
> the calling conventions and expectations. Function readbytes() in
> i2c-algo-bit is also worth reading. So your patch is not correct.
>
> To be honest, I think I recall being the one designing things that way
> but I can't remember why I did so. Some git and mailing list digging
> might be needed. Might be related to the support of SMBus PEC but I'm
> not sure.
>
> Unfortunately there is only i2c_msg.len available to pass length
> information, so it isn't possible to dissociate the buffer size from
> the used size. It happens to be the same in most cases so it has never
> been a problem in practice. Only for the I2C_M_RECV_LEN case it would
> be useful to distinguish between both, and presumably SMBus PEC too.
>
> It has never been a problem so far because only the SMBus layer is
> using I2C_M_RECV_LEN and PEC, and there we know that the block size
> cannot exceed I2C_SMBUS_BLOCK_MAX == 32. Every bus driver can (and
> should) enforce that, and buffers are always large enough to contain 32
> bytes, by design.
>
> Passing I2C_M_RECV_LEN at the I2C (not SMBus) level wouldn't work
> safely, not even in the kernel. The only non-SMBus implementation is in
> i2c-algo-bit as far as I know, and it enforces the I2C_SMBUS_BLOCK_MAX
> limit. So it should be pretty clear that flag I2C_M_RECV_LEN was
> introduced for and designed with SMBus in mind. Using it for I2C
> messaging just doesn't work, thus the ban in i2c-dev.
>
> This makes me wonder how you did test your patch, as I can't see how it
> would work with the upstream driver code. But more importantly, can you
> please explain what you are trying to achieve in the first place?
> Receiving block length as the first data byte is an SMBus thing,
> traditionally non-SMBus devices don't do that. And for SMBus devices
> through i2c-dev, you'd be using ioctl I2C_SMBUS not I2C_RDWR.
>
> If you have a legitimate use case for I2C_M_RECV_LEN, then we can
> discuss it, but it will take a lot more than your 2-line patch to get
> it right.

In the embedded space I only see I2C (TWI) so I don't understand
the fixation with SMBus (some subset I believe).

My illegitimate use case is:
Sonmicro 13.56 MHz RFID Mifare Module:
http://www.sonmicro.com/en/downloads/Mifare/ds_SM130.pdf
http://www.sonmicro.com/en/downloads/Mifare/AN601.pdf

I can make it work by requesting the maximum number of bytes it
will ever respond with on all reads.


Some suggestions for when and if the I2C pass-through is rewritten:
   - make it clean to the user space, don't use it for internal
     plumbing within the kernel (to avoid horrors like those you
     allude to above)
   - put some version number in it so when you want to put some
     extra fields through it (e.g. extra i2c_msg.len field) you
     can bump version number ***
   - the multiple I2C transfers in one structure is great, but
     would be more useful if a delay could be placed between
     each one.


*** Getting new ioctls into the kernel is really difficult since
     the management seems to think pass-throughs subvert the OS
     (they do indeed) and where absolutely necessary sysfs can be
     used for the purpose.

Doug Gilbert

Re: [PATCH] i2c-dev: relax ban on I2C_M_RECV_LEN

[Jean Delvare]

Hi Doug,

On Mon, 20 Feb 2012 22:45:03 -0500, Douglas Gilbert wrote:
> In the embedded space I only see I2C (TWI) so I don't understand
> the fixation with SMBus (some subset I believe).

Yes, SMBus is essentially a subset of I2C with better defined semantics.

> My illegitimate use case is:
> Sonmicro 13.56 MHz RFID Mifare Module:
> http://www.sonmicro.com/en/downloads/Mifare/ds_SM130.pdf
> http://www.sonmicro.com/en/downloads/Mifare/AN601.pdf
> 
> I can make it work by requesting the maximum number of bytes it
> will ever respond with on all reads.

I have read these two documents quickly and I don't see any use case
for I2C_M_RECV_LEN. The two block reads I see (5.6 READ BLOCK and 5.7
READ VALUE BLOCK in ds_SM130.pdf) have predefined response lengths of
18 and 6 bytes respectively. Nowhere I see a read command that would
reply a block length as the first byte.

Please point me to the page and section of these documents where you
think I2C_M_RECV_LEN is needed.

> Some suggestions for when and if the I2C pass-through is rewritten:
>    - make it clean to the user space, don't use it for internal
>      plumbing within the kernel (to avoid horrors like those you
>      allude to above)

I'm not even sure what you call "I2C pass through", sorry. Most of the
I2C device drivers are in the kernel so it seems reasonable to offer a
convenient in-kernel interface for these. i2c-dev has been historically
used for debugging, development and investigation more than anything
else. I definitely agree that it isn't friendly for writing user-space
drivers, it wasn't designed for this in the first place.

>    - put some version number in it so when you want to put some
>      extra fields through it (e.g. extra i2c_msg.len field) you
>      can bump version number ***

That's obviously too late.

>    - the multiple I2C transfers in one structure is great, but
>      would be more useful if a delay could be placed between
>      each one.

You are not the first one to complain about the lack of flexibility of
i2c_transfer. But the fun thing is that everybody has been asking for
something different. You want delays, others asked for the possibility
to defined future writes based on the value of past reads, others
wanted to be able to skip repeated starts between messages, etc. It
might make more sense to add pre/post message hooks and let the drivers
do whatever they want there. That's something for in-kernel drivers
though, not user-space.

> *** Getting new ioctls into the kernel is really difficult since
>      the management seems to think pass-throughs subvert the OS
>      (they do indeed) and where absolutely necessary sysfs can be
>      used for the purpose.

These are technical details that can always be sorted out. What is
important is to figure out what is needed and how it would be best
implemented.

-- 
Jean Delvare

Re: [PATCH] i2c-dev: relax ban on I2C_M_RECV_LEN

[Jean Delvare]

On Tue, 21 Feb 2012 08:31:26 +0100, Jean Delvare wrote:
> On Mon, 20 Feb 2012 22:45:03 -0500, Douglas Gilbert wrote:
> > My illegitimate use case is:
> > Sonmicro 13.56 MHz RFID Mifare Module:
> > http://www.sonmicro.com/en/downloads/Mifare/ds_SM130.pdf
> > http://www.sonmicro.com/en/downloads/Mifare/AN601.pdf
> > 
> > I can make it work by requesting the maximum number of bytes it
> > will ever respond with on all reads.
> 
> I have read these two documents quickly and I don't see any use case
> for I2C_M_RECV_LEN. The two block reads I see (5.6 READ BLOCK and 5.7
> READ VALUE BLOCK in ds_SM130.pdf) have predefined response lengths of
> 18 and 6 bytes respectively. Nowhere I see a read command that would
> reply a block length as the first byte.
> 
> Please point me to the page and section of these documents where you
> think I2C_M_RECV_LEN is needed.

Err, sorry, I see it now. Must have been blind this morning.

Unfortunately the length doesn't include the checksum byte, so in
practice you always read or write one more byte than the length byte
says. This wouldn't be an issue if the checksum was compatible with
SMBus PEC, as you would then be able to use the SMBus emulation layer,
but it is not. It's really a pity to be so close to SMBus yes be
incompatible with it. Shame on the manufacturer.

Still, everything isn't lost. Your largest command is 18 byte long,
this fits in the SMBus limit (32 bytes) so underlying bus drivers will
be happy, if you provide the buffer size they expect
(I2C_SMBUS_BLOCK_MAX + 2.) If you were using a kernel driver instead of
going through i2c-dev, I think it would work just fine without changing
anything to the existing code. You would just allocate a 34-byte buffer
and pass len = 2 and flags |= I2C_M_RECV_LEN, and everything would work
fine.

Which brings me to a question: why are you writing your driver in
user-space instead of kernel-space as everybody else is doing?

I am also curious which i2c bus driver your system is using?

For i2c-dev I have an idea but it's not exactly clean and I'm not sure
if it'll work. Unfortunately I have no device supporting I2C_M_RECV_LEN
at hand so testing is quite difficult.

-- 
Jean Delvare