Hi
On Mon, 20 May 2024 at 08:41, Jiri Slaby <jirislaby@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxx>
> 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
