| Message ID | 20201209091728.2357-2-s.trumtrar@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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 |
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); + if ((lsr & BOTH_EMPTY) != BOTH_EMPTY) return;
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(-)