Hello Ian,
(there are two Ians in the recipents of this mail, are you really two
different persons?)
On Tue, Jul 18, 2017 at 10:42:52AM +0100, Ian Arkver wrote:
> 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.
IMHO this should be further investigated even if the patch is already
merged. Ian, do you care to follow up?
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
