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

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

[Alex Horn]

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.horn@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)
         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 ++;
     return 0;
 }
 
-- 
1.7.6.5

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

[Blue Swirl]

On Wed, Dec 5, 2012 at 7:48 PM, Alex Horn <alex.horn@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.horn@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
>
>

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 <blauwirbel@gmail.com> wrote:
> On Wed, Dec 5, 2012 at 7:48 PM, Alex Horn <alex.horn@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.horn@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
>>
>>

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

[Andreas Färber]

Am 05.12.2012 20:48, schrieb Alex Horn:
> 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.horn@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)
>          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 ++;
>      return 0;
>  }
>  

This fix leaves setting the upper and lower limits (commands 2 and 3)
broken. The following works for me:

diff --git a/hw/tmp105.c b/hw/tmp105.c
index 8e8dbd9..512fe0c 100644
--- a/hw/tmp105.c
+++ b/hw/tmp105.c
@@ -152,11 +152,14 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data)
 {
     TMP105State *s = (TMP105State *) i2c;

-    if (!s->len ++)
+    if (s->len == 0) {
         s->pointer = data;
-    else {
-        if (s->len <= 2)
+        s->len++;
+    } else {
+        if (s->len <= 2) {
             s->buf[s->len - 1] = data;
+        }
+        s->len++;
         tmp105_write(s);
     }


The trick is incrementing before the tmp105_write() call but after the
length check and buffer fill.

I've hacked together a real qtest using the n800's omap_i2c that I'll
send out. It sets and reads back the limits, failing in master.

Andreas