From patchwork Wed Dec 12 04:34:51 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Andreas_F=C3=A4rber?= X-Patchwork-Id: 205378 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id ACD222C008C for ; Wed, 12 Dec 2012 15:35:15 +1100 (EST) Received: from localhost ([::1]:43813 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tie2J-0002tD-QY for incoming@patchwork.ozlabs.org; Tue, 11 Dec 2012 23:35:11 -0500 Received: from eggs.gnu.org ([208.118.235.92]:37989) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tie28-0002qM-L2 for qemu-devel@nongnu.org; Tue, 11 Dec 2012 23:35:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tie26-0006Df-3w for qemu-devel@nongnu.org; Tue, 11 Dec 2012 23:35:00 -0500 Received: from mout.web.de ([212.227.17.11]:54876) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tie25-0006DS-Pn for qemu-devel@nongnu.org; Tue, 11 Dec 2012 23:34:58 -0500 Received: from envy.site ([84.148.43.42]) by smtp.web.de (mrweb102) with ESMTPSA (Nemesis) id 0LtWsC-1T1zaq3hx6-010nbf; Wed, 12 Dec 2012 05:34:53 +0100 Message-ID: <50C8096B.60202@web.de> Date: Wed, 12 Dec 2012 05:34:51 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: Alex Horn References: <1354736883-22417-1-git-send-email-alex.horn@cs.ox.ac.uk> In-Reply-To: <1354736883-22417-1-git-send-email-alex.horn@cs.ox.ac.uk> X-Provags-ID: V02:K0:5uRhRKDk7eUzm/ohehsIvJ4yif491/dO8K2lAq/pLR/ 1Y7sdhyNzagBUAavVR9s7g3JIxCHopLuWIuArJ5djEPLg5E1fl oKMM5OOpCG2/HCMMbJ/KqdJ2xWT9nM2/u4DtAHFSw9g2uo2Eh1 ae7M33njMnxHT3EAJK8Pf3qimodko5WLJ0JuuueXcKWO37ckKP PN9D10jTsqy4V/J2ewv8g== X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4.x-2.6.x [generic] X-Received-From: 212.227.17.11 Cc: Blue Swirl , peter.maydell@linaro.org, qemu-devel@nongnu.org, anthony@codemonkey.ws Subject: Re: [Qemu-devel] [PATCH 1/1] tmp105: Fix I2C protocol bug X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 > --- > 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: 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 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); }