i2c: omap: ensure writes to dev->buf_len are ordered

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 <[email protected]>

---

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(-)

drivers/i2c/busses/i2c-omap.c +1 -2

index db31eae..1ec4e6e 100644
@@ -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);
 

@@ -579,7 +580,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

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

---

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(-)

drivers/i2c/busses/i2c-omap.c +1 -1

index db31eae..1ec4e6e 100644
@@ -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);
 

@@ -579,7 +580,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);

+Paul

Quoted 3 lines · Felipe Balbi

if we allow compiler reorder our writes, we could
fall into a situation where dev->buf_len is reset
for no apparent reason.

Any chance this would help with the bug Paul found with I2C timeouts on beagle userspace startup?

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 <[email protected]>

---

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(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index db31eae..1ec4e6e 100644

Hi Felipe

just two quick comments

Quoted 42 lines · Felipe Balbi

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 <[email protected]>

---

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(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index db31eae..1ec4e6e 100644

drivers/i2c/busses/i2c-omap.c static int omap_i2c_xfer_msg(struct i2c_adapter *adap, · line 527

Quoted 7 lines

drivers/i2c/busses/i2c-omap.c +1 -0

@@ -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.

Quoted 3 lines · Santosh Shilimkar

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.

Those barriers will disappear if CONFIG_ARM_DMA_MEM_BUFFERABLE is set to N, so that's probably not the right thing to do in this case. The barrier here isn't DMA-related, it's needed due to the design of the driver.

In fact the wmb() is probably overkill, since only a compiler reordering barrier is needed. It can probably just be barrier().

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 <[email protected]>

---

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.

Quoted 29 lines · Felipe Balbi

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 <[email protected]>

---

Quoted 4 lines

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.

I would add this into 3.7, but what about the comments suggesting to use barrier()?

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 <[email protected]>

---

Changes since v1:
- use barrier() instead of wmb()

Note: this version was compile-tested only

 drivers/i2c/busses/i2c-omap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index db31eae..ba03bec 100644

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 <[email protected]>