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 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.
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?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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
