Skip to contentSkip to conversation

tty: serial: msm_serial regression and add info message

PATCH In review Frank Rowand 5 participants 12 messages Apr 23, 2016 – May 5, 2016

Subsystems (2)
Arm Qualcomm mailing list TTY layer and serial drivers
lore
Share & tools
lore

Frank Rowand

author
#0 lore raw

cover tty: serial: msm_serial regression and add info message

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).

author
#1 lore raw

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

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(-)
drivers/tty/serial/msm_serial.c +2 -3
Index: 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
Reviewed-by on v1 1/2
#2 lore raw

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

Quoted 7 lines · Frank Rowand

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.

Quoted 7 lines

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]

---

Reviewed-by Stephen Boyd <[email protected]>

Thanks for finding this!

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
#3 lore raw

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

In reply to Frank Rowand
Quoted 22 lines · Frank Rowand

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
===================================================================

drivers/tty/serial/msm_serial.c static void msm_handle_tx(struct uart_po · line 730

Quoted 4 lines
drivers/tty/serial/msm_serial.c +1 -0
@@ -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()

drivers/tty/serial/msm_serial.c static void msm_handle_tx(struct uart_po · line 731

Quoted 1 line
drivers/tty/serial/msm_serial.c +1 -0
+			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);

drivers/tty/serial/msm_serial.c static void msm_handle_tx(struct uart_po · line 745

Quoted 13 lines
drivers/tty/serial/msm_serial.c +0 -3
 	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

Tested-by on v1 1/2
#4 lore raw

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

In reply to Frank Rowand
Quoted 7 lines · Frank Rowand

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.

Quoted 5 lines

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]>

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

Tested-by Andy Gross <[email protected]>
author
#5 lore raw

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

In reply to Frank Rowand

From: Frank Rowand <[email protected]>

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

Signed-off-by Frank Rowand <[email protected]>
---
 drivers/tty/serial/msm_serial.c |    1 +
 1 file changed, 1 insertion(+)
drivers/tty/serial/msm_serial.c +1 -0
Index: 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)
#6 lore raw

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

Quoted 15 lines · Frank Rowand

From: Frank Rowand <[email protected]>

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

Signed-off-by Frank Rowand <[email protected]>
---
 drivers/tty/serial/msm_serial.c |    1 +
 1 file changed, 1 insertion(+)

Index: b/drivers/tty/serial/msm_serial.c
===================================================================

drivers/tty/serial/msm_serial.c rel_tx: · line 175

Quoted 6 lines
drivers/tty/serial/msm_serial.c +1 -0
@@ -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.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#7 lore raw

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

Quoted 11 lines · Stephen Boyd
Quoted 10 lines · Frank Rowand

From: Frank Rowand <[email protected]>

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

Signed-off-by Frank Rowand <[email protected]>
---
 drivers/tty/serial/msm_serial.c |    1 +
 1 file changed, 1 insertion(+)
Quoted 15 lines

drivers/tty/serial/msm_serial.c rel_tx: · line 175

Quoted 11 lines

Index: b/drivers/tty/serial/msm_serial.c
===================================================================

drivers/tty/serial/msm_serial.c +1 -0
@@ -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

#8 lore raw

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

Quoted 8 lines · Frank Rowand

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.

Quoted 3 lines

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.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#9 lore raw

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

Quoted 13 lines · Stephen Boyd
Quoted 8 lines · Frank Rowand

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.

Quoted 1 line
Quoted 7 lines
Quoted 2 lines

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

maintainer
#10 lore raw

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

In reply to Frank Rowand
Quoted 15 lines · Frank Rowand

From: Frank Rowand <[email protected]>

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

Signed-off-by Frank Rowand <[email protected]>
---
 drivers/tty/serial/msm_serial.c |    1 +
 1 file changed, 1 insertion(+)

Index: b/drivers/tty/serial/msm_serial.c
===================================================================

drivers/tty/serial/msm_serial.c rel_tx: · line 173

Quoted 4 lines
drivers/tty/serial/msm_serial.c +1 -0
@@ -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

author
#11 lore raw

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

Quoted 23 lines · Greg Kroah-Hartman
Quoted 15 lines · Frank Rowand

From: Frank Rowand <[email protected]>

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

Signed-off-by Frank Rowand <[email protected]>
---
 drivers/tty/serial/msm_serial.c |    1 +
 1 file changed, 1 insertion(+)

Index: b/drivers/tty/serial/msm_serial.c
===================================================================

drivers/tty/serial/msm_serial.c rel_tx: · line 173

Quoted 4 lines
drivers/tty/serial/msm_serial.c +1 -0
@@ -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 :(

Quoted 4 lines

So I agree, this is not needed, sorry.

greg k-h

I am ok with dropping patch 2. No problem.

-Frank