* [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock @ 2020-12-09 9:17 Steffen Trumtrar 2020-12-09 9:17 ` [PATCH 2/2] tty: serial: 8250: wait till transmitter is empty Steffen Trumtrar 2020-12-09 11:23 ` [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock Johan Hovold 0 siblings, 2 replies; 7+ messages in thread From: Steffen Trumtrar @ 2020-12-09 9:17 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-serial In most cases serial8250_tx_chars is called with spinlock held. Fix the remaining location, too. Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> --- drivers/tty/serial/8250/8250_port.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index b0af13074cd3..3310c2b70138 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -1559,6 +1559,7 @@ static void serial8250_stop_tx(struct uart_port *port) static inline void __start_tx(struct uart_port *port) { struct uart_8250_port *up = up_to_u8250p(port); + unsigned long flags; if (up->dma && !up->dma->tx_dma(up)) return; @@ -1569,8 +1570,11 @@ static inline void __start_tx(struct uart_port *port) lsr = serial_in(up, UART_LSR); up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS; - if (lsr & UART_LSR_THRE) + if (lsr & UART_LSR_THRE) { + spin_lock_irqsave(&port->lock, flags); serial8250_tx_chars(up); + spin_unlock_irqrestore(&port->lock, flags); + } } } -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] tty: serial: 8250: wait till transmitter is empty 2020-12-09 9:17 [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock Steffen Trumtrar @ 2020-12-09 9:17 ` Steffen Trumtrar 2020-12-09 11:26 ` Johan Hovold 2020-12-09 11:23 ` [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock Johan Hovold 1 sibling, 1 reply; 7+ messages in thread From: Steffen Trumtrar @ 2020-12-09 9:17 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-serial 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); + if ((lsr & BOTH_EMPTY) != BOTH_EMPTY) return; -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] tty: serial: 8250: wait till transmitter is empty 2020-12-09 9:17 ` [PATCH 2/2] tty: serial: 8250: wait till transmitter is empty Steffen Trumtrar @ 2020-12-09 11:26 ` Johan Hovold 2021-01-04 11:47 ` Steffen Trumtrar 0 siblings, 1 reply; 7+ messages in thread From: Johan Hovold @ 2020-12-09 11:26 UTC (permalink / raw) To: Steffen Trumtrar; +Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] tty: serial: 8250: wait till transmitter is empty 2020-12-09 11:26 ` Johan Hovold @ 2021-01-04 11:47 ` Steffen Trumtrar 0 siblings, 0 replies; 7+ messages in thread From: Steffen Trumtrar @ 2021-01-04 11:47 UTC (permalink / raw) To: Johan Hovold; +Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial 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 | ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock 2020-12-09 9:17 [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock Steffen Trumtrar 2020-12-09 9:17 ` [PATCH 2/2] tty: serial: 8250: wait till transmitter is empty Steffen Trumtrar @ 2020-12-09 11:23 ` Johan Hovold 2021-01-04 11:42 ` Steffen Trumtrar [not found] ` <87ft3hdj3n.fsf@pengutronix.de> 1 sibling, 2 replies; 7+ messages in thread From: Johan Hovold @ 2020-12-09 11:23 UTC (permalink / raw) To: Steffen Trumtrar; +Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial On Wed, Dec 09, 2020 at 10:17:27AM +0100, Steffen Trumtrar wrote: > In most cases serial8250_tx_chars is called with spinlock held. > Fix the remaining location, too. Please explain where __start_tx() is called without holding the port lock and consider fixing that up. > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> > --- > drivers/tty/serial/8250/8250_port.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index b0af13074cd3..3310c2b70138 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -1559,6 +1559,7 @@ static void serial8250_stop_tx(struct uart_port *port) > static inline void __start_tx(struct uart_port *port) > { > struct uart_8250_port *up = up_to_u8250p(port); > + unsigned long flags; > > if (up->dma && !up->dma->tx_dma(up)) > return; > @@ -1569,8 +1570,11 @@ static inline void __start_tx(struct uart_port *port) > > lsr = serial_in(up, UART_LSR); > up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS; > - if (lsr & UART_LSR_THRE) > + if (lsr & UART_LSR_THRE) { > + spin_lock_irqsave(&port->lock, flags); > serial8250_tx_chars(up); > + spin_unlock_irqrestore(&port->lock, flags); > + } Since several callers of __start_tx() do hold the lock, this change will introduce a deadlock. > } > } Johan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock 2020-12-09 11:23 ` [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock Johan Hovold @ 2021-01-04 11:42 ` Steffen Trumtrar [not found] ` <87ft3hdj3n.fsf@pengutronix.de> 1 sibling, 0 replies; 7+ messages in thread From: Steffen Trumtrar @ 2021-01-04 11:42 UTC (permalink / raw) To: Johan Hovold; +Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial Hi! Johan Hovold <johan@kernel.org> writes: > On Wed, Dec 09, 2020 at 10:17:27AM +0100, Steffen Trumtrar wrote: >> In most cases serial8250_tx_chars is called with spinlock held. >> Fix the remaining location, too. > > Please explain where __start_tx() is called without holding the port > lock and consider fixing that up. > >> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> >> --- >> drivers/tty/serial/8250/8250_port.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c >> index b0af13074cd3..3310c2b70138 100644 >> --- a/drivers/tty/serial/8250/8250_port.c >> +++ b/drivers/tty/serial/8250/8250_port.c >> @@ -1559,6 +1559,7 @@ static void serial8250_stop_tx(struct uart_port *port) >> static inline void __start_tx(struct uart_port *port) >> { >> struct uart_8250_port *up = up_to_u8250p(port); >> + unsigned long flags; >> >> if (up->dma && !up->dma->tx_dma(up)) >> return; >> @@ -1569,8 +1570,11 @@ static inline void __start_tx(struct uart_port *port) >> >> lsr = serial_in(up, UART_LSR); >> up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS; >> - if (lsr & UART_LSR_THRE) >> + if (lsr & UART_LSR_THRE) { >> + spin_lock_irqsave(&port->lock, flags); >> serial8250_tx_chars(up); >> + spin_unlock_irqrestore(&port->lock, flags); >> + } > > Since several callers of __start_tx() do hold the lock, this change will > introduce a deadlock. > Meh, yeah :( Seem like the correct solution would be to fix start_tx_rs485 and serial8250_start_tx instead. 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 | ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <87ft3hdj3n.fsf@pengutronix.de>]
* Re: [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock [not found] ` <87ft3hdj3n.fsf@pengutronix.de> @ 2021-01-05 7:33 ` Steffen Trumtrar 0 siblings, 0 replies; 7+ messages in thread From: Steffen Trumtrar @ 2021-01-05 7:33 UTC (permalink / raw) To: Johan Hovold; +Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial Steffen Trumtrar <s.trumtrar@pengutronix.de> writes: > Hi! > > Johan Hovold <johan@kernel.org> writes: > >> On Wed, Dec 09, 2020 at 10:17:27AM +0100, Steffen Trumtrar wrote: >>> In most cases serial8250_tx_chars is called with spinlock held. >>> Fix the remaining location, too. >> >> Please explain where __start_tx() is called without holding the port >> lock and consider fixing that up. >> >>> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> >>> --- >>> drivers/tty/serial/8250/8250_port.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c >>> index b0af13074cd3..3310c2b70138 100644 >>> --- a/drivers/tty/serial/8250/8250_port.c >>> +++ b/drivers/tty/serial/8250/8250_port.c >>> @@ -1559,6 +1559,7 @@ static void serial8250_stop_tx(struct uart_port *port) >>> static inline void __start_tx(struct uart_port *port) >>> { >>> struct uart_8250_port *up = up_to_u8250p(port); >>> + unsigned long flags; >>> >>> if (up->dma && !up->dma->tx_dma(up)) >>> return; >>> @@ -1569,8 +1570,11 @@ static inline void __start_tx(struct uart_port *port) >>> >>> lsr = serial_in(up, UART_LSR); >>> up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS; >>> - if (lsr & UART_LSR_THRE) >>> + if (lsr & UART_LSR_THRE) { >>> + spin_lock_irqsave(&port->lock, flags); >>> serial8250_tx_chars(up); >>> + spin_unlock_irqrestore(&port->lock, flags); >>> + } >> >> Since several callers of __start_tx() do hold the lock, this change will >> introduce a deadlock. >> > > Meh, yeah :( > > Seem like the correct solution would be to fix start_tx_rs485 and > serial8250_start_tx instead. > Scratch that. This patch is bogus, I'm stupid, port-lock is already held by uart_write apart from the hrtimer-callback. Sorry for the noise. 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 | ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-05 7:34 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-09 9:17 [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock Steffen Trumtrar
2020-12-09 9:17 ` [PATCH 2/2] tty: serial: 8250: wait till transmitter is empty Steffen Trumtrar
2020-12-09 11:26 ` Johan Hovold
2021-01-04 11:47 ` Steffen Trumtrar
2020-12-09 11:23 ` [PATCH 1/2] tty: serial: 8250: always call tx_chars under spinlock Johan Hovold
2021-01-04 11:42 ` Steffen Trumtrar
[not found] ` <87ft3hdj3n.fsf@pengutronix.de>
2021-01-05 7:33 ` Steffen Trumtrar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).