RE: [PATCH v2 2/2] serial: 8250: Fix serial8250_tx_empty() race with DMA Tx

[Ilpo Järvinen]

On Sat, 18 Mar 2023, David Laight wrote:

> From: Ilpo Järvinen
> > Sent: 17 March 2023 11:33
> > To: linux-serial@xxxxxxxxxxxxxxx; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jiri Slaby
> > 
> > There's a potential race before THRE/TEMT deasserts when DMA Tx is
> > starting up (or the next batch of continuous Tx is being submitted).
> > This can lead to misdetecting Tx empty condition.
> > 
> > It is entirely normal for THRE/TEMT to be set for some time after the
> > DMA Tx had been setup in serial8250_tx_dma(). As Tx side is definitely
> > not empty at that point, it seems incorrect for serial8250_tx_empty()
> > claim Tx is empty.
> > 
> > Fix the race by also checking in serial8250_tx_empty() whether there's
> > DMA Tx active.
> > 
> > Note: This fix only addresses in-kernel race mainly to make using
> > TCSADRAIN/FLUSH robust. Userspace can still cause other races but they
> > seem userspace concurrency control problems.
> 
> Looks better, but I'm not sure it actually works.
> 
> If interrupts are being used to copy data to the tx fifo then
> (depending on interrupt latency and exactly when the interrupt
> is requested) the code might report 'tx empty' when the ISR
> is about to copy in more data.
>
> Now the drain/flush code might already have checked there is
> no more data queued in the driver before calling this,

Thanks for taking a look, it's really appreciated.

Yes, set_termios() checks for tty_chars_in_buffer() which calls into 
serial_core's ->chars_in_buffer(). This does check 
uart_circ_chars_pending() so it's not possible to have such chars in the 
circular buffer in the drain/flush case.

> but more generally shouldn't it be checking:
> 	no_data_queued_in_driver && hardware_fifo_empty.
> 
> Any 'no_data_queued_in_driver' check would probably include
> data that dma is copying - so the explicit dma check might
> not be needed.

What for you'd want this change? Refactor the code? uart_get_lsr_info() 
already does check for uart_circ_chars_pending() so what you'd want more?

I suppose uart_get_lsr_info() should hold port's lock across the checks 
though, having that wishful comment about a racing interrupt messing 
things up doesn't really help that much :-).

Also, now that I looked into uart_get_lsr_info() I guess 
uart_chars_in_buffer() should also consider x_char like 
uart_get_lsr_info() does.


-- 
 i.

> > Fixes: 9ee4b83e51f74 ("serial: 8250: Add support for dmaengine")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/tty/serial/8250/8250.h      | 12 ++++++++++++
> >  drivers/tty/serial/8250/8250_port.c |  7 ++++---
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > index 287153d32536..1e8fe44a7099 100644
> > --- a/drivers/tty/serial/8250/8250.h
> > +++ b/drivers/tty/serial/8250/8250.h
> > @@ -365,6 +365,13 @@ static inline void serial8250_do_prepare_rx_dma(struct uart_8250_port *p)
> >  	if (dma->prepare_rx_dma)
> >  		dma->prepare_rx_dma(p);
> >  }
> > +
> > +static inline bool serial8250_tx_dma_running(struct uart_8250_port *p)
> > +{
> > +	struct uart_8250_dma *dma = p->dma;
> > +
> > +	return dma && dma->tx_running;
> > +}
> >  #else
> >  static inline int serial8250_tx_dma(struct uart_8250_port *p)
> >  {
> > @@ -380,6 +387,11 @@ static inline int serial8250_request_dma(struct uart_8250_port *p)
> >  	return -1;
> >  }
> >  static inline void serial8250_release_dma(struct uart_8250_port *p) { }
> > +
> > +static inline bool serial8250_tx_dma_running(struct uart_8250_port *p)
> > +{
> > +	return false;
> > +}
> >  #endif
> > 
> >  static inline int ns16550a_goto_highspeed(struct uart_8250_port *up)
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index fa43df05342b..107bcdfb119c 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -2005,18 +2005,19 @@ static int serial8250_tx_threshold_handle_irq(struct uart_port *port)
> >  static unsigned int serial8250_tx_empty(struct uart_port *port)
> >  {
> >  	struct uart_8250_port *up = up_to_u8250p(port);
> > +	unsigned int result = 0;
> >  	unsigned long flags;
> > -	u16 lsr;
> > 
> >  	serial8250_rpm_get(up);
> > 
> >  	spin_lock_irqsave(&port->lock, flags);
> > -	lsr = serial_lsr_in(up);
> > +	if (!serial8250_tx_dma_running(up) && uart_lsr_tx_empty(serial_lsr_in(up)))
> > +		result = TIOCSER_TEMT;
> >  	spin_unlock_irqrestore(&port->lock, flags);
> > 
> >  	serial8250_rpm_put(up);
> > 
> > -	return uart_lsr_tx_empty(lsr) ? TIOCSER_TEMT : 0;
> > +	return result;
> >  }
> > 
> >  unsigned int serial8250_do_get_mctrl(struct uart_port *port)
> > --
> > 2.30.2
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>