| Message ID | 20200802192842.13527-1-wsa+renesas@sang-engineering.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [i2c-tools] i2ctransfer: add support for I2C_M_RECV_LEN | expand |
> On Aug 2, 2020, at 12:28 PM, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Strictly spoken, this is for emulating SMBus transfers, not I2C. But > hey, we still want to test devices supporting these transfers. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > I created this to be able to play around with I2C_M_RECV_LEN and with > SMBUS2 and SMBUS3 max block sizes. Tested with a Renesas Lager board and > my slave-testunit hacked with SMBus block transfer support (to be > upstreamed later). > > Fun fact: printing the correct length in the output took longer than > implementing the actual functionality. > > tools/i2ctransfer.8 | 1 + > tools/i2ctransfer.c | 42 +++++++++++++++++++++++++++++++----------- > 2 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/tools/i2ctransfer.8 b/tools/i2ctransfer.8 > index d16e34e..152d20d 100644 > --- a/tools/i2ctransfer.8 > +++ b/tools/i2ctransfer.8 > @@ -91,6 +91,7 @@ specifies if the message is read or write > .B <length_of_message> > specifies the number of bytes read or written in this message. > It is parsed as an unsigned 16 bit integer, but note that the Linux Kernel applies an additional upper limit (8192 as of v4.10). > +For read messages to targets which support SMBus Block transactions, it can also be '?', then the target will determine the length. > .TP > .B [@address] > specifies the 7-bit address of the chip to be accessed for this message, and is an integer. > diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c > index 7b95d48..2b8a3fb 100644 > --- a/tools/i2ctransfer.c > +++ b/tools/i2ctransfer.c > @@ -45,7 +45,8 @@ static void help(void) > "Usage: i2ctransfer [-f] [-y] [-v] [-V] [-a] I2CBUS DESC [DATA] [DESC [DATA]]...\n" > " I2CBUS is an integer or an I2C bus name\n" > " DESC describes the transfer in the form: {r|w}LENGTH[@address]\n" > - " 1) read/write-flag 2) LENGTH (range 0-65535) 3) I2C address (use last one if omitted)\n" > + " 1) read/write-flag 2) LENGTH (range 0-65535, or '?')\n" > + " 3) I2C address (use last one if omitted)\n" > " DATA are LENGTH bytes for a write message. They can be shortened by a suffix:\n" > " = (keep value constant until LENGTH)\n" > " + (increase value by 1 until LENGTH)\n" > @@ -84,17 +85,24 @@ static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags) > > for (i = 0; i < nmsgs; i++) { > int read = msgs[i].flags & I2C_M_RD; > + int recv_len = msgs[i].flags & I2C_M_RECV_LEN; > int print_buf = (read && (flags & PRINT_READ_BUF)) || > (!read && (flags & PRINT_WRITE_BUF)); > - > - if (flags & PRINT_HEADER) > - fprintf(output, "msg %u: addr 0x%02x, %s, len %u", > - i, msgs[i].addr, read ? "read" : "write", msgs[i].len); > + __u16 len = recv_len ? msgs[i].buf[0] + 1 : msgs[i].len; This is post-ioctl()? (Sorry, still not familiar enough with i2c-tools.) It isn’t wrong, but shouldn’t be necessary. Unless the adapter driver you’re using went astray. Not ruling that out. Before I2C_RDWR: - msg.len is sizeof(msg.buf) - msg.block[0] is <extra_bytes> After I2C_RDWR: - msg.block[0] is <len> of transfer - msg.len is <len> + <extra_bytes> (but never > sizeof(msg.buf)) Hth, Daniel > + > + if (flags & PRINT_HEADER) { > + fprintf(output, "msg %u: addr 0x%02x, %s, len ", > + i, msgs[i].addr, read ? "read" : "write"); > + if (!recv_len || flags & PRINT_READ_BUF) > + fprintf(output, "%u", len); > + else > + fprintf(output, "TBD"); > + } > > if (msgs[i].len && print_buf) { > if (flags & PRINT_HEADER) > fprintf(output, ", buf "); > - for (j = 0; j < msgs[i].len - 1; j++) > + for (j = 0; j < len - 1; j++) > fprintf(output, "0x%02x ", msgs[i].buf[j]); > /* Print final byte with newline */ > fprintf(output, "0x%02x\n", msgs[i].buf[j]); > @@ -192,13 +200,23 @@ int main(int argc, char *argv[]) > goto err_out_with_arg; > } > > - len = strtoul(arg_ptr, &end, 0); > - if (len > 0xffff || arg_ptr == end) { > - fprintf(stderr, "Error: Length invalid\n"); > - goto err_out_with_arg; > + if (*arg_ptr == '?') { > + if (!(flags & I2C_M_RD)) { > + fprintf(stderr, "Error: variable length not allowed with write\n"); > + goto err_out_with_arg; > + } > + len = 256; /* SMBUS3_MAX_BLOCK_LEN + 1 */ > + flags |= I2C_M_RECV_LEN; > + arg_ptr++; > + } else { > + len = strtoul(arg_ptr, &end, 0); > + if (len > 0xffff || arg_ptr == end) { > + fprintf(stderr, "Error: Length invalid\n"); > + goto err_out_with_arg; > + } > + arg_ptr = end; > } > > - arg_ptr = end; > if (*arg_ptr) { > if (*arg_ptr++ != '@') { > fprintf(stderr, "Error: Unknown separator after length\n"); > @@ -237,6 +255,8 @@ int main(int argc, char *argv[]) > } > memset(buf, 0, len); > msgs[nmsgs].buf = buf; > + if (flags & I2C_M_RECV_LEN) > + buf[0] = 1; /* number of extra bytes */ > } > > if (flags & I2C_M_RD || len == 0) { > -- > 2.27.0 >
Hi Daniel, > > + __u16 len = recv_len ? msgs[i].buf[0] + 1 : msgs[i].len; > > This is post-ioctl()? (Sorry, still not familiar enough with i2c-tools.) Yes, read buffers are only printed after the ioctl. And 'print_msgs' is probably the most complex function within this tool :/ > It isn’t wrong, but shouldn’t be necessary. > Unless the adapter driver you’re using went astray. Not ruling that out. I have just checked existing I2C_M_RECV_LEN handling. Quite some drivers do it wrong. And there is no consistency in what they return. Lots of things to fix there... Thanks for the quick comments, Wolfram
> On Aug 2, 2020, at 2:38 PM, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Hi Daniel, > >>> + __u16 len = recv_len ? msgs[i].buf[0] + 1 : msgs[i].len; >> >> This is post-ioctl()? (Sorry, still not familiar enough with i2c-tools.) > > Yes, read buffers are only printed after the ioctl. And 'print_msgs' is > probably the most complex function within this tool :/ > >> It isn’t wrong, but shouldn’t be necessary. >> Unless the adapter driver you’re using went astray. Not ruling that out. > > I have just checked existing I2C_M_RECV_LEN handling. Quite some drivers > do it wrong. And there is no consistency in what they return. Lots of > things to fix there... Would be curious about what variants are there. Note that msgs[i].len isn’t updated, you only get <extra_bytes> of data back, so the difference would be severe: msgs[i].len is what guides copy_to_user(). https://github.com/torvalds/linux/blob/master/drivers/i2c/i2c-dev.c#L301 Daniel
> > I have just checked existing I2C_M_RECV_LEN handling. Quite some drivers > > do it wrong. And there is no consistency in what they return. Lots of > > things to fix there... > > Would be curious about what variants are there. 1) some do it correctly 2) some hardcode the new length as recv_len + 1 (or recv_len + 2 if they think about PEC). But they don't do extra_bytes + recv_len 3) some don't touch msg->len at all 4) some also remove the flag I2C_M_RECV_LEN while processing 5) one driver always sets length to I2C_SMBUS_MAX_BLOCK_LEN no matter what the device responds ...maybe more, but I gave up. > Note that msgs[i].len isn’t updated, you only get <extra_bytes> of data back, > so the difference would be severe: msgs[i].len is what guides copy_to_user(). I think you can clearly see what was actually tested and what was coded after the specs without proper testing (or maybe just kernel-space testing?). This is why I hope my slave-testunit helps a little by providing a device to test against.
> + __u16 len = recv_len ? msgs[i].buf[0] + 1 : msgs[i].len; > + > + if (flags & PRINT_HEADER) { > + fprintf(output, "msg %u: addr 0x%02x, %s, len ", > + i, msgs[i].addr, read ? "read" : "write"); > + if (!recv_len || flags & PRINT_READ_BUF) > + fprintf(output, "%u", len); > + else > + fprintf(output, "TBD"); > + } > > if (msgs[i].len && print_buf) { This must be 'len', of course.
> On Aug 3, 2020, at 1:38 AM, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > >>> I have just checked existing I2C_M_RECV_LEN handling. Quite some drivers >>> do it wrong. And there is no consistency in what they return. Lots of >>> things to fix there... >> >> Would be curious about what variants are there. > > 1) some do it correctly > 2) some hardcode the new length as recv_len + 1 (or recv_len + 2 > if they think about PEC). But they don't do extra_bytes + recv_len > 3) some don't touch msg->len at all > 4) some also remove the flag I2C_M_RECV_LEN while processing > 5) one driver always sets length to I2C_SMBUS_MAX_BLOCK_LEN no matter > what the device responds > > ...maybe more, but I gave up. Yaah. Right. I think I see how this comes together. If the driver author only looks at __i2_transfer => master_xfer invocations as employed by i2c_smbus_xfer_emulated, and PEC isn’t used either, then that code path let’s you get away with pretty much any msgs[i].len you come up with. The smbus block reads are only looking at msgs[i].block[0] in that case. Daniel >> Note that msgs[i].len isn’t updated, you only get <extra_bytes> of data back, >> so the difference would be severe: msgs[i].len is what guides copy_to_user(). > > I think you can clearly see what was actually tested and what was coded > after the specs without proper testing (or maybe just kernel-space > testing?). This is why I hope my slave-testunit helps a little by > providing a device to test against. >
> If the driver author only looks at __i2_transfer => master_xfer invocations > as employed by i2c_smbus_xfer_emulated, and PEC isn’t used either, then that > code path let’s you get away with pretty much any msgs[i].len you come up with. Which explains the patch we are discussing here. I wanted to have a simple way to trigger I2C_M_RECV_LEN from userspace. I am thinking of adding a check to this patch: complain if msg->len is not what we expect and tell people they should report it.
diff --git a/tools/i2ctransfer.8 b/tools/i2ctransfer.8 index d16e34e..152d20d 100644 --- a/tools/i2ctransfer.8 +++ b/tools/i2ctransfer.8 @@ -91,6 +91,7 @@ specifies if the message is read or write .B <length_of_message> specifies the number of bytes read or written in this message. It is parsed as an unsigned 16 bit integer, but note that the Linux Kernel applies an additional upper limit (8192 as of v4.10). +For read messages to targets which support SMBus Block transactions, it can also be '?', then the target will determine the length. .TP .B [@address] specifies the 7-bit address of the chip to be accessed for this message, and is an integer. diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c index 7b95d48..2b8a3fb 100644 --- a/tools/i2ctransfer.c +++ b/tools/i2ctransfer.c @@ -45,7 +45,8 @@ static void help(void) "Usage: i2ctransfer [-f] [-y] [-v] [-V] [-a] I2CBUS DESC [DATA] [DESC [DATA]]...\n" " I2CBUS is an integer or an I2C bus name\n" " DESC describes the transfer in the form: {r|w}LENGTH[@address]\n" - " 1) read/write-flag 2) LENGTH (range 0-65535) 3) I2C address (use last one if omitted)\n" + " 1) read/write-flag 2) LENGTH (range 0-65535, or '?')\n" + " 3) I2C address (use last one if omitted)\n" " DATA are LENGTH bytes for a write message. They can be shortened by a suffix:\n" " = (keep value constant until LENGTH)\n" " + (increase value by 1 until LENGTH)\n" @@ -84,17 +85,24 @@ static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags) for (i = 0; i < nmsgs; i++) { int read = msgs[i].flags & I2C_M_RD; + int recv_len = msgs[i].flags & I2C_M_RECV_LEN; int print_buf = (read && (flags & PRINT_READ_BUF)) || (!read && (flags & PRINT_WRITE_BUF)); - - if (flags & PRINT_HEADER) - fprintf(output, "msg %u: addr 0x%02x, %s, len %u", - i, msgs[i].addr, read ? "read" : "write", msgs[i].len); + __u16 len = recv_len ? msgs[i].buf[0] + 1 : msgs[i].len; + + if (flags & PRINT_HEADER) { + fprintf(output, "msg %u: addr 0x%02x, %s, len ", + i, msgs[i].addr, read ? "read" : "write"); + if (!recv_len || flags & PRINT_READ_BUF) + fprintf(output, "%u", len); + else + fprintf(output, "TBD"); + } if (msgs[i].len && print_buf) { if (flags & PRINT_HEADER) fprintf(output, ", buf "); - for (j = 0; j < msgs[i].len - 1; j++) + for (j = 0; j < len - 1; j++) fprintf(output, "0x%02x ", msgs[i].buf[j]); /* Print final byte with newline */ fprintf(output, "0x%02x\n", msgs[i].buf[j]); @@ -192,13 +200,23 @@ int main(int argc, char *argv[]) goto err_out_with_arg; } - len = strtoul(arg_ptr, &end, 0); - if (len > 0xffff || arg_ptr == end) { - fprintf(stderr, "Error: Length invalid\n"); - goto err_out_with_arg; + if (*arg_ptr == '?') { + if (!(flags & I2C_M_RD)) { + fprintf(stderr, "Error: variable length not allowed with write\n"); + goto err_out_with_arg; + } + len = 256; /* SMBUS3_MAX_BLOCK_LEN + 1 */ + flags |= I2C_M_RECV_LEN; + arg_ptr++; + } else { + len = strtoul(arg_ptr, &end, 0); + if (len > 0xffff || arg_ptr == end) { + fprintf(stderr, "Error: Length invalid\n"); + goto err_out_with_arg; + } + arg_ptr = end; } - arg_ptr = end; if (*arg_ptr) { if (*arg_ptr++ != '@') { fprintf(stderr, "Error: Unknown separator after length\n"); @@ -237,6 +255,8 @@ int main(int argc, char *argv[]) } memset(buf, 0, len); msgs[nmsgs].buf = buf; + if (flags & I2C_M_RECV_LEN) + buf[0] = 1; /* number of extra bytes */ } if (flags & I2C_M_RD || len == 0) {
Strictly spoken, this is for emulating SMBus transfers, not I2C. But hey, we still want to test devices supporting these transfers. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- I created this to be able to play around with I2C_M_RECV_LEN and with SMBUS2 and SMBUS3 max block sizes. Tested with a Renesas Lager board and my slave-testunit hacked with SMBus block transfer support (to be upstreamed later). Fun fact: printing the correct length in the output took longer than implementing the actual functionality. tools/i2ctransfer.8 | 1 + tools/i2ctransfer.c | 42 +++++++++++++++++++++++++++++++----------- 2 files changed, 32 insertions(+), 11 deletions(-)