[1/4] dmaengine: imx-sdma: fix buffer ownership
diff mbox series

Message ID 20190911144943.21554-2-philipp.puschmann@emlix.com
State Changes Requested
Headers show
Series
  • Fix UART DMA freezes for iMX6
Related show

Commit Message

Philipp Puschmann Sept. 11, 2019, 2:49 p.m. UTC
BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the buffer,
when 0 ARM owns it. When processing the buffers in
sdma_update_channel_loop the ownership of the currently processed buffer
was set to SDMA again before running the callback function of the the
buffer and while the sdma script may be running in parallel. So there was
the possibility to get the buffer overwritten by SDMA before it has been
processed by kernel leading to kind of random errors in the upper layers,
e.g. bluetooth.

It may be further a good idea to make the status struct member volatile or
access it using writel or similar to rule out that the compiler sets the
BD_DONE flag before the callback routine has finished.

Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
---
 drivers/dma/imx-sdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Lucas Stach Sept. 16, 2019, 2:17 p.m. UTC | #1
On Mi, 2019-09-11 at 16:49 +0200, Philipp Puschmann wrote:
> BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the buffer,
> when 0 ARM owns it. When processing the buffers in
> sdma_update_channel_loop the ownership of the currently processed buffer
> was set to SDMA again before running the callback function of the the
> buffer and while the sdma script may be running in parallel. So there was
> the possibility to get the buffer overwritten by SDMA before it has been
> processed by kernel leading to kind of random errors in the upper layers,
> e.g. bluetooth.
> 
> It may be further a good idea to make the status struct member volatile or
> access it using writel or similar to rule out that the compiler sets the
> BD_DONE flag before the callback routine has finished.
> 
> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
> ---
>  drivers/dma/imx-sdma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index a01f4b5d793c..1abb14ff394d 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -802,7 +802,6 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  		*/
>  
>  		desc->chn_real_count = bd->mode.count;
> -		bd->mode.status |= BD_DONE;
>  		bd->mode.count = desc->period_len;
>  		desc->buf_ptail = desc->buf_tail;
>  		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
> @@ -817,6 +816,8 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
>  		spin_lock(&sdmac->vc.lock);

To address your comment from the second paragraph of the commit message
there should be a dma_wmb() here before changing the status flag.

Regards,
Lucas

> +		bd->mode.status |= BD_DONE;
> +
>  		if (error)
>  			sdmac->status = old_status;
>  	}
Philipp Puschmann Sept. 19, 2019, 9:20 a.m. UTC | #2
Am 16.09.19 um 16:17 schrieb Lucas Stach:
> On Mi, 2019-09-11 at 16:49 +0200, Philipp Puschmann wrote:
>> BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the buffer,
>> when 0 ARM owns it. When processing the buffers in
>> sdma_update_channel_loop the ownership of the currently processed buffer
>> was set to SDMA again before running the callback function of the the
>> buffer and while the sdma script may be running in parallel. So there was
>> the possibility to get the buffer overwritten by SDMA before it has been
>> processed by kernel leading to kind of random errors in the upper layers,
>> e.g. bluetooth.
>>
>> It may be further a good idea to make the status struct member volatile or
>> access it using writel or similar to rule out that the compiler sets the
>> BD_DONE flag before the callback routine has finished.
>>
>> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
>> ---
>>  drivers/dma/imx-sdma.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
>> index a01f4b5d793c..1abb14ff394d 100644
>> --- a/drivers/dma/imx-sdma.c
>> +++ b/drivers/dma/imx-sdma.c
>> @@ -802,7 +802,6 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>>  		*/
>>  
>>  		desc->chn_real_count = bd->mode.count;
>> -		bd->mode.status |= BD_DONE;
>>  		bd->mode.count = desc->period_len;
>>  		desc->buf_ptail = desc->buf_tail;
>>  		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
>> @@ -817,6 +816,8 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>>  		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
>>  		spin_lock(&sdmac->vc.lock);
> 
> To address your comment from the second paragraph of the commit message
> there should be a dma_wmb() here before changing the status flag.
> 
> Regards,
> Lucas

Hi Lucas,

thanks for your feedback. I will apply the hints to v2 of the patches.

Regards,
Philipp
> 
>> +		bd->mode.status |= BD_DONE;
>> +
>>  		if (error)
>>  			sdmac->status = old_status;
>>  	}
>

Patch
diff mbox series

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index a01f4b5d793c..1abb14ff394d 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -802,7 +802,6 @@  static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		*/
 
 		desc->chn_real_count = bd->mode.count;
-		bd->mode.status |= BD_DONE;
 		bd->mode.count = desc->period_len;
 		desc->buf_ptail = desc->buf_tail;
 		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
@@ -817,6 +816,8 @@  static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
 		spin_lock(&sdmac->vc.lock);
 
+		bd->mode.status |= BD_DONE;
+
 		if (error)
 			sdmac->status = old_status;
 	}