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@xxxxxxxxxxxxxx>
> ---
> 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
