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: 1654841 Return-Path: X-Original-To: patchwork-linux-omap@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id B3CBA3FD4F for ; Sat, 27 Oct 2012 10:50:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751850Ab2J0Kud (ORCPT ); Sat, 27 Oct 2012 06:50:33 -0400 Received: from bear.ext.ti.com ([192.94.94.41]:60284 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751736Ab2J0Kuc (ORCPT ); Sat, 27 Oct 2012 06:50:32 -0400 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 CC: Paul Walmsley , Tony Lindgren , , , , Linux OMAP Mailing List , Linux ARM Kernel Mailing List 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: Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.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 --- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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)); }