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.puschm...@emlix.com>
>> Reviewed-by: Lucas Stach <l.st...@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)
