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 <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)
>          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