| Message ID | 1352902965-25666-1-git-send-email-balbi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 14, 2012 at 04:22:45PM +0200, 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 <balbi@ti.com> Applied to for-current, thanks!
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);
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(-)