diff mbox series

i2cset: Fix short writes with mask

Message ID 20200903110054.52a3a69f@endymion
State New
Headers show
Series i2cset: Fix short writes with mask | expand

Commit Message

Jean Delvare Sept. 3, 2020, 9 a.m. UTC 
Short writes used "daddress" for the value, but the masking code did
not expect that, and instead applied the mask to a variable that was
never used.

So change short writes to use "value" for the value, as all other
commands do. Adjust all code paths accordingly.

Reported by David Jedynak.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
I was finally able to give this some (convoluted) testing using
i2c-stub, so it's about time to get this fix merged.

 tools/i2cset.c |   25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

Comments

Wolfram Sang Sept. 8, 2020, 3:40 p.m. UTC | #1 
Hi Jean,

> Funny story, while I only posted this last week, I wrote the fix
> several months ago, so last week I actually got to review my own code
> with fresh eyes, and when I stumbled upon that specific change, first
> thing that came to my mind was "this is a useless change, why did I do
> that ?" Then I scrolled up, checked the other changes in that function,
> noticed the removed "else" and said OK, I'm not that stupid after all
> ;-)

:) Now that there are two of us, maybe this justifies a short comment
explaining it?

Happy hacking,

   Wolfram
diff mbox series

Patch

--- i2c-tools.orig/tools/i2cset.c	2020-08-03 16:27:42.557814067 +0200
+++ i2c-tools/tools/i2cset.c	2020-09-03 10:32:40.842931585 +0200
@@ -125,11 +125,11 @@  static int confirm(const char *filename,
 	}
 
 	fprintf(stderr, "I will write to device file %s, chip address "
-		"0x%02x, data address\n0x%02x, ", filename, address, daddress);
-	if (size == I2C_SMBUS_BYTE)
-		fprintf(stderr, "no data.\n");
-	else if (size == I2C_SMBUS_BLOCK_DATA ||
-		 size == I2C_SMBUS_I2C_BLOCK_DATA) {
+		"0x%02x,\n", filename, address);
+	if (size != I2C_SMBUS_BYTE)
+		fprintf(stderr, "data address 0x%02x, ", daddress);
+	if (size == I2C_SMBUS_BLOCK_DATA ||
+	    size == I2C_SMBUS_I2C_BLOCK_DATA) {
 		int i;
 
 		fprintf(stderr, "data");
@@ -140,7 +140,7 @@  static int confirm(const char *filename,
 	} else
 		fprintf(stderr, "data 0x%02x%s, mode %s.\n", value,
 			vmask ? " (masked)" : "",
-			size == I2C_SMBUS_BYTE_DATA ? "byte" : "word");
+			size == I2C_SMBUS_WORD_DATA ? "word" : "byte");
 	if (pec)
 		fprintf(stderr, "PEC checking enabled.\n");
 
@@ -264,6 +264,10 @@  int main(int argc, char *argv[])
 
 	/* read values from command line */
 	switch (size) {
+	case I2C_SMBUS_BYTE:
+		/* short write: data address was not really data address */
+		value = daddress;
+		break;
 	case I2C_SMBUS_BYTE_DATA:
 	case I2C_SMBUS_WORD_DATA:
 		value = strtol(argv[flags+4], &end, 0);
@@ -344,12 +348,10 @@  int main(int argc, char *argv[])
 
 		if (!yes) {
 			fprintf(stderr, "Old value 0x%0*x, write mask "
-				"0x%0*x: Will write 0x%0*x to register "
-				"0x%02x\n",
+				"0x%0*x, will write 0x%0*x\n",
 				size == I2C_SMBUS_WORD_DATA ? 4 : 2, oldvalue,
 				size == I2C_SMBUS_WORD_DATA ? 4 : 2, vmask,
-				size == I2C_SMBUS_WORD_DATA ? 4 : 2, value,
-				daddress);
+				size == I2C_SMBUS_WORD_DATA ? 4 : 2, value);
 
 			fprintf(stderr, "Continue? [Y/n] ");
 			fflush(stderr);
@@ -369,7 +371,7 @@  int main(int argc, char *argv[])
 
 	switch (size) {
 	case I2C_SMBUS_BYTE:
-		res = i2c_smbus_write_byte(file, daddress);
+		res = i2c_smbus_write_byte(file, value);
 		break;
 	case I2C_SMBUS_WORD_DATA:
 		res = i2c_smbus_write_word_data(file, daddress, value);
@@ -407,7 +409,6 @@  int main(int argc, char *argv[])
 	switch (size) {
 	case I2C_SMBUS_BYTE:
 		res = i2c_smbus_read_byte(file);
-		value = daddress;
 		break;
 	case I2C_SMBUS_WORD_DATA:
 		res = i2c_smbus_read_word_data(file, daddress);