[PATCH] i2cset: Fix short writes with mask

[PATCH] i2cset: Fix short writes with mask

[Jean Delvare]

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(-)

--- 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);


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2cset: Fix short writes with mask

[Wolfram Sang]

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

On Thu, Sep 03, 2020 at 11:00:54AM +0200, Jean Delvare wrote:
> 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>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Passed my test as well. One question:

> @@ -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");

Does this really change something or is it a refactoring leftover?


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

Re: [PATCH] i2cset: Fix short writes with mask

[Jean Delvare]

On Tue, 8 Sep 2020 08:51:11 +0200, Wolfram Sang wrote:
> On Thu, Sep 03, 2020 at 11:00:54AM +0200, Jean Delvare wrote:
> > 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>  
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Passed my test as well. One question:

Thank you very much for testing! I'll commit that now.

> > @@ -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");  
> 
> Does this really change something or is it a refactoring leftover?

It does change something. Before the changes, size could only have two
values here, I2C_SMBUS_BYTE_DATA and I2C_SMBUS_WORD_DATA, so the result
would indeed be the same. But after the changes, size can also have
value I2C_SMBUS_BYTE, for which we want to display "mode byte", not
"mode word". Which is why we test explicitly for I2C_SMBUS_WORD_DATA,
else default to "byte", rather than the other way around.

Note that this also aligns nicely with the rest of the code in this
file, which does the same in various places.

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
;-)

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2cset: Fix short writes with mask

[Wolfram Sang]

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

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


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

Re: [PATCH] i2cset: Fix short writes with mask

[Jean Delvare]

On Tue, 8 Sep 2020 17:40:29 +0200, Wolfram Sang wrote:
> 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?

Well, the code itself isn't tricky, and it's only more of the same,
nothing new. The surprise was only for the patch reviewers, and now
this is cleared, and I committed the patch already. So I don't think
there's anything left to do.

Thanks,
-- 
Jean Delvare
SUSE L3 Support