2018-05-23 09:57:21

by Esben Haabendal

[permalink] [raw]
Subject: [PATCH 1/4] i2c: imx: Fix reinit_completion() use

From: Esben Haabendal <[email protected]>

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



2018-07-04 07:04:59

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH 1/4] i2c: imx: Fix reinit_completion() use

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 <[email protected]>
>
> 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 <[email protected]>
Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")
Reviewed-by: Uwe Kleine-K?nig <[email protected]>

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

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |

2018-07-09 09:21:18

by Esben Haabendal

[permalink] [raw]
Subject: Re: [PATCH 1/4] i2c: imx: Fix reinit_completion() use

Uwe Kleine-König <[email protected]> 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 <[email protected]>
>>
>> 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