[PATCH 0/2] tty: serial: msm_serial regression and add info message

[PATCH 0/2] tty: serial: msm_serial regression and add info message

[Frank Rowand]

Commit 3a878c430fd6 ("tty: serial: msm: Add TX DMA support") resulted
in dropped characters and invalid characters in pio mode.  Fix the
problem and add an additional information message that was important
in diagnosing the problem (reporting that DMA mode was not enabled).

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

[Frank Rowand]

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

[PATCH 2/2] tty: serial: msm_serial add info message

[Frank Rowand]

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

Failure to enable DMA by the msm_serial driver is silent.
Add a message to report the failure.

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/tty/serial/msm_serial.c |    1 +
 1 file changed, 1 insertion(+)

Index: b/drivers/tty/serial/msm_serial.c
===================================================================
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -170,6 +170,7 @@ rel_tx:
 	dma_release_channel(dma->chan);
 no_tx:
 	memset(dma, 0, sizeof(*dma));
+	dev_info(dev, "msm_serial: DMA not enabled\n");
 }
 
 static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base)

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

[Stephen Boyd]

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

Re: [PATCH 2/2] tty: serial: msm_serial add info message

[Stephen Boyd]

On 04/23, Frank Rowand wrote:
> From: Frank Rowand <frank.rowand@am.sony.com>
> 
> Failure to enable DMA by the msm_serial driver is silent.
> Add a message to report the failure.
> 
> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> ---
>  drivers/tty/serial/msm_serial.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: b/drivers/tty/serial/msm_serial.c
> ===================================================================
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -170,6 +170,7 @@ rel_tx:
>  	dma_release_channel(dma->chan);
>  no_tx:
>  	memset(dma, 0, sizeof(*dma));
> +	dev_info(dev, "msm_serial: DMA not enabled\n");
>  }
>  

Wouldn't this print twice for TX and RX channels? I'd prefer we
not print anything when this driver probes, just because it's a
bunch of log spam that we don't really need.

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

Re: [PATCH 2/2] tty: serial: msm_serial add info message

[Frank Rowand]

On 4/25/2016 1:48 PM, Stephen Boyd wrote:
> On 04/23, Frank Rowand wrote:
>> From: Frank Rowand <frank.rowand@am.sony.com>
>>
>> Failure to enable DMA by the msm_serial driver is silent.
>> Add a message to report the failure.
>>
>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>> ---
>>  drivers/tty/serial/msm_serial.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> Index: b/drivers/tty/serial/msm_serial.c
>> ===================================================================
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -170,6 +170,7 @@ rel_tx:
>>  	dma_release_channel(dma->chan);
>>  no_tx:
>>  	memset(dma, 0, sizeof(*dma));
>> +	dev_info(dev, "msm_serial: DMA not enabled\n");
>>  }
>>  
> 
> Wouldn't this print twice for TX and RX channels? I'd prefer we
> not print anything when this driver probes, just because it's a
> bunch of log spam that we don't really need.

This is in msm_request_tx_dma().  I should have made the message
"msm_serial: TX DMA not enabled\n" and added a similar message
to msm_request_rx_dma().

Then it could print twice, once for TX and once for RX. :-)
For my board it would print twice because both requests would
fail for the same reason.

Should I add it to msm_request_rx_dma() also, but make both
locations dev_debug() instead of dev_info()?

-Frank

Re: [PATCH 2/2] tty: serial: msm_serial add info message

[Stephen Boyd]

On 04/25, Frank Rowand wrote:
>
> This is in msm_request_tx_dma().  I should have made the message
> "msm_serial: TX DMA not enabled\n" and added a similar message
> to msm_request_rx_dma().
> 
> Then it could print twice, once for TX and once for RX. :-)
> For my board it would print twice because both requests would
> fail for the same reason.

Ah right, the 3 line diff window caught me here.

> 
> Should I add it to msm_request_rx_dma() also, but make both
> locations dev_debug() instead of dev_info()?

Honestly I don't see much point in having this at all. Why does
the user care if DMA is used or not? Don't they just want the
hardware to work? Maybe dev_dbg(), but again, debug junk. I'll
leave it up to you and Greg.

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

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

[Bjorn Andersson]

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

Re: [PATCH 2/2] tty: serial: msm_serial add info message

[Frank Rowand]

On 4/25/2016 2:35 PM, Stephen Boyd wrote:
> On 04/25, Frank Rowand wrote:
>>
>> This is in msm_request_tx_dma().  I should have made the message
>> "msm_serial: TX DMA not enabled\n" and added a similar message
>> to msm_request_rx_dma().
>>
>> Then it could print twice, once for TX and once for RX. :-)
>> For my board it would print twice because both requests would
>> fail for the same reason.
> 
> Ah right, the 3 line diff window caught me here.
> 
>>
>> Should I add it to msm_request_rx_dma() also, but make both
>> locations dev_debug() instead of dev_info()?
> 
> Honestly I don't see much point in having this at all. Why does
> the user care if DMA is used or not? Don't they just want the
> hardware to work? Maybe dev_dbg(), but again, debug junk. I'll
> leave it up to you and Greg.

If the user doesn't care if DMA is used then why even bother
implementing it in the driver?  :-)

I don't _need_ the messages, I just need the driver to quit
dropping bytes and writing corrupt bytes.  So patch 1 of 2 is
sufficient for my needs.

-Frank

Re: [PATCH 2/2] tty: serial: msm_serial add info message

[Greg Kroah-Hartman]

On Sat, Apr 23, 2016 at 10:17:05AM -0700, Frank Rowand wrote:
> From: Frank Rowand <frank.rowand@am.sony.com>
> 
> Failure to enable DMA by the msm_serial driver is silent.
> Add a message to report the failure.
> 
> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> ---
>  drivers/tty/serial/msm_serial.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: b/drivers/tty/serial/msm_serial.c
> ===================================================================
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -170,6 +170,7 @@ rel_tx:
>  	dma_release_channel(dma->chan);
>  no_tx:
>  	memset(dma, 0, sizeof(*dma));
> +	dev_info(dev, "msm_serial: DMA not enabled\n");

Don't print out messages that a user can't do something with :(

So I agree, this is not needed, sorry.

greg k-h

Re: [PATCH 2/2] tty: serial: msm_serial add info message

[Frank Rowand]

On 4/28/2016 1:51 PM, Greg Kroah-Hartman wrote:
> On Sat, Apr 23, 2016 at 10:17:05AM -0700, Frank Rowand wrote:
>> From: Frank Rowand <frank.rowand@am.sony.com>
>>
>> Failure to enable DMA by the msm_serial driver is silent.
>> Add a message to report the failure.
>>
>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>> ---
>>  drivers/tty/serial/msm_serial.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> Index: b/drivers/tty/serial/msm_serial.c
>> ===================================================================
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -170,6 +170,7 @@ rel_tx:
>>  	dma_release_channel(dma->chan);
>>  no_tx:
>>  	memset(dma, 0, sizeof(*dma));
>> +	dev_info(dev, "msm_serial: DMA not enabled\n");
> 
> Don't print out messages that a user can't do something with :(
> 
> So I agree, this is not needed, sorry.
> 
> greg k-h
> 

I am ok with dropping patch 2.  No problem.

-Frank

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

[Andy Gross]

On Sat, Apr 23, 2016 at 10:14:32AM -0700, 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>

This allowed me to transfer files over the console without any CRC errors.
Zmodem transfer at 115k worked great.

Tested-by: Andy Gross <andy.gross@linaro.org>