| Message ID | 20200903110054.52a3a69f@endymion |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | i2cset: Fix short writes with mask | expand |
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?
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 ;-)
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
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,
--- 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);
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(-)