[PATCH 0/2] new tool 'i2ctransfer'

[PATCH 0/2] new tool 'i2ctransfer'

[Wolfram Sang]

So, here is my updated, better tested, much improved version of i2ctransfer:

* rewrote parser to match agreed syntax (even improved it futher by reusing
  last address if not specified again)

* fixed 0-byte length transfers

* way more error checking

* better error reporting

And further details and cleanups. I still believe we should let Linux clean up
the buffers, but I suspect Jean won't like it, so I added a patch to just do
that.

Please review, test, apply...

Thanks,

   Wolfram


Wolfram Sang (2):
  i2c-tools: add new tool 'i2ctransfer'
  i2c-tools: i2ctransfer: clean up allocated resources

 tools/Module.mk     |   8 +-
 tools/i2ctransfer.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 337 insertions(+), 1 deletion(-)
 create mode 100644 tools/i2ctransfer.c

-- 
2.1.4

[PATCH 0/2] new tool 'i2ctransfer'

[Wolfram Sang]

So, here is my updated, better tested, much improved version of i2ctransfer:

* rewrote parser to match agreed syntax (even improved it futher by reusing
  last address if not specified again)

* fixed 0-byte length transfers

* way more error checking

* better error reporting

And further details and cleanups. I still believe we should let Linux clean up
the buffers, but I suspect Jean won't like it, so I added a patch to just do
that.

Please review, test, apply...

Thanks,

   Wolfram


Wolfram Sang (2):
  i2c-tools: add new tool 'i2ctransfer'
  i2c-tools: i2ctransfer: clean up allocated resources

 tools/Module.mk     |   8 +-
 tools/i2ctransfer.c | 330 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 337 insertions(+), 1 deletion(-)
 create mode 100644 tools/i2ctransfer.c

-- 
2.1.4

[PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

This tool allows to construct and concat multiple I2C messages into one
single transfer. Its aim is to test I2C master controllers, and so there
is no SMBus fallback.

I've been missing such a tool a number of times now, so I finally got
around to writing it myself. As with all I2C tools, it can be dangerous,
but it can also be very useful when developing. I am not sure if distros
should supply it, I'll leave that to Jean's experience. For embedded
build systems, I think this should be selectable.

Tested with various Renesas I2C IP cores as well as Tegra and AT91.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 tools/Module.mk     |   8 +-
 tools/i2ctransfer.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 327 insertions(+), 1 deletion(-)
 create mode 100644 tools/i2ctransfer.c

diff --git a/tools/Module.mk b/tools/Module.mk
index 641ac81..7192361 100644
--- a/tools/Module.mk
+++ b/tools/Module.mk
@@ -18,7 +18,7 @@ else
 TOOLS_LDFLAGS	+= -Llib -li2c
 endif
 
-TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget
+TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget i2ctransfer
 
 #
 # Programs
@@ -36,6 +36,9 @@ $(TOOLS_DIR)/i2cset: $(TOOLS_DIR)/i2cset.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)
 $(TOOLS_DIR)/i2cget: $(TOOLS_DIR)/i2cget.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
 	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
 
+$(TOOLS_DIR)/i2ctransfer: $(TOOLS_DIR)/i2ctransfer.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
+	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
+
 #
 # Objects
 #
@@ -52,6 +55,9 @@ $(TOOLS_DIR)/i2cset.o: $(TOOLS_DIR)/i2cset.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DI
 $(TOOLS_DIR)/i2cget.o: $(TOOLS_DIR)/i2cget.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h $(INCLUDE_DIR)/i2c/smbus.h
 	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
 
+$(TOOLS_DIR)/i2ctransfer.o: $(TOOLS_DIR)/i2ctransfer.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h
+	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
+
 $(TOOLS_DIR)/i2cbusses.o: $(TOOLS_DIR)/i2cbusses.c $(TOOLS_DIR)/i2cbusses.h
 	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
 
diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
new file mode 100644
index 0000000..27f4d7a
--- /dev/null
+++ b/tools/i2ctransfer.c
@@ -0,0 +1,320 @@
+/*
+    i2ctransfer.c - A user-space program to send concatenated i2c messages
+    Copyright (C) 2015 Wolfram Sang <wsa@sang-engineering.com>
+    Copyright (C) 2015 Renesas Electronics Corporation
+
+    Based on i2cget.c:
+    Copyright (C) 2005-2012  Jean Delvare <jdelvare@suse.de>
+
+    which is based on i2cset.c:
+    Copyright (C) 2001-2003  Frodo Looijaard <frodol@dds.nl>, and
+                             Mark D. Studebaker <mdsxyz123@yahoo.com>
+    Copyright (C) 2004-2005  Jean Delvare
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 2 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+*/
+
+#include <sys/ioctl.h>
+#include <errno.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <linux/i2c.h>
+#include <linux/i2c-dev.h>
+#include "i2cbusses.h"
+#include "util.h"
+#include "../version.h"
+
+enum parse_state {
+	PARSE_GET_DESC,
+	PARSE_GET_DATA
+};
+
+#define PRINT_STDERR	(1 << 0)
+#define PRINT_READ_BUF	(1 << 1)
+#define PRINT_WRITE_BUF	(1 << 2)
+#define PRINT_HEADER	(1 << 3)
+
+static void help(void)
+{
+	fprintf(stderr,
+		"Usage: i2ctransfer [-f] [-y] [-v] [-V] 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"
+		"  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"
+		"    - (decrease value by 1 until LENGTH)\n"
+		"\nExample (bus 0, read 8 byte at offset 0x64 from eeprom at 0x50):\n"
+		"  # i2ctransfer 0 w1@0x50 0x64 r8\n"
+		"Example (same eeprom, at offset 0x42 write 0xff 0xfe .. 0x00 ):\n"
+		"  # i2ctransfer 0 w257@0x50 0x42 0xff-\n"
+		);
+}
+
+static int check_funcs(int file)
+{
+	unsigned long funcs;
+
+	/* check adapter functionality */
+	if (ioctl(file, I2C_FUNCS, &funcs) < 0) {
+		fprintf(stderr, "Error: Could not get the adapter "
+			"functionality matrix: %s\n", strerror(errno));
+		return -1;
+	}
+
+	if (!(funcs & I2C_FUNC_I2C)) {
+		fprintf(stderr, MISSING_FUNC_FMT, "I2C transfers");
+		return -1;
+	}
+
+	return 0;
+}
+
+static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
+{
+	__u32 i, j;
+	FILE *output = flags & PRINT_STDERR ? stderr : stdout;
+
+	for (i = 0; i < nmsgs; i++) {
+		int read = !!(msgs[i].flags & I2C_M_RD);
+		int newline = !!(flags & PRINT_HEADER);
+
+		if (flags & PRINT_HEADER)
+			fprintf(output, "Msg %u: addr 0x%02x, %s, len %u",
+				i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
+		if (msgs[i].len &&
+		   (read = !!(flags & PRINT_READ_BUF) ||
+		   !read = !!(flags & PRINT_WRITE_BUF))) {
+			if (flags & PRINT_HEADER)
+				fprintf(output, ", buf ");
+			for (j = 0; j < msgs[i].len; j++)
+				fprintf(output, "0x%02x ", msgs[i].buf[j]);
+			newline = 1;
+		}
+		if (newline)
+			fprintf(output, "\n");
+	}
+}
+
+static int confirm(const char *filename, struct i2c_msg *msgs, __u32 nmsgs)
+{
+	fprintf(stderr, "WARNING! This program can confuse your I2C bus, cause data loss and worse!\n");
+	fprintf(stderr, "I will send the following messages to device file %s:\n", filename);
+	print_msgs(msgs, nmsgs, PRINT_STDERR | PRINT_HEADER | PRINT_WRITE_BUF);
+
+	fprintf(stderr, "Continue? [y/N] ");
+	fflush(stderr);
+	if (!user_ack(0)) {
+		fprintf(stderr, "Aborting on user request.\n");
+		return 0;
+	}
+
+	return 1;
+}
+
+int main(int argc, char *argv[])
+{
+	char filename[20];
+	char *end;
+	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
+	int force = 0, yes = 0, version = 0, verbose = 0;
+	unsigned buf_idx = 0;
+	unsigned long len, raw_data;
+	__u8 data;
+	__u8 *buf;
+	__u16 flags;
+	struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
+	struct i2c_rdwr_ioctl_data rdwr;
+	enum parse_state state = PARSE_GET_DESC;
+
+	/* handle (optional) arg_idx first */
+	while (arg_idx < argc && argv[arg_idx][0] = '-') {
+		switch (argv[arg_idx][1]) {
+		case 'V': version = 1; break;
+		case 'v': verbose = 1; break;
+		case 'f': force = 1; break;
+		case 'y': yes = 1; break;
+		default:
+			fprintf(stderr, "Error: Unsupported option "
+				"\"%s\"!\n", argv[arg_idx]);
+			help();
+			exit(1);
+		}
+		arg_idx++;
+	}
+
+	if (version) {
+		fprintf(stderr, "i2ctransfer version %s\n", VERSION);
+		exit(0);
+	}
+
+	if (arg_idx = argc) {
+		help();
+		exit(0);
+	}
+
+	i2cbus = lookup_i2c_bus(argv[arg_idx++]);
+	if (i2cbus < 0)
+		exit(1);
+
+	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
+	if (file < 0 || check_funcs(file))
+		exit(1);
+
+	while (arg_idx < argc) {
+		char *arg_ptr = argv[arg_idx];
+
+		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
+			fprintf(stderr, "Error: Too many messages (max: %d)\n",
+				I2C_RDRW_IOCTL_MAX_MSGS);
+			exit(1);
+		}
+
+		switch (state) {
+		case PARSE_GET_DESC:
+			flags = 0;
+
+			switch (*arg_ptr++) {
+			case 'r': flags |= I2C_M_RD; break;
+			case 'w': break;
+			default:
+				fprintf(stderr, "Error: Invalid direction\n");
+				goto err_out;
+			}
+
+			len = strtoul(arg_ptr, &end, 0);
+			if (len > 65535) {
+				fprintf(stderr, "Error: Length invalid\n");
+				goto err_out;
+			}
+
+			arg_ptr = end;
+			if (*arg_ptr) {
+				if (*arg_ptr++ != '@') {
+					fprintf(stderr, "Error: No '@' after length\n");
+					goto err_out;
+				}
+
+				/* We skip 10-bit support for now. If we want it, it
+				 * should be marked with a 't' flag before the address
+				 * here.
+				 */
+
+				address = parse_i2c_address(arg_ptr);
+				if (address < 0)
+					goto err_out;
+
+				if (!force && set_slave_addr(file, address, 0))
+					goto err_out;
+
+			} else {
+				/* Reuse last address if possible */
+				if (address < 0) {
+					fprintf(stderr, "Error: No address given\n");
+					goto err_out;
+				}
+			}
+
+			msgs[nmsgs].addr = address;
+			msgs[nmsgs].flags = flags;
+			msgs[nmsgs].len = len;
+
+			if (len) {
+				buf = malloc(len);
+				if (!buf) {
+					fprintf(stderr, "Error: No memory for buffer\n");
+					goto err_out;
+				}
+				memset(buf, 0, len);
+				msgs[nmsgs].buf = buf;
+			}
+
+			if (flags & I2C_M_RD || len = 0) {
+				nmsgs++;
+			} else {
+				buf_idx = 0;
+				state = PARSE_GET_DATA;
+			}
+
+			break;
+
+		case PARSE_GET_DATA:
+			raw_data = strtoul(arg_ptr, &end, 0);
+			if (raw_data > 255) {
+				fprintf(stderr, "Error: Data byte invalid\n");
+				goto err_out;
+			}
+			data = raw_data;
+			len = msgs[nmsgs].len;
+
+			while (buf_idx < len) {
+				msgs[nmsgs].buf[buf_idx++] = data;
+
+				if (!*end)
+					break;
+
+				switch (*end) {
+				case '+': data++; break;
+				case '-': data--; break;
+				case '=': break;
+				default:
+					fprintf(stderr, "Error: Invalid data byte suffix\n");
+					goto err_out;
+				}
+			}
+
+			if (buf_idx = len) {
+				nmsgs++;
+				state = PARSE_GET_DESC;
+			}
+
+			break;
+
+		default:
+			fprintf(stderr, "Error: Unnkown state in state machine!\n");
+			goto err_out;
+		}
+
+		arg_idx++;
+	}
+
+	if (state != PARSE_GET_DESC || nmsgs = 0) {
+		fprintf(stderr, "Error: Incomplete message\n");
+		exit(1);
+	}
+
+	if (!yes && !confirm(filename, msgs, nmsgs))
+		exit(0);
+
+	rdwr.msgs = msgs;
+	rdwr.nmsgs = nmsgs;
+	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
+	if (nmsgs_sent < 0) {
+		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
+		exit(errno);
+	} else if (nmsgs_sent < nmsgs) {
+		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
+	}
+
+	close(file);
+
+	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
+
+	/* let Linux free malloced memory on termination */
+	exit(0);
+
+err_out:
+	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
+	exit(1);
+}
-- 
2.1.4

[PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

This tool allows to construct and concat multiple I2C messages into one
single transfer. Its aim is to test I2C master controllers, and so there
is no SMBus fallback.

I've been missing such a tool a number of times now, so I finally got
around to writing it myself. As with all I2C tools, it can be dangerous,
but it can also be very useful when developing. I am not sure if distros
should supply it, I'll leave that to Jean's experience. For embedded
build systems, I think this should be selectable.

Tested with various Renesas I2C IP cores as well as Tegra and AT91.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 tools/Module.mk     |   8 +-
 tools/i2ctransfer.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 327 insertions(+), 1 deletion(-)
 create mode 100644 tools/i2ctransfer.c

diff --git a/tools/Module.mk b/tools/Module.mk
index 641ac81..7192361 100644
--- a/tools/Module.mk
+++ b/tools/Module.mk
@@ -18,7 +18,7 @@ else
 TOOLS_LDFLAGS	+= -Llib -li2c
 endif
 
-TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget
+TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget i2ctransfer
 
 #
 # Programs
@@ -36,6 +36,9 @@ $(TOOLS_DIR)/i2cset: $(TOOLS_DIR)/i2cset.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)
 $(TOOLS_DIR)/i2cget: $(TOOLS_DIR)/i2cget.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
 	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
 
+$(TOOLS_DIR)/i2ctransfer: $(TOOLS_DIR)/i2ctransfer.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
+	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
+
 #
 # Objects
 #
@@ -52,6 +55,9 @@ $(TOOLS_DIR)/i2cset.o: $(TOOLS_DIR)/i2cset.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DI
 $(TOOLS_DIR)/i2cget.o: $(TOOLS_DIR)/i2cget.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h $(INCLUDE_DIR)/i2c/smbus.h
 	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
 
+$(TOOLS_DIR)/i2ctransfer.o: $(TOOLS_DIR)/i2ctransfer.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h
+	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
+
 $(TOOLS_DIR)/i2cbusses.o: $(TOOLS_DIR)/i2cbusses.c $(TOOLS_DIR)/i2cbusses.h
 	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
 
diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
new file mode 100644
index 0000000..27f4d7a
--- /dev/null
+++ b/tools/i2ctransfer.c
@@ -0,0 +1,320 @@
+/*
+    i2ctransfer.c - A user-space program to send concatenated i2c messages
+    Copyright (C) 2015 Wolfram Sang <wsa@sang-engineering.com>
+    Copyright (C) 2015 Renesas Electronics Corporation
+
+    Based on i2cget.c:
+    Copyright (C) 2005-2012  Jean Delvare <jdelvare@suse.de>
+
+    which is based on i2cset.c:
+    Copyright (C) 2001-2003  Frodo Looijaard <frodol@dds.nl>, and
+                             Mark D. Studebaker <mdsxyz123@yahoo.com>
+    Copyright (C) 2004-2005  Jean Delvare
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 2 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+*/
+
+#include <sys/ioctl.h>
+#include <errno.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <linux/i2c.h>
+#include <linux/i2c-dev.h>
+#include "i2cbusses.h"
+#include "util.h"
+#include "../version.h"
+
+enum parse_state {
+	PARSE_GET_DESC,
+	PARSE_GET_DATA
+};
+
+#define PRINT_STDERR	(1 << 0)
+#define PRINT_READ_BUF	(1 << 1)
+#define PRINT_WRITE_BUF	(1 << 2)
+#define PRINT_HEADER	(1 << 3)
+
+static void help(void)
+{
+	fprintf(stderr,
+		"Usage: i2ctransfer [-f] [-y] [-v] [-V] 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"
+		"  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"
+		"    - (decrease value by 1 until LENGTH)\n"
+		"\nExample (bus 0, read 8 byte at offset 0x64 from eeprom at 0x50):\n"
+		"  # i2ctransfer 0 w1@0x50 0x64 r8\n"
+		"Example (same eeprom, at offset 0x42 write 0xff 0xfe .. 0x00 ):\n"
+		"  # i2ctransfer 0 w257@0x50 0x42 0xff-\n"
+		);
+}
+
+static int check_funcs(int file)
+{
+	unsigned long funcs;
+
+	/* check adapter functionality */
+	if (ioctl(file, I2C_FUNCS, &funcs) < 0) {
+		fprintf(stderr, "Error: Could not get the adapter "
+			"functionality matrix: %s\n", strerror(errno));
+		return -1;
+	}
+
+	if (!(funcs & I2C_FUNC_I2C)) {
+		fprintf(stderr, MISSING_FUNC_FMT, "I2C transfers");
+		return -1;
+	}
+
+	return 0;
+}
+
+static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
+{
+	__u32 i, j;
+	FILE *output = flags & PRINT_STDERR ? stderr : stdout;
+
+	for (i = 0; i < nmsgs; i++) {
+		int read = !!(msgs[i].flags & I2C_M_RD);
+		int newline = !!(flags & PRINT_HEADER);
+
+		if (flags & PRINT_HEADER)
+			fprintf(output, "Msg %u: addr 0x%02x, %s, len %u",
+				i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
+		if (msgs[i].len &&
+		   (read == !!(flags & PRINT_READ_BUF) ||
+		   !read == !!(flags & PRINT_WRITE_BUF))) {
+			if (flags & PRINT_HEADER)
+				fprintf(output, ", buf ");
+			for (j = 0; j < msgs[i].len; j++)
+				fprintf(output, "0x%02x ", msgs[i].buf[j]);
+			newline = 1;
+		}
+		if (newline)
+			fprintf(output, "\n");
+	}
+}
+
+static int confirm(const char *filename, struct i2c_msg *msgs, __u32 nmsgs)
+{
+	fprintf(stderr, "WARNING! This program can confuse your I2C bus, cause data loss and worse!\n");
+	fprintf(stderr, "I will send the following messages to device file %s:\n", filename);
+	print_msgs(msgs, nmsgs, PRINT_STDERR | PRINT_HEADER | PRINT_WRITE_BUF);
+
+	fprintf(stderr, "Continue? [y/N] ");
+	fflush(stderr);
+	if (!user_ack(0)) {
+		fprintf(stderr, "Aborting on user request.\n");
+		return 0;
+	}
+
+	return 1;
+}
+
+int main(int argc, char *argv[])
+{
+	char filename[20];
+	char *end;
+	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
+	int force = 0, yes = 0, version = 0, verbose = 0;
+	unsigned buf_idx = 0;
+	unsigned long len, raw_data;
+	__u8 data;
+	__u8 *buf;
+	__u16 flags;
+	struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
+	struct i2c_rdwr_ioctl_data rdwr;
+	enum parse_state state = PARSE_GET_DESC;
+
+	/* handle (optional) arg_idx first */
+	while (arg_idx < argc && argv[arg_idx][0] == '-') {
+		switch (argv[arg_idx][1]) {
+		case 'V': version = 1; break;
+		case 'v': verbose = 1; break;
+		case 'f': force = 1; break;
+		case 'y': yes = 1; break;
+		default:
+			fprintf(stderr, "Error: Unsupported option "
+				"\"%s\"!\n", argv[arg_idx]);
+			help();
+			exit(1);
+		}
+		arg_idx++;
+	}
+
+	if (version) {
+		fprintf(stderr, "i2ctransfer version %s\n", VERSION);
+		exit(0);
+	}
+
+	if (arg_idx == argc) {
+		help();
+		exit(0);
+	}
+
+	i2cbus = lookup_i2c_bus(argv[arg_idx++]);
+	if (i2cbus < 0)
+		exit(1);
+
+	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
+	if (file < 0 || check_funcs(file))
+		exit(1);
+
+	while (arg_idx < argc) {
+		char *arg_ptr = argv[arg_idx];
+
+		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
+			fprintf(stderr, "Error: Too many messages (max: %d)\n",
+				I2C_RDRW_IOCTL_MAX_MSGS);
+			exit(1);
+		}
+
+		switch (state) {
+		case PARSE_GET_DESC:
+			flags = 0;
+
+			switch (*arg_ptr++) {
+			case 'r': flags |= I2C_M_RD; break;
+			case 'w': break;
+			default:
+				fprintf(stderr, "Error: Invalid direction\n");
+				goto err_out;
+			}
+
+			len = strtoul(arg_ptr, &end, 0);
+			if (len > 65535) {
+				fprintf(stderr, "Error: Length invalid\n");
+				goto err_out;
+			}
+
+			arg_ptr = end;
+			if (*arg_ptr) {
+				if (*arg_ptr++ != '@') {
+					fprintf(stderr, "Error: No '@' after length\n");
+					goto err_out;
+				}
+
+				/* We skip 10-bit support for now. If we want it, it
+				 * should be marked with a 't' flag before the address
+				 * here.
+				 */
+
+				address = parse_i2c_address(arg_ptr);
+				if (address < 0)
+					goto err_out;
+
+				if (!force && set_slave_addr(file, address, 0))
+					goto err_out;
+
+			} else {
+				/* Reuse last address if possible */
+				if (address < 0) {
+					fprintf(stderr, "Error: No address given\n");
+					goto err_out;
+				}
+			}
+
+			msgs[nmsgs].addr = address;
+			msgs[nmsgs].flags = flags;
+			msgs[nmsgs].len = len;
+
+			if (len) {
+				buf = malloc(len);
+				if (!buf) {
+					fprintf(stderr, "Error: No memory for buffer\n");
+					goto err_out;
+				}
+				memset(buf, 0, len);
+				msgs[nmsgs].buf = buf;
+			}
+
+			if (flags & I2C_M_RD || len == 0) {
+				nmsgs++;
+			} else {
+				buf_idx = 0;
+				state = PARSE_GET_DATA;
+			}
+
+			break;
+
+		case PARSE_GET_DATA:
+			raw_data = strtoul(arg_ptr, &end, 0);
+			if (raw_data > 255) {
+				fprintf(stderr, "Error: Data byte invalid\n");
+				goto err_out;
+			}
+			data = raw_data;
+			len = msgs[nmsgs].len;
+
+			while (buf_idx < len) {
+				msgs[nmsgs].buf[buf_idx++] = data;
+
+				if (!*end)
+					break;
+
+				switch (*end) {
+				case '+': data++; break;
+				case '-': data--; break;
+				case '=': break;
+				default:
+					fprintf(stderr, "Error: Invalid data byte suffix\n");
+					goto err_out;
+				}
+			}
+
+			if (buf_idx == len) {
+				nmsgs++;
+				state = PARSE_GET_DESC;
+			}
+
+			break;
+
+		default:
+			fprintf(stderr, "Error: Unnkown state in state machine!\n");
+			goto err_out;
+		}
+
+		arg_idx++;
+	}
+
+	if (state != PARSE_GET_DESC || nmsgs == 0) {
+		fprintf(stderr, "Error: Incomplete message\n");
+		exit(1);
+	}
+
+	if (!yes && !confirm(filename, msgs, nmsgs))
+		exit(0);
+
+	rdwr.msgs = msgs;
+	rdwr.nmsgs = nmsgs;
+	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
+	if (nmsgs_sent < 0) {
+		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
+		exit(errno);
+	} else if (nmsgs_sent < nmsgs) {
+		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
+	}
+
+	close(file);
+
+	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
+
+	/* let Linux free malloced memory on termination */
+	exit(0);
+
+err_out:
+	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
+	exit(1);
+}
-- 
2.1.4

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Jean Delvare]

Hi Wolfram,

On Fri, 19 Jun 2015 12:40:31 +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> This tool allows to construct and concat multiple I2C messages into one
> single transfer. Its aim is to test I2C master controllers, and so there
> is no SMBus fallback.

Which is fine. That wouldn't make much sense anyway as not all I2C
transactions fit into the SMBus set. For SMBus transactions we already
have i2cdump, i2cget and i2cset.

> I've been missing such a tool a number of times now, so I finally got
> around to writing it myself. As with all I2C tools, it can be dangerous,
> but it can also be very useful when developing. I am not sure if distros
> should supply it, I'll leave that to Jean's experience. For embedded
> build systems, I think this should be selectable.

I think it can be included together with the other tools. It's just as
dangerous a tool as the other ones, not more. The fact that it can't be
used on SMBus-only controllers even kind of makes it less dangerous.

> Tested with various Renesas I2C IP cores as well as Tegra and AT91.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Not needed for i2c-tools contributions.

> ---
>  tools/Module.mk     |   8 +-
>  tools/i2ctransfer.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>
>  2 files changed, 327 insertions(+), 1 deletion(-)
>  create mode 100644 tools/i2ctransfer.c

Where is the manual page? We need one, it's mandatory for some
distributions. And "make install" currently fails because
tools/i2ctransfer.8 is missing.

While this is not kernel code, I would recommend that you run the
kernel's scripts/checkpatch.pl on tools/i2ctransfer.c. Most of the
problems reported are relevant and fixing them would improve
readability.

Overall it looks really good. I made a lot of comments below but most
of them only express my preference and you are free to ignore them if
you disagree.

> diff --git a/tools/Module.mk b/tools/Module.mk
> index 641ac81..7192361 100644
> --- a/tools/Module.mk
> +++ b/tools/Module.mk
> @@ -18,7 +18,7 @@ else
>  TOOLS_LDFLAGS	+= -Llib -li2c
>  endif
>  
> -TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget
> +TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget i2ctransfer
>  
>  #
>  # Programs
> @@ -36,6 +36,9 @@ $(TOOLS_DIR)/i2cset: $(TOOLS_DIR)/i2cset.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)
>  $(TOOLS_DIR)/i2cget: $(TOOLS_DIR)/i2cget.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
>  	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
>  
> +$(TOOLS_DIR)/i2ctransfer: $(TOOLS_DIR)/i2ctransfer.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
> +	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
> +
>  #
>  # Objects
>  #
> @@ -52,6 +55,9 @@ $(TOOLS_DIR)/i2cset.o: $(TOOLS_DIR)/i2cset.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DI
>  $(TOOLS_DIR)/i2cget.o: $(TOOLS_DIR)/i2cget.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h $(INCLUDE_DIR)/i2c/smbus.h
>  	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
>  
> +$(TOOLS_DIR)/i2ctransfer.o: $(TOOLS_DIR)/i2ctransfer.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h
> +	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
> +
>  $(TOOLS_DIR)/i2cbusses.o: $(TOOLS_DIR)/i2cbusses.c $(TOOLS_DIR)/i2cbusses.h
>  	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
>  
> diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
> new file mode 100644
> index 0000000..27f4d7a
> --- /dev/null
> +++ b/tools/i2ctransfer.c
> @@ -0,0 +1,320 @@
> +/*
> +    i2ctransfer.c - A user-space program to send concatenated i2c messages
> +    Copyright (C) 2015 Wolfram Sang <wsa@sang-engineering.com>
> +    Copyright (C) 2015 Renesas Electronics Corporation
> +
> +    Based on i2cget.c:
> +    Copyright (C) 2005-2012  Jean Delvare <jdelvare@suse.de>
> +
> +    which is based on i2cset.c:
> +    Copyright (C) 2001-2003  Frodo Looijaard <frodol@dds.nl>, and
> +                             Mark D. Studebaker <mdsxyz123@yahoo.com>
> +    Copyright (C) 2004-2005  Jean Delvare

I think you can skip this. If anyone really cares, it's already
mentioned in i2cget.c.

> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 2 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +*/
> +
> +#include <sys/ioctl.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-dev.h>
> +#include "i2cbusses.h"
> +#include "util.h"
> +#include "../version.h"
> +
> +enum parse_state {
> +	PARSE_GET_DESC,
> +	PARSE_GET_DATA

There should be a trailing comma, in case you ever need to add a state.

> +};
> +
> +#define PRINT_STDERR	(1 << 0)
> +#define PRINT_READ_BUF	(1 << 1)
> +#define PRINT_WRITE_BUF	(1 << 2)
> +#define PRINT_HEADER	(1 << 3)
> +
> +static void help(void)
> +{
> +	fprintf(stderr,
> +		"Usage: i2ctransfer [-f] [-y] [-v] [-V] 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"

Note that the I2C_RDWR ioctl interface currently limits the per-message
length to 8192 bytes. I can't really think of a good reason for doing
so, other than the fact that buffers larger than this may be difficult
to allocate.

> +		"  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"
> +		"    - (decrease value by 1 until LENGTH)\n"
> +		"\nExample (bus 0, read 8 byte at offset 0x64 from eeprom at 0x50):\n"

The leading newline would rather go at end of previous line IMHO, or on
a line on its own to make it even clearer what the output will look
like.

> +		"  # i2ctransfer 0 w1@0x50 0x64 r8\n"
> +		"Example (same eeprom, at offset 0x42 write 0xff 0xfe .. 0x00 ):\n"

No space before the closing parenthesis. A shorter write would probably
serve better as an example, as it's unclear what will happen when the
register offset reaches 0x100.

> +		"  # i2ctransfer 0 w257@0x50 0x42 0xff-\n"
> +		);

The closing parenthesis can go at the end of the previous line, I think.

> +}
> +
> +static int check_funcs(int file)
> +{
> +	unsigned long funcs;
> +
> +	/* check adapter functionality */
> +	if (ioctl(file, I2C_FUNCS, &funcs) < 0) {
> +		fprintf(stderr, "Error: Could not get the adapter "
> +			"functionality matrix: %s\n", strerror(errno));
> +		return -1;
> +	}
> +
> +	if (!(funcs & I2C_FUNC_I2C)) {
> +		fprintf(stderr, MISSING_FUNC_FMT, "I2C transfers");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
> +{
> +	__u32 i, j;
> +	FILE *output = flags & PRINT_STDERR ? stderr : stdout;

Why don't you just pass the FILE * argument as the first parameter of
this function?

> +
> +	for (i = 0; i < nmsgs; i++) {
> +		int read = !!(msgs[i].flags & I2C_M_RD);
> +		int newline = !!(flags & PRINT_HEADER);

The double negation isn't needed.

> +
> +		if (flags & PRINT_HEADER)
> +			fprintf(output, "Msg %u: addr 0x%02x, %s, len %u",
> +				i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
> +		if (msgs[i].len &&
> +		   (read = !!(flags & PRINT_READ_BUF) ||
> +		   !read = !!(flags & PRINT_WRITE_BUF))) {

Could be indented (aligned) better to improve readability. Also the
last two statements seems needlessly complex. I'd write:

		if (msgs[i].len &&
		    (read && (flags & PRINT_READ_BUF) ||
		     !read && (flags & PRINT_WRITE_BUF))) {

This approach would avoid all the double negations in this function.

> +			if (flags & PRINT_HEADER)
> +				fprintf(output, ", buf ");

You'd rather drop the trailing white space...

> +			for (j = 0; j < msgs[i].len; j++)
> +				fprintf(output, "0x%02x ", msgs[i].buf[j]);

... and move the white space at the beginning here. This avoids printing
a trailing space before the newline (which could cause an extra blank
line on display if you are unlucky with the terminal width.)

> +			newline = 1;
> +		}
> +		if (newline)
> +			fprintf(output, "\n");

In which case exactly would the newline not be printed? Read of length
0? I doubt it is worth caring about. I'd even say that printing a blank
line in this case is preferable, so that the output really corresponds
to what happened. Otherwise you can't tell the difference between read
of length 0 followed by read of length 1 and the other way around.

> +	}
> +}
> +
> +static int confirm(const char *filename, struct i2c_msg *msgs, __u32 nmsgs)
> +{
> +	fprintf(stderr, "WARNING! This program can confuse your I2C bus, cause data loss and worse!\n");
> +	fprintf(stderr, "I will send the following messages to device file %s:\n", filename);
> +	print_msgs(msgs, nmsgs, PRINT_STDERR | PRINT_HEADER | PRINT_WRITE_BUF);
> +
> +	fprintf(stderr, "Continue? [y/N] ");
> +	fflush(stderr);
> +	if (!user_ack(0)) {
> +		fprintf(stderr, "Aborting on user request.\n");
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	char filename[20];
> +	char *end;
> +	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
> +	int force = 0, yes = 0, version = 0, verbose = 0;
> +	unsigned buf_idx = 0;
> +	unsigned long len, raw_data;
> +	__u8 data;
> +	__u8 *buf;
> +	__u16 flags;
> +	struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
> +	struct i2c_rdwr_ioctl_data rdwr;
> +	enum parse_state state = PARSE_GET_DESC;

That's a lot of local variables. I think some of them could be declared
inside the state machine. Did you try moving the state machine to a
separate function? Maybe if would improve readability.

> +
> +	/* handle (optional) arg_idx first */
> +	while (arg_idx < argc && argv[arg_idx][0] = '-') {
> +		switch (argv[arg_idx][1]) {
> +		case 'V': version = 1; break;
> +		case 'v': verbose = 1; break;
> +		case 'f': force = 1; break;
> +		case 'y': yes = 1; break;
> +		default:
> +			fprintf(stderr, "Error: Unsupported option "
> +				"\"%s\"!\n", argv[arg_idx]);
> +			help();
> +			exit(1);
> +		}
> +		arg_idx++;
> +	}
> +
> +	if (version) {
> +		fprintf(stderr, "i2ctransfer version %s\n", VERSION);
> +		exit(0);
> +	}
> +
> +	if (arg_idx = argc) {
> +		help();
> +		exit(0);

I think you want to return an error code here, not 0.

> +	}
> +
> +	i2cbus = lookup_i2c_bus(argv[arg_idx++]);
> +	if (i2cbus < 0)
> +		exit(1);
> +
> +	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
> +	if (file < 0 || check_funcs(file))
> +		exit(1);
> +
> +	while (arg_idx < argc) {
> +		char *arg_ptr = argv[arg_idx];
> +
> +		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
> +			fprintf(stderr, "Error: Too many messages (max: %d)\n",
> +				I2C_RDRW_IOCTL_MAX_MSGS);
> +			exit(1);
> +		}
> +
> +		switch (state) {
> +		case PARSE_GET_DESC:
> +			flags = 0;
> +
> +			switch (*arg_ptr++) {
> +			case 'r': flags |= I2C_M_RD; break;
> +			case 'w': break;
> +			default:
> +				fprintf(stderr, "Error: Invalid direction\n");
> +				goto err_out;
> +			}
> +
> +			len = strtoul(arg_ptr, &end, 0);
> +			if (len > 65535) {

0xffff would be easier to understand IMHO. Also you must check that
arg_ptr != end, i.e. you managed to parse anything. Otherwise a faulty
argument like w@0x50 will be parsed successfully (silently assuming len
= 0) and odds are that the following argument won't be parsed
successfully. In that case the error message will point the user to the
wrong argument.

> +				fprintf(stderr, "Error: Length invalid\n");
> +				goto err_out;
> +			}
> +
> +			arg_ptr = end;
> +			if (*arg_ptr) {
> +				if (*arg_ptr++ != '@') {
> +					fprintf(stderr, "Error: No '@' after length\n");

No @ after length is actually fine, that's what the "else" branch below
handles. The problem is no @ before address.

> +					goto err_out;
> +				}
> +
> +				/* We skip 10-bit support for now. If we want it, it
> +				 * should be marked with a 't' flag before the address
> +				 * here.
> +				 */
> +
> +				address = parse_i2c_address(arg_ptr);
> +				if (address < 0)
> +					goto err_out;
> +
> +				if (!force && set_slave_addr(file, address, 0))
> +					goto err_out;

It took me a while to realize that 1) this was not broken and 2) this
was actually useful. Please add a comment explaining that the only
purpose is to check that the slave address isn't busy. You don't
actually need to set the slave address at this point, the I2C_RDWR
ioctl doesn't make use of it.

> +
> +			} else {
> +				/* Reuse last address if possible */
> +				if (address < 0) {
> +					fprintf(stderr, "Error: No address given\n");
> +					goto err_out;
> +				}
> +			}
> +
> +			msgs[nmsgs].addr = address;
> +			msgs[nmsgs].flags = flags;
> +			msgs[nmsgs].len = len;
> +
> +			if (len) {
> +				buf = malloc(len);
> +				if (!buf) {
> +					fprintf(stderr, "Error: No memory for buffer\n");
> +					goto err_out;
> +				}
> +				memset(buf, 0, len);
> +				msgs[nmsgs].buf = buf;
> +			}

Else msgs[nmsgs].buf should be set to NULL. Otherwise you're passing a
random address value to the kernel.

> +
> +			if (flags & I2C_M_RD || len = 0) {
> +				nmsgs++;
> +			} else {
> +				buf_idx = 0;
> +				state = PARSE_GET_DATA;
> +			}
> +
> +			break;
> +
> +		case PARSE_GET_DATA:
> +			raw_data = strtoul(arg_ptr, &end, 0);
> +			if (raw_data > 255) {

0xff would be easier to understand IMHO. Here too you must check that
end != arg_ptr, otherwise '' or '+' will be parsed successfully
(assuming raw_data = 0.) 'x' would fail but with a confusing error
message ("Invalid data byte suffix".)

> +				fprintf(stderr, "Error: Data byte invalid\n");

"Invalid data byte" would sound better and be more consistent with
other error messages.

> +				goto err_out;
> +			}
> +			data = raw_data;
> +			len = msgs[nmsgs].len;
> +
> +			while (buf_idx < len) {
> +				msgs[nmsgs].buf[buf_idx++] = data;
> +
> +				if (!*end)
> +					break;
> +
> +				switch (*end) {
> +				case '+': data++; break;
> +				case '-': data--; break;
> +				case '=': break;
> +				default:
> +					fprintf(stderr, "Error: Invalid data byte suffix\n");
> +					goto err_out;
> +				}
> +			}
> +
> +			if (buf_idx = len) {
> +				nmsgs++;
> +				state = PARSE_GET_DESC;
> +			}
> +
> +			break;
> +
> +		default:
> +			fprintf(stderr, "Error: Unnkown state in state machine!\n");

Typo: Unknown

This can't actually happen, right? A comment saying so would be
appreciated. And "Internal error" instead of just "Error", so that the
user understands, if it ever happens.

> +			goto err_out;
> +		}
> +
> +		arg_idx++;
> +	}
> +
> +	if (state != PARSE_GET_DESC || nmsgs = 0) {

I'd be surprised if nmsgs = 0 can actually happen. You checked that at
least one parameter was left to parse on the command line before
entering the state machine. If you couldn't parse it then it must have
triggered some error earlier and you'll never reach this test.

> +		fprintf(stderr, "Error: Incomplete message\n");
> +		exit(1);
> +	}
> +
> +	if (!yes && !confirm(filename, msgs, nmsgs))
> +		exit(0);
> +
> +	rdwr.msgs = msgs;
> +	rdwr.nmsgs = nmsgs;
> +	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
> +	if (nmsgs_sent < 0) {
> +		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
> +		exit(errno);

I wouldn't recommend leaking errno back to the caller. Imagine errno is
EPERM (1), the user couldn't tell the difference with all other error
cases. IMHO there's no point in returning different error values unless
1) this is done consistently and 2) this is clearly documented in the
man page.

> +	} else if (nmsgs_sent < nmsgs) {
> +		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
> +	}
> +
> +	close(file);
> +
> +	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
> +
> +	/* let Linux free malloced memory on termination */
> +	exit(0);
> +
> +err_out:
> +	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
> +	exit(1);
> +}


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Jean Delvare]

Hi Wolfram,

On Fri, 19 Jun 2015 12:40:31 +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> 
> This tool allows to construct and concat multiple I2C messages into one
> single transfer. Its aim is to test I2C master controllers, and so there
> is no SMBus fallback.

Which is fine. That wouldn't make much sense anyway as not all I2C
transactions fit into the SMBus set. For SMBus transactions we already
have i2cdump, i2cget and i2cset.

> I've been missing such a tool a number of times now, so I finally got
> around to writing it myself. As with all I2C tools, it can be dangerous,
> but it can also be very useful when developing. I am not sure if distros
> should supply it, I'll leave that to Jean's experience. For embedded
> build systems, I think this should be selectable.

I think it can be included together with the other tools. It's just as
dangerous a tool as the other ones, not more. The fact that it can't be
used on SMBus-only controllers even kind of makes it less dangerous.

> Tested with various Renesas I2C IP cores as well as Tegra and AT91.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

Not needed for i2c-tools contributions.

> ---
>  tools/Module.mk     |   8 +-
>  tools/i2ctransfer.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>
>  2 files changed, 327 insertions(+), 1 deletion(-)
>  create mode 100644 tools/i2ctransfer.c

Where is the manual page? We need one, it's mandatory for some
distributions. And "make install" currently fails because
tools/i2ctransfer.8 is missing.

While this is not kernel code, I would recommend that you run the
kernel's scripts/checkpatch.pl on tools/i2ctransfer.c. Most of the
problems reported are relevant and fixing them would improve
readability.

Overall it looks really good. I made a lot of comments below but most
of them only express my preference and you are free to ignore them if
you disagree.

> diff --git a/tools/Module.mk b/tools/Module.mk
> index 641ac81..7192361 100644
> --- a/tools/Module.mk
> +++ b/tools/Module.mk
> @@ -18,7 +18,7 @@ else
>  TOOLS_LDFLAGS	+= -Llib -li2c
>  endif
>  
> -TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget
> +TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget i2ctransfer
>  
>  #
>  # Programs
> @@ -36,6 +36,9 @@ $(TOOLS_DIR)/i2cset: $(TOOLS_DIR)/i2cset.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)
>  $(TOOLS_DIR)/i2cget: $(TOOLS_DIR)/i2cget.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
>  	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
>  
> +$(TOOLS_DIR)/i2ctransfer: $(TOOLS_DIR)/i2ctransfer.o $(TOOLS_DIR)/i2cbusses.o $(TOOLS_DIR)/util.o
> +	$(CC) $(LDFLAGS) -o $@ $^ $(TOOLS_LDFLAGS)
> +
>  #
>  # Objects
>  #
> @@ -52,6 +55,9 @@ $(TOOLS_DIR)/i2cset.o: $(TOOLS_DIR)/i2cset.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DI
>  $(TOOLS_DIR)/i2cget.o: $(TOOLS_DIR)/i2cget.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h $(INCLUDE_DIR)/i2c/smbus.h
>  	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
>  
> +$(TOOLS_DIR)/i2ctransfer.o: $(TOOLS_DIR)/i2ctransfer.c $(TOOLS_DIR)/i2cbusses.h $(TOOLS_DIR)/util.h version.h
> +	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
> +
>  $(TOOLS_DIR)/i2cbusses.o: $(TOOLS_DIR)/i2cbusses.c $(TOOLS_DIR)/i2cbusses.h
>  	$(CC) $(CFLAGS) $(TOOLS_CFLAGS) -c $< -o $@
>  
> diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
> new file mode 100644
> index 0000000..27f4d7a
> --- /dev/null
> +++ b/tools/i2ctransfer.c
> @@ -0,0 +1,320 @@
> +/*
> +    i2ctransfer.c - A user-space program to send concatenated i2c messages
> +    Copyright (C) 2015 Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> +    Copyright (C) 2015 Renesas Electronics Corporation
> +
> +    Based on i2cget.c:
> +    Copyright (C) 2005-2012  Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
> +
> +    which is based on i2cset.c:
> +    Copyright (C) 2001-2003  Frodo Looijaard <frodol-B0qZmFHriGg@public.gmane.org>, and
> +                             Mark D. Studebaker <mdsxyz123-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
> +    Copyright (C) 2004-2005  Jean Delvare

I think you can skip this. If anyone really cares, it's already
mentioned in i2cget.c.

> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 2 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +*/
> +
> +#include <sys/ioctl.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-dev.h>
> +#include "i2cbusses.h"
> +#include "util.h"
> +#include "../version.h"
> +
> +enum parse_state {
> +	PARSE_GET_DESC,
> +	PARSE_GET_DATA

There should be a trailing comma, in case you ever need to add a state.

> +};
> +
> +#define PRINT_STDERR	(1 << 0)
> +#define PRINT_READ_BUF	(1 << 1)
> +#define PRINT_WRITE_BUF	(1 << 2)
> +#define PRINT_HEADER	(1 << 3)
> +
> +static void help(void)
> +{
> +	fprintf(stderr,
> +		"Usage: i2ctransfer [-f] [-y] [-v] [-V] 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"

Note that the I2C_RDWR ioctl interface currently limits the per-message
length to 8192 bytes. I can't really think of a good reason for doing
so, other than the fact that buffers larger than this may be difficult
to allocate.

> +		"  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"
> +		"    - (decrease value by 1 until LENGTH)\n"
> +		"\nExample (bus 0, read 8 byte at offset 0x64 from eeprom at 0x50):\n"

The leading newline would rather go at end of previous line IMHO, or on
a line on its own to make it even clearer what the output will look
like.

> +		"  # i2ctransfer 0 w1@0x50 0x64 r8\n"
> +		"Example (same eeprom, at offset 0x42 write 0xff 0xfe .. 0x00 ):\n"

No space before the closing parenthesis. A shorter write would probably
serve better as an example, as it's unclear what will happen when the
register offset reaches 0x100.

> +		"  # i2ctransfer 0 w257@0x50 0x42 0xff-\n"
> +		);

The closing parenthesis can go at the end of the previous line, I think.

> +}
> +
> +static int check_funcs(int file)
> +{
> +	unsigned long funcs;
> +
> +	/* check adapter functionality */
> +	if (ioctl(file, I2C_FUNCS, &funcs) < 0) {
> +		fprintf(stderr, "Error: Could not get the adapter "
> +			"functionality matrix: %s\n", strerror(errno));
> +		return -1;
> +	}
> +
> +	if (!(funcs & I2C_FUNC_I2C)) {
> +		fprintf(stderr, MISSING_FUNC_FMT, "I2C transfers");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
> +{
> +	__u32 i, j;
> +	FILE *output = flags & PRINT_STDERR ? stderr : stdout;

Why don't you just pass the FILE * argument as the first parameter of
this function?

> +
> +	for (i = 0; i < nmsgs; i++) {
> +		int read = !!(msgs[i].flags & I2C_M_RD);
> +		int newline = !!(flags & PRINT_HEADER);

The double negation isn't needed.

> +
> +		if (flags & PRINT_HEADER)
> +			fprintf(output, "Msg %u: addr 0x%02x, %s, len %u",
> +				i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
> +		if (msgs[i].len &&
> +		   (read == !!(flags & PRINT_READ_BUF) ||
> +		   !read == !!(flags & PRINT_WRITE_BUF))) {

Could be indented (aligned) better to improve readability. Also the
last two statements seems needlessly complex. I'd write:

		if (msgs[i].len &&
		    (read && (flags & PRINT_READ_BUF) ||
		     !read && (flags & PRINT_WRITE_BUF))) {

This approach would avoid all the double negations in this function.

> +			if (flags & PRINT_HEADER)
> +				fprintf(output, ", buf ");

You'd rather drop the trailing white space...

> +			for (j = 0; j < msgs[i].len; j++)
> +				fprintf(output, "0x%02x ", msgs[i].buf[j]);

... and move the white space at the beginning here. This avoids printing
a trailing space before the newline (which could cause an extra blank
line on display if you are unlucky with the terminal width.)

> +			newline = 1;
> +		}
> +		if (newline)
> +			fprintf(output, "\n");

In which case exactly would the newline not be printed? Read of length
0? I doubt it is worth caring about. I'd even say that printing a blank
line in this case is preferable, so that the output really corresponds
to what happened. Otherwise you can't tell the difference between read
of length 0 followed by read of length 1 and the other way around.

> +	}
> +}
> +
> +static int confirm(const char *filename, struct i2c_msg *msgs, __u32 nmsgs)
> +{
> +	fprintf(stderr, "WARNING! This program can confuse your I2C bus, cause data loss and worse!\n");
> +	fprintf(stderr, "I will send the following messages to device file %s:\n", filename);
> +	print_msgs(msgs, nmsgs, PRINT_STDERR | PRINT_HEADER | PRINT_WRITE_BUF);
> +
> +	fprintf(stderr, "Continue? [y/N] ");
> +	fflush(stderr);
> +	if (!user_ack(0)) {
> +		fprintf(stderr, "Aborting on user request.\n");
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	char filename[20];
> +	char *end;
> +	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
> +	int force = 0, yes = 0, version = 0, verbose = 0;
> +	unsigned buf_idx = 0;
> +	unsigned long len, raw_data;
> +	__u8 data;
> +	__u8 *buf;
> +	__u16 flags;
> +	struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
> +	struct i2c_rdwr_ioctl_data rdwr;
> +	enum parse_state state = PARSE_GET_DESC;

That's a lot of local variables. I think some of them could be declared
inside the state machine. Did you try moving the state machine to a
separate function? Maybe if would improve readability.

> +
> +	/* handle (optional) arg_idx first */
> +	while (arg_idx < argc && argv[arg_idx][0] == '-') {
> +		switch (argv[arg_idx][1]) {
> +		case 'V': version = 1; break;
> +		case 'v': verbose = 1; break;
> +		case 'f': force = 1; break;
> +		case 'y': yes = 1; break;
> +		default:
> +			fprintf(stderr, "Error: Unsupported option "
> +				"\"%s\"!\n", argv[arg_idx]);
> +			help();
> +			exit(1);
> +		}
> +		arg_idx++;
> +	}
> +
> +	if (version) {
> +		fprintf(stderr, "i2ctransfer version %s\n", VERSION);
> +		exit(0);
> +	}
> +
> +	if (arg_idx == argc) {
> +		help();
> +		exit(0);

I think you want to return an error code here, not 0.

> +	}
> +
> +	i2cbus = lookup_i2c_bus(argv[arg_idx++]);
> +	if (i2cbus < 0)
> +		exit(1);
> +
> +	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
> +	if (file < 0 || check_funcs(file))
> +		exit(1);
> +
> +	while (arg_idx < argc) {
> +		char *arg_ptr = argv[arg_idx];
> +
> +		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
> +			fprintf(stderr, "Error: Too many messages (max: %d)\n",
> +				I2C_RDRW_IOCTL_MAX_MSGS);
> +			exit(1);
> +		}
> +
> +		switch (state) {
> +		case PARSE_GET_DESC:
> +			flags = 0;
> +
> +			switch (*arg_ptr++) {
> +			case 'r': flags |= I2C_M_RD; break;
> +			case 'w': break;
> +			default:
> +				fprintf(stderr, "Error: Invalid direction\n");
> +				goto err_out;
> +			}
> +
> +			len = strtoul(arg_ptr, &end, 0);
> +			if (len > 65535) {

0xffff would be easier to understand IMHO. Also you must check that
arg_ptr != end, i.e. you managed to parse anything. Otherwise a faulty
argument like w@0x50 will be parsed successfully (silently assuming len
= 0) and odds are that the following argument won't be parsed
successfully. In that case the error message will point the user to the
wrong argument.

> +				fprintf(stderr, "Error: Length invalid\n");
> +				goto err_out;
> +			}
> +
> +			arg_ptr = end;
> +			if (*arg_ptr) {
> +				if (*arg_ptr++ != '@') {
> +					fprintf(stderr, "Error: No '@' after length\n");

No @ after length is actually fine, that's what the "else" branch below
handles. The problem is no @ before address.

> +					goto err_out;
> +				}
> +
> +				/* We skip 10-bit support for now. If we want it, it
> +				 * should be marked with a 't' flag before the address
> +				 * here.
> +				 */
> +
> +				address = parse_i2c_address(arg_ptr);
> +				if (address < 0)
> +					goto err_out;
> +
> +				if (!force && set_slave_addr(file, address, 0))
> +					goto err_out;

It took me a while to realize that 1) this was not broken and 2) this
was actually useful. Please add a comment explaining that the only
purpose is to check that the slave address isn't busy. You don't
actually need to set the slave address at this point, the I2C_RDWR
ioctl doesn't make use of it.

> +
> +			} else {
> +				/* Reuse last address if possible */
> +				if (address < 0) {
> +					fprintf(stderr, "Error: No address given\n");
> +					goto err_out;
> +				}
> +			}
> +
> +			msgs[nmsgs].addr = address;
> +			msgs[nmsgs].flags = flags;
> +			msgs[nmsgs].len = len;
> +
> +			if (len) {
> +				buf = malloc(len);
> +				if (!buf) {
> +					fprintf(stderr, "Error: No memory for buffer\n");
> +					goto err_out;
> +				}
> +				memset(buf, 0, len);
> +				msgs[nmsgs].buf = buf;
> +			}

Else msgs[nmsgs].buf should be set to NULL. Otherwise you're passing a
random address value to the kernel.

> +
> +			if (flags & I2C_M_RD || len == 0) {
> +				nmsgs++;
> +			} else {
> +				buf_idx = 0;
> +				state = PARSE_GET_DATA;
> +			}
> +
> +			break;
> +
> +		case PARSE_GET_DATA:
> +			raw_data = strtoul(arg_ptr, &end, 0);
> +			if (raw_data > 255) {

0xff would be easier to understand IMHO. Here too you must check that
end != arg_ptr, otherwise '' or '+' will be parsed successfully
(assuming raw_data = 0.) 'x' would fail but with a confusing error
message ("Invalid data byte suffix".)

> +				fprintf(stderr, "Error: Data byte invalid\n");

"Invalid data byte" would sound better and be more consistent with
other error messages.

> +				goto err_out;
> +			}
> +			data = raw_data;
> +			len = msgs[nmsgs].len;
> +
> +			while (buf_idx < len) {
> +				msgs[nmsgs].buf[buf_idx++] = data;
> +
> +				if (!*end)
> +					break;
> +
> +				switch (*end) {
> +				case '+': data++; break;
> +				case '-': data--; break;
> +				case '=': break;
> +				default:
> +					fprintf(stderr, "Error: Invalid data byte suffix\n");
> +					goto err_out;
> +				}
> +			}
> +
> +			if (buf_idx == len) {
> +				nmsgs++;
> +				state = PARSE_GET_DESC;
> +			}
> +
> +			break;
> +
> +		default:
> +			fprintf(stderr, "Error: Unnkown state in state machine!\n");

Typo: Unknown

This can't actually happen, right? A comment saying so would be
appreciated. And "Internal error" instead of just "Error", so that the
user understands, if it ever happens.

> +			goto err_out;
> +		}
> +
> +		arg_idx++;
> +	}
> +
> +	if (state != PARSE_GET_DESC || nmsgs == 0) {

I'd be surprised if nmsgs == 0 can actually happen. You checked that at
least one parameter was left to parse on the command line before
entering the state machine. If you couldn't parse it then it must have
triggered some error earlier and you'll never reach this test.

> +		fprintf(stderr, "Error: Incomplete message\n");
> +		exit(1);
> +	}
> +
> +	if (!yes && !confirm(filename, msgs, nmsgs))
> +		exit(0);
> +
> +	rdwr.msgs = msgs;
> +	rdwr.nmsgs = nmsgs;
> +	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
> +	if (nmsgs_sent < 0) {
> +		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
> +		exit(errno);

I wouldn't recommend leaking errno back to the caller. Imagine errno is
EPERM (1), the user couldn't tell the difference with all other error
cases. IMHO there's no point in returning different error values unless
1) this is done consistently and 2) this is clearly documented in the
man page.

> +	} else if (nmsgs_sent < nmsgs) {
> +		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
> +	}
> +
> +	close(file);
> +
> +	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
> +
> +	/* let Linux free malloced memory on termination */
> +	exit(0);
> +
> +err_out:
> +	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
> +	exit(1);
> +}


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Wolfram Sang]

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

Hi Jean,

I think we are approaching 1 year since I submitted the first version,
so I thought it might finally be time for getting this done :)

Thank you for your careful review. All comments which I did not respond
to are silently acknowledged, same for most comments to patch 2/2. I
have implemented most of the changes already, but still need to do the
testing and the man page. However, here are already the responses to
your comments.

> I think it can be included together with the other tools. It's just as
> dangerous a tool as the other ones, not more. The fact that it can't be
> used on SMBus-only controllers even kind of makes it less dangerous.

Yes, I happily agree.

> > Tested with various Renesas I2C IP cores as well as Tegra and AT91.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Not needed for i2c-tools contributions.

Feel free to drop it. I'd still like to be clear when posting patches on
public mailing lists.

> >  tools/Module.mk     |   8 +-
> >  tools/i2ctransfer.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >
> >  2 files changed, 327 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/i2ctransfer.c
> 
> Where is the manual page? We need one, it's mandatory for some
> distributions. And "make install" currently fails because
> tools/i2ctransfer.8 is missing.

I wanted to check the review before doing one.

> While this is not kernel code, I would recommend that you run the
> kernel's scripts/checkpatch.pl on tools/i2ctransfer.c. Most of the
> problems reported are relevant and fixing them would improve
> readability.

Some of them are to be consistent with the rest of the I2C tools, e.g.

ERROR: trailing statements should be on next line
#136: FILE: /home/ninja/Tools/i2c-tools/tools/i2ctransfer.c:136:
+		case 'V': version = 1; break;

And I am not too strict on the 80 char limit. Are you?

> > +	fprintf(stderr,
> > +		"Usage: i2ctransfer [-f] [-y] [-v] [-V] 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"
> 
> Note that the I2C_RDWR ioctl interface currently limits the per-message
> length to 8192 bytes. I can't really think of a good reason for doing
> so, other than the fact that buffers larger than this may be difficult
> to allocate.

I'd guess it was introduced to prevent congestion on the bus? I am
tempted to remove it in i2c-dev and allow 65535 bytes. What do you
think?

> > +static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
> > +{
> > +	__u32 i, j;
> > +	FILE *output = flags & PRINT_STDERR ? stderr : stdout;
> 
> Why don't you just pass the FILE * argument as the first parameter of
> this function?

To keep the argument list of this function small.

> > +
> > +		if (flags & PRINT_HEADER)
> > +			fprintf(output, "Msg %u: addr 0x%02x, %s, len %u",
> > +				i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
> > +		if (msgs[i].len &&
> > +		   (read == !!(flags & PRINT_READ_BUF) ||
> > +		   !read == !!(flags & PRINT_WRITE_BUF))) {
> 
> Could be indented (aligned) better to improve readability. Also the
> last two statements seems needlessly complex. I'd write:
> 
> 		if (msgs[i].len &&
> 		    (read && (flags & PRINT_READ_BUF) ||
> 		     !read && (flags & PRINT_WRITE_BUF))) {

Yes, I like. I even moved the last two lines into a seperate assignment
of a variable 'print_buf' and do now:

	if (msgs[i].len && print_buf) { ... }

> 
> > +			if (flags & PRINT_HEADER)
> > +				fprintf(output, ", buf ");
> 
> You'd rather drop the trailing white space...
> 
> > +			for (j = 0; j < msgs[i].len; j++)
> > +				fprintf(output, "0x%02x ", msgs[i].buf[j]);
> 
> ... and move the white space at the beginning here. This avoids printing
> a trailing space before the newline (which could cause an extra blank
> line on display if you are unlucky with the terminal width.)

I can't do that because I then I get a leading white space when
!PRINT_HEADER. I use an extra fprintf for the last byte now. That also
gave me an idea how to refactor the newline handling. All the double
negation is gone, too. Looks much better now!

> > +			newline = 1;
> > +		}
> > +		if (newline)
> > +			fprintf(output, "\n");
> 
> In which case exactly would the newline not be printed? Read of length
> 0? I doubt it is worth caring about. I'd even say that printing a blank
> line in this case is preferable, so that the output really corresponds
> to what happened. Otherwise you can't tell the difference between read
> of length 0 followed by read of length 1 and the other way around.

The idea is: For !PRINT_HEADER, print only the data bytes received. Because the
user crafted the transfer, I consider it known which line means what. If this
is not the case, '-v' (PRINT_HEADER) should be used for debugging. With that in
mind, I don't think a newline should be printed for read messages with length
0.

> > +int main(int argc, char *argv[])
> > +{
> > +	char filename[20];
> > +	char *end;
> > +	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
> > +	int force = 0, yes = 0, version = 0, verbose = 0;
> > +	unsigned buf_idx = 0;
> > +	unsigned long len, raw_data;
> > +	__u8 data;
> > +	__u8 *buf;
> > +	__u16 flags;
> > +	struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
> > +	struct i2c_rdwr_ioctl_data rdwr;
> > +	enum parse_state state = PARSE_GET_DESC;
> 
> That's a lot of local variables. I think some of them could be declared
> inside the state machine. Did you try moving the state machine to a
> separate function? Maybe if would improve readability.

I reduced the scope of a few variables. TBH I don't feel like putting the state
machine into a seperate function right now. It does have overhead and it feels
a bit like an academic exercise to me. And I had my share of academic excercise
these days with freeing the buffers (see later) ;)

> > +			len = strtoul(arg_ptr, &end, 0);
> > +			if (len > 65535) {
> 
> 0xffff would be easier to understand IMHO. Also you must check that
> arg_ptr != end, i.e. you managed to parse anything. Otherwise a faulty
> argument like w@0x50 will be parsed successfully (silently assuming len
> = 0) and odds are that the following argument won't be parsed
> successfully. In that case the error message will point the user to the
> wrong argument.

Great catch, thanks!

> 
> > +				fprintf(stderr, "Error: Length invalid\n");
> > +				goto err_out;
> > +			}
> > +
> > +			arg_ptr = end;
> > +			if (*arg_ptr) {
> > +				if (*arg_ptr++ != '@') {
> > +					fprintf(stderr, "Error: No '@' after length\n");
> > +			if (len) {
> > +				buf = malloc(len);
> > +				if (!buf) {
> > +					fprintf(stderr, "Error: No memory for buffer\n");
> > +					goto err_out;
> > +				}
> > +				memset(buf, 0, len);
> > +				msgs[nmsgs].buf = buf;
> > +			}
> 
> Else msgs[nmsgs].buf should be set to NULL. Otherwise you're passing a
> random address value to the kernel.

I can't do the else branch as you suggested. If I hit an error path before this
if-block, the pointer is still uninitialized and the cleanup loop will try to
free this dangling pointer. So, I need to do it earlier which is why I had the
extra loop initializing all messages in the next patch. I tried clearing the
pointer at the beginning of the switch-statement. But that caused problems,
too, when nmsgs == 0 as mentioned in the next paragraph. Then we skip the
parsing while-loop and ALL pointers are dangling.

> 
> > +
> > +	if (state != PARSE_GET_DESC || nmsgs == 0) {
> 
> I'd be surprised if nmsgs == 0 can actually happen. You checked that at
> least one parameter was left to parse on the command line before
> entering the state machine. If you couldn't parse it then it must have
> triggered some error earlier and you'll never reach this test.

It can. 'i2ctransfer 0'

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Wolfram Sang]

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

Hi Jean,

I think we are approaching 1 year since I submitted the first version,
so I thought it might finally be time for getting this done :)

Thank you for your careful review. All comments which I did not respond
to are silently acknowledged, same for most comments to patch 2/2. I
have implemented most of the changes already, but still need to do the
testing and the man page. However, here are already the responses to
your comments.

> I think it can be included together with the other tools. It's just as
> dangerous a tool as the other ones, not more. The fact that it can't be
> used on SMBus-only controllers even kind of makes it less dangerous.

Yes, I happily agree.

> > Tested with various Renesas I2C IP cores as well as Tegra and AT91.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Not needed for i2c-tools contributions.

Feel free to drop it. I'd still like to be clear when posting patches on
public mailing lists.

> >  tools/Module.mk     |   8 +-
> >  tools/i2ctransfer.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >
> >  2 files changed, 327 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/i2ctransfer.c
> 
> Where is the manual page? We need one, it's mandatory for some
> distributions. And "make install" currently fails because
> tools/i2ctransfer.8 is missing.

I wanted to check the review before doing one.

> While this is not kernel code, I would recommend that you run the
> kernel's scripts/checkpatch.pl on tools/i2ctransfer.c. Most of the
> problems reported are relevant and fixing them would improve
> readability.

Some of them are to be consistent with the rest of the I2C tools, e.g.

ERROR: trailing statements should be on next line
#136: FILE: /home/ninja/Tools/i2c-tools/tools/i2ctransfer.c:136:
+		case 'V': version = 1; break;

And I am not too strict on the 80 char limit. Are you?

> > +	fprintf(stderr,
> > +		"Usage: i2ctransfer [-f] [-y] [-v] [-V] 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"
> 
> Note that the I2C_RDWR ioctl interface currently limits the per-message
> length to 8192 bytes. I can't really think of a good reason for doing
> so, other than the fact that buffers larger than this may be difficult
> to allocate.

I'd guess it was introduced to prevent congestion on the bus? I am
tempted to remove it in i2c-dev and allow 65535 bytes. What do you
think?

> > +static void print_msgs(struct i2c_msg *msgs, __u32 nmsgs, unsigned flags)
> > +{
> > +	__u32 i, j;
> > +	FILE *output = flags & PRINT_STDERR ? stderr : stdout;
> 
> Why don't you just pass the FILE * argument as the first parameter of
> this function?

To keep the argument list of this function small.

> > +
> > +		if (flags & PRINT_HEADER)
> > +			fprintf(output, "Msg %u: addr 0x%02x, %s, len %u",
> > +				i, msgs[i].addr, read ? "read" : "write", msgs[i].len);
> > +		if (msgs[i].len &&
> > +		   (read == !!(flags & PRINT_READ_BUF) ||
> > +		   !read == !!(flags & PRINT_WRITE_BUF))) {
> 
> Could be indented (aligned) better to improve readability. Also the
> last two statements seems needlessly complex. I'd write:
> 
> 		if (msgs[i].len &&
> 		    (read && (flags & PRINT_READ_BUF) ||
> 		     !read && (flags & PRINT_WRITE_BUF))) {

Yes, I like. I even moved the last two lines into a seperate assignment
of a variable 'print_buf' and do now:

	if (msgs[i].len && print_buf) { ... }

> 
> > +			if (flags & PRINT_HEADER)
> > +				fprintf(output, ", buf ");
> 
> You'd rather drop the trailing white space...
> 
> > +			for (j = 0; j < msgs[i].len; j++)
> > +				fprintf(output, "0x%02x ", msgs[i].buf[j]);
> 
> ... and move the white space at the beginning here. This avoids printing
> a trailing space before the newline (which could cause an extra blank
> line on display if you are unlucky with the terminal width.)

I can't do that because I then I get a leading white space when
!PRINT_HEADER. I use an extra fprintf for the last byte now. That also
gave me an idea how to refactor the newline handling. All the double
negation is gone, too. Looks much better now!

> > +			newline = 1;
> > +		}
> > +		if (newline)
> > +			fprintf(output, "\n");
> 
> In which case exactly would the newline not be printed? Read of length
> 0? I doubt it is worth caring about. I'd even say that printing a blank
> line in this case is preferable, so that the output really corresponds
> to what happened. Otherwise you can't tell the difference between read
> of length 0 followed by read of length 1 and the other way around.

The idea is: For !PRINT_HEADER, print only the data bytes received. Because the
user crafted the transfer, I consider it known which line means what. If this
is not the case, '-v' (PRINT_HEADER) should be used for debugging. With that in
mind, I don't think a newline should be printed for read messages with length
0.

> > +int main(int argc, char *argv[])
> > +{
> > +	char filename[20];
> > +	char *end;
> > +	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
> > +	int force = 0, yes = 0, version = 0, verbose = 0;
> > +	unsigned buf_idx = 0;
> > +	unsigned long len, raw_data;
> > +	__u8 data;
> > +	__u8 *buf;
> > +	__u16 flags;
> > +	struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
> > +	struct i2c_rdwr_ioctl_data rdwr;
> > +	enum parse_state state = PARSE_GET_DESC;
> 
> That's a lot of local variables. I think some of them could be declared
> inside the state machine. Did you try moving the state machine to a
> separate function? Maybe if would improve readability.

I reduced the scope of a few variables. TBH I don't feel like putting the state
machine into a seperate function right now. It does have overhead and it feels
a bit like an academic exercise to me. And I had my share of academic excercise
these days with freeing the buffers (see later) ;)

> > +			len = strtoul(arg_ptr, &end, 0);
> > +			if (len > 65535) {
> 
> 0xffff would be easier to understand IMHO. Also you must check that
> arg_ptr != end, i.e. you managed to parse anything. Otherwise a faulty
> argument like w@0x50 will be parsed successfully (silently assuming len
> = 0) and odds are that the following argument won't be parsed
> successfully. In that case the error message will point the user to the
> wrong argument.

Great catch, thanks!

> 
> > +				fprintf(stderr, "Error: Length invalid\n");
> > +				goto err_out;
> > +			}
> > +
> > +			arg_ptr = end;
> > +			if (*arg_ptr) {
> > +				if (*arg_ptr++ != '@') {
> > +					fprintf(stderr, "Error: No '@' after length\n");
> > +			if (len) {
> > +				buf = malloc(len);
> > +				if (!buf) {
> > +					fprintf(stderr, "Error: No memory for buffer\n");
> > +					goto err_out;
> > +				}
> > +				memset(buf, 0, len);
> > +				msgs[nmsgs].buf = buf;
> > +			}
> 
> Else msgs[nmsgs].buf should be set to NULL. Otherwise you're passing a
> random address value to the kernel.

I can't do the else branch as you suggested. If I hit an error path before this
if-block, the pointer is still uninitialized and the cleanup loop will try to
free this dangling pointer. So, I need to do it earlier which is why I had the
extra loop initializing all messages in the next patch. I tried clearing the
pointer at the beginning of the switch-statement. But that caused problems,
too, when nmsgs == 0 as mentioned in the next paragraph. Then we skip the
parsing while-loop and ALL pointers are dangling.

> 
> > +
> > +	if (state != PARSE_GET_DESC || nmsgs == 0) {
> 
> I'd be surprised if nmsgs == 0 can actually happen. You checked that at
> least one parameter was left to parse on the command line before
> entering the state machine. If you couldn't parse it then it must have
> triggered some error earlier and you'll never reach this test.

It can. 'i2ctransfer 0'

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Uwe Kleine-König]

Hello,

On Fri, Jun 19, 2015 at 12:40:31PM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> This tool allows to construct and concat multiple I2C messages into one
> single transfer. Its aim is to test I2C master controllers, and so there
> is no SMBus fallback.
> 
> I've been missing such a tool a number of times now, so I finally got
> around to writing it myself. As with all I2C tools, it can be dangerous,
> but it can also be very useful when developing. I am not sure if distros
> should supply it, I'll leave that to Jean's experience. For embedded
> build systems, I think this should be selectable.
> 
> Tested with various Renesas I2C IP cores as well as Tegra and AT91.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I didn't look at the patch, but it would be great to get this
functionality into i2c-tools.

Note, I didn't check if this patch got applied in the meantime, my
excuse is that http://lm-sensors.org where AFAIK the i2c-tools repo is
located isn't reachable for me.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Wolfram Sang]

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


> I didn't look at the patch, but it would be great to get this
> functionality into i2c-tools.

The code is ready, but I got stuck writing the man page :(


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Uwe Kleine-König]

Hello,

On Wed, Apr 27, 2016 at 07:33:09PM +0200, Uwe Kleine-König wrote:
> On Fri, Jun 19, 2015 at 12:40:31PM +0200, Wolfram Sang wrote:
> > This tool allows to construct and concat multiple I2C messages into one
> > single transfer. Its aim is to test I2C master controllers, and so there
> > is no SMBus fallback.
> > [...]
> 
> I didn't look at the patch, but it would be great to get this
> functionality into i2c-tools.

In the meantime I tested i2ctransfer, I really like it and it helped me
debugging a problem with an i2c rtc device that isn't usable with
i2cget/i2cset.
 
> Note, I didn't check if this patch got applied in the meantime, my
> excuse is that http://lm-sensors.org where AFAIK the i2c-tools repo is
> located isn't reachable for me.

This still stands. Where is the upstream repository of i2c-tools?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Wolfram Sang]

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


> In the meantime I tested i2ctransfer, I really like it and it helped me
> debugging a problem with an i2c rtc device that isn't usable with
> i2cget/i2cset.

I read this as Tested-by :)

> > Note, I didn't check if this patch got applied in the meantime, my
> > excuse is that http://lm-sensors.org where AFAIK the i2c-tools repo is
> > located isn't reachable for me.
> 
> This still stands. Where is the upstream repository of i2c-tools?

Jean, did we already discuss moving it to kernel.org?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Uwe Kleine-König]

Hello Wolfram,

On Sun, Jun 19, 2016 at 10:00:30PM +0200, Wolfram Sang wrote:
> > In the meantime I tested i2ctransfer, I really like it and it helped me
> > debugging a problem with an i2c rtc device that isn't usable with
> > i2cget/i2cset.
> 
> I read this as Tested-by :)

That's fine. For maximal correctness use my private email address for
the tag as I did test during my free time.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Wolfram Sang]

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


> That's fine. For maximal correctness use my private email address for
> the tag as I did test during my free time.

Can't you just provide the line I should use?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Uwe Kleine-König]

On Mon, Jun 20, 2016 at 08:08:07AM +0200, Wolfram Sang wrote:
> 
> > That's fine. For maximal correctness use my private email address for
> > the tag as I did test during my free time.
> 
> Can't you just provide the line I should use?

Silly me, That would be

	Tested-by: Uwe Kleine-König <uwe@kleine-koenig.org>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Jean Delvare]

Hallo Uwe,

On Sun, 19 Jun 2016 21:56:41 +0200, Uwe Kleine-König wrote:
> > Note, I didn't check if this patch got applied in the meantime, my
> > excuse is that http://lm-sensors.org where AFAIK the i2c-tools repo is
> > located isn't reachable for me.
> 
> This still stands. Where is the upstream repository of i2c-tools?

lm-sensors.org is dead, I am in the process of creating an i2c-tools
repository at kernel.org. In the meantime you can use Guenter's git
tree as a reference:

https://github.com/groeck/i2c-tools

(This is what I will use as a base for the kernel.org repository.)

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Jean Delvare]

Hi all,

On Tue, 21 Jun 2016 11:12:55 +0200, Jean Delvare wrote:
> On Sun, 19 Jun 2016 21:56:41 +0200, Uwe Kleine-König wrote:
> > > Note, I didn't check if this patch got applied in the meantime, my
> > > excuse is that http://lm-sensors.org where AFAIK the i2c-tools repo is
> > > located isn't reachable for me.
> > 
> > This still stands. Where is the upstream repository of i2c-tools?
> 
> lm-sensors.org is dead, I am in the process of creating an i2c-tools
> repository at kernel.org. In the meantime you can use Guenter's git
> tree as a reference:
> 
> https://github.com/groeck/i2c-tools
> 
> (This is what I will use as a base for the kernel.org repository.)

This is (almost) done now. The new home of the i2c-tools repository is:

http://git.kernel.org/cgit/utils/i2c-tools/i2c-tools.git/

Guenter, Wolfram and myself have write access. As explained before, it
is based on Guenter's git mirror of the SVN repository, with the
following changes:

1* I removed the SVN tags and added clean git tags instead. SVN tags
were not so great because SVN treats tags as separate "commits" which
do not appear on the working branches. The tags I added are the parents
of the the tags I removed, they reference the same code, but in a way
that they do show up on the working branches.

2* I replaced my obsolete e-mail address by a working one for all
commits I was the author of.

When I pushed the result to kernel.org, it showed a numbers of strange
branches, maybe the result of the SVN conversion, or I messed up at
some point. I tried to clean up, please let me know if you spot
anything weird left.

We are almost there, except that a few commits went to the
i2c-tools-3.1 branch AFTER Guenter mirrored it for the last time, so
they are missing from the git tree. I will extract them from my local
copy tomorrow and commit them.

Wolfram, I was thinking maybe we could create
Documentation/i2c/userspace-tools with a pointer to the new repository?
Or if you have any other idea where it should be referenced? I've added
a pointer to the wiki already.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Guenter Roeck]

Hi Jean,

On Wed, Jun 22, 2016 at 06:52:52PM +0200, Jean Delvare wrote:
> Hi all,
> 
> On Tue, 21 Jun 2016 11:12:55 +0200, Jean Delvare wrote:
> > On Sun, 19 Jun 2016 21:56:41 +0200, Uwe Kleine-König wrote:
> > > > Note, I didn't check if this patch got applied in the meantime, my
> > > > excuse is that http://lm-sensors.org where AFAIK the i2c-tools repo is
> > > > located isn't reachable for me.
> > > 
> > > This still stands. Where is the upstream repository of i2c-tools?
> > 
> > lm-sensors.org is dead, I am in the process of creating an i2c-tools
> > repository at kernel.org. In the meantime you can use Guenter's git
> > tree as a reference:
> > 
> > https://github.com/groeck/i2c-tools
> > 
> > (This is what I will use as a base for the kernel.org repository.)
> 
> This is (almost) done now. The new home of the i2c-tools repository is:
> 
> http://git.kernel.org/cgit/utils/i2c-tools/i2c-tools.git/
> 
Thanks a lot!

Wonder if we should do the same with the lm-sensors repository,
such as utils/lm-sensors/lm-sensors.git. What do you think ?

Thanks,
Guenter

Re: [PATCH 1/2] i2c-tools: add new tool 'i2ctransfer'

[Wolfram Sang]

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


> Guenter, Wolfram and myself have write access.

Wow, thanks!

> Wolfram, I was thinking maybe we could create
> Documentation/i2c/userspace-tools with a pointer to the new repository?

I like the idea.

Thanks for this work,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

New location for i2c-tools.git

[Uwe Kleine-König]

Hello,

mainly for the archives I reply with an updated Subject.

On Wed, Jun 22, 2016 at 06:52:52PM +0200, Jean Delvare wrote:
> The new home of the i2c-tools repository is:
> 
> http://git.kernel.org/cgit/utils/i2c-tools/i2c-tools.git/

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: New location for i2c-tools.git

[Angelo Compagnucci]

Hi Uwe,

2016-06-23 9:32 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> mainly for the archives I reply with an updated Subject.
>
> On Wed, Jun 22, 2016 at 06:52:52PM +0200, Jean Delvare wrote:
>> The new home of the i2c-tools repository is:
>>
>> http://git.kernel.org/cgit/utils/i2c-tools/i2c-tools.git/

This repository presents something strange, the tagged release 3.1.2
of some months ago is no more.

This is quite awful cause some distributions (ex: buildroot ) used
that tag as the latest released version and now they are broken.

Can anyone fix this?

Thanks!


>
> Thanks
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Profile: http://it.linkedin.com/in/compagnucciangelo

Re: New location for i2c-tools.git

[Jean Delvare]

Hi Angelo,

On Thu, 23 Jun 2016 09:57:49 +0200, Angelo Compagnucci wrote:
> 2016-06-23 9:32 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > Hello,
> >
> > mainly for the archives I reply with an updated Subject.
> >
> > On Wed, Jun 22, 2016 at 06:52:52PM +0200, Jean Delvare wrote:
> >> The new home of the i2c-tools repository is:
> >>
> >> http://git.kernel.org/cgit/utils/i2c-tools/i2c-tools.git/
> 
> This repository presents something strange, the tagged release 3.1.2
> of some months ago is no more.
> 
> This is quite awful cause some distributions (ex: buildroot ) used
> that tag as the latest released version and now they are broken.
> 
> Can anyone fix this?

Quoting myself from yesterday in this very thread:

"We are almost there, except that a few commits went to the
i2c-tools-3.1 branch AFTER Guenter mirrored it for the last time, so
they are missing from the git tree. I will extract them from my local
copy tomorrow and commit them."

Yesterday's tomorrow is today.

-- 
Jean Delvare
SUSE L3 Support

Re: New location for i2c-tools.git

[Angelo Compagnucci]

2016-06-23 11:53 GMT+02:00 Jean Delvare <jdelvare@suse.de>:
> Hi Angelo,
>
> On Thu, 23 Jun 2016 09:57:49 +0200, Angelo Compagnucci wrote:
>> 2016-06-23 9:32 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > Hello,
>> >
>> > mainly for the archives I reply with an updated Subject.
>> >
>> > On Wed, Jun 22, 2016 at 06:52:52PM +0200, Jean Delvare wrote:
>> >> The new home of the i2c-tools repository is:
>> >>
>> >> http://git.kernel.org/cgit/utils/i2c-tools/i2c-tools.git/
>>
>> This repository presents something strange, the tagged release 3.1.2
>> of some months ago is no more.
>>
>> This is quite awful cause some distributions (ex: buildroot ) used
>> that tag as the latest released version and now they are broken.
>>
>> Can anyone fix this?
>
> Quoting myself from yesterday in this very thread:
>
> "We are almost there, except that a few commits went to the
> i2c-tools-3.1 branch AFTER Guenter mirrored it for the last time, so
> they are missing from the git tree. I will extract them from my local
> copy tomorrow and commit them."
>
> Yesterday's tomorrow is today.

Sorry for the noise, I totally missed that conversation. A big thank you!

>
> --
> Jean Delvare
> SUSE L3 Support



-- 
Profile: http://it.linkedin.com/in/compagnucciangelo

Re: New location for i2c-tools.git

[Jean Delvare]

On Thu, 23 Jun 2016 12:36:38 +0200, Angelo Compagnucci wrote:
> 2016-06-23 11:53 GMT+02:00 Jean Delvare <jdelvare@suse.de>:
> > Hi Angelo,
> >
> > On Thu, 23 Jun 2016 09:57:49 +0200, Angelo Compagnucci wrote:
> >> 2016-06-23 9:32 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> > Hello,
> >> >
> >> > mainly for the archives I reply with an updated Subject.
> >> >
> >> > On Wed, Jun 22, 2016 at 06:52:52PM +0200, Jean Delvare wrote:
> >> >> The new home of the i2c-tools repository is:
> >> >>
> >> >> http://git.kernel.org/cgit/utils/i2c-tools/i2c-tools.git/
> >>
> >> This repository presents something strange, the tagged release 3.1.2
> >> of some months ago is no more.
> >>
> >> This is quite awful cause some distributions (ex: buildroot ) used
> >> that tag as the latest released version and now they are broken.
> >>
> >> Can anyone fix this?
> >
> > Quoting myself from yesterday in this very thread:
> >
> > "We are almost there, except that a few commits went to the
> > i2c-tools-3.1 branch AFTER Guenter mirrored it for the last time, so
> > they are missing from the git tree. I will extract them from my local
> > copy tomorrow and commit them."
> >
> > Yesterday's tomorrow is today.
> 
> Sorry for the noise, I totally missed that conversation. A big thank you!

Everything should be there now.

The only remaining problem is that some files have occurrences of $Id$,
$Revision$ and $Date$, which SVN would replace on the fly but git does
not. Not sure what to do with them, replace them with something git
supports (not sure that exists?), or discard them.

-- 
Jean Delvare
SUSE L3 Support

Re: New location for i2c-tools.git

[Wolfram Sang]

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


> The only remaining problem is that some files have occurrences of $Id$,
> $Revision$ and $Date$, which SVN would replace on the fly but git does
> not. Not sure what to do with them, replace them with something git
> supports (not sure that exists?), or discard them.

Discard them, I'd say. You have all the history on your disk, so you
have all the information from there.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: New location for i2c-tools.git

[Jean Delvare]

Hi Wolfram,

On Thu, 23 Jun 2016 20:50:00 +0200, Wolfram Sang wrote:
> > The only remaining problem is that some files have occurrences of $Id$,
> > $Revision$ and $Date$, which SVN would replace on the fly but git does
> > not. Not sure what to do with them, replace them with something git
> > supports (not sure that exists?), or discard them.
> 
> Discard them, I'd say. You have all the history on your disk, so you
> have all the information from there.

For C source files, I agree, these keywords are useless.

The problem is for scripts, like decode-dimms (and sensors-detect, for
the lm-sensors part.) We can't "#include <version.h>" from these, and
my experience is that including a "manual" version string in such
scripts doesn't work because we tend to forget to update it (can
probably be addressed with some discipline.)

Also including the date turned out to be very convenient to figure out
quickly if a user report was from a recent enough version of the script
(particularly useful for sensors-detect, of which the latest snapshot
was available for direct download and in-place execution.)

-- 
Jean Delvare
SUSE L3 Support

Re: New location for i2c-tools.git

[Wolfram Sang]

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


> The problem is for scripts, like decode-dimms (and sensors-detect, for
> the lm-sensors part.) We can't "#include <version.h>" from these, and

Well, version.h only changes per release, or? So, when preparing a
release, the release script could parse version.h and modify the
scripts.

> Also including the date turned out to be very convenient to figure out
> quickly if a user report was from a recent enough version of the script
> (particularly useful for sensors-detect, of which the latest snapshot
> was available for direct download and in-place execution.)

This one is more tricky, agreed.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] i2c-tools: i2ctransfer: clean up allocated resources

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

I still think this makes the code less readable and unnecessarily big [1],
but I assume Jean insists on it :) So, here is an add-on patch to squash.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

[1] http://www.gnu.org/software/libc/manual/html_node/Freeing-after-Malloc.html#Freeing-after-Malloc

"There is no point in freeing blocks at the end of a program, because
all of the program’s space is given back to the system when the process
terminates."
---
 tools/i2ctransfer.c | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
index 27f4d7a..418e303 100644
--- a/tools/i2ctransfer.c
+++ b/tools/i2ctransfer.c
@@ -127,7 +127,7 @@ int main(int argc, char *argv[])
 {
 	char filename[20];
 	char *end;
-	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
+	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent, i;
 	int force = 0, yes = 0, version = 0, verbose = 0;
 	unsigned buf_idx = 0;
 	unsigned long len, raw_data;
@@ -138,6 +138,9 @@ int main(int argc, char *argv[])
 	struct i2c_rdwr_ioctl_data rdwr;
 	enum parse_state state = PARSE_GET_DESC;
 
+	for (i = 0; i < I2C_RDRW_IOCTL_MAX_MSGS; i++)
+		msgs[i].buf = NULL;
+
 	/* handle (optional) arg_idx first */
 	while (arg_idx < argc && argv[arg_idx][0] = '-') {
 		switch (argv[arg_idx][1]) {
@@ -178,7 +181,7 @@ int main(int argc, char *argv[])
 		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
 			fprintf(stderr, "Error: Too many messages (max: %d)\n",
 				I2C_RDRW_IOCTL_MAX_MSGS);
-			exit(1);
+			goto err_out;
 		}
 
 		switch (state) {
@@ -190,20 +193,20 @@ int main(int argc, char *argv[])
 			case 'w': break;
 			default:
 				fprintf(stderr, "Error: Invalid direction\n");
-				goto err_out;
+				goto err_out_with_arg;
 			}
 
 			len = strtoul(arg_ptr, &end, 0);
 			if (len > 65535) {
 				fprintf(stderr, "Error: Length invalid\n");
-				goto err_out;
+				goto err_out_with_arg;
 			}
 
 			arg_ptr = end;
 			if (*arg_ptr) {
 				if (*arg_ptr++ != '@') {
 					fprintf(stderr, "Error: No '@' after length\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 
 				/* We skip 10-bit support for now. If we want it, it
@@ -213,16 +216,16 @@ int main(int argc, char *argv[])
 
 				address = parse_i2c_address(arg_ptr);
 				if (address < 0)
-					goto err_out;
+					goto err_out_with_arg;
 
 				if (!force && set_slave_addr(file, address, 0))
-					goto err_out;
+					goto err_out_with_arg;
 
 			} else {
 				/* Reuse last address if possible */
 				if (address < 0) {
 					fprintf(stderr, "Error: No address given\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 			}
 
@@ -234,7 +237,7 @@ int main(int argc, char *argv[])
 				buf = malloc(len);
 				if (!buf) {
 					fprintf(stderr, "Error: No memory for buffer\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 				memset(buf, 0, len);
 				msgs[nmsgs].buf = buf;
@@ -253,7 +256,7 @@ int main(int argc, char *argv[])
 			raw_data = strtoul(arg_ptr, &end, 0);
 			if (raw_data > 255) {
 				fprintf(stderr, "Error: Data byte invalid\n");
-				goto err_out;
+				goto err_out_with_arg;
 			}
 			data = raw_data;
 			len = msgs[nmsgs].len;
@@ -270,7 +273,7 @@ int main(int argc, char *argv[])
 				case '=': break;
 				default:
 					fprintf(stderr, "Error: Invalid data byte suffix\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 			}
 
@@ -283,7 +286,7 @@ int main(int argc, char *argv[])
 
 		default:
 			fprintf(stderr, "Error: Unnkown state in state machine!\n");
-			goto err_out;
+			goto err_out_with_arg;
 		}
 
 		arg_idx++;
@@ -291,18 +294,18 @@ int main(int argc, char *argv[])
 
 	if (state != PARSE_GET_DESC || nmsgs = 0) {
 		fprintf(stderr, "Error: Incomplete message\n");
-		exit(1);
+		goto err_out;
 	}
 
 	if (!yes && !confirm(filename, msgs, nmsgs))
-		exit(0);
+		goto out;
 
 	rdwr.msgs = msgs;
 	rdwr.nmsgs = nmsgs;
 	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
 	if (nmsgs_sent < 0) {
 		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
-		exit(errno);
+		goto err_out;
 	} else if (nmsgs_sent < nmsgs) {
 		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
 	}
@@ -311,10 +314,17 @@ int main(int argc, char *argv[])
 
 	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
 
-	/* let Linux free malloced memory on termination */
+out:
+	for (i = 0; i <= nmsgs; i++)
+		free(msgs[i].buf);
+
 	exit(0);
 
-err_out:
+err_out_with_arg:
 	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
+err_out:
+	for (i = 0; i <= nmsgs; i++)
+		free(msgs[i].buf);
+
 	exit(1);
 }
-- 
2.1.4

[PATCH 2/2] i2c-tools: i2ctransfer: clean up allocated resources

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

I still think this makes the code less readable and unnecessarily big [1],
but I assume Jean insists on it :) So, here is an add-on patch to squash.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

[1] http://www.gnu.org/software/libc/manual/html_node/Freeing-after-Malloc.html#Freeing-after-Malloc

"There is no point in freeing blocks at the end of a program, because
all of the program’s space is given back to the system when the process
terminates."
---
 tools/i2ctransfer.c | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
index 27f4d7a..418e303 100644
--- a/tools/i2ctransfer.c
+++ b/tools/i2ctransfer.c
@@ -127,7 +127,7 @@ int main(int argc, char *argv[])
 {
 	char filename[20];
 	char *end;
-	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
+	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent, i;
 	int force = 0, yes = 0, version = 0, verbose = 0;
 	unsigned buf_idx = 0;
 	unsigned long len, raw_data;
@@ -138,6 +138,9 @@ int main(int argc, char *argv[])
 	struct i2c_rdwr_ioctl_data rdwr;
 	enum parse_state state = PARSE_GET_DESC;
 
+	for (i = 0; i < I2C_RDRW_IOCTL_MAX_MSGS; i++)
+		msgs[i].buf = NULL;
+
 	/* handle (optional) arg_idx first */
 	while (arg_idx < argc && argv[arg_idx][0] == '-') {
 		switch (argv[arg_idx][1]) {
@@ -178,7 +181,7 @@ int main(int argc, char *argv[])
 		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
 			fprintf(stderr, "Error: Too many messages (max: %d)\n",
 				I2C_RDRW_IOCTL_MAX_MSGS);
-			exit(1);
+			goto err_out;
 		}
 
 		switch (state) {
@@ -190,20 +193,20 @@ int main(int argc, char *argv[])
 			case 'w': break;
 			default:
 				fprintf(stderr, "Error: Invalid direction\n");
-				goto err_out;
+				goto err_out_with_arg;
 			}
 
 			len = strtoul(arg_ptr, &end, 0);
 			if (len > 65535) {
 				fprintf(stderr, "Error: Length invalid\n");
-				goto err_out;
+				goto err_out_with_arg;
 			}
 
 			arg_ptr = end;
 			if (*arg_ptr) {
 				if (*arg_ptr++ != '@') {
 					fprintf(stderr, "Error: No '@' after length\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 
 				/* We skip 10-bit support for now. If we want it, it
@@ -213,16 +216,16 @@ int main(int argc, char *argv[])
 
 				address = parse_i2c_address(arg_ptr);
 				if (address < 0)
-					goto err_out;
+					goto err_out_with_arg;
 
 				if (!force && set_slave_addr(file, address, 0))
-					goto err_out;
+					goto err_out_with_arg;
 
 			} else {
 				/* Reuse last address if possible */
 				if (address < 0) {
 					fprintf(stderr, "Error: No address given\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 			}
 
@@ -234,7 +237,7 @@ int main(int argc, char *argv[])
 				buf = malloc(len);
 				if (!buf) {
 					fprintf(stderr, "Error: No memory for buffer\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 				memset(buf, 0, len);
 				msgs[nmsgs].buf = buf;
@@ -253,7 +256,7 @@ int main(int argc, char *argv[])
 			raw_data = strtoul(arg_ptr, &end, 0);
 			if (raw_data > 255) {
 				fprintf(stderr, "Error: Data byte invalid\n");
-				goto err_out;
+				goto err_out_with_arg;
 			}
 			data = raw_data;
 			len = msgs[nmsgs].len;
@@ -270,7 +273,7 @@ int main(int argc, char *argv[])
 				case '=': break;
 				default:
 					fprintf(stderr, "Error: Invalid data byte suffix\n");
-					goto err_out;
+					goto err_out_with_arg;
 				}
 			}
 
@@ -283,7 +286,7 @@ int main(int argc, char *argv[])
 
 		default:
 			fprintf(stderr, "Error: Unnkown state in state machine!\n");
-			goto err_out;
+			goto err_out_with_arg;
 		}
 
 		arg_idx++;
@@ -291,18 +294,18 @@ int main(int argc, char *argv[])
 
 	if (state != PARSE_GET_DESC || nmsgs == 0) {
 		fprintf(stderr, "Error: Incomplete message\n");
-		exit(1);
+		goto err_out;
 	}
 
 	if (!yes && !confirm(filename, msgs, nmsgs))
-		exit(0);
+		goto out;
 
 	rdwr.msgs = msgs;
 	rdwr.nmsgs = nmsgs;
 	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
 	if (nmsgs_sent < 0) {
 		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
-		exit(errno);
+		goto err_out;
 	} else if (nmsgs_sent < nmsgs) {
 		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
 	}
@@ -311,10 +314,17 @@ int main(int argc, char *argv[])
 
 	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
 
-	/* let Linux free malloced memory on termination */
+out:
+	for (i = 0; i <= nmsgs; i++)
+		free(msgs[i].buf);
+
 	exit(0);
 
-err_out:
+err_out_with_arg:
 	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
+err_out:
+	for (i = 0; i <= nmsgs; i++)
+		free(msgs[i].buf);
+
 	exit(1);
 }
-- 
2.1.4

Re: [PATCH 2/2] i2c-tools: i2ctransfer: clean up allocated resources

[Jean Delvare]

Hi Wolfram,

On Fri, 19 Jun 2015 12:40:32 +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> I still think this makes the code less readable and unnecessarily big [1],
> but I assume Jean insists on it :) So, here is an add-on patch to squash.

Oh yeah. I'd also love if you could close the i2c device file before
leaving, even in error cases ;-)

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> [1] http://www.gnu.org/software/libc/manual/html_node/Freeing-after-Malloc.html#Freeing-after-Malloc
> 
> "There is no point in freeing blocks at the end of a program, because
> all of the program’s space is given back to the system when the process
> terminates."

Yeah, like the GNU folks are right on everything. Just see their
recommended coding style... :D

I know that the memory would be freed anyway. But I think there is
value in consistency. Also freeing the memory documents the memory
allocation model as a nice side effect. And avoids bad surprises when
one copies code from a command line tool to a GUI tool or a daemon. And
it lets you run the code under valgrind.

So I see the cost but I still believe that the benefits outweigh that
cost.

> ---
>  tools/i2ctransfer.c | 44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
> index 27f4d7a..418e303 100644
> --- a/tools/i2ctransfer.c
> +++ b/tools/i2ctransfer.c
> @@ -127,7 +127,7 @@ int main(int argc, char *argv[])
>  {
>  	char filename[20];
>  	char *end;
> -	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
> +	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent, i;
>  	int force = 0, yes = 0, version = 0, verbose = 0;
>  	unsigned buf_idx = 0;
>  	unsigned long len, raw_data;
> @@ -138,6 +138,9 @@ int main(int argc, char *argv[])
>  	struct i2c_rdwr_ioctl_data rdwr;
>  	enum parse_state state = PARSE_GET_DESC;
>  
> +	for (i = 0; i < I2C_RDRW_IOCTL_MAX_MSGS; i++)
> +		msgs[i].buf = NULL;
> +

If you explicitly set "buf = NULL" for zero-length messages in the
state machine as recommended in my review of the previous patch, this
is no longer needed.

>  	/* handle (optional) arg_idx first */
>  	while (arg_idx < argc && argv[arg_idx][0] = '-') {
>  		switch (argv[arg_idx][1]) {
> @@ -178,7 +181,7 @@ int main(int argc, char *argv[])
>  		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
>  			fprintf(stderr, "Error: Too many messages (max: %d)\n",
>  				I2C_RDRW_IOCTL_MAX_MSGS);
> -			exit(1);
> +			goto err_out;
>  		}
>  
>  		switch (state) {
> @@ -190,20 +193,20 @@ int main(int argc, char *argv[])
>  			case 'w': break;
>  			default:
>  				fprintf(stderr, "Error: Invalid direction\n");
> -				goto err_out;
> +				goto err_out_with_arg;
>  			}
>  
>  			len = strtoul(arg_ptr, &end, 0);
>  			if (len > 65535) {
>  				fprintf(stderr, "Error: Length invalid\n");
> -				goto err_out;
> +				goto err_out_with_arg;
>  			}
>  
>  			arg_ptr = end;
>  			if (*arg_ptr) {
>  				if (*arg_ptr++ != '@') {
>  					fprintf(stderr, "Error: No '@' after length\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  
>  				/* We skip 10-bit support for now. If we want it, it
> @@ -213,16 +216,16 @@ int main(int argc, char *argv[])
>  
>  				address = parse_i2c_address(arg_ptr);
>  				if (address < 0)
> -					goto err_out;
> +					goto err_out_with_arg;
>  
>  				if (!force && set_slave_addr(file, address, 0))
> -					goto err_out;
> +					goto err_out_with_arg;
>  
>  			} else {
>  				/* Reuse last address if possible */
>  				if (address < 0) {
>  					fprintf(stderr, "Error: No address given\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  			}
>  
> @@ -234,7 +237,7 @@ int main(int argc, char *argv[])
>  				buf = malloc(len);
>  				if (!buf) {
>  					fprintf(stderr, "Error: No memory for buffer\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  				memset(buf, 0, len);
>  				msgs[nmsgs].buf = buf;
> @@ -253,7 +256,7 @@ int main(int argc, char *argv[])
>  			raw_data = strtoul(arg_ptr, &end, 0);
>  			if (raw_data > 255) {
>  				fprintf(stderr, "Error: Data byte invalid\n");
> -				goto err_out;
> +				goto err_out_with_arg;
>  			}
>  			data = raw_data;
>  			len = msgs[nmsgs].len;
> @@ -270,7 +273,7 @@ int main(int argc, char *argv[])
>  				case '=': break;
>  				default:
>  					fprintf(stderr, "Error: Invalid data byte suffix\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  			}
>  
> @@ -283,7 +286,7 @@ int main(int argc, char *argv[])
>  
>  		default:
>  			fprintf(stderr, "Error: Unnkown state in state machine!\n");
> -			goto err_out;
> +			goto err_out_with_arg;

I'd stick to err_out in this case. As this isn't supposed to happen,
you have no idea if printing argv[arg_idx] is relevant or not. And it
is likely to confuse the user.

>  		}
>  
>  		arg_idx++;
> @@ -291,18 +294,18 @@ int main(int argc, char *argv[])
>  
>  	if (state != PARSE_GET_DESC || nmsgs = 0) {
>  		fprintf(stderr, "Error: Incomplete message\n");
> -		exit(1);
> +		goto err_out;
>  	}
>  
>  	if (!yes && !confirm(filename, msgs, nmsgs))
> -		exit(0);
> +		goto out;
>  
>  	rdwr.msgs = msgs;
>  	rdwr.nmsgs = nmsgs;
>  	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
>  	if (nmsgs_sent < 0) {
>  		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
> -		exit(errno);
> +		goto err_out;
>  	} else if (nmsgs_sent < nmsgs) {
>  		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
>  	}
> @@ -311,10 +314,17 @@ int main(int argc, char *argv[])
>  
>  	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
>  
> -	/* let Linux free malloced memory on termination */
> +out:

One space before labels please, so as to not break "diff -p".

> +	for (i = 0; i <= nmsgs; i++)
> +		free(msgs[i].buf);

It would be <, not <=.

Another approach is:

	for (; nmsgs >= 0; nmsgs--)
		free(msgs[nmsgs].buf);

which avoids introducing another loop variable.

> +
>  	exit(0);
>  
> -err_out:
> +err_out_with_arg:
>  	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
> +err_out:
> +	for (i = 0; i <= nmsgs; i++)
> +		free(msgs[i].buf);
> +
>  	exit(1);
>  }

Thanks for doing that.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] i2c-tools: i2ctransfer: clean up allocated resources

[Jean Delvare]

Hi Wolfram,

On Fri, 19 Jun 2015 12:40:32 +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> I still think this makes the code less readable and unnecessarily big [1],
> but I assume Jean insists on it :) So, here is an add-on patch to squash.

Oh yeah. I'd also love if you could close the i2c device file before
leaving, even in error cases ;-)

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> [1] http://www.gnu.org/software/libc/manual/html_node/Freeing-after-Malloc.html#Freeing-after-Malloc
> 
> "There is no point in freeing blocks at the end of a program, because
> all of the program’s space is given back to the system when the process
> terminates."

Yeah, like the GNU folks are right on everything. Just see their
recommended coding style... :D

I know that the memory would be freed anyway. But I think there is
value in consistency. Also freeing the memory documents the memory
allocation model as a nice side effect. And avoids bad surprises when
one copies code from a command line tool to a GUI tool or a daemon. And
it lets you run the code under valgrind.

So I see the cost but I still believe that the benefits outweigh that
cost.

> ---
>  tools/i2ctransfer.c | 44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/i2ctransfer.c b/tools/i2ctransfer.c
> index 27f4d7a..418e303 100644
> --- a/tools/i2ctransfer.c
> +++ b/tools/i2ctransfer.c
> @@ -127,7 +127,7 @@ int main(int argc, char *argv[])
>  {
>  	char filename[20];
>  	char *end;
> -	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent;
> +	int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent, i;
>  	int force = 0, yes = 0, version = 0, verbose = 0;
>  	unsigned buf_idx = 0;
>  	unsigned long len, raw_data;
> @@ -138,6 +138,9 @@ int main(int argc, char *argv[])
>  	struct i2c_rdwr_ioctl_data rdwr;
>  	enum parse_state state = PARSE_GET_DESC;
>  
> +	for (i = 0; i < I2C_RDRW_IOCTL_MAX_MSGS; i++)
> +		msgs[i].buf = NULL;
> +

If you explicitly set "buf = NULL" for zero-length messages in the
state machine as recommended in my review of the previous patch, this
is no longer needed.

>  	/* handle (optional) arg_idx first */
>  	while (arg_idx < argc && argv[arg_idx][0] == '-') {
>  		switch (argv[arg_idx][1]) {
> @@ -178,7 +181,7 @@ int main(int argc, char *argv[])
>  		if (nmsgs > I2C_RDRW_IOCTL_MAX_MSGS) {
>  			fprintf(stderr, "Error: Too many messages (max: %d)\n",
>  				I2C_RDRW_IOCTL_MAX_MSGS);
> -			exit(1);
> +			goto err_out;
>  		}
>  
>  		switch (state) {
> @@ -190,20 +193,20 @@ int main(int argc, char *argv[])
>  			case 'w': break;
>  			default:
>  				fprintf(stderr, "Error: Invalid direction\n");
> -				goto err_out;
> +				goto err_out_with_arg;
>  			}
>  
>  			len = strtoul(arg_ptr, &end, 0);
>  			if (len > 65535) {
>  				fprintf(stderr, "Error: Length invalid\n");
> -				goto err_out;
> +				goto err_out_with_arg;
>  			}
>  
>  			arg_ptr = end;
>  			if (*arg_ptr) {
>  				if (*arg_ptr++ != '@') {
>  					fprintf(stderr, "Error: No '@' after length\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  
>  				/* We skip 10-bit support for now. If we want it, it
> @@ -213,16 +216,16 @@ int main(int argc, char *argv[])
>  
>  				address = parse_i2c_address(arg_ptr);
>  				if (address < 0)
> -					goto err_out;
> +					goto err_out_with_arg;
>  
>  				if (!force && set_slave_addr(file, address, 0))
> -					goto err_out;
> +					goto err_out_with_arg;
>  
>  			} else {
>  				/* Reuse last address if possible */
>  				if (address < 0) {
>  					fprintf(stderr, "Error: No address given\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  			}
>  
> @@ -234,7 +237,7 @@ int main(int argc, char *argv[])
>  				buf = malloc(len);
>  				if (!buf) {
>  					fprintf(stderr, "Error: No memory for buffer\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  				memset(buf, 0, len);
>  				msgs[nmsgs].buf = buf;
> @@ -253,7 +256,7 @@ int main(int argc, char *argv[])
>  			raw_data = strtoul(arg_ptr, &end, 0);
>  			if (raw_data > 255) {
>  				fprintf(stderr, "Error: Data byte invalid\n");
> -				goto err_out;
> +				goto err_out_with_arg;
>  			}
>  			data = raw_data;
>  			len = msgs[nmsgs].len;
> @@ -270,7 +273,7 @@ int main(int argc, char *argv[])
>  				case '=': break;
>  				default:
>  					fprintf(stderr, "Error: Invalid data byte suffix\n");
> -					goto err_out;
> +					goto err_out_with_arg;
>  				}
>  			}
>  
> @@ -283,7 +286,7 @@ int main(int argc, char *argv[])
>  
>  		default:
>  			fprintf(stderr, "Error: Unnkown state in state machine!\n");
> -			goto err_out;
> +			goto err_out_with_arg;

I'd stick to err_out in this case. As this isn't supposed to happen,
you have no idea if printing argv[arg_idx] is relevant or not. And it
is likely to confuse the user.

>  		}
>  
>  		arg_idx++;
> @@ -291,18 +294,18 @@ int main(int argc, char *argv[])
>  
>  	if (state != PARSE_GET_DESC || nmsgs == 0) {
>  		fprintf(stderr, "Error: Incomplete message\n");
> -		exit(1);
> +		goto err_out;
>  	}
>  
>  	if (!yes && !confirm(filename, msgs, nmsgs))
> -		exit(0);
> +		goto out;
>  
>  	rdwr.msgs = msgs;
>  	rdwr.nmsgs = nmsgs;
>  	nmsgs_sent = ioctl(file, I2C_RDWR, &rdwr);
>  	if (nmsgs_sent < 0) {
>  		fprintf(stderr, "Error: Sending messages failed: %s\n", strerror(errno));
> -		exit(errno);
> +		goto err_out;
>  	} else if (nmsgs_sent < nmsgs) {
>  		fprintf(stderr, "Warning: only %d/%d messages were sent\n", nmsgs_sent, nmsgs);
>  	}
> @@ -311,10 +314,17 @@ int main(int argc, char *argv[])
>  
>  	print_msgs(msgs, nmsgs_sent, PRINT_READ_BUF | (verbose ? PRINT_HEADER | PRINT_WRITE_BUF : 0));
>  
> -	/* let Linux free malloced memory on termination */
> +out:

One space before labels please, so as to not break "diff -p".

> +	for (i = 0; i <= nmsgs; i++)
> +		free(msgs[i].buf);

It would be <, not <=.

Another approach is:

	for (; nmsgs >= 0; nmsgs--)
		free(msgs[nmsgs].buf);

which avoids introducing another loop variable.

> +
>  	exit(0);
>  
> -err_out:
> +err_out_with_arg:
>  	fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
> +err_out:
> +	for (i = 0; i <= nmsgs; i++)
> +		free(msgs[i].buf);
> +
>  	exit(1);
>  }

Thanks for doing that.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] i2c-tools: i2ctransfer: clean up allocated resources

[Wolfram Sang]

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


> > +	for (i = 0; i < I2C_RDRW_IOCTL_MAX_MSGS; i++)
> > +		msgs[i].buf = NULL;
> > +
> 
> If you explicitly set "buf = NULL" for zero-length messages in the
> state machine as recommended in my review of the previous patch, this
> is no longer needed.

It is as stated in my previous mail.

We should really play safe here. If we optimize too much, the cleanup
loops become fragile (I tried some options) and one easily misses a
case. Cleaning up is hard, we know that from Linux drivers.

(For me, this is still a good example how letting the OS free memory can
actually prevent bugs. But I give in already ;))


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] i2c-tools: i2ctransfer: clean up allocated resources

[Wolfram Sang]

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


> > +	for (i = 0; i < I2C_RDRW_IOCTL_MAX_MSGS; i++)
> > +		msgs[i].buf = NULL;
> > +
> 
> If you explicitly set "buf = NULL" for zero-length messages in the
> state machine as recommended in my review of the previous patch, this
> is no longer needed.

It is as stated in my previous mail.

We should really play safe here. If we optimize too much, the cleanup
loops become fragile (I tried some options) and one easily misses a
case. Cleaning up is hard, we know that from Linux drivers.

(For me, this is still a good example how letting the OS free memory can
actually prevent bugs. But I give in already ;))


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]