Hi!
Johan Hovold <johan@xxxxxxxxxx> 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@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.
>
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 |
