From: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
To: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Linux OMAP Mailing List
<linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>,
Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>,
Linux ARM Kernel Mailing List
<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Santosh Shilimkar
<santosh.shilimkar-l0cyMroinI0@public.gmane.org>,
Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
Subject: [PATCH v3] i2c: omap: ensure writes to dev->buf_len are ordered
Date: Wed, 14 Nov 2012 16:22:45 +0200 [thread overview]
Message-ID: <1352902965-25666-1-git-send-email-balbi@ti.com> (raw)
In-Reply-To: <20121114112050.GG5954-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
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 <balbi-l0cyMroinI0@public.gmane.org>
---
Changes since v2:
- add a comment
Changes since v1:
- use barrier() instead of wmb()
drivers/i2c/busses/i2c-omap.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..e88fc26 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -522,6 +522,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
dev->buf = msg->buf;
dev->buf_len = msg->len;
+ /* make sure writes to dev->buf_len are ordered */
+ barrier();
+
omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len);
/* Clear the FIFO Buffers */
@@ -579,7 +582,6 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
*/
timeout = wait_for_completion_timeout(&dev->cmd_complete,
OMAP_I2C_TIMEOUT);
- dev->buf_len = 0;
if (timeout == 0) {
dev_err(dev->dev, "controller timed out\n");
omap_i2c_init(dev);
--
1.8.0
WARNING: multiple messages have this Message-ID (diff)
From: balbi@ti.com (Felipe Balbi)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v3] i2c: omap: ensure writes to dev->buf_len are ordered
Date: Wed, 14 Nov 2012 16:22:45 +0200 [thread overview]
Message-ID: <1352902965-25666-1-git-send-email-balbi@ti.com> (raw)
In-Reply-To: <20121114112050.GG5954@pengutronix.de>
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 <balbi@ti.com>
---
Changes since v2:
- add a comment
Changes since v1:
- use barrier() instead of wmb()
drivers/i2c/busses/i2c-omap.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..e88fc26 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -522,6 +522,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
dev->buf = msg->buf;
dev->buf_len = msg->len;
+ /* make sure writes to dev->buf_len are ordered */
+ barrier();
+
omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len);
/* Clear the FIFO Buffers */
@@ -579,7 +582,6 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
*/
timeout = wait_for_completion_timeout(&dev->cmd_complete,
OMAP_I2C_TIMEOUT);
- dev->buf_len = 0;
if (timeout == 0) {
dev_err(dev->dev, "controller timed out\n");
omap_i2c_init(dev);
--
1.8.0
next prev parent reply other threads:[~2012-11-14 14:22 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-25 9:00 [PATCH] i2c: omap: ensure writes to dev->buf_len are ordered Felipe Balbi
2012-10-25 9:00 ` Felipe Balbi
2012-10-25 16:38 ` Kevin Hilman
2012-10-25 16:38 ` Kevin Hilman
[not found] ` <8739124idt.fsf-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
2012-10-25 18:03 ` Felipe Balbi
2012-10-25 18:03 ` Felipe Balbi
[not found] ` <1351155648-20429-1-git-send-email-balbi-l0cyMroinI0@public.gmane.org>
2012-10-25 9:16 ` Shubhrajyoti Datta
2012-10-25 9:16 ` Shubhrajyoti Datta
2012-10-26 23:01 ` Paul Walmsley
2012-10-26 23:01 ` Paul Walmsley
2012-10-27 10:50 ` Santosh Shilimkar
2012-10-27 10:50 ` Santosh Shilimkar
2012-10-27 15:59 ` Paul Walmsley
2012-10-27 15:59 ` Paul Walmsley
[not found] ` <alpine.DEB.2.00.1210271545390.16409-rwI8Ez+7Ko+d5PgPZx9QOdBPR1lH4CV8@public.gmane.org>
2012-10-28 4:11 ` Santosh Shilimkar
2012-10-28 4:11 ` Santosh Shilimkar
2012-11-01 22:23 ` Wolfram Sang
2012-11-01 22:23 ` Wolfram Sang
[not found] ` <20121101222316.GC22956-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2012-11-02 8:54 ` Felipe Balbi
2012-11-02 8:54 ` Felipe Balbi
[not found] ` <20121102085447.GE17063-S8G//mZuvNWo5Im9Ml3/Zg@public.gmane.org>
2012-11-05 8:04 ` [PATCH v2] " Felipe Balbi
2012-11-05 8:04 ` Felipe Balbi
2012-11-14 11:20 ` Wolfram Sang
2012-11-14 11:20 ` Wolfram Sang
[not found] ` <20121114112050.GG5954-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2012-11-14 14:22 ` Felipe Balbi [this message]
2012-11-14 14:22 ` [PATCH v3] " Felipe Balbi
2012-11-14 16:46 ` Wolfram Sang
2012-11-14 16:46 ` Wolfram Sang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1352902965-25666-1-git-send-email-balbi@ti.com \
--to=balbi-l0cymroini0@public.gmane.org \
--cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
--cc=linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org \
--cc=santosh.shilimkar-l0cyMroinI0@public.gmane.org \
--cc=tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org \
--cc=w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.