git: 5ab41ff8e92d - main - More refactoring:

Poul-Henning Kamp phk at FreeBSD.org
Wed May 12 21:39:30 UTC 2021


The branch main has been updated by phk:

URL: https://cgit.FreeBSD.org/src/commit/?id=5ab41ff8e92da548acf06b50384df538737aa8ee

commit 5ab41ff8e92da548acf06b50384df538737aa8ee
Author:     Poul-Henning Kamp <phk at FreeBSD.org>
AuthorDate: 2021-05-12 21:34:58 +0000
Commit:     Poul-Henning Kamp <phk at FreeBSD.org>
CommitDate: 2021-05-12 21:39:19 +0000

    More refactoring:
    
    Extract the '-a' mode into a separate function, and simplify the
    hexdumping logic.
    
    Dont call usage() without telling why.
---
 usr.sbin/i2c/i2c.8 |  36 +++--------
 usr.sbin/i2c/i2c.c | 181 +++++++++++++++++++++++++----------------------------
 2 files changed, 97 insertions(+), 120 deletions(-)

diff --git a/usr.sbin/i2c/i2c.8 b/usr.sbin/i2c/i2c.8
index 1c5174e63f32..352b13157968 100644
--- a/usr.sbin/i2c/i2c.8
+++ b/usr.sbin/i2c/i2c.8
@@ -64,12 +64,12 @@ The options are as follows:
 7-bit address on the I2C device to operate on (hex).
 .It Fl b
 binary mode - when performing a read operation, the data read from the device
-is output in binary format on stdout; when doing a write, the binary data to
-be written to the device is read from stdin.
+is output in binary format on stdout.
 .It Fl c Ar count
-number of bytes to transfer (dec).
+number of bytes to transfer (decimal).
 .It Fl d Ar r|w
 transfer direction: r - read, w - write.
+Data to be written is read from stdin as binary bytes.
 .It Fl f Ar device
 I2C bus to use (default is /dev/iic0).
 .It Fl m Ar tr|ss|rs|no
@@ -113,7 +113,7 @@ scan the bus for devices.
 .It Fl v
 be verbose.
 .It Fl w Ar 0|8|16|16LE|16BE
-device addressing width (in bits).
+device offset width (in bits).
 This is used to determine how to pass
 .Ar offset
 specified with
@@ -122,28 +122,6 @@ to the slave.
 Zero means that the offset is ignored and not passed to the slave at all.
 The endianess defaults to little-endian.
 .El
-.Sh WARNINGS
-Great care must be taken when manipulating slave I2C devices with the
-.Nm
-utility.
-Often times important configuration data for the system is kept in non-volatile
-but write enabled memories located on the I2C bus, for example Ethernet hardware
-addresses, RAM module parameters (SPD), processor reset configuration word etc.
-.Pp
-It is very easy to render the whole system unusable when such configuration
-data is deleted or altered, so use the
-.Dq -d w
-(write) command only if you know exactly what you are doing.
-.Pp
-Also avoid ungraceful interrupting of an ongoing transaction on the I2C bus,
-as it can lead to potentially dangerous effects.
-Consider the following scenario: when the host CPU is reset (for whatever reason)
-in the middle of a started I2C transaction, the I2C slave device could be left
-in write mode waiting for data or offset to arrive.
-When the CPU reinitializes itself and talks to this I2C slave device again,
-the commands and other control info it sends are treated by the slave device
-as data or offset it was waiting for, and there's great potential for
-corruption if such a write is performed.
 .Sh EXAMPLES
 .Bl -bullet
 .It
@@ -177,9 +155,15 @@ Reset the controller:
 .Pp
 i2c -f /dev/iic1 -r
 .El
+.Sh WARNING
+Many systems store critical low-level information in I2C memories, and
+may contain other I2C devices, such as temperature or voltage sensors.
+Reading these can disturb the firmware's operation and writing to them
+can "brick" the hardware.
 .Sh SEE ALSO
 .Xr iic 4 ,
 .Xr iicbus 4
+.Xr smbus 4
 .Sh HISTORY
 The
 .Nm
diff --git a/usr.sbin/i2c/i2c.c b/usr.sbin/i2c/i2c.c
index 64233366a0a8..c862666fc9a7 100644
--- a/usr.sbin/i2c/i2c.c
+++ b/usr.sbin/i2c/i2c.c
@@ -73,9 +73,11 @@ struct skip_range {
 };
 
 __dead2 static void
-usage(void)
+usage(const char *msg)
 {
 
+	if (msg != NULL)
+		fprintf(stderr, "%s\n", msg);
 	fprintf(stderr, "usage: %s -a addr [-f device] [-d [r|w]] [-o offset] "
 	    "[-w [0|8|16]] [-c count] [-m [tr|ss|rs|no]] [-b] [-v]\n",
 	    getprogname());
@@ -561,19 +563,71 @@ i2c_rdwr_transfer(int fd, struct options i2c_opt, char *i2c_buf)
 	return (0);
 }
 
+static int
+access_bus(int fd, struct options i2c_opt)
+{
+	char *i2c_buf;
+	int error, chunk_size = 16, i, ch;
+
+	i2c_buf = malloc(i2c_opt.count);
+	if (i2c_buf == NULL)
+		err(1, "data malloc");
+
+	/*
+	 * For a write, read the data to be written to the chip from stdin.
+	 */
+	if (i2c_opt.dir == 'w') {
+		if (i2c_opt.verbose && !i2c_opt.binary)
+			fprintf(stderr, "Enter %d bytes of data: ",
+			    i2c_opt.count);
+		for (i = 0; i < i2c_opt.count; i++) {
+			ch = getchar();
+			if (ch == EOF) {
+				free(i2c_buf);
+				err(1, "not enough data, exiting\n");
+			}
+			i2c_buf[i] = ch;
+		}
+	}
+
+	if (i2c_opt.mode == I2C_MODE_TRANSFER)
+		error = i2c_rdwr_transfer(fd, i2c_opt, i2c_buf);
+	else if (i2c_opt.dir == 'w')
+		error = i2c_write(fd, i2c_opt, i2c_buf);
+	else
+		error = i2c_read(fd, i2c_opt, i2c_buf);
+
+	if (error == 0) {
+		if (i2c_opt.dir == 'r' && i2c_opt.binary) {
+			(void)fwrite(i2c_buf, 1, i2c_opt.count, stdout);
+		} else if (i2c_opt.verbose || i2c_opt.dir == 'r') {
+			if (i2c_opt.verbose)
+				fprintf(stderr, "\nData %s (hex):\n",
+				    i2c_opt.dir == 'r' ?  "read" : "written");
+
+			for (i = 0; i < i2c_opt.count; i++) {
+				fprintf (stderr, "%02hhx ", i2c_buf[i]);
+				if ((i % chunk_size) == chunk_size - 1)
+					fprintf(stderr, "\n");
+			}
+			if ((i % chunk_size) != 0)
+				fprintf(stderr, "\n");
+		}
+	}
+
+	free(i2c_buf);
+	return (error);
+}
+
 int
 main(int argc, char** argv)
 {
 	struct options i2c_opt;
-	char *skip_addr = NULL, *i2c_buf;
+	char *skip_addr = NULL;
 	const char *dev, *err_msg;
-	int fd, error, chunk_size, i, j, ch;
+	int fd, error, ch;
 
 	errno = 0;
-	error = 0;
-
-	/* Line-break the output every chunk_size bytes */
-	chunk_size = 16;
 
 	dev = I2C_DEV;
 
@@ -595,9 +649,8 @@ main(int argc, char** argv)
 		case 'a':
 			i2c_opt.addr = (strtoul(optarg, 0, 16) << 1);
 			if (i2c_opt.addr == 0 && errno == EINVAL)
-				i2c_opt.addr_set = 0;
-			else
-				i2c_opt.addr_set = 1;
+				usage("Bad -a argument (hex)");
+			i2c_opt.addr_set = 1;
 			break;
 		case 'f':
 			dev = optarg;
@@ -608,13 +661,15 @@ main(int argc, char** argv)
 		case 'o':
 			i2c_opt.off = strtoul(optarg, 0, 16);
 			if (i2c_opt.off == 0 && errno == EINVAL)
-				error = 1;
+				usage("Bad -o argument (hex)");
 			break;
 		case 'w':
 			i2c_opt.width = optarg;
 			break;
 		case 'c':
-			i2c_opt.count = atoi(optarg);
+			i2c_opt.count = (strtoul(optarg, 0, 10));
+			if (i2c_opt.count == 0 && errno == EINVAL)
+				usage("Bad -c argument (decimal)");
 			break;
 		case 'm':
 			if (!strcmp(optarg, "no"))
@@ -626,7 +681,7 @@ main(int argc, char** argv)
 			else if (!strcmp(optarg, "tr"))
 				i2c_opt.mode = I2C_MODE_TRANSFER;
 			else
-				usage();
+				usage("Bad -m argument ([no|ss|rs|tr])");
 			break;
 		case 'n':
 			i2c_opt.skip = 1;
@@ -645,16 +700,16 @@ main(int argc, char** argv)
 			i2c_opt.reset = 1;
 			break;
 		case 'h':
+			usage("Help:");
+			break;
 		default:
-			usage();
+			usage("Bad argument");
 		}
 	}
 	argc -= optind;
 	argv += optind;
-	if (argc > 0) {
-		fprintf(stderr, "Too many arguments\n");
-		usage();
-	}
+	if (argc > 0)
+		usage("Too many arguments");
 
 	/* Set default mode if option -m is not specified */
 	if (i2c_opt.mode == I2C_MODE_NOTSET) {
@@ -664,24 +719,20 @@ main(int argc, char** argv)
 			i2c_opt.mode = I2C_MODE_NONE;
 	}
 
-	if (i2c_opt.addr_set) {
-		err_msg = encode_offset(i2c_opt.width, i2c_opt.off,
-		    i2c_opt.off_buf, &i2c_opt.off_len);
-		if (err_msg != NULL) {
-			fprintf(stderr, "%s", err_msg);
-			exit(EX_USAGE);
-		}
+	err_msg = encode_offset(i2c_opt.width, i2c_opt.off,
+	    i2c_opt.off_buf, &i2c_opt.off_len);
+	if (err_msg != NULL) {
+		fprintf(stderr, "%s", err_msg);
+		exit(EX_USAGE);
 	}
 
 	/* Basic sanity check of command line arguments */
 	if (i2c_opt.scan) {
 		if (i2c_opt.addr_set)
-			usage();
+			usage("-s and -a are incompatible");
 	} else if (i2c_opt.reset) {
 		if (i2c_opt.addr_set)
-			usage();
-	} else if (error) {
-		usage();
+			usage("-r and -a are incompatible");
 	}
 
 	if (i2c_opt.verbose)
@@ -698,71 +749,13 @@ main(int argc, char** argv)
 	}
 
 	if (i2c_opt.scan)
-		exit(scan_bus(dev, fd, i2c_opt.skip, skip_addr));
-
-	if (i2c_opt.reset)
-		exit(reset_bus(dev, fd));
-
-	i2c_buf = malloc(i2c_opt.count);
-	if (i2c_buf == NULL)
-		err(1, "data malloc");
-
-	/*
-	 * For a write, read the data to be written to the chip from stdin.
-	 */
-	if (i2c_opt.dir == 'w') {
-		if (i2c_opt.verbose && !i2c_opt.binary)
-			fprintf(stderr, "Enter %d bytes of data: ",
-			    i2c_opt.count);
-		for (i = 0; i < i2c_opt.count; i++) {
-			ch = getchar();
-			if (ch == EOF) {
-				free(i2c_buf);
-				err(1, "not enough data, exiting\n");
-			}
-			i2c_buf[i] = ch;
-		}
-	}
-
-	if (i2c_opt.mode == I2C_MODE_TRANSFER)
-		error = i2c_rdwr_transfer(fd, i2c_opt, i2c_buf);
-	else if (i2c_opt.dir == 'w')
-		error = i2c_write(fd, i2c_opt, i2c_buf);
+		error = scan_bus(dev, fd, i2c_opt.skip, skip_addr);
+	else if (i2c_opt.reset)
+		error = reset_bus(dev, fd);
 	else
-		error = i2c_read(fd, i2c_opt, i2c_buf);
-
-	if (error != 0) {
-		free(i2c_buf);
-		return (1);
-	}
-
-	error = close(fd);
-	assert(error == 0);
+		error = access_bus(fd, i2c_opt);
 
-	if (i2c_opt.verbose)
-		fprintf(stderr, "\nData %s (hex):\n", i2c_opt.dir == 'r' ?
-		    "read" : "written");
-
-	i = 0;
-	j = 0;
-	while (i < i2c_opt.count) {
-		if (i2c_opt.verbose || (i2c_opt.dir == 'r' &&
-		    !i2c_opt.binary))
-			fprintf (stderr, "%02hhx ", i2c_buf[i++]);
-
-		if (i2c_opt.dir == 'r' && i2c_opt.binary) {
-			fprintf(stdout, "%c", i2c_buf[j++]);
-			if(!i2c_opt.verbose)
-				i++;
-		}
-		if (!i2c_opt.verbose && (i2c_opt.dir == 'w'))
-			break;
-		if ((i % chunk_size) == 0)
-			fprintf(stderr, "\n");
-	}
-	if ((i % chunk_size) != 0)
-		fprintf(stderr, "\n");
-
-	free(i2c_buf);
-	return (0);
+	ch = close(fd);
+	assert(ch == 0);
+	return (error);
 }


More information about the dev-commits-src-main mailing list