Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1385689 > unrolled thread

[PATCH 1/2] tty: serial: msm_serial regression fix data corruption

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] tty: serial: msm_serial regression fix data corruption Frank Rowand <frowand.list@gmail.com> - 2016-04-23 19:20 +0200
    Re: [PATCH 1/2] tty: serial: msm_serial regression fix data  corruption Stephen Boyd <sboyd@codeaurora.org> - 2016-04-25 22:50 +0200
    Re: [PATCH 1/2] tty: serial: msm_serial regression fix data  corruption Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-04-26 00:40 +0200

#1385689 — [PATCH 1/2] tty: serial: msm_serial regression fix data corruption

From: Frank Rowand <frank.rowand@am.sony.com>

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 <frank.rowand@am.sony.com>
Cc: stable@vger.kernel.org
---
 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,
+			port->fifosize);
 	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

[toc] | [next] | [standalone]

#1386876 — Re: [PATCH 1/2] tty: serial: msm_serial regression fix data corruption

On 04/23, Frank Rowand wrote:
> From: Frank Rowand <frank.rowand@am.sony.com>
> 
> 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 <frank.rowand@am.sony.com>
> Cc: stable@vger.kernel.org
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

Thanks for finding this!

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1386971 — Re: [PATCH 1/2] tty: serial: msm_serial regression fix data corruption

On Sat 23 Apr 10:14 PDT 2016, Frank Rowand wrote:

> From: Frank Rowand <frank.rowand@am.sony.com>
> 
> 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 <frank.rowand@am.sony.com>
> Cc: stable@vger.kernel.org
> ---
>  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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web