Re: [PATCH v4 2/3] dmaengine: imx-sdma: fix dma freezes

[Philipp Puschmann <philipp.puschmann@emlix.com>]

Hi Jan,

Am 19.09.19 um 17:19 schrieb Jan Lübbe:
> Hi Philipp,
> 
> see below...
> 
> On Thu, 2019-09-19 at 16:29 +0200, Philipp Puschmann wrote:
>> For some years and since many kernel versions there are reports that the
>> RX UART SDMA channel stops working at some point. The workaround was to
>> disable DMA for RX. This commit tries to fix the problem itself.
>>
>> Due to its license i wasn't able to debug the sdma script itself but it
>> somehow leads to blocking the scheduling of the channel script when a
>> running sdma script does not find any free descriptor in the ring to put
>> its data into.
>>
>> If we detect such a potential case we manually restart the channel.
>>
>> As sdmac->desc is constant we can move desc out of the loop.
>>
>> Fixes: 1ec1e82f2510 ("dmaengine: Add Freescale i.MX SDMA support")
>> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
>> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
>> ---
>>
>> Changelog v4:
>>  - fixed the fixes tag
>>  
>> Changelog v3:
>>  - use correct dma_wmb() instead of dma_wb()
>>  - add fixes tag
>>  
>> Changelog v2:
>>  - clarify comment and commit description
>>
>>  drivers/dma/imx-sdma.c | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
>> index e029a2443cfc..a32b5962630e 100644
>> --- a/drivers/dma/imx-sdma.c
>> +++ b/drivers/dma/imx-sdma.c
>> @@ -775,21 +775,23 @@ static void sdma_start_desc(struct sdma_channel *sdmac)
>>  static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>>  {
>>  	struct sdma_buffer_descriptor *bd;
>> -	int error = 0;
>> -	enum dma_status	old_status = sdmac->status;
>> +	struct sdma_desc *desc = sdmac->desc;
>> +	int error = 0, cnt = 0;
>> +	enum dma_status old_status = sdmac->status;
>>  
>>  	/*
>>  	 * loop mode. Iterate over descriptors, re-setup them and
>>  	 * call callback function.
>>  	 */
>> -	while (sdmac->desc) {
>> -		struct sdma_desc *desc = sdmac->desc;
>> +	while (desc) {
>>  
>>  		bd = &desc->bd[desc->buf_tail];
>>  
>>  		if (bd->mode.status & BD_DONE)
>>  			break;
>>  
>> +		cnt++;
>> +
>>  		if (bd->mode.status & BD_RROR) {
>>  			bd->mode.status &= ~BD_RROR;
>>  			sdmac->status = DMA_ERROR;
>> @@ -822,6 +824,17 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>>  		if (error)
>>  			sdmac->status = old_status;
>>  	}
>> +
>> +	/* In some situations it may happen that the sdma does not found any
>                                                           ^ hasn't
>> +	 * usable descriptor in the ring to put data into. The channel is
>> +	 * stopped then. While there is no specific error condition we can
>> +	 * check for, a necessary condition is that all available buffers for
>> +	 * the current channel have been written to by the sdma script. In
>> +	 * this case and after we have made the buffers available again,
>> +	 * we restart the channel.
>> +	 */
> 
> Are you sure we can't miss cases where we only had to make some buffers
> available again, but the SDMA already ran out of buffers before?
Think so, yes.
> 
> A while ago, I was debugging a similar issue triggered by receiving
> data with a wrong baud rate, which leads to all descriptors being
> marked with the error flag very quickly (and the SDMA stalling).
> I noticed that you can check if the channel is still running by
> checking the SDMA_H_STATSTOP register & BIT(sdmac->channel).

I think checking for this register is the better approach. Then i could drop the
cnt variable. And by droppting cnt i would propose to move the check and reenabling
to the end of the while loop to reenable the channel after freeing first buffer.

> 
> I also added a flag for the sdmac->flags field to allow stopping the
> channel from the callback (otherwise it would enable the channel
> again).

Could memory and compiler ordering a problem here?
I'm not that into these kind of problems, but is this
	sdmac->flags &= ~IMX_DMA_ACTIVE;
  	writel_relaxed(BIT(channel), sdma->regs + SDMA_H_STATSTOP);
guaranteed to be free of race conditions?

Regards,
Philipp

> 
> Attached is my current version of that patch for reference.
> 
>> +	if (cnt >= desc->num_bd)
>> +		sdma_enable_channel(sdmac->sdma, sdmac->channel);
>>  }
>>  
>>  static void mxc_sdma_handle_channel_normal(struct sdma_channel *data)