> [T]he change is entirely correct since the 'else' clause currently seems > to take the increment into account: > if (s->len <= 2) > s->buf[s->len - 1] = data; > tmp105_write(s); > > Shouldn't the '- 1' in the middle line be removed?
Several clarifications follow. The bug seems to occur because the else clause does not correctly account for the increment (i.e. !s->len ++). If this increment operation in the conditional statement should remain there, the assignment "s->buf[s->len - 1] = data" would appear to have to be changed to "s->buf[s->len - 2] = data". However, this offset adjustment would only shadow the root cause of the bug. To further clarify this bug, I have also created a standalone unit test: https://github.com/ahorn/benchmarks/blob/master/qemu-hw/tmp105/tmp105-test.c#L130 This test case would fail in the unmodified tmp105.c hardware model. That is, you can produce the runtime assertion failure by reverting the fix I have implemented in tmp105_tx() in the file https://github.com/ahorn/benchmarks/blob/master/qemu-hw/tmp105/tmp105.c#L140 There is also one correction about the bug description itself (see below). >> 8) Start transfer with I2C_START_RECV >> 9) Receive high-order byte of temperature >> ... >> Thus, when we read the higher-order byte in step (9), it is zero! This description should read instead: [as before] ... 8) Start transfer with I2C_START_RECV 9) Receive high-order byte of temperature 10) Receive low-order byte of temperature Thus, when we read the low-order byte in step (10), it is zero. The rest of the argument remains the same. I hope this correction and the unit test help in clarifying the source of the error. I would greatly appreciate your thoughts on this. With best regards, Alex On 6 December 2012 21:21, Blue Swirl <blauwir...@gmail.com> wrote: > On Wed, Dec 5, 2012 at 7:48 PM, Alex Horn <alex.h...@cs.ox.ac.uk> wrote: >> The private buffer length field must only be incremented after the I2C >> frame has been transmitted. >> >> To expose this bug, assume the temperature in the TMP105 hardware model >> is +0.125 C (e.g. snow slush). Note that eleven bit precision is required >> to read this value; otherwise the reading is equal to zero centigrade (ice). >> >> Continue by considering the following I2C protocol steps: >> >> 1) Start transfer with I2C_START_SEND >> 2) Send byte 0x01 (i.e. configuration register) >> 3) Send byte 0x40 (i.e. eleven bit precision) >> 4) End transfer with I2C_FINISH >> >> 5) Start transfer with I2C_START_SEND >> 6) Send byte 0x00 (i.e. temperature register) >> 7) End transfer I2C_FINISH >> >> 8) Start transfer with I2C_START_RECV >> 9) Receive high-order byte of temperature >> ... >> >> In step (1), the function tmp105_tx() is called. By the conditional >> check !s->len and the side effect with ++, s->len is equal to 1 when >> step (2) begins. Thus, 0x40 is written to s->buf[1] in step (3). >> By definition of tmp105_write(), s->config is set to zero in step (3). >> Thus, when we read the higher-order byte in step (9), it is zero! >> >> In other words, the TMP105 hardware model allows us to measure 0 C (ice) >> even with eleven bit precision when, in fact, it should be 0.125 C (slush)! >> >> Signed-off-by: Alex Horn <alex.h...@cs.ox.ac.uk> >> --- >> hw/tmp105.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/hw/tmp105.c b/hw/tmp105.c >> index 8e8dbd9..5f41a3f 100644 >> --- a/hw/tmp105.c >> +++ b/hw/tmp105.c >> @@ -152,7 +152,7 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data) >> { >> TMP105State *s = (TMP105State *) i2c; >> >> - if (!s->len ++) >> + if (s->len == 0) > > Please fix coding style (add braces) while touching this line. > > QEMU code is not yet consistent with our CODING_STYLE, but changes > should make progress towards that. > >> s->pointer = data; >> else { >> if (s->len <= 2) >> @@ -160,6 +160,7 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data) >> tmp105_write(s); >> } >> >> + s->len ++; > > Please remove the space between s->len and ++. However, I don't think > the change is entirely correct since the 'else' clause currently seems > to take the increment into account: > if (s->len <= 2) > s->buf[s->len - 1] = data; > tmp105_write(s); > > Shouldn't the '- 1' in the middle line be removed? > >> return 0; >> } >> >> -- >> 1.7.6.5 >> >>
