| Message ID | 20240519193109.122466-1-doug@schmorgal.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: pxa: Disable TX interrupt if there is no more data to transmit | expand |
On 19. 05. 24, 21:31, Doug Brown wrote: > If a TX interrupt occurs and no new data gets loaded into the TX FIFO, > the UART will never fire another TX interrupt until the UART_IER_THRI > flag is toggled off and back on. If nothing ever calls stop_tx(), this > effectively results in transmissions getting hung up until another > unrelated UART IRQ occurs, such as an RX interrupt. > > The driver used to do this correctly until the transition to > uart_port_tx_limited(). This didn't matter until the behavior of > __uart_port_tx changed in commit 7bfb915a597a ("serial: core: only stop > transmit when HW fifo is empty"). > > Fixes: d11cc8c3c4b6 ("tty: serial: use uart_port_tx_limited()") > Signed-off-by: Doug Brown <doug@schmorgal.com> > --- > > Note: I based this on v6.9 instead of tty-next since it's a fix; please > let me know if that was the wrong move and I would be happy to resubmit > it based on tty-next. The patch changes ever so slightly because of the > circ_buf -> kfifo transition. The only difference is it needs this > condition in the "if" statement instead: > > kfifo_is_empty(&up->port.state->port.xmit_fifo) > > This has been tested to work properly on tty-next as well. > > drivers/tty/serial/pxa.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c > index e395ff29c1a2..8abb85bee87c 100644 > --- a/drivers/tty/serial/pxa.c > +++ b/drivers/tty/serial/pxa.c > @@ -176,6 +176,14 @@ static void transmit_chars(struct uart_pxa_port *up) > { > u8 ch; > > + /* If there is nothing left to transmit, disable the TX interrupt. > + * Otherwise we can get stuck waiting for another IRQ that never happens. > + */ > + if (uart_circ_empty(&up->port.state->xmit)) { > + serial_pxa_stop_tx(&up->port); > + return; > + } This does not make sense. If the circ buf is empty, uart_port_tx_limited() should stop the TX already. You simply revert to the state before 7bfb915a597a, but on a per-driver basis. IOW all drivers using the helper would have exactly this issue after 7bfb915a597a. What driver was 7bfb915a597a about after all? The commit log of the commit is hopeless (it's very vague) in this respect: commit 7bfb915a597a301abb892f620fe5c283a9fdbd77 Author: Jonas Gorski <jonas.gorski@gmail.com> Date: Sun Mar 3 16:08:07 2024 +0100 serial: core: only stop transmit when HW fifo is empty If the circular buffer is empty, it just means we fit all characters to send into the HW fifo, but not that the hardware finished transmitting them. So if we immediately call stop_tx() after that, this may abort any pending characters in the HW fifo, and cause dropped characters on the console. Fix this by only stopping tx when the tx HW fifo is actually empty. Fixes: 8275b48b2780 ("tty: serial: introduce transmit helpers") (And it barely fixes 8275b48b2780 per se. 8275b48b2780 only moved the processing to a single place, most/all of the drivers already did it that way.) /me confused. Perhaps, it should be reverted and the affected driver should just pass UART_TX_NOSTOP instead? > uart_port_tx_limited(&up->port, ch, up->port.fifosize / 2, > true, > serial_out(up, UART_TX, ch), thanks,
Hi On Mon, 20 May 2024 at 08:41, Jiri Slaby <jirislaby@kernel.org> wrote: > > On 19. 05. 24, 21:31, Doug Brown wrote: > > If a TX interrupt occurs and no new data gets loaded into the TX FIFO, > > the UART will never fire another TX interrupt until the UART_IER_THRI > > flag is toggled off and back on. If nothing ever calls stop_tx(), this > > effectively results in transmissions getting hung up until another > > unrelated UART IRQ occurs, such as an RX interrupt. > > > > The driver used to do this correctly until the transition to > > uart_port_tx_limited(). This didn't matter until the behavior of > > __uart_port_tx changed in commit 7bfb915a597a ("serial: core: only stop > > transmit when HW fifo is empty"). > > > > Fixes: d11cc8c3c4b6 ("tty: serial: use uart_port_tx_limited()") > > Signed-off-by: Doug Brown <doug@schmorgal.com> > > --- > > > > Note: I based this on v6.9 instead of tty-next since it's a fix; please > > let me know if that was the wrong move and I would be happy to resubmit > > it based on tty-next. The patch changes ever so slightly because of the > > circ_buf -> kfifo transition. The only difference is it needs this > > condition in the "if" statement instead: > > > > kfifo_is_empty(&up->port.state->port.xmit_fifo) > > > > This has been tested to work properly on tty-next as well. > > > > drivers/tty/serial/pxa.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c > > index e395ff29c1a2..8abb85bee87c 100644 > > --- a/drivers/tty/serial/pxa.c > > +++ b/drivers/tty/serial/pxa.c > > @@ -176,6 +176,14 @@ static void transmit_chars(struct uart_pxa_port *up) > > { > > u8 ch; > > > > + /* If there is nothing left to transmit, disable the TX interrupt. > > + * Otherwise we can get stuck waiting for another IRQ that never happens. > > + */ > > + if (uart_circ_empty(&up->port.state->xmit)) { > > + serial_pxa_stop_tx(&up->port); > > + return; > > + } > > This does not make sense. If the circ buf is empty, > uart_port_tx_limited() should stop the TX already. You simply revert to > the state before 7bfb915a597a, but on a per-driver basis. > > IOW all drivers using the helper would have exactly this issue after > 7bfb915a597a. > > What driver was 7bfb915a597a about after all? The commit log of the > commit is hopeless (it's very vague) in this respect: > commit 7bfb915a597a301abb892f620fe5c283a9fdbd77 > Author: Jonas Gorski <jonas.gorski@gmail.com> > Date: Sun Mar 3 16:08:07 2024 +0100 > > serial: core: only stop transmit when HW fifo is empty > > If the circular buffer is empty, it just means we fit all characters to > send into the HW fifo, but not that the hardware finished transmitting > them. > > So if we immediately call stop_tx() after that, this may abort any > pending characters in the HW fifo, and cause dropped characters on the > console. > > Fix this by only stopping tx when the tx HW fifo is actually empty. > > Fixes: 8275b48b2780 ("tty: serial: introduce transmit helpers") > > > > (And it barely fixes 8275b48b2780 per se. 8275b48b2780 only moved the > processing to a single place, most/all of the drivers already did it > that way.) > > /me confused. > > Perhaps, it should be reverted and the affected driver should just pass > UART_TX_NOSTOP instead? > > > uart_port_tx_limited(&up->port, ch, up->port.fifosize / 2, > > true, > > serial_out(up, UART_TX, ch), > > thanks, Sorry, I should have added that this is for bcm63xx_uart.c. My reasoning for doing it this way and not via a flag was: 1. The kernel doc for uart_ops::stop_tx() says "The driver should stop transmitting characters as soon as possible." 2. bcm63xx_uart.c's stop_tx() does exactly that, by immediately stopping tx (leading to potentially lost output). 3. Therefore uart_port_tx_limited() is wrong by calling stop_tx() while the transmitter is still busy, and the core issue is this. So adding the check for the hw fifo being empty seemed to me as the "right" fix, fixing it for all drivers that abort tx in stop_tx(). Best Regards, Jonas
Hi Jonas and Jiri, On 5/20/2024 2:30 AM, Jonas Gorski wrote: > Hi > > On Mon, 20 May 2024 at 08:41, Jiri Slaby <jirislaby@kernel.org> wrote: >> >> This does not make sense. If the circ buf is empty, >> uart_port_tx_limited() should stop the TX already. You simply revert to >> the state before 7bfb915a597a, but on a per-driver basis. >> >> IOW all drivers using the helper would have exactly this issue after >> 7bfb915a597a. I tested more, and it appears you are correct, Jiri. I think 7bfb915a597a has broken other drivers too. For some background on this, after upgrading to 6.9 I observed that transmits would stop working on my PXA168-based device with the pxa driver until I caused another UART IRQ to fire, for example by typing a character in the console. I bisected the problem to 7bfb915a597a. I don't have a bunch of hardware to test with, but I cobbled together a way to emulate a BeagleBoard in an old Linaro QEMU branch, and the omap- serial driver also appears to be affected. Shortly after boot, the UART just completely hangs and all transmits stop working. I can't even type a character to recover it like I can with the pxa driver. Reverting 7bfb915a597a fixes the issue. I think the reason the pxa driver recovers when I type a character is because it always checks the THRE bit of the LSR in the IRQ handler, even if the IIR doesn't say there's a TX interrupt ready. (This appears to be a workaround for an erratum in the PXA serial IP...) >> Perhaps, it should be reverted and the affected driver should just pass >> UART_TX_NOSTOP instead? >> >>> uart_port_tx_limited(&up->port, ch, up->port.fifosize / 2, >>> true, >>> serial_out(up, UART_TX, ch), I can't speak for whether that would fix the issue Jonas originally saw, but it does look like it needs to be reverted because other drivers are also broken due to this change. > 1. The kernel doc for uart_ops::stop_tx() says "The driver should stop > transmitting characters as soon as possible." Very interesting. I guess the correct behavior depends on the exact interpretation of "stop transmitting characters as soon as possible". :-) It looks like, at least for the existing 16550-esque drivers that are using the helper, they are treating stop_tx() more as a signal to disable the TX interrupt, but still allow any characters already in the FIFO to go out. All I know is, in the PXA UART, I was observing the TX IRQ firing when we had no data to load in the FIFO, and we weren't disabling the interrupt. This causes a 16550A-style UART to get hung up because the TX interrupt never fires again unless you disable it and re-enable it. Doug
diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c index e395ff29c1a2..8abb85bee87c 100644 --- a/drivers/tty/serial/pxa.c +++ b/drivers/tty/serial/pxa.c @@ -176,6 +176,14 @@ static void transmit_chars(struct uart_pxa_port *up) { u8 ch; + /* If there is nothing left to transmit, disable the TX interrupt. + * Otherwise we can get stuck waiting for another IRQ that never happens. + */ + if (uart_circ_empty(&up->port.state->xmit)) { + serial_pxa_stop_tx(&up->port); + return; + } + uart_port_tx_limited(&up->port, ch, up->port.fifosize / 2, true, serial_out(up, UART_TX, ch),
If a TX interrupt occurs and no new data gets loaded into the TX FIFO, the UART will never fire another TX interrupt until the UART_IER_THRI flag is toggled off and back on. If nothing ever calls stop_tx(), this effectively results in transmissions getting hung up until another unrelated UART IRQ occurs, such as an RX interrupt. The driver used to do this correctly until the transition to uart_port_tx_limited(). This didn't matter until the behavior of __uart_port_tx changed in commit 7bfb915a597a ("serial: core: only stop transmit when HW fifo is empty"). Fixes: d11cc8c3c4b6 ("tty: serial: use uart_port_tx_limited()") Signed-off-by: Doug Brown <doug@schmorgal.com> --- Note: I based this on v6.9 instead of tty-next since it's a fix; please let me know if that was the wrong move and I would be happy to resubmit it based on tty-next. The patch changes ever so slightly because of the circ_buf -> kfifo transition. The only difference is it needs this condition in the "if" statement instead: kfifo_is_empty(&up->port.state->port.xmit_fifo) This has been tested to work properly on tty-next as well. drivers/tty/serial/pxa.c | 8 ++++++++ 1 file changed, 8 insertions(+)