Re: [PATCH 2/2] tty: serial: 8250: wait till transmitter is empty

[Johan Hovold <johan@kernel.org>]

On Wed, Dec 09, 2020 at 10:17:28AM +0100, Steffen Trumtrar wrote:
> When only one single character is sent and RS485 signaling is used,
> the driver runs into timing issues.
> 
> When serial8250_tx_chars is called the single character is transmitted.
> The check on uart_circ_empty will be positive and __stop_tx is called.
> The check on UART_LSR_TEMT in BOTH_EMPTY will then be negativ and the
> function will return. On the next call to serial8250_tx_chars
> uart_circ_empty will still be true but the check on BOTH_EMPTY in
> __stop_tx might still fail. This leads to a deadlock.
> 
> Use readx_poll_timeout_atomic to allow the shift register to be emptied
> before checking on BOTH_EMPTY.
> 
> The timeout value is copied from 8250_dw.c.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/tty/serial/8250/8250_port.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 3310c2b70138..87daf3758ff0 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -18,6 +18,7 @@
>  #include <linux/console.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/sysrq.h>
> +#include <linux/iopoll.h>
>  #include <linux/delay.h>
>  #include <linux/platform_device.h>
>  #include <linux/tty.h>
> @@ -1519,18 +1520,27 @@ static inline void __do_stop_tx(struct uart_8250_port *p)
>  		serial8250_rpm_put_tx(p);
>  }
>  
> +static unsigned char serial8250_read_lsr(struct uart_8250_port *p)
> +{
> +	return serial_in(p, UART_LSR);
> +}
> +
>  static inline void __stop_tx(struct uart_8250_port *p)
>  {
>  	struct uart_8250_em485 *em485 = p->em485;
>  
>  	if (em485) {
> -		unsigned char lsr = serial_in(p, UART_LSR);
> +		unsigned char lsr;
> +
>  		/*
>  		 * To provide required timeing and allow FIFO transfer,
>  		 * __stop_tx_rs485() must be called only when both FIFO and
>  		 * shift register are empty. It is for device driver to enable
>  		 * interrupt on TEMT.
>  		 */
> +		readx_poll_timeout_atomic(serial8250_read_lsr, p, lsr,
> +					  lsr & UART_LSR_TEMT, 1, 20000);

Tight polling (1 us) for 20 ms with interrupts disabled?!

Without having looked at the details, there's got to be a better way to
handle this.

> +
>  		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>  			return;

Johan