Re: 8250_dw DMA issue with BYT ?

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Loic,

On Wed, Apr 16, 2014 at 04:34:58PM +0000, Poulain, Loic wrote:
> After analysis, here is my status:
> 
> The Bluetooth chip generates hardware errors due to unexpected
> data. These unexpected data are caused by 8250 concurrent TX.
> 
> - Concurrent DMA TX (8250_dma.c):
> 
> After each DMA transfer, __dma_tx_complete() is called.
> This function updates the circular buffer tail and calls
> serial8250_tx_dma() if data remains.
> 
> However these operations are not protected against concurrent
> call of serial8250_tx_dma() (via uart start_tx). So, this concurrent 
> call may use non-updated tail index and may be called in parallel 
> of an other serial8250_tx_dma().
> 
> 
> - Concurrent Chars TX (8250_corec.c):
> 
> In the serial8250_handle_irq(), serial8250_tx_chars is called if
> UART_LSR_THRE. However we should not call this function
> if we are using the DMA. Moreover, for the same reason as above,
> serial8250_tx_chars could be called with bad tail index or in parallel
> of a serial8250_tx_dma.

Nice job! Thanks a lot for the analysis!

> I think two fixes are necessary:
> 
> to avoid tx chars usage:

<snip>

> The last one fixes TX issues but causes random freeze of my
> platform (when uploading file via bluetooth). It seems to be a deadlock.
> I have no kernel trace in my console, system is stuck.
> 
> To temporally workaround this, I simply moved "tx_running=0" after updating
> tail index (in __dma_tx_complete) so that no dma_tx can happen before 
> this update.
> 
> 	xmit->tail += dma->tx_size;
> 	xmit->tail &= UART_XMIT_SIZE - 1;
> 	p->port.icount.tx += dma->tx_size;
> 
> 	dma->tx_running = 0;

As a side note. Since you are in any case playing with the tx_running
flag, could you check if it's possible to drop it and just rely on the
status from the dma engine (with dmaengine_tx_status())?


Thanks,

-- 
heikki