[1/4] i2c: imx: Fix reinit_completion() use

Message ID 20180523095623.3347-2-esben.haabendal@gmail.com
State Superseded
Headers show
Series
  • i2c: imx: Fix and enable DMA support for LS1021A
Related show

Commit Message

Esben Haabendal May 23, 2018, 9:56 a.m.
From: Esben Haabendal <eha@deif.com>

Make sure to call reinit_completion() before dma is started to avoid race
condition where reinit_compleition() is called after complete() and before
wait_for_completion_timeout().

Signed-off-by: Esben Haabendal <eha@deif.com>
---
 drivers/i2c/busses/i2c-imx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Uwe Kleine-König July 4, 2018, 7:03 a.m. | #1
Cc += Yuan Yao who authored DMA support and the NXP team.

On Wed, May 23, 2018 at 11:56:20AM +0200, Esben Haabendal wrote:
> From: Esben Haabendal <eha@deif.com>
> 
> Make sure to call reinit_completion() before dma is started to avoid race
> condition where reinit_compleition() is called after complete() and before

s/compleition/completion/

> wait_for_completion_timeout().

Is this a theoretical problem, or did it trigger on your side?
 
> Signed-off-by: Esben Haabendal <eha@deif.com>
Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

> ---
>  drivers/i2c/busses/i2c-imx.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index d7267dd9c7bf..6fca5e64cffb 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -377,6 +377,7 @@ static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx,
>  		goto err_desc;
>  	}
>  
> +	reinit_completion(&dma->cmd_complete);
>  	txdesc->callback = i2c_imx_dma_callback;
>  	txdesc->callback_param = i2c_imx;
>  	if (dma_submit_error(dmaengine_submit(txdesc))) {
> @@ -631,7 +632,6 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
>  	 * The first byte must be transmitted by the CPU.
>  	 */
>  	imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> -	reinit_completion(&i2c_imx->dma->cmd_complete);
>  	time_left = wait_for_completion_timeout(
>  				&i2c_imx->dma->cmd_complete,
>  				msecs_to_jiffies(DMA_TIMEOUT));
> @@ -690,7 +690,6 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
>  	if (result)
>  		return result;
>  
> -	reinit_completion(&i2c_imx->dma->cmd_complete);
>  	time_left = wait_for_completion_timeout(
>  				&i2c_imx->dma->cmd_complete,
>  				msecs_to_jiffies(DMA_TIMEOUT));

Best regards
Uwe
Esben Haabendal July 9, 2018, 9:20 a.m. | #2
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> Cc += Yuan Yao who authored DMA support and the NXP team.
>
> On Wed, May 23, 2018 at 11:56:20AM +0200, Esben Haabendal wrote:
>> From: Esben Haabendal <eha@deif.com>
>> 
>> Make sure to call reinit_completion() before dma is started to avoid race
>> condition where reinit_compleition() is called after complete() and before
>
> s/compleition/completion/

Will fix in v2.

>
>> wait_for_completion_timeout().
>
> Is this a theoretical problem, or did it trigger on your side?

I thought I did trigger it, but haven't been able to reproduce.
So it might or might not be theoretical based on my experiences, but it
does look like something that can (and thus should) trigger sometimes.

/Esben

Patch

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index d7267dd9c7bf..6fca5e64cffb 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -377,6 +377,7 @@  static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx,
 		goto err_desc;
 	}
 
+	reinit_completion(&dma->cmd_complete);
 	txdesc->callback = i2c_imx_dma_callback;
 	txdesc->callback_param = i2c_imx;
 	if (dma_submit_error(dmaengine_submit(txdesc))) {
@@ -631,7 +632,6 @@  static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
 	 * The first byte must be transmitted by the CPU.
 	 */
 	imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
-	reinit_completion(&i2c_imx->dma->cmd_complete);
 	time_left = wait_for_completion_timeout(
 				&i2c_imx->dma->cmd_complete,
 				msecs_to_jiffies(DMA_TIMEOUT));
@@ -690,7 +690,6 @@  static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 	if (result)
 		return result;
 
-	reinit_completion(&i2c_imx->dma->cmd_complete);
 	time_left = wait_for_completion_timeout(
 				&i2c_imx->dma->cmd_complete,
 				msecs_to_jiffies(DMA_TIMEOUT));