[PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Jean Delvare]

As the bus driver side implementation of I2C_M_RECV_LEN is heavily
tied to SMBus, we can't support received length over 32 bytes, but
let's at least support that.

In practice, the caller will have to setup a buffer large enough to
cover the case where received length byte has value 32, so minimum
32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
added for the specific slave (for example a checksum.)

Signed-off-by: Jean Delvare <khali-PUYAD+***@public.gmane.org>
Cc: Douglas Gilbert <dgilbert-qazKcTl6WRFWk0Htik3J/***@public.gmane.org>
---
This is an alternative to Doug's implementation. Doug, I sent this to
you one week ago, did you have the time to give it a try, do you have
any comment? Again I can't test this myself so someone else will have
to do it.

drivers/i2c/i2c-dev.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)

--- linux-3.3-rc7.orig/drivers/i2c/i2c-dev.c 2012-03-15 18:00:02.502988008 +0100
+++ linux-3.3-rc7/drivers/i2c/i2c-dev.c 2012-03-15 18:04:46.394992216 +0100
@@ -265,19 +265,30 @@ static noinline int i2cdev_ioctl_rdrw(st

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. */
- if ((rdwr_pa[i].len > 8192) ||
- (rdwr_pa[i].flags & I2C_M_RECV_LEN)) {
+ /* Limit the size of the message to a sane amount */
+ if (rdwr_pa[i].len > 8192) {
res = -EINVAL;
break;
}
+
+ /* Use the same RECV_LEN semantics as SMBus */
+ if (rdwr_pa[i].flags & I2C_M_RECV_LEN) {
+ if (rdwr_pa[i].len < I2C_SMBUS_BLOCK_MAX + 1) {
+ res = -EINVAL;
+ break;
+ }
+ }
+
data_ptrs[i] = (u8 __user *)rdwr_pa[i].buf;
rdwr_pa[i].buf = memdup_user(data_ptrs[i], rdwr_pa[i].len);
if (IS_ERR(rdwr_pa[i].buf)) {
res = PTR_ERR(rdwr_pa[i].buf);
break;
}
+
+ /* Bus driver will add the value of the first byte received */
+ if (rdwr_pa[i].flags & I2C_M_RECV_LEN)
+ rdwr_pa[i].len -= I2C_SMBUS_BLOCK_MAX;
}
if (res < 0) {
int j;

[Jean Delvare]

Post by Jean Delvare
As the bus driver side implementation of I2C_M_RECV_LEN is heavily
tied to SMBus, we can't support received length over 32 bytes, but
let's at least support that.
In practice, the caller will have to setup a buffer large enough to
cover the case where received length byte has value 32, so minimum
32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
added for the specific slave (for example a checksum.)
---
This is an alternative to Doug's implementation. Doug, I sent this to
you one week ago, did you have the time to give it a try, do you have
any comment? Again I can't test this myself so someone else will have
to do it.

Douglas, please?

[Douglas Gilbert]

Post by Jean Delvare

Post by Jean Delvare
As the bus driver side implementation of I2C_M_RECV_LEN is heavily
tied to SMBus, we can't support received length over 32 bytes, but
let's at least support that.
In practice, the caller will have to setup a buffer large enough to
cover the case where received length byte has value 32, so minimum
32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
added for the specific slave (for example a checksum.)
---
This is an alternative to Doug's implementation. Doug, I sent this to
you one week ago, did you have the time to give it a try, do you have
any comment? Again I can't test this myself so someone else will have
to do it.

Douglas, please?

Jean,
Sorry about the delay in responding. The patch didn't work
in the case of the Sonmicro SM130 RFID but I could see it was close.

The correct response (for the SM130) when reading the firmware
version is this 10 byte response:
08 81 49 32 43 20 32 2e 38 ff ["I2C 2.8"]
so the count in the first byte excludes itself and the checksum
trailing byte. With the I2C_M_RECV_LEN patch I see this response:
08 81 49 32 43 20 32 2e 00 00
so the count (leading) byte includes itself and makes no
provision for a checksum. [So 8 bytes are returned and the two
trailing zeros are just buffer pre-fill.]

You might argue that the I2C_M_RECV_LEN patch is sensible
and the SM130 is strange. My solution is to read 32 bytes
which is more than I ever expect.

Doug Gilbert

[Jean Delvare]

Hi Douglas,

Post by Douglas Gilbert
Sorry about the delay in responding. The patch didn't work
in the case of the Sonmicro SM130 RFID but I could see it was close.
The correct response (for the SM130) when reading the firmware
08 81 49 32 43 20 32 2e 38 ff ["I2C 2.8"]
so the count in the first byte excludes itself and the checksum
08 81 49 32 43 20 32 2e 00 00
so the count (leading) byte includes itself and makes no
provision for a checksum. [So 8 bytes are returned and the two
trailing zeros are just buffer pre-fill.]

What value did you set msg->buf[0] to before calling? You were supposed
to set it to 2 in your case, exactly because the driver can't guess how
many extra bytes the chip will return, that aren't included in the byte
count. Your results suggest that you let msg->buf[0] to 0.

I've improved my patch to properly reject the transaction if buf[0] is
not set properly. Please test and report.

Post by Douglas Gilbert
You might argue that the I2C_M_RECV_LEN patch is sensible
and the SM130 is strange. My solution is to read 32 bytes
which is more than I ever expect.

The SM130 is a bit strange but it should be supportable.

* * * * *

As the bus driver side implementation of I2C_M_RECV_LEN is heavily
tied to SMBus, we can't support received length over 32 bytes, but
let's at least support that.

In practice, the caller will have to setup a buffer large enough to
cover the case where received length byte has value 32, so minimum
32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
added for the specific slave (for example a checksum.)

Signed-off-by: Jean Delvare <khali-PUYAD+***@public.gmane.org>
Cc: Douglas Gilbert <dgilbert-qazKcTl6WRFWk0Htik3J/***@public.gmane.org>
---
drivers/i2c/i2c-dev.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)

--- linux-3.4-rc1.orig/drivers/i2c/i2c-dev.c 2012-04-02 17:16:53.000000000 +0200
+++ linux-3.4-rc1/drivers/i2c/i2c-dev.c 2012-04-05 09:12:26.385033151 +0200
@@ -265,19 +265,41 @@ static noinline int i2cdev_ioctl_rdrw(st

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. */
- if ((rdwr_pa[i].len > 8192) ||
- (rdwr_pa[i].flags & I2C_M_RECV_LEN)) {
+ /* Limit the size of the message to a sane amount */
+ if (rdwr_pa[i].len > 8192) {
res = -EINVAL;
break;
}
+
data_ptrs[i] = (u8 __user *)rdwr_pa[i].buf;
rdwr_pa[i].buf = memdup_user(data_ptrs[i], rdwr_pa[i].len);
if (IS_ERR(rdwr_pa[i].buf)) {
res = PTR_ERR(rdwr_pa[i].buf);
break;
}
+
+ /*
+ * If the message length is received from the slave (similar
+ * to SMBus block read), we must ensure that the buffer will
+ * be large enough to cope with a message length of
+ * I2C_SMBUS_BLOCK_MAX as this is the maximum underlying bus
+ * drivers allow. The first byte in the buffer must be
+ * pre-filled with the number of extra bytes, which must be
+ * at least one to hold the message length, but can be
+ * greater (for example to account for a checksum byte at
+ * the end of the message.)
+ */
+ if (rdwr_pa[i].flags & I2C_M_RECV_LEN) {
+ if (!(rdwr_pa[i].flags & I2C_M_RD) ||
+ rdwr_pa[i].buf[0] < 1 ||
+ rdwr_pa[i].len < rdwr_pa[i].buf[0] +
+ I2C_SMBUS_BLOCK_MAX) {
+ res = -EINVAL;
+ break;
+ }
+
+ rdwr_pa[i].len = rdwr_pa[i].buf[0];
+ }
}
if (res < 0) {
int j;

[Douglas Gilbert]

Post by Jean Delvare
Hi Douglas,

Post by Douglas Gilbert
Sorry about the delay in responding. The patch didn't work
in the case of the Sonmicro SM130 RFID but I could see it was close.
The correct response (for the SM130) when reading the firmware
08 81 49 32 43 20 32 2e 38 ff ["I2C 2.8"]
so the count in the first byte excludes itself and the checksum
08 81 49 32 43 20 32 2e 00 00
so the count (leading) byte includes itself and makes no
provision for a checksum. [So 8 bytes are returned and the two
trailing zeros are just buffer pre-fill.]

What value did you set msg->buf[0] to before calling? You were supposed
to set it to 2 in your case, exactly because the driver can't guess how
many extra bytes the chip will return, that aren't included in the byte
count. Your results suggest that you let msg->buf[0] to 0.
I've improved my patch to properly reject the transaction if buf[0] is
not set properly. Please test and report.

Post by Douglas Gilbert
You might argue that the I2C_M_RECV_LEN patch is sensible
and the SM130 is strange. My solution is to read 32 bytes
which is more than I ever expect.

The SM130 is a bit strange but it should be supportable.
* * * * *
As the bus driver side implementation of I2C_M_RECV_LEN is heavily
tied to SMBus, we can't support received length over 32 bytes, but
let's at least support that.
In practice, the caller will have to setup a buffer large enough to
cover the case where received length byte has value 32, so minimum
32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
added for the specific slave (for example a checksum.)

Jean,
Either I am misunderstanding how to use this new patch or it is
broken. After replacing the original patch with this one, setting
msg->buf[0] to 2, my test program only sees the first two bytes
of expected data:
08 81
That is down from 8 bytes from the previous patch and 10 bytes
expected from the SM130.

Doug Gilbert

Post by Jean Delvare
---
drivers/i2c/i2c-dev.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
--- linux-3.4-rc1.orig/drivers/i2c/i2c-dev.c 2012-04-02 17:16:53.000000000 +0200
+++ linux-3.4-rc1/drivers/i2c/i2c-dev.c 2012-04-05 09:12:26.385033151 +0200
@@ -265,19 +265,41 @@ static noinline int i2cdev_ioctl_rdrw(st
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. */
- if ((rdwr_pa[i].len> 8192) ||
- (rdwr_pa[i].flags& I2C_M_RECV_LEN)) {
+ /* Limit the size of the message to a sane amount */
+ if (rdwr_pa[i].len> 8192) {
res = -EINVAL;
break;
}
+
data_ptrs[i] = (u8 __user *)rdwr_pa[i].buf;
rdwr_pa[i].buf = memdup_user(data_ptrs[i], rdwr_pa[i].len);
if (IS_ERR(rdwr_pa[i].buf)) {
res = PTR_ERR(rdwr_pa[i].buf);
break;
}
+
+ /*
+ * If the message length is received from the slave (similar
+ * to SMBus block read), we must ensure that the buffer will
+ * be large enough to cope with a message length of
+ * I2C_SMBUS_BLOCK_MAX as this is the maximum underlying bus
+ * drivers allow. The first byte in the buffer must be
+ * pre-filled with the number of extra bytes, which must be
+ * at least one to hold the message length, but can be
+ * greater (for example to account for a checksum byte at
+ * the end of the message.)
+ */
+ if (rdwr_pa[i].flags& I2C_M_RECV_LEN) {
+ if (!(rdwr_pa[i].flags& I2C_M_RD) ||
+ rdwr_pa[i].buf[0]< 1 ||
+ rdwr_pa[i].len< rdwr_pa[i].buf[0] +
+ I2C_SMBUS_BLOCK_MAX) {
+ res = -EINVAL;
+ break;
+ }
+
+ rdwr_pa[i].len = rdwr_pa[i].buf[0];
+ }
}
if (res< 0) {
int j;

[Jean Delvare]

Post by Douglas Gilbert

Post by Jean Delvare
As the bus driver side implementation of I2C_M_RECV_LEN is heavily
tied to SMBus, we can't support received length over 32 bytes, but
let's at least support that.
In practice, the caller will have to setup a buffer large enough to
cover the case where received length byte has value 32, so minimum
32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
added for the specific slave (for example a checksum.)

Either I am misunderstanding how to use this new patch or it is
broken. After replacing the original patch with this one, setting
msg->buf[0] to 2, my test program only sees the first two bytes
08 81
That is down from 8 bytes from the previous patch and 10 bytes
expected from the SM130.

Does your I2C bus driver process I2C_M_RECV_LEN at all? I bet not.
You'll have to fix that. It's fairly easy, see the reference
implementation in i2c-algo-bit.c:readbytes(). The completely untested
attempt below may do, if not you'll have to fix my code:

---
drivers/i2c/busses/i2c-at91.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)

--- linux-3.4-rc1.orig/drivers/i2c/busses/i2c-at91.c 2012-04-06 08:27:38.000000000 +0200
+++ linux-3.4-rc1/drivers/i2c/busses/i2c-at91.c 2012-04-06 08:36:47.379360574 +0200
@@ -159,13 +159,14 @@ static short at91_poll_status(unsigned l
return (loop_cntr > 0);
}

-static int xfer_read(struct i2c_adapter *adap, unsigned char *buf, int length)
+static int xfer_read(struct i2c_adapter *adap, unsigned char *buf, int length,
+ int recv_len)
{
int nack_seen = 0;
int sent_stop = 0;

/* Send Start */
- if (1 == length) {
+ if ((1 == length) && !recv_len) {
at91_twi_write(AT91_TWI_CR, AT91_TWI_START | AT91_TWI_STOP);
sent_stop = 1;
} else
@@ -174,7 +175,7 @@ static int xfer_read(struct i2c_adapter
/* Read data */
while (length--) {
/* send Stop before reading last byte (if not already done) */
- if ((0 == length) && (0 == sent_stop))
+ if ((0 == length) && (0 == sent_stop) && !recv_len)
at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
if (!at91_poll_status(AT91_TWI_RXRDY, &nack_seen)) {
dev_dbg(&adap->dev, "RXRDY timeout\n");
@@ -184,7 +185,12 @@ static int xfer_read(struct i2c_adapter
/* NACK supplies Stop */
return -EREMOTEIO;
}
- *buf++ = (at91_twi_read(AT91_TWI_RHR) & 0xff);
+ *buf = (at91_twi_read(AT91_TWI_RHR) & 0xff);
+ if (recv_len) {
+ length += *buf;
+ recv_len = 0;
+ }
+ buf++;
}

return 0;
@@ -257,7 +263,8 @@ static int at91_xfer(struct i2c_adapter

if (pmsg->len && pmsg->buf) { /* sanity check */
if (pmsg->flags & I2C_M_RD)
- ret = xfer_read(adap, pmsg->buf, pmsg->len);
+ ret = xfer_read(adap, pmsg->buf, pmsg->len,
+ pmsg->flags & I2C_M_RECV_LEN);
else
ret = xfer_write(adap, pmsg->buf, pmsg->len);

[Douglas Gilbert]

Post by Jean Delvare

Post by Douglas Gilbert

Post by Jean Delvare
As the bus driver side implementation of I2C_M_RECV_LEN is heavily
tied to SMBus, we can't support received length over 32 bytes, but
let's at least support that.
In practice, the caller will have to setup a buffer large enough to
cover the case where received length byte has value 32, so minimum
32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
added for the specific slave (for example a checksum.)

Either I am misunderstanding how to use this new patch or it is
broken. After replacing the original patch with this one, setting
msg->buf[0] to 2, my test program only sees the first two bytes
08 81
That is down from 8 bytes from the previous patch and 10 bytes
expected from the SM130.

Does your I2C bus driver process I2C_M_RECV_LEN at all? I bet not.
You'll have to fix that. It's fairly easy, see the reference
implementation in i2c-algo-bit.c:readbytes(). The completely untested

Jean,
Yes again, the modified i2c-at91.c driver that I am using does not
have support for I2C_M_RECV_LEN.

However stepping back from the minor I2C_M_RECV_LEN issue and looking
directly at the i2c-at91 driver itself. My patch to this broken
driver is included below and applies clean against lk 3.2.8
(but I note that it needs work to apply against lk 3.3). My patch
works for the AT91SAM9G20 and I have positive feedback from
the users of board-foxg20 based on that MCU.

However I see that Nikolaus Voss has submitted a replacement driver
for the i2c-at91 that works for the G45 which has two TWI units.
Is that driver going into the mainline? Surely it is much better
than the broken i2c-at91 driver that has been sitting broken for
way too long. Atmel are bringing out new MCUs in that family which
have 3 TWI units (e.g. AT91SAM9G25). Apart from the limitations
about repeated starts surely Atmel have fixed the problems referred
to in existing mainline i2c-at91.c driver from circa 2006.

My vote would be to add Nikolaus Voss's driver ASAP.

Doug Gilbert

[Jean Delvare]

Post by Douglas Gilbert
Yes again, the modified i2c-at91.c driver that I am using does not
have support for I2C_M_RECV_LEN.

Did you try my patch?

Post by Douglas Gilbert
However stepping back from the minor I2C_M_RECV_LEN issue and looking
directly at the i2c-at91 driver itself. My patch to this broken
driver is included below and applies clean against lk 3.2.8
(but I note that it needs work to apply against lk 3.3). My patch
works for the AT91SAM9G20 and I have positive feedback from
the users of board-foxg20 based on that MCU.

I picked this patch from your website already, and forward ported it,
my own patch was on top of yours.

Post by Douglas Gilbert
However I see that Nikolaus Voss has submitted a replacement driver
for the i2c-at91 that works for the G45 which has two TWI units.
Is that driver going into the mainline? Surely it is much better
than the broken i2c-at91 driver that has been sitting broken for
way too long. Atmel are bringing out new MCUs in that family which
have 3 TWI units (e.g. AT91SAM9G25). Apart from the limitations
about repeated starts surely Atmel have fixed the problems referred
to in existing mainline i2c-at91.c driver from circa 2006.
My vote would be to add Nikolaus Voss's driver ASAP.

I'm not into embedded devices, so this isn't my call.

[Douglas Gilbert]

Post by Jean Delvare

Post by Douglas Gilbert
Yes again, the modified i2c-at91.c driver that I am using does not
have support for I2C_M_RECV_LEN.

Did you try my patch?

No. I didn't realize it was on top of my patches to the
i2c-at91 driver, sorry.

Post by Jean Delvare

Post by Douglas Gilbert
However stepping back from the minor I2C_M_RECV_LEN issue and looking
directly at the i2c-at91 driver itself. My patch to this broken
driver is included below and applies clean against lk 3.2.8
(but I note that it needs work to apply against lk 3.3). My patch
works for the AT91SAM9G20 and I have positive feedback from
the users of board-foxg20 based on that MCU.

I picked this patch from your website already, and forward ported it,
my own patch was on top of yours.

So I have now applied your patch over my patch to the i2c-at91
driver and tested it. The result is the same as the previous
iteration: only two non-zero bytes: "08 81".

Post by Jean Delvare

Post by Douglas Gilbert
However I see that Nikolaus Voss has submitted a replacement driver
for the i2c-at91 that works for the G45 which has two TWI units.
Is that driver going into the mainline? Surely it is much better
than the broken i2c-at91 driver that has been sitting broken for
way too long. Atmel are bringing out new MCUs in that family which
have 3 TWI units (e.g. AT91SAM9G25). Apart from the limitations
about repeated starts surely Atmel have fixed the problems referred
to in existing mainline i2c-at91.c driver from circa 2006.
My vote would be to add Nikolaus Voss's driver ASAP.

I'm not into embedded devices, so this isn't my call.

A pity. I checked lk 3.4-rc1 and the bad old driver is still there.

Doug Gilbert

[Jean Delvare]

Post by Douglas Gilbert
So I have now applied your patch over my patch to the i2c-at91
driver and tested it. The result is the same as the previous
iteration: only two non-zero bytes: "08 81".

Bah. I already spent more time on this than I wanted, and it seems I am
going nowhere, due to the lack of hardware to test my changes. Are you
willing to debug my code and fix it?

[Voss, Nikolaus]

Hi,

Post by Douglas Gilbert

Post by Jean Delvare

Post by Douglas Gilbert
However I see that Nikolaus Voss has submitted a replacement driver
for the i2c-at91 that works for the G45 which has two TWI units.
Is that driver going into the mainline? Surely it is much better
than the broken i2c-at91 driver that has been sitting broken for
way too long. Atmel are bringing out new MCUs in that family which
have 3 TWI units (e.g. AT91SAM9G25). Apart from the limitations
about repeated starts surely Atmel have fixed the problems referred
to in existing mainline i2c-at91.c driver from circa 2006.
My vote would be to add Nikolaus Voss's driver ASAP.

I'm not into embedded devices, so this isn't my call.

A pity. I checked lk 3.4-rc1 and the bad old driver is still there.

as I see it, my driver has only been tested with the G45. If you
can confirm it works with other devices, too, maybe this would help
to get it into next.

Niko