* [PATCH] tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx @ 2016-12-13 16:27 Richard Genoud 2017-01-02 11:13 ` Nicolas Ferre 2017-03-15 11:37 ` Nicolas Ferre 0 siblings, 2 replies; 12+ messages in thread From: Richard Genoud @ 2016-12-13 16:27 UTC (permalink / raw) To: linux-arm-kernel If we don't disable the transmitter in atmel_stop_tx, the DMA buffer continues to send data until it is emptied. This cause problems with the flow control (CTS is asserted and data are still sent). So, disabling the transmitter in atmel_stop_tx is a sane thing to do. Tested on at91sam9g35-cm(DMA) Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) Signed-off-by: Richard Genoud <richard.genoud@gmail.com> --- drivers/tty/serial/atmel_serial.c | 11 +++++++++++ 1 file changed, 11 insertions(+) NB: this is not for the 4.10 merge window, I'm just sending it now to have some comments if someone is againts it. diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c index 168b10cad47b..f9d42de5ab2d 100644 --- a/drivers/tty/serial/atmel_serial.c +++ b/drivers/tty/serial/atmel_serial.c @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) /* disable PDC transmit */ atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); } + + /* + * Disable the transmitter. + * This is mandatory when DMA is used, otherwise the DMA buffer + * is fully transmitted. + */ + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); + /* Disable interrupts */ atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) /* Enable interrupts */ atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); + + /* re-enable the transmitter */ + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); } /* ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx 2016-12-13 16:27 [PATCH] tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx Richard Genoud @ 2017-01-02 11:13 ` Nicolas Ferre 2017-01-02 14:16 ` Richard Genoud 2017-03-15 11:37 ` Nicolas Ferre 1 sibling, 1 reply; 12+ messages in thread From: Nicolas Ferre @ 2017-01-02 11:13 UTC (permalink / raw) To: linux-arm-kernel Le 13/12/2016 ? 17:27, Richard Genoud a ?crit : > If we don't disable the transmitter in atmel_stop_tx, the DMA buffer > continues to send data until it is emptied. > This cause problems with the flow control (CTS is asserted and data are > still sent). > > So, disabling the transmitter in atmel_stop_tx is a sane thing to do. > > Tested on at91sam9g35-cm(DMA) > Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) > > Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) > Signed-off-by: Richard Genoud <richard.genoud@gmail.com> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > drivers/tty/serial/atmel_serial.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > NB: this is not for the 4.10 merge window, I'm just sending it now to > have some comments if someone is againts it. > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index 168b10cad47b..f9d42de5ab2d 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) > /* disable PDC transmit */ > atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); > } > + > + /* > + * Disable the transmitter. > + * This is mandatory when DMA is used, otherwise the DMA buffer > + * is fully transmitted. > + */ > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); > + > /* Disable interrupts */ > atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); > > @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) > > /* Enable interrupts */ > atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); > + > + /* re-enable the transmitter */ > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); > } > > /* > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx 2017-01-02 11:13 ` Nicolas Ferre @ 2017-01-02 14:16 ` Richard Genoud 2017-01-11 7:09 ` Greg Kroah-Hartman 0 siblings, 1 reply; 12+ messages in thread From: Richard Genoud @ 2017-01-02 14:16 UTC (permalink / raw) To: linux-arm-kernel 2017-01-02 12:13 GMT+01:00 Nicolas Ferre <nicolas.ferre@atmel.com>: > Le 13/12/2016 ? 17:27, Richard Genoud a ?crit : >> If we don't disable the transmitter in atmel_stop_tx, the DMA buffer >> continues to send data until it is emptied. >> This cause problems with the flow control (CTS is asserted and data are >> still sent). >> >> So, disabling the transmitter in atmel_stop_tx is a sane thing to do. >> >> Tested on at91sam9g35-cm(DMA) >> Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) >> >> Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > >> --- >> drivers/tty/serial/atmel_serial.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> NB: this is not for the 4.10 merge window, I'm just sending it now to >> have some comments if someone is againts it. >> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c >> index 168b10cad47b..f9d42de5ab2d 100644 >> --- a/drivers/tty/serial/atmel_serial.c >> +++ b/drivers/tty/serial/atmel_serial.c >> @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) >> /* disable PDC transmit */ >> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); >> } >> + >> + /* >> + * Disable the transmitter. >> + * This is mandatory when DMA is used, otherwise the DMA buffer >> + * is fully transmitted. >> + */ >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); >> + >> /* Disable interrupts */ >> atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); >> >> @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) >> >> /* Enable interrupts */ >> atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); >> + >> + /* re-enable the transmitter */ >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); >> } >> >> /* >> > > > -- > Nicolas Ferre Greg, could you also take this patch in your tree ? Thanks ! Richard. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx 2017-01-02 14:16 ` Richard Genoud @ 2017-01-11 7:09 ` Greg Kroah-Hartman 0 siblings, 0 replies; 12+ messages in thread From: Greg Kroah-Hartman @ 2017-01-11 7:09 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 02, 2017 at 03:16:46PM +0100, Richard Genoud wrote: > 2017-01-02 12:13 GMT+01:00 Nicolas Ferre <nicolas.ferre@atmel.com>: > > Le 13/12/2016 ? 17:27, Richard Genoud a ?crit : > >> If we don't disable the transmitter in atmel_stop_tx, the DMA buffer > >> continues to send data until it is emptied. > >> This cause problems with the flow control (CTS is asserted and data are > >> still sent). > >> > >> So, disabling the transmitter in atmel_stop_tx is a sane thing to do. > >> > >> Tested on at91sam9g35-cm(DMA) > >> Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) > >> > >> Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) > >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > > > >> --- > >> drivers/tty/serial/atmel_serial.c | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> NB: this is not for the 4.10 merge window, I'm just sending it now to > >> have some comments if someone is againts it. > >> > >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > >> index 168b10cad47b..f9d42de5ab2d 100644 > >> --- a/drivers/tty/serial/atmel_serial.c > >> +++ b/drivers/tty/serial/atmel_serial.c > >> @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) > >> /* disable PDC transmit */ > >> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); > >> } > >> + > >> + /* > >> + * Disable the transmitter. > >> + * This is mandatory when DMA is used, otherwise the DMA buffer > >> + * is fully transmitted. > >> + */ > >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); > >> + > >> /* Disable interrupts */ > >> atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); > >> > >> @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) > >> > >> /* Enable interrupts */ > >> atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); > >> + > >> + /* re-enable the transmitter */ > >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); > >> } > >> > >> /* > >> > > > > > > -- > > Nicolas Ferre > > Greg, could you also take this patch in your tree ? Yup! ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx 2016-12-13 16:27 [PATCH] tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx Richard Genoud 2017-01-02 11:13 ` Nicolas Ferre @ 2017-03-15 11:37 ` Nicolas Ferre 2017-03-15 13:36 ` Richard Genoud 1 sibling, 1 reply; 12+ messages in thread From: Nicolas Ferre @ 2017-03-15 11:37 UTC (permalink / raw) To: linux-arm-kernel Le 13/12/2016 ? 17:27, Richard Genoud a ?crit : > If we don't disable the transmitter in atmel_stop_tx, the DMA buffer > continues to send data until it is emptied. > This cause problems with the flow control (CTS is asserted and data are > still sent). > > So, disabling the transmitter in atmel_stop_tx is a sane thing to do. > > Tested on at91sam9g35-cm(DMA) > Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) > > Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) > Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > --- > drivers/tty/serial/atmel_serial.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > NB: this is not for the 4.10 merge window, I'm just sending it now to > have some comments if someone is againts it. > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index 168b10cad47b..f9d42de5ab2d 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) > /* disable PDC transmit */ > atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); > } > + > + /* > + * Disable the transmitter. > + * This is mandatory when DMA is used, otherwise the DMA buffer > + * is fully transmitted. > + */ > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); > + > /* Disable interrupts */ > atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); > > @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) > > /* Enable interrupts */ > atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); > + > + /* re-enable the transmitter */ > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); > } > > /* Hi Richard, I've just discovered that I have some weird behavior with this patch. On current Linus' tree, with sama5d2 + DMA, I see some garbage characters coming out of the console when I try to stop my system (reboot/halt) [1]. Moreover, and I do understand that it's not the problem right here, when applied on our linux-4.4-at91 branch (our vendor tree actually), it hangs the boot process as it seems that a burst of open/close of the serial port happens while starting the rootfs. It's definitively my own problem, but it can bring light to what we are seeing on Mainline... I think that we may need to flush the DMA channel in this atmel_stop_tx() function. Best regards, [1] If you want to test, you need to apply this patch for eMMC BTW: https://patchwork.kernel.org/patch/9617489/ -- Nicolas Ferre ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx 2017-03-15 11:37 ` Nicolas Ferre @ 2017-03-15 13:36 ` Richard Genoud 2017-03-15 15:29 ` [RFC PATCH] tty/serial: atmel: fix TX path in atmel_console_write() Nicolas Ferre 0 siblings, 1 reply; 12+ messages in thread From: Richard Genoud @ 2017-03-15 13:36 UTC (permalink / raw) To: linux-arm-kernel 2017-03-15 12:37 GMT+01:00 Nicolas Ferre <nicolas.ferre@atmel.com>: > Le 13/12/2016 ? 17:27, Richard Genoud a ?crit : >> If we don't disable the transmitter in atmel_stop_tx, the DMA buffer >> continues to send data until it is emptied. >> This cause problems with the flow control (CTS is asserted and data are >> still sent). >> >> So, disabling the transmitter in atmel_stop_tx is a sane thing to do. >> >> Tested on at91sam9g35-cm(DMA) >> Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC) >> >> Cc: <stable@vger.kernel.org> (beware, this won't apply before 4.3) >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> >> --- >> drivers/tty/serial/atmel_serial.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> NB: this is not for the 4.10 merge window, I'm just sending it now to >> have some comments if someone is againts it. >> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c >> index 168b10cad47b..f9d42de5ab2d 100644 >> --- a/drivers/tty/serial/atmel_serial.c >> +++ b/drivers/tty/serial/atmel_serial.c >> @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port) >> /* disable PDC transmit */ >> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); >> } >> + >> + /* >> + * Disable the transmitter. >> + * This is mandatory when DMA is used, otherwise the DMA buffer >> + * is fully transmitted. >> + */ >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS); >> + >> /* Disable interrupts */ >> atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask); >> >> @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port) >> >> /* Enable interrupts */ >> atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask); >> + >> + /* re-enable the transmitter */ >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); >> } >> >> /* > > Hi Richard, Hi ! > > I've just discovered that I have some weird behavior with this patch. On > current Linus' tree, with sama5d2 + DMA, I see some garbage characters > coming out of the console when I try to stop my system (reboot/halt) [1]. Yes, I've also seen that on my board, it was on my todo list. (Although I didn't know this patch was the culprit !) > > Moreover, and I do understand that it's not the problem right here, when > applied on our linux-4.4-at91 branch (our vendor tree actually), it > hangs the boot process as it seems that a burst of open/close of the > serial port happens while starting the rootfs. It's definitively my own > problem, but it can bring light to what we are seeing on Mainline... a?e ! This is clearly something we'll have to understand ! > I think that we may need to flush the DMA channel in this > atmel_stop_tx() function. > yes, I'll look into that. > Best regards, > > [1] If you want to test, you need to apply this patch for eMMC BTW: > https://patchwork.kernel.org/patch/9617489/ > > -- > Nicolas Ferre Thanks ! Richard. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH] tty/serial: atmel: fix TX path in atmel_console_write() 2017-03-15 13:36 ` Richard Genoud @ 2017-03-15 15:29 ` Nicolas Ferre 2017-03-15 16:19 ` Richard Genoud 2017-03-20 10:33 ` Richard Genoud 0 siblings, 2 replies; 12+ messages in thread From: Nicolas Ferre @ 2017-03-15 15:29 UTC (permalink / raw) To: linux-arm-kernel A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx") is that the console can be called with TX path disabled. Then the system would hang trying to push charecters out in atmel_console_putchar(). Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx") Cc: stable <stable@vger.kernel.org> # 4.4+ --- Hi Richard, I found this to fix the problem with system hang in my linux-4.4-at91 branch (in the atmel_console_putchar() waiting loop actually). I'm open to more insignt. As we cannot figure out if this bit is set or not, I didn't preserve the current status... Regards, drivers/tty/serial/atmel_serial.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c index dcebb28ffbc4..7372dbdb7a4c 100644 --- a/drivers/tty/serial/atmel_serial.c +++ b/drivers/tty/serial/atmel_serial.c @@ -2483,6 +2483,9 @@ static void atmel_console_write(struct console *co, const char *s, u_int count) pdc_tx = atmel_uart_readl(port, ATMEL_PDC_PTSR) & ATMEL_PDC_TXTEN; atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); + /* Make sure that tx path is actually able to send characters */ + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); + uart_console_write(port, s, count, atmel_console_putchar); /* -- 2.9.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH] tty/serial: atmel: fix TX path in atmel_console_write() 2017-03-15 15:29 ` [RFC PATCH] tty/serial: atmel: fix TX path in atmel_console_write() Nicolas Ferre @ 2017-03-15 16:19 ` Richard Genoud 2017-03-15 16:56 ` Nicolas Ferre 2017-03-20 10:33 ` Richard Genoud 1 sibling, 1 reply; 12+ messages in thread From: Richard Genoud @ 2017-03-15 16:19 UTC (permalink / raw) To: linux-arm-kernel On 15/03/2017 16:29, Nicolas Ferre wrote: > A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA > from transmitting in stop_tx") is that the console can be called with > TX path disabled. Then the system would hang trying to push charecters > out in atmel_console_putchar(). > > Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> > Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting > in stop_tx") > Cc: stable <stable@vger.kernel.org> # 4.4+ > --- > Hi Richard, > > I found this to fix the problem with system hang in my linux-4.4-at91 branch > (in the atmel_console_putchar() waiting loop actually). I'm open to more > insignt. > As we cannot figure out if this bit is set or not, I didn't preserve the > current status... > > Regards, So, I'm guessing that you may see some lines/characters printed twice on the screen, don't you ? > drivers/tty/serial/atmel_serial.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index dcebb28ffbc4..7372dbdb7a4c 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -2483,6 +2483,9 @@ static void atmel_console_write(struct console *co, const char *s, u_int count) > pdc_tx = atmel_uart_readl(port, ATMEL_PDC_PTSR) & ATMEL_PDC_TXTEN; > atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); > > + /* Make sure that tx path is actually able to send characters */ > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); > + > uart_console_write(port, s, count, atmel_console_putchar); > > /* > Richard. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH] tty/serial: atmel: fix TX path in atmel_console_write() 2017-03-15 16:19 ` Richard Genoud @ 2017-03-15 16:56 ` Nicolas Ferre 2017-03-17 15:11 ` Richard Genoud 0 siblings, 1 reply; 12+ messages in thread From: Nicolas Ferre @ 2017-03-15 16:56 UTC (permalink / raw) To: linux-arm-kernel Le 15/03/2017 ? 17:19, Richard Genoud a ?crit : > On 15/03/2017 16:29, Nicolas Ferre wrote: >> A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA >> from transmitting in stop_tx") is that the console can be called with >> TX path disabled. Then the system would hang trying to push charecters >> out in atmel_console_putchar(). >> >> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> >> Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting >> in stop_tx") >> Cc: stable <stable@vger.kernel.org> # 4.4+ >> --- >> Hi Richard, >> >> I found this to fix the problem with system hang in my linux-4.4-at91 branch >> (in the atmel_console_putchar() waiting loop actually). I'm open to more >> insignt. >> As we cannot figure out if this bit is set or not, I didn't preserve the >> current status... >> >> Regards, > > So, I'm guessing that you may see some lines/characters printed twice on > the screen, don't you ? Well, actually, I don't think so because the repetitions that I see are probably due to open/close/open/close/re-open/... of the serial console itself. Same with the line "random: udevd: uninitialized urandom read (16 bytes read, 21 bits of entropy available)", they happen at different moment in time => the printk log timestamping seem to indicate that they are different. Here is my log: http://code.bulix.org/bok23n-123056 Regards, >> drivers/tty/serial/atmel_serial.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c >> index dcebb28ffbc4..7372dbdb7a4c 100644 >> --- a/drivers/tty/serial/atmel_serial.c >> +++ b/drivers/tty/serial/atmel_serial.c >> @@ -2483,6 +2483,9 @@ static void atmel_console_write(struct console *co, const char *s, u_int count) >> pdc_tx = atmel_uart_readl(port, ATMEL_PDC_PTSR) & ATMEL_PDC_TXTEN; >> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); >> >> + /* Make sure that tx path is actually able to send characters */ >> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); >> + >> uart_console_write(port, s, count, atmel_console_putchar); >> >> /* >> > > Richard. > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH] tty/serial: atmel: fix TX path in atmel_console_write() 2017-03-15 16:56 ` Nicolas Ferre @ 2017-03-17 15:11 ` Richard Genoud 2017-03-17 17:16 ` Nicolas Ferre 0 siblings, 1 reply; 12+ messages in thread From: Richard Genoud @ 2017-03-17 15:11 UTC (permalink / raw) To: linux-arm-kernel 2017-03-15 17:56 GMT+01:00 Nicolas Ferre <nicolas.ferre@microchip.com>: > Le 15/03/2017 ? 17:19, Richard Genoud a ?crit : >> On 15/03/2017 16:29, Nicolas Ferre wrote: >>> A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA >>> from transmitting in stop_tx") is that the console can be called with >>> TX path disabled. Then the system would hang trying to push charecters >>> out in atmel_console_putchar(). >>> >>> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> >>> Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting >>> in stop_tx") >>> Cc: stable <stable@vger.kernel.org> # 4.4+ >>> --- >>> Hi Richard, >>> >>> I found this to fix the problem with system hang in my linux-4.4-at91 branch >>> (in the atmel_console_putchar() waiting loop actually). I'm open to more >>> insignt. >>> As we cannot figure out if this bit is set or not, I didn't preserve the >>> current status... >>> >>> Regards, >> >> So, I'm guessing that you may see some lines/characters printed twice on >> the screen, don't you ? > > Well, actually, I don't think so because the repetitions that I see are > probably due to open/close/open/close/re-open/... of the serial console > itself. > > Same with the line "random: udevd: uninitialized urandom read (16 bytes > read, 21 bits of entropy available)", they happen at different moment in > time => the printk log timestamping seem to indicate that they are > different. Hi Nicolas, It seems that the problem is between atmel_tx_dma() and its callback atmel_complete_tx_dma(). At some point, atmel_tx_dma() is called, does the job, and then, just before the callback is called, the xmit->head and xmit->tail pointers are set to zero (by uart_flush_buffer()) So, when atmel_complete_tx_dma() is called, it does: xmit->tail += atmel_port->tx_len; not knowing that the head and tail pointers have been reseted. => it's like there's (UART_XMIT_SIZE - atmel_port->tx_len) characters to transmit on the serial line. PS: I can trigger this bug by holding down the d key at login and then ctrl - basically, a ctrl-d just after sending text - with a rate success of about 1/5 :) Could you try this patch to see if it corrects also your system hang ? (The patch is small, but the bug hunt was a headache :)) [PATCH] tty/serial: atmel: fix race condition (TX+DMA) If uart_flush_buffer() is called between atmel_tx_dma() and atmel_complete_tx_dma(), the circular buffer has been cleared, but not atmel_port->tx_len. That leads to a circular buffer overflow (dumping (UART_XMIT_SIZE - atmel_port->tx_len) bytes). Signed-off-by: Richard Genoud <richard.genoud@gmail.com> --- drivers/tty/serial/atmel_serial.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c index 833d3d80446f..89552157e334 100644 --- a/drivers/tty/serial/atmel_serial.c +++ b/drivers/tty/serial/atmel_serial.c @@ -1934,6 +1934,11 @@ static void atmel_flush_buffer(struct uart_port *port) atmel_uart_writel(port, ATMEL_PDC_TCR, 0); atmel_port->pdc_tx.ofs = 0; } + /* + * in uart_flush_buffer(), the xmit circular buffer has just + * been cleared, so we have to reset tx_len accordingly. + */ + atmel_port->tx_len = 0; } /* ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH] tty/serial: atmel: fix TX path in atmel_console_write() 2017-03-17 15:11 ` Richard Genoud @ 2017-03-17 17:16 ` Nicolas Ferre 0 siblings, 0 replies; 12+ messages in thread From: Nicolas Ferre @ 2017-03-17 17:16 UTC (permalink / raw) To: linux-arm-kernel Le 17/03/2017 ? 16:11, Richard Genoud a ?crit : > 2017-03-15 17:56 GMT+01:00 Nicolas Ferre <nicolas.ferre@microchip.com>: >> Le 15/03/2017 ? 17:19, Richard Genoud a ?crit : >>> On 15/03/2017 16:29, Nicolas Ferre wrote: >>>> A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA >>>> from transmitting in stop_tx") is that the console can be called with >>>> TX path disabled. Then the system would hang trying to push charecters >>>> out in atmel_console_putchar(). >>>> >>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> >>>> Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting >>>> in stop_tx") >>>> Cc: stable <stable@vger.kernel.org> # 4.4+ >>>> --- >>>> Hi Richard, >>>> >>>> I found this to fix the problem with system hang in my linux-4.4-at91 branch >>>> (in the atmel_console_putchar() waiting loop actually). I'm open to more >>>> insignt. >>>> As we cannot figure out if this bit is set or not, I didn't preserve the >>>> current status... >>>> >>>> Regards, >>> >>> So, I'm guessing that you may see some lines/characters printed twice on >>> the screen, don't you ? >> >> Well, actually, I don't think so because the repetitions that I see are >> probably due to open/close/open/close/re-open/... of the serial console >> itself. >> >> Same with the line "random: udevd: uninitialized urandom read (16 bytes >> read, 21 bits of entropy available)", they happen at different moment in >> time => the printk log timestamping seem to indicate that they are >> different. > Hi Nicolas, > > It seems that the problem is between atmel_tx_dma() and its callback > atmel_complete_tx_dma(). > > At some point, atmel_tx_dma() is called, does the job, and then, just > before the callback is called, the xmit->head and xmit->tail pointers > are set to zero (by uart_flush_buffer()) > So, when atmel_complete_tx_dma() is called, it does: > xmit->tail += atmel_port->tx_len; > not knowing that the head and tail pointers have been reseted. > => it's like there's (UART_XMIT_SIZE - atmel_port->tx_len) characters to > transmit on the serial line. > > PS: I can trigger this bug by holding down the d key at login and then > ctrl - basically, a ctrl-d just after sending text - with a rate success > of about 1/5 :) > > Could you try this patch to see if it corrects also your system hang ? Just tried, it doesn't fix the system hang. But it seems to solve the issue that I had while halting the system: a kind of flush of a previous buffer in the console. So, I think it does solve something. So, with both my patch and yours: Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com> Regards, > (The patch is small, but the bug hunt was a headache :)) > > [PATCH] tty/serial: atmel: fix race condition (TX+DMA) > > If uart_flush_buffer() is called between atmel_tx_dma() and > atmel_complete_tx_dma(), the circular buffer has been cleared, but not > atmel_port->tx_len. > That leads to a circular buffer overflow (dumping (UART_XMIT_SIZE - > atmel_port->tx_len) bytes). > > Signed-off-by: Richard Genoud <richard.genoud@gmail.com> > --- > drivers/tty/serial/atmel_serial.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/tty/serial/atmel_serial.c > b/drivers/tty/serial/atmel_serial.c > index 833d3d80446f..89552157e334 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -1934,6 +1934,11 @@ static void atmel_flush_buffer(struct uart_port > *port) > atmel_uart_writel(port, ATMEL_PDC_TCR, 0); > atmel_port->pdc_tx.ofs = 0; > } > + /* > + * in uart_flush_buffer(), the xmit circular buffer has just > + * been cleared, so we have to reset tx_len accordingly. > + */ > + atmel_port->tx_len = 0; > } > > /* > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH] tty/serial: atmel: fix TX path in atmel_console_write() 2017-03-15 15:29 ` [RFC PATCH] tty/serial: atmel: fix TX path in atmel_console_write() Nicolas Ferre 2017-03-15 16:19 ` Richard Genoud @ 2017-03-20 10:33 ` Richard Genoud 1 sibling, 0 replies; 12+ messages in thread From: Richard Genoud @ 2017-03-20 10:33 UTC (permalink / raw) To: linux-arm-kernel On 15/03/2017 16:29, Nicolas Ferre wrote: > A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA > from transmitting in stop_tx") is that the console can be called with > TX path disabled. Then the system would hang trying to push charecters > out in atmel_console_putchar(). > > Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> > Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting > in stop_tx") > Cc: stable <stable@vger.kernel.org> # 4.4+ > --- > Hi Richard, > > I found this to fix the problem with system hang in my linux-4.4-at91 branch > (in the atmel_console_putchar() waiting loop actually). I'm open to more > insignt. > As we cannot figure out if this bit is set or not, I didn't preserve the > current status... > > Regards, > > drivers/tty/serial/atmel_serial.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index dcebb28ffbc4..7372dbdb7a4c 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -2483,6 +2483,9 @@ static void atmel_console_write(struct console *co, const char *s, u_int count) > pdc_tx = atmel_uart_readl(port, ATMEL_PDC_PTSR) & ATMEL_PDC_TXTEN; > atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS); > > + /* Make sure that tx path is actually able to send characters */ > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN); > + > uart_console_write(port, s, count, atmel_console_putchar); > > /* > Acked-by: Richard Genoud <richard.genoud@gmail.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-03-20 10:33 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-13 16:27 [PATCH] tty/serial: atmel_serial: BUG: stop DMA from transmitting in stop_tx Richard Genoud 2017-01-02 11:13 ` Nicolas Ferre 2017-01-02 14:16 ` Richard Genoud 2017-01-11 7:09 ` Greg Kroah-Hartman 2017-03-15 11:37 ` Nicolas Ferre 2017-03-15 13:36 ` Richard Genoud 2017-03-15 15:29 ` [RFC PATCH] tty/serial: atmel: fix TX path in atmel_console_write() Nicolas Ferre 2017-03-15 16:19 ` Richard Genoud 2017-03-15 16:56 ` Nicolas Ferre 2017-03-17 15:11 ` Richard Genoud 2017-03-17 17:16 ` Nicolas Ferre 2017-03-20 10:33 ` Richard Genoud
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).