[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] tmp105: Fix I2C protocol bug
From: |
Alex Horn |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] tmp105: Fix I2C protocol bug |
Date: |
Fri, 7 Dec 2012 13:12:43 +0000 |
> [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
>>
>>