Hi David,
Thanks for reporting and sorry for the delay.
On Thu, 27 Dec 2018 00:29:59 -0600, David Jedynak wrote:
> There's a bug in i2cset when using mask and a simple command
> (I2C_SMBUS_BYTE). I'm currently using this with a single byte 8-bit
> I/O expander device (PCF8574). but this is not device dependent.
>
> With and without a mask option, the byte to be written (argv[flags +
> 3]) is stored in "daddress" for later use.
>
> When argc == flags + 4, it's an implicit "c" and I2C_SMBUS_BYTE mode is set.
True, although an explicit "c" would make no difference in this case.
> In the case of I2C_SMBUS_BYTE, the write is perfomed with
> "i2c_smbus_write_byte(file, daddress)", which works fine as long as a
> mask was not set.
>
> If a mask is set (vmask), then the new value to write is constructed
> with the old value and the mask, and stored in "value" to be written
> back to the part, but this doesn't work for I2C_SMBUS_BYTE case, as it
> writes a single byte using "daddress" instead of "value" (which had
> the mask applied).
>
> The problem is that if the user is providing a mask and just the bits
> to be set or cleared according to the mask, then the only those bits
> will be set / cleared as the data on the command line, which is stored
> in daddress, and then written without any masking (e.g. the vmask
> routine to create "value").
That's right, your analysis is correct, this is a bug. I will take the
blame as clearly this was never working. The faulty commit is:
commit 0c270e09de8fd686234812f4fb73c6acfd34315f
Author: Jean Delvare
Date: Thu Nov 20 17:54:10 2008 +0000
Allow masking on short writes.
The cause is indeed that "short writes" have a different structure
compared to all other writes (it does not assume that the target device
has multiple registers, therefore the "data address" is omitted).
Originally it was not much of a problem, but as more features were
added to i2cset, I lost track of this difference and this bug was
introduced.
> The readback check succeeds because in the I2C_SMBUS_BYTE case, right
> after the read (to get "res"), it sets the vmask constructed "value"
> equal to the original "daddress" (which was just the bits to set
> without mask), and the "res" back from the device will be equal to
> that, never compared to the "value" that correctly constructed with
> the mask since it was just overwritten with the original "daddress"
That second bug is probably the reason why the first bug went unnoticed
for 10 years.
> For now, I'm doing the reads / masks / writes via the calling script,
> as a workaround, but with 4.x in works, looks like a good opportunity
> to fix.
Well, every moment is a good time to fix bugs anyway ;-)
Can you please test, and maybe review, the following patch? I tested
with i2c-stub for the I2C_SMBUS_BYTE_DATA case, but the emulated device
is not suitable for I2C_SMBUS_BYTE testing, and I don't have any
physical device supporting it at hand.
From: Jean Delvare <jdelvare@xxxxxxx>
Subject: i2cset: Fix short writes with mask
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@xxxxxxx>
---
tools/i2cset.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
--- i2c-tools.orig/tools/i2cset.c 2019-01-09 15:09:31.138576857 +0100
+++ i2c-tools/tools/i2cset.c 2019-01-09 15:35:42.678553819 +0100
@@ -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 realy 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);
Thanks,
--
Jean Delvare
SUSE L3 Support
