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

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

[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 <balbi@ti.com>
---

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);
 
@@ -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

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

[Shubhrajyoti Datta]

On Thu, Oct 25, 2012 at 2:30 PM, Felipe Balbi <balbi@ti.com> 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.

looks good to me
Acked-by: Shubhrajyoti D <shubhrajyoti@ti.com>

>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>
> 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);
>
> @@ -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

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

[Kevin Hilman]

+Paul

Felipe Balbi <balbi@ti.com> writes:

> 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?

Kevin

> 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>
> ---
>
> 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);
>  
> @@ -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);

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

[Felipe Balbi]

Hi,

On Thu, Oct 25, 2012 at 09:38:06AM -0700, Kevin Hilman wrote:
> +Paul
> 
> Felipe Balbi <balbi@ti.com> writes:
> 
> > 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?

I replied to the original thread asking the same ;-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121025/8550556d/attachment-0001.sig>

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

[Paul Walmsley]

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 <balbi@ti.com>
> ---
> 
> 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'.

checkpatch is supposed to flag uncommented barriers, but maybe that's only 
with --strict.

# check for memory barriers without a comment.
		if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
			if (!ctx_has_comment($first_line, $linenr)) {
				CHK("MEMORY_BARRIER",
				    "memory barrier without comment\n" . 
$herecurr);
			}
		}


- Paul

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

[Santosh Shilimkar]

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 <balbi@ti.com>
>> ---
>>
>> 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.

-->
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));
  }

Patch might be damaged because of copy paste.

Regards
Santosh

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

[Paul Walmsley]

On Sat, 27 Oct 2012, Santosh Shilimkar wrote:

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


- Paul

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

[Santosh Shilimkar]

On Saturday 27 October 2012 09:29 PM, Paul Walmsley wrote:
> On Sat, 27 Oct 2012, Santosh Shilimkar wrote:
>
>> 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.
>
Good point.

> In fact the wmb() is probably overkill, since only a compiler reordering
> barrier is needed.  It can probably just be barrier().
>
I agree. Just barrier() is enough to avoid compiler re-ordering.

Regards
Santosh

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

[Wolfram Sang]

On Thu, Oct 25, 2012 at 12:00:48PM +0300, 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>
> ---
> 
> 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()?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121101/928fba26/attachment.sig>

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

[Felipe Balbi]

Hi,

On Thu, Nov 01, 2012 at 11:23:16PM +0100, Wolfram Sang wrote:
> On Thu, Oct 25, 2012 at 12:00:48PM +0300, 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>
> > ---
> > 
> > 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()?

I was waiting for more comments, will respin the patch next week.

cheers

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121102/1206e9bb/attachment.sig>

[PATCH v2] i2c: omap: ensure writes to dev->buf_len are ordered

[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 <balbi@ti.com>
---

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..ba03bec 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;
+	barrier();
 
 	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

[PATCH v2] i2c: omap: ensure writes to dev->buf_len are ordered

[Wolfram Sang]

On Mon, Nov 05, 2012 at 10:04:43AM +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>
> ---
> 
> 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(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index db31eae..ba03bec 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;
> +	barrier();

I agree adding a comment here is a good idea.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121114/5b44e45c/attachment.sig>

[PATCH v3] i2c: omap: ensure writes to dev->buf_len are ordered

[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 <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

[PATCH v3] i2c: omap: ensure writes to dev->buf_len are ordered

[Wolfram Sang]

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!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121114/0b0eb476/attachment.sig>