Re: [Qemu-devel] [PATCH 1/1] tmp105: Fix I2C protocol bug

[Alex Horn]

> [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 <address@hidden> wrote:
> On Wed, Dec 5, 2012 at 7:48 PM, Alex Horn <address@hidden> 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 <address@hidden>
>> ---
>>  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
>>
>>