Re: [PATCH v2 1/2] tty: serial: 8250_omap: fix tx with dma

[Jiri Slaby <jirislaby@kernel.org>]

On 07. 05. 25, 9:49, Måns Rullgård wrote:
> Jiri Slaby <jirislaby@kernel.org> writes:
> 
>> On 06. 05. 25, 17:07, Mans Rullgard wrote:
>>> Commit 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
>>> introduced an error in the TX DMA handling for 8250_omap.
>>> When the OMAP_DMA_TX_KICK flag is set, one byte is pulled from the
>>> kfifo and emitted directly in order to start the DMA.  This is done
>>> without updating DMA tx_size which leads to uart_xmit_advance() called
>>> in the DMA complete callback advancing the kfifo by one too much.
>>> In practice, transmitting N bytes has been seen to result in the last
>>> N-1 bytes being sent repeatedly.
>>> This change fixes the problem by moving all of the dma setup after
>>> the OMAP_DMA_TX_KICK handling and using kfifo_len() instead of the
>>> dma size for the 4-byte cutoff check. This slightly changes the
>>> behaviour at buffer wraparound, but it still transmits the correct
>>> bytes somehow. At the point kfifo_dma_out_prepare_mapped is called,
>>> at least one byte is guaranteed to be in the fifo, so checking the
>>> return value is not necessary.
>>> Fixes: 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>>> ---
>>> v2: split patch in two
>>> ---
>>>    drivers/tty/serial/8250/8250_omap.c | 24 +++++++++---------------
>>>    1 file changed, 9 insertions(+), 15 deletions(-)
>>> diff --git a/drivers/tty/serial/8250/8250_omap.c
>>> b/drivers/tty/serial/8250/8250_omap.c
>>> index f1aee915bc02..180466e09605 100644
>>> --- a/drivers/tty/serial/8250/8250_omap.c
>>> +++ b/drivers/tty/serial/8250/8250_omap.c
>>> @@ -1173,16 +1173,6 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>>>    		return 0;
>>>    	}
>>>    -	sg_init_table(&sg, 1);
>>> -	ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
>>> -					   UART_XMIT_SIZE, dma->tx_addr);
>>> -	if (ret != 1) {
>>> -		serial8250_clear_THRI(p);
>>> -		return 0;
>>> -	}
>>> -
>>> -	dma->tx_size = sg_dma_len(&sg);
>>> -
>>>    	if (priv->habit & OMAP_DMA_TX_KICK) {
>>>    		unsigned char c;
>>>    		u8 tx_lvl;
>> ...
>>> @@ -1216,11 +1206,12 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>>>    			goto err;
>>>    		}
>>>    		skip_byte = c;
>>> -		/* now we need to recompute due to kfifo_get */
>>> -		kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
>>> -				UART_XMIT_SIZE, dma->tx_addr);
>>>    	}
>>>    +	sg_init_table(&sg, 1);
>>> +	kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
>>> +				     UART_XMIT_SIZE, dma->tx_addr);
>>
>> This can fail (note the first call to this was checked). The latter
>> (deliberately) not.
> 
> No, it can't.  The fifo has already been checked to contain something
> right at the top of the function.  There is no other failure mode for
> kfifo_dma_out_prepare_mapped.

That it cannot fail now does not mean it cannot in the future. Simply do 
it properly and check the retval.

>>> +
>>>    	desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1, DMA_MEM_TO_DEV,
>>>    			DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>>>    	if (!desc) {
>> ...
>>> @@ -1248,8 +1240,10 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>>>    err:
>>>    	dma->tx_err = 1;
>>>    out_skip:
>>> -	if (skip_byte >= 0)
>>> +	if (skip_byte >= 0) {
>>>    		serial_out(p, UART_TX, skip_byte);
>>> +		p->port.icount.tx++;
>>
>> This is still unrelated.
> 
> No, it's not.  Your broken code called uart_xmit_advance with the full
> amount due to the incorrect tx_size value.  This compensates for that.

Then document it properly in the commit log.

> You made this mess.

I can only say that I am sorry for the breakage of this driver. This TX 
way with one byte via FIFO and the rest via DMA, and only if > 4 chars 
to be sent is indeed cumbersome and apparently uneasy to do right.

> Now fix it.  I don't care how.  It's wasted enough
> of my time already.

How exactly does this help to get the code in shape? You apparently have 
the HW, you spent some time debugging that, you have patches, so you 
deserve the credits.

thanks,
-- 
js
suse labs