From patchwork Sat Oct 27 10:50:01 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Santosh Shilimkar X-Patchwork-Id: 1654851 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork2.kernel.org (Postfix) with ESMTP id 152E1DFAC4 for ; Sat, 27 Oct 2012 10:52:14 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TS3yC-0007L0-AP; Sat, 27 Oct 2012 10:50:24 +0000 Received: from bear.ext.ti.com ([192.94.94.41]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TS3y9-0007Km-A3 for linux-arm-kernel@lists.infradead.org; Sat, 27 Oct 2012 10:50:22 +0000 Received: from dlelxv30.itg.ti.com ([172.17.2.17]) by bear.ext.ti.com (8.13.7/8.13.7) with ESMTP id q9RAo79f009285; Sat, 27 Oct 2012 05:50:07 -0500 Received: from DLEE74.ent.ti.com (dlee74.ent.ti.com [157.170.170.8]) by dlelxv30.itg.ti.com (8.13.8/8.13.8) with ESMTP id q9RAo7rW025875; Sat, 27 Oct 2012 05:50:07 -0500 Received: from dlelxv22.itg.ti.com (172.17.1.197) by DLEE74.ent.ti.com (157.170.170.8) with Microsoft SMTP Server id 14.1.323.3; Sat, 27 Oct 2012 05:50:06 -0500 Received: from [172.24.73.131] (h73-131.vpn.ti.com [172.24.73.131]) by dlelxv22.itg.ti.com (8.13.8/8.13.8) with ESMTP id q9RAo24f008018; Sat, 27 Oct 2012 05:50:03 -0500 Message-ID: <508BBC59.60504@ti.com> Date: Sat, 27 Oct 2012 16:20:01 +0530 From: Santosh Shilimkar User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120912 Thunderbird/15.0.1 MIME-Version: 1.0 To: Felipe Balbi Subject: Re: [PATCH] i2c: omap: ensure writes to dev->buf_len are ordered References: <1351155648-20429-1-git-send-email-balbi@ti.com> In-Reply-To: X-Spam-Note: CRM114 invocation failed X-Spam-Score: -7.6 (-------) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-7.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [192.94.94.41 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -0.7 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Paul Walmsley , Tony Lindgren , w.sang@pengutronix.de, linux-i2c@vger.kernel.org, ben-linux@fluff.org, Linux OMAP Mailing List , Linux ARM Kernel Mailing List X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On Saturday 27 October 2012 04:31 AM, Paul Walmsley wrote: > Hi Felipe > > just two quick comments > > On Thu, 25 Oct 2012, Felipe Balbi wrote: > >> if we allow compiler reorder our writes, we could >> fall into a situation where dev->buf_len is reset >> for no apparent reason. >> >> This bug was found with a simple script which would >> transfer data to an i2c client from 1 to 1024 bytes >> (a simple for loop), when we got to transfer sizes >> bigger than the fifo size, dev->buf_len was reset >> to zero before we had an oportunity to handle XDR >> Interrupt. Because dev->buf_len was zero, we entered >> omap_i2c_transmit_data() to transfer zero bytes, >> which would mean we would just silently exit >> omap_i2c_transmit_data() without actually writing >> anything to DATA register. That would cause XDR >> IRQ to trigger forever and we would never transfer >> the remaining bytes. >> >> After adding the memory barrier, we also drop resetting >> dev->buf_len to zero in omap_i2c_xfer_msg() because >> both omap_i2c_transmit_data() and omap_i2c_receive_data() >> will act until dev->buf_len reaches zero, rendering the >> other write in omap_i2c_xfer_msg() redundant. >> >> This patch has been tested with pandaboard for a few >> iterations of the script mentioned above. >> >> Signed-off-by: Felipe Balbi >> --- >> >> This bug has been there forever, but it's quite annoying. >> I think it deserves being pushed upstream during this -rc >> cycle, but if Wolfram decides to wait until v3.8, I don't >> mind. >> >> drivers/i2c/busses/i2c-omap.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index db31eae..1ec4e6e 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -521,6 +521,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, >> /* REVISIT: Could the STB bit of I2C_CON be used with probing? */ >> dev->buf = msg->buf; >> dev->buf_len = msg->len; >> + wmb(); >> >> omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len); >> > > Would suggest moving the wmb() immediately before the point at which the > interrupt can occur. Looks to me that's when the OMAP_I2C_CON_REG write > occurs. > > Also would suggest adding a comment to clarify what the wmb() is intended > to do. Maybe something like 'Prevent the compiler from moving earlier > changes to dev->buf and dev->buf_len after the write to CON_REG. This > write enables interrupts and those variables are used in the interrupt > handler'. > Another alternative, which I will recommend to just make use of the read*/wrire* instead __raw versions. The barriers are taken care already and driver point of view, it is transparent. --> Patch might be damaged because of copy paste. Regards Santosh diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index db31eae..0cd6365 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -265,13 +265,13 @@ static const u8 reg_map_ip_v2[] = { static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev, int reg, u16 val) { - __raw_writew(val, i2c_dev->base + + writew(val, i2c_dev->base + (i2c_dev->regs[reg] << i2c_dev->reg_shift)); } static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg) { - return __raw_readw(i2c_dev->base + + return readw(i2c_dev->base + (i2c_dev->regs[reg] << i2c_dev->reg_shift)); }