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

Commit Message

Santosh Shilimkar Oct. 27, 2012, 10:50 a.m. UTC

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 <[e-mail]>
>> ---
>>
>> 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

Comments

Paul Walmsley Oct. 27, 2012, 3:59 p.m. UTC | #1

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

Santosh Shilimkar Oct. 28, 2012, 4:11 a.m. UTC | #2

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

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