On Sat 23 Apr 10:14 PDT 2016, Frank Rowand wrote:
> From: Frank Rowand <[email protected]>
>
> Commit 3a878c430fd6 ("tty: serial: msm: Add TX DMA support") regression.
> The calculation of tx_count was moved from the old msm_handle_tx(),
> now renamed msm_handle_tx_pio(), to the new msm_handle_tx(). The
> move left out one size test.
>
> The regression seen on the qcom-apq8074-dragonboard is dropped
> characters and corrupted characters (values greater than 0x7f)
> when DMA is not enabled.
>
> Signed-off-by: Frank Rowand <[email protected]>
> Cc: [email protected]
> ---
> drivers/tty/serial/msm_serial.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> Index: b/drivers/tty/serial/msm_serial.c
> ===================================================================
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -727,6 +727,8 @@ static void msm_handle_tx(struct uart_po
> }
>
> pio_count = CIRC_CNT(xmit->head, xmit->tail, UART_XMIT_SIZE);
> + pio_count = min3(pio_count, (unsigned int)UART_XMIT_SIZE - xmit->tail,
These two lines essentially reimplements CIRC_CNT_TO_END()
> + port->fifosize);
And this looks equivalent to the removed part below.
So I think a smaller patch would be to change the calculation to:
pio_count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
> dma_count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
>
> dma_min = 1; /* Always DMA */
> @@ -738,9 +740,6 @@ static void msm_handle_tx(struct uart_po
> dma_count = UARTDM_TX_MAX;
> }
>
> - if (pio_count > port->fifosize)
> - pio_count = port->fifosize;
> -
> if (!dma->chan || dma_count < dma_min)
> msm_handle_tx_pio(port, pio_count);
> else
However, as you've concluded that the problem is that we don't handle
wrapping writes let's look at msm_handle_tx_pio():
int tf_pointer = 0;
while (tf_pointer < pio_count) {
char buf[4];
if (is_uartdm)
num_chars = min(pio_count - tf_pointer,
(unsigned int)sizeof(buf));
else
num_chars = 1;
for (i = 0; i < num_chars; i++)
buf[i] = xmit->buf[xmit->tail + i];
xmit->tail = (xmit->tail + num_chars) & (UART_XMIT_SIZE - 1);
tf_pointer += num_chars;
}
So the problem you seem to run into is that we copy num_chars bytes
sequentially from xmit->tail, running outside the buffer.
So the problem is that the num_chars calculation isn't limited, perhaps
something like this instead:
num_chars = min3(tx_count - tf_pointer,
sizeof(buf),
(unsigned int)CIRC_CNT_TO_END(xmit->head,
xmit->tail,
UART_XMIT_SIZE));
You should either make msm_handle_tx_pio() handle wrapping buffers or
make the pio_count calculation follow the dma case (with _TO_END).
Regards,
Bjorn
