Hi Uwe.
On 18/07/17 09:26, Uwe Kleine-König wrote:
Hello Ian,
On Fri, Jul 14, 2017 at 05:31:57PM +0100, Ian Jamison wrote:
Function imx_transmit_buffer starts a TX DMA if DMA is enabled, since
commit 91a1a909f921 ("serial: imx: Support sw flow control in DMA mode").
It also carries on and attempts to write the same TX buffer using PIO.
This results in TX data corruption and double-incrementing xmit->tail
with the knock-on effect of tail passing head and a page of garbage
being sent out.
Good catch, thank you.
This seems to be triggered mostly when using RS485 half duplex on SMP
systems, but is probably not limited to just those.
Tested locally on an i.MX6Q with an RS485 half duplex transceiver on
UART3, and also by Clemens Gruber.
Tested-by: Clemens Gruber <clemens.gruber@xxxxxxxxxxxx>
Signed-off-by: Ian Jamison <ian.dev@xxxxxxxxxx>
---
drivers/tty/serial/imx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 9e3162bf3bd1..f6c37ce457c1 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -464,7 +464,7 @@ static inline void imx_transmit_buffer(struct imx_port *sport)
}
}
- while (!uart_circ_empty(xmit) &&
+ while (!uart_circ_empty(xmit) && !sport->dma_is_txing &&
!(readl(sport->port.membase + uts_reg(sport)) & UTS_TXFULL)) {
/* send xmit->buf[xmit->tail]
* out the port here */
The bigger context looks as follows:
1 static inline void imx_transmit_buffer(struct imx_port *sport)
2 {
3 if (sport->port.x_char) {
4 writel(sport->port.x_char, sport->port.membase + URTX0);
5 sport->port.icount.tx++;
6 sport->port.x_char = 0;
7 return;
8 }
9
10 if (uart_circ_empty(xmit) || uart_tx_stopped(&sport->port)) {
11 imx_stop_tx(&sport->port);
12 return;
13 }
14
15 led_trigger_blink_oneshot(...)
16
17 if (sport->dma_is_enabled) {
18 /*
19 * We've just sent a X-char Ensure the TX DMA is enabled
20 * and the TX IRQ is disabled.
21 */
22 temp = readl(sport->port.membase + UCR1);
23 temp &= ~UCR1_TXMPTYEN;
24 if (sport->dma_is_txing) {
25 temp |= UCR1_TDMAEN;
26 writel(temp, sport->port.membase + UCR1);
27 } else {
28 writel(temp, sport->port.membase + UCR1);
29 imx_dma_tx(sport);
30 }
31 }
32
33 while (!uart_circ_empty(xmit) &&
34 !(readl(sport->port.membase + uts_reg(sport)) & UTS_TXFULL)) {
35 /* send xmit->buf[xmit->tail]
36 * out the port here */
37 writel(xmit->buf[xmit->tail], sport->port.membase + URTX0);
38 xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
39 sport->port.icount.tx++;
40 }
41
42 if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
43 uart_write_wakeup(&sport->port);
44
45 if (uart_circ_empty(xmit))
46 imx_stop_tx(&sport->port);
47 }
It is obvious that Ian's finding is right. After DMA is started in the
block 18 - 30 the driver starts doing PIO.
I wonder why this problem didn't hit us harder, i.e. it was only
reported when doing rs485 on a multi-core machine. So in the more normal
case, the data is sent only once. Looking at the driver I don't see how
the data cannot be sent twice: imx_transmit_buffer is called with
port.lock taken and irqs off. Then dma is set up to send the pending
chars and kicked to eventually start sending. When imx_transmit_buffer
reaches line 33 the DMA part didn't update the pointers in the xmit
buffer yet because this is only done in the dma_tx_callback that grabs
the lock before. The only thing that could prevent the data going out
twice is that after PIO imx_stop_tx is called before the DMA sends out
the data.
I don't really understand why this isn't more noticeable either, though
reading through some of the older comments on RS485 half-duplex errors,
I think I saw some people mentioning problems in other contexts. No
references handy, sorry.
I think the problem needs a different fix: If dma_is_enabled in line 17,
lines 33 to 46 should not be executed because dma_tx_callback also
implements the logic of lines 42 to 46.
Maybe imx_transmit_buffer should just return before the PIO loop if the
TX DMA has been started?
I think GregKH has already merged this patch, so any change will have to
be a new patch rather than a v2, I guess.
Further I wonder about the x_char handling: What if dma is currently
active when the driver sends the x_char? Can the fifo be full?
There's code in imx_start_tx to temporarily disable the TDMAEN bit and
wake up imx_txint (which calls imx_transmit_buffer) to send the x_char
and then resume the DMA (or start a new one). I haven't tried to trace
out all the various code paths though.
Regards,
Ian
Best regards
Uwe
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
