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

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

Hi!

Johan Hovold <johan@kernel.org> writes:

> 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.
>

I'm sure there is. I just "copied" from 8250_dw.c O:-)


Best regards,
Steffen Trumtrar

--
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |