[PATCH] i2c: davinci: Fix race when setting up for TX

[PATCH] i2c: davinci: Fix race when setting up for TX

[Jon Povey]

When setting up to transmit, a race exists between the ISR and
i2c_davinci_xfer_msg() trying to load the first byte and adjust counters.
This is mostly visible for transmits > 1 byte long.

The ISR may run at any time after the mode register has been set.
While we are setting up and loading the first byte, protect this critical
section from the ISR with a spinlock.

The RX path or zero-length transmits do not need this locking.

Tested on DM355 connected to Techwell TW2836 and Wolfson WM8985

Signed-off-by: Jon Povey <jon.povey-Ean/AyPsLtfkYMGBc/C6ZA@public.gmane.org>
---
I suspect this hasn't shown up for others using single-byte transmits as the
interrupt tends to either run entirely before or entirely after this block
in i2c_davinci_xfer_msg():

	/*
	 * First byte should be set here, not after interrupt,
	 * because transmit-data-ready interrupt can come before
	 * NACK-interrupt during sending of previous message and
	 * ICDXR may have wrong data
	 */
	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
 		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
 		dev->buf_len--;
 	}

Often the entire message would be sent before that test was executed
(observed with LED wiggling and a logic analyser), so dev->buf_len would
be untrue and things merrily went on their way. That seems to be counter
to the intent in the comment.

I tried some fiddling around reordering the register loads but couldn't
get things reliable so stuck in a spinlock. Better solutions welcome.

P.S.: Having run into the the bus reset code a lot during testing, I
am pretty sure that that generic_i2c_clock_pulse() does NOTHING due to
pinmuxing, at least on DM355, it is misleading and may be better
replaced with a comment saying "It would be great to toggle SCL here".

 drivers/i2c/busses/i2c-davinci.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 2222c87..43aa55d 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -107,6 +107,7 @@ struct davinci_i2c_dev {
 	u8			*buf;
 	size_t			buf_len;
 	int			irq;
+	spinlock_t		lock;
 	int			stop;
 	u8			terminate;
 	struct i2c_adapter	adapter;
@@ -312,6 +313,8 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 	u32 flag;
 	u16 w;
 	int r;
+	unsigned long flags;
+	int preload = 0;
 
 	if (!pdata)
 		pdata = &davinci_i2c_platform_data_default;
@@ -347,6 +350,15 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 		flag &= ~DAVINCI_I2C_MDR_STP;
 	}
 
+	/*
+	 * When transmitting, lock ISR out to avoid it racing on the buffer and
+	 * DXR register before we are done
+	 */
+	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
+		preload = 1;
+		spin_lock_irqsave(&dev->lock, flags);
+	}
+
 	/* Enable receive or transmit interrupts */
 	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
 	if (msg->flags & I2C_M_RD)
@@ -366,13 +378,15 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 	 * NACK-interrupt during sending of previous message and
 	 * ICDXR may have wrong data
 	 */
-	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
+	if (preload) {
 		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
 		dev->buf_len--;
+		spin_unlock_irqrestore(&dev->lock, flags);
 	}
 
 	r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
 						      dev->adapter.timeout);
+
 	if (r == 0) {
 		dev_err(dev->dev, "controller timed out\n");
 		i2c_recover_bus(dev);
@@ -490,6 +504,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 	int count = 0;
 	u16 w;
 
+	spin_lock(&dev->lock);
+
 	while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
 		dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
 		if (count++ == 100) {
@@ -579,6 +595,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 		}
 	}
 
+	spin_unlock(&dev->lock);
+
 	return count ? IRQ_HANDLED : IRQ_NONE;
 }
 
@@ -662,6 +680,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 		goto err_release_region;
 	}
 
+	spin_lock_init(&dev->lock);
 	init_completion(&dev->cmd_complete);
 #ifdef CONFIG_CPU_FREQ
 	init_completion(&dev->xfr_complete);
-- 
1.6.3.3

Re: [PATCH] i2c: davinci: Fix race when setting up for TX

[Kevin Hilman]

Jon Povey <jon.povey-Ean/AyPsLtfkYMGBc/C6ZA@public.gmane.org> writes:

> When setting up to transmit, a race exists between the ISR and
> i2c_davinci_xfer_msg() trying to load the first byte and adjust counters.
> This is mostly visible for transmits > 1 byte long.
>
> The ISR may run at any time after the mode register has been set.
> While we are setting up and loading the first byte, protect this critical
> section from the ISR with a spinlock.
>
> The RX path or zero-length transmits do not need this locking.
>
> Tested on DM355 connected to Techwell TW2836 and Wolfson WM8985
>
> Signed-off-by: Jon Povey <jon.povey-Ean/AyPsLtfkYMGBc/C6ZA@public.gmane.org>

This looks like a good fix. 

Anyone else care to test on other platforms and add a 'Tested-by'?

Thanks,

Kevin

> ---
> I suspect this hasn't shown up for others using single-byte transmits as the
> interrupt tends to either run entirely before or entirely after this block
> in i2c_davinci_xfer_msg():
>
> 	/*
> 	 * First byte should be set here, not after interrupt,
> 	 * because transmit-data-ready interrupt can come before
> 	 * NACK-interrupt during sending of previous message and
> 	 * ICDXR may have wrong data
> 	 */
> 	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
>  		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
>  		dev->buf_len--;
>  	}
>
> Often the entire message would be sent before that test was executed
> (observed with LED wiggling and a logic analyser), so dev->buf_len would
> be untrue and things merrily went on their way. That seems to be counter
> to the intent in the comment.
>
> I tried some fiddling around reordering the register loads but couldn't
> get things reliable so stuck in a spinlock. Better solutions welcome.
>
> P.S.: Having run into the the bus reset code a lot during testing, I
> am pretty sure that that generic_i2c_clock_pulse() does NOTHING due to
> pinmuxing, at least on DM355, it is misleading and may be better
> replaced with a comment saying "It would be great to toggle SCL here".
>
>  drivers/i2c/busses/i2c-davinci.c |   21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 2222c87..43aa55d 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -107,6 +107,7 @@ struct davinci_i2c_dev {
>  	u8			*buf;
>  	size_t			buf_len;
>  	int			irq;
> +	spinlock_t		lock;
>  	int			stop;
>  	u8			terminate;
>  	struct i2c_adapter	adapter;
> @@ -312,6 +313,8 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  	u32 flag;
>  	u16 w;
>  	int r;
> +	unsigned long flags;
> +	int preload = 0;
>  
>  	if (!pdata)
>  		pdata = &davinci_i2c_platform_data_default;
> @@ -347,6 +350,15 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  		flag &= ~DAVINCI_I2C_MDR_STP;
>  	}
>  
> +	/*
> +	 * When transmitting, lock ISR out to avoid it racing on the buffer and
> +	 * DXR register before we are done
> +	 */
> +	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
> +		preload = 1;
> +		spin_lock_irqsave(&dev->lock, flags);
> +	}
> +
>  	/* Enable receive or transmit interrupts */
>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
>  	if (msg->flags & I2C_M_RD)
> @@ -366,13 +378,15 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  	 * NACK-interrupt during sending of previous message and
>  	 * ICDXR may have wrong data
>  	 */
> -	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
> +	if (preload) {
>  		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
>  		dev->buf_len--;
> +		spin_unlock_irqrestore(&dev->lock, flags);
>  	}
>  
>  	r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
>  						      dev->adapter.timeout);
> +
>  	if (r == 0) {
>  		dev_err(dev->dev, "controller timed out\n");
>  		i2c_recover_bus(dev);
> @@ -490,6 +504,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  	int count = 0;
>  	u16 w;
>  
> +	spin_lock(&dev->lock);
> +
>  	while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
>  		dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
>  		if (count++ == 100) {
> @@ -579,6 +595,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  		}
>  	}
>  
> +	spin_unlock(&dev->lock);
> +
>  	return count ? IRQ_HANDLED : IRQ_NONE;
>  }
>  
> @@ -662,6 +680,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>  		goto err_release_region;
>  	}
>  
> +	spin_lock_init(&dev->lock);
>  	init_completion(&dev->cmd_complete);
>  #ifdef CONFIG_CPU_FREQ
>  	init_completion(&dev->xfr_complete);

Re: [PATCH] i2c: davinci: Fix race when setting up for TX

[Troy Kisky]

On 9/16/2010 10:37 AM, Kevin Hilman wrote:
> Jon Povey <jon.povey-Ean/AyPsLtfkYMGBc/C6ZA@public.gmane.org> writes:
> 
>> When setting up to transmit, a race exists between the ISR and
>> i2c_davinci_xfer_msg() trying to load the first byte and adjust counters.
>> This is mostly visible for transmits > 1 byte long.
>>
>> The ISR may run at any time after the mode register has been set.
>> While we are setting up and loading the first byte, protect this critical
>> section from the ISR with a spinlock.
>>
>> The RX path or zero-length transmits do not need this locking.
>>
>> Tested on DM355 connected to Techwell TW2836 and Wolfson WM8985
>>
>> Signed-off-by: Jon Povey <jon.povey-Ean/AyPsLtfkYMGBc/C6ZA@public.gmane.org>
> 
> This looks like a good fix. 
> 
> Anyone else care to test on other platforms and add a 'Tested-by'?
> 
> Thanks,
> 
> Kevin
> 
>> ---
>> I suspect this hasn't shown up for others using single-byte transmits as the
>> interrupt tends to either run entirely before or entirely after this block
>> in i2c_davinci_xfer_msg():
>>
>> 	/*
>> 	 * First byte should be set here, not after interrupt,
>> 	 * because transmit-data-ready interrupt can come before
>> 	 * NACK-interrupt during sending of previous message and
>> 	 * ICDXR may have wrong data
>> 	 */
>> 	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
>>  		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
>>  		dev->buf_len--;
>>  	}
>>
>> Often the entire message would be sent before that test was executed
>> (observed with LED wiggling and a logic analyser), so dev->buf_len would
>> be untrue and things merrily went on their way. That seems to be counter
>> to the intent in the comment.
>>
>> I tried some fiddling around reordering the register loads but couldn't
>> get things reliable so stuck in a spinlock. Better solutions welcome.
>>
>> P.S.: Having run into the the bus reset code a lot during testing, I
>> am pretty sure that that generic_i2c_clock_pulse() does NOTHING due to
>> pinmuxing, at least on DM355, it is misleading and may be better
>> replaced with a comment saying "It would be great to toggle SCL here".
>>
>>  drivers/i2c/busses/i2c-davinci.c |   21 ++++++++++++++++++++-
>>  1 files changed, 20 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index 2222c87..43aa55d 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -107,6 +107,7 @@ struct davinci_i2c_dev {
>>  	u8			*buf;
>>  	size_t			buf_len;
>>  	int			irq;
>> +	spinlock_t		lock;
>>  	int			stop;
>>  	u8			terminate;
>>  	struct i2c_adapter	adapter;
>> @@ -312,6 +313,8 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>  	u32 flag;
>>  	u16 w;
>>  	int r;
>> +	unsigned long flags;
>> +	int preload = 0;
>>  
>>  	if (!pdata)
>>  		pdata = &davinci_i2c_platform_data_default;
>> @@ -347,6 +350,15 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>  		flag &= ~DAVINCI_I2C_MDR_STP;
>>  	}
>>  
>> +	/*
>> +	 * When transmitting, lock ISR out to avoid it racing on the buffer and
>> +	 * DXR register before we are done
>> +	 */
>> +	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
>> +		preload = 1;
>> +		spin_lock_irqsave(&dev->lock, flags);
>> +	}
>> +
>>  	/* Enable receive or transmit interrupts */
>>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);


Maybe you can write 0 to IMR here, and move this interrupt enable (IMR) write to after the DXR write.
That would seem a better patch.


>>  	if (msg->flags & I2C_M_RD)
>> @@ -366,13 +378,15 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>  	 * NACK-interrupt during sending of previous message and
>>  	 * ICDXR may have wrong data
>>  	 */
>> -	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
>> +	if (preload) {
>>  		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
>>  		dev->buf_len--;
>> +		spin_unlock_irqrestore(&dev->lock, flags);
>>  	}
>>  
>>  	r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
>>  						      dev->adapter.timeout);
>> +
>>  	if (r == 0) {
>>  		dev_err(dev->dev, "controller timed out\n");
>>  		i2c_recover_bus(dev);
>> @@ -490,6 +504,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>>  	int count = 0;
>>  	u16 w;
>>  
>> +	spin_lock(&dev->lock);
>> +
>>  	while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
>>  		dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
>>  		if (count++ == 100) {
>> @@ -579,6 +595,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>>  		}
>>  	}
>>  
>> +	spin_unlock(&dev->lock);
>> +
>>  	return count ? IRQ_HANDLED : IRQ_NONE;
>>  }
>>  
>> @@ -662,6 +680,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>  		goto err_release_region;
>>  	}
>>  
>> +	spin_lock_init(&dev->lock);
>>  	init_completion(&dev->cmd_complete);
>>  #ifdef CONFIG_CPU_FREQ
>>  	init_completion(&dev->xfr_complete);
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
> 

[PATCH] i2c: davinci: Fix race when setting up for TX

[Jon Povey]

Move the interrupt enable after the first byte load for TX, as it
was racing with the interrupt handler and corrupting dev->buf_len
for messages > 1 byte.

Tested on DM355.

Signed-off-by: Jon Povey <jon.povey-Ean/AyPsLtfkYMGBc/C6ZA@public.gmane.org>
---
The race seems to have been introduced by 869e6692d527983958216f13c3c8e095791909df

This fixes the problem for me, but someone from TI might want to comment
on the validity of starting the transfer before enabling the RDY interrupts.
Any possibility of losing interrupts?

Considered a spinlock but if this reordering is OK then it's neater.

I'm guessing not many people have been sending multi-byte messages..

 drivers/i2c/busses/i2c-davinci.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 2222c87..72df4af 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -347,14 +347,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 		flag &= ~DAVINCI_I2C_MDR_STP;
 	}
 
-	/* Enable receive or transmit interrupts */
-	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
-	if (msg->flags & I2C_M_RD)
-		w |= DAVINCI_I2C_IMR_RRDY;
-	else
-		w |= DAVINCI_I2C_IMR_XRDY;
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
-
 	dev->terminate = 0;
 
 	/* write the data into mode register */
@@ -371,6 +363,14 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 		dev->buf_len--;
 	}
 
+	/* Enable receive or transmit interrupts */
+	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
+	if (msg->flags & I2C_M_RD)
+		w |= DAVINCI_I2C_IMR_RRDY;
+	else
+		w |= DAVINCI_I2C_IMR_XRDY;
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
+
 	r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
 						      dev->adapter.timeout);
 	if (r == 0) {
-- 
1.6.3.3

RE: [PATCH] i2c: davinci: Fix race when setting up for TX

[Sudhakar Rajashekhara]

Hi,

On Tue, Jun 22, 2010 at 13:01:03, Jon Povey wrote:
> Move the interrupt enable after the first byte load for TX, as it
> was racing with the interrupt handler and corrupting dev->buf_len
> for messages > 1 byte.
> 
> Tested on DM355.
> 
> Signed-off-by: Jon Povey <jon.povey-Ean/AyPsLtfkYMGBc/C6ZA@public.gmane.org>
> ---
> The race seems to have been introduced by 869e6692d527983958216f13c3c8e095791909df
> 
> This fixes the problem for me, but someone from TI might want to comment
> on the validity of starting the transfer before enabling the RDY interrupts.
> Any possibility of losing interrupts?
> 

This fix can cause interrupts to be missed as there will be a race between
writing to DXR and setting of interrupts. I think you can decrement dev->buf_len
before writing to DXR register. This should resolve the original race condition
which you had encountered.

Regards,
Sudhakar