serial: sysrq cleanup and stm32 fixes
Subsystems (5)
cover serial: sysrq cleanup and stm32 fixes
The first patch cleans up the interrupt handlers that rely on deferred sysrq processing by not needlessly saving the interrupt state.
The second fixes the threaded interrupt handling of the stm32 driver and properly fixes a couple of deadlocks that were incidentally worked around by a recent commit.
The third patch cleans up the stm32 console implementation by switching to deferred sysrq processing, thereby making the console code more robust and allowing it to be used with PREEMPT_RT.
This series is against tty-next and and have only been compile tested.
Johan
Johan Hovold (3):
serial: do not restore interrupt state in sysrq helper
serial: stm32: fix threaded interrupt handling
serial: stm32: defer sysrq processing
drivers/tty/serial/8250/8250_aspeed_vuart.c | 5 ++- drivers/tty/serial/8250/8250_fsl.c | 11 ++++--- drivers/tty/serial/8250/8250_omap.c | 6 ++-- drivers/tty/serial/8250/8250_port.c | 6 ++-- drivers/tty/serial/qcom_geni_serial.c | 6 ++-- drivers/tty/serial/stm32-usart.c | 36 ++++++++++----------- include/linux/serial_core.h | 10 +++--- 7 files changed, 39 insertions(+), 41 deletions(-)
Signature · 1 line
2.26.3
PATCH 1/3 serial: do not restore interrupt state in sysrq helper
The uart_unlock_and_check_sysrq() helper can be used to defer processing of sysrq until the interrupt handler has released the port lock and is about to return.
Since commit 81e2073c175b ("genirq: Disable interrupts for force threaded handlers") interrupt handlers that are not explicitly requested as threaded are always called with interrupts disabled and there is no need to save the interrupt state when taking the port lock.
Instead of adding another sysrq helper for when the interrupt state has not needlessly been saved, drop the state parameter from uart_unlock_and_check_sysrq() and update its callers to no longer explicitly disable interrupts in their interrupt handlers.
--- drivers/tty/serial/8250/8250_aspeed_vuart.c | 5 ++--- drivers/tty/serial/8250/8250_fsl.c | 11 ++++++----- drivers/tty/serial/8250/8250_omap.c | 6 +++--- drivers/tty/serial/8250/8250_port.c | 6 +++--- drivers/tty/serial/qcom_geni_serial.c | 6 +++--- include/linux/serial_core.h | 10 +++++----- 6 files changed, 22 insertions(+), 22 deletions(-)
drivers/tty/serial/8250/8250_aspeed_vuart.c +2 -3
@@ -320,7 +320,6 @@ static int aspeed_vuart_handle_irq(struct uart_port *port) {struct uart_8250_port *up =up_to_u8250p (port);unsigned int iir, lsr; -unsigned long flags;int space, count; iir =serial_port_in (port,UART_IIR );
@@ -328,7 +327,7 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)if (iir &UART_IIR_NO_INT )return 0 ; -spin_lock_irqsave (&port->lock, flags); +spin_lock (&port->lock); lsr =serial_port_in (port,UART_LSR );
@@ -364,7 +363,7 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)if (lsr &UART_LSR_THRE )serial8250_tx_chars (up); -uart_unlock_and_check_sysrq (port, flags); +uart_unlock_and_check_sysrq (port);return 1 ; }
drivers/tty/serial/8250/8250_fsl.c +6 -5
@@ -30,15 +30,14 @@ struct fsl8250_data {int fsl8250_handle_irq (struct uart_port *port) {unsigned char lsr, orig_lsr; -unsigned long flags;unsigned int iir;struct uart_8250_port *up =up_to_u8250p (port); -spin_lock_irqsave (&up->port.lock, flags); +spin_lock (&up->port.lock); iir = port->serial_in (port,UART_IIR );if (iir &UART_IIR_NO_INT ) { -spin_unlock_irqrestore (&up->port.lock, flags); +spin_unlock (&up->port.lock);return 0 ; }
@@ -46,7 +45,7 @@ int fsl8250_handle_irq(struct uart_port *port)if (unlikely (up->lsr_saved_flags &UART_LSR_BI )) { up->lsr_saved_flags &= ~UART_LSR_BI ; port->serial_in (port,UART_RX ); -spin_unlock_irqrestore (&up->port.lock, flags); +spin_unlock (&up->port.lock);return 1 ; }
@@ -82,7 +81,9 @@ int fsl8250_handle_irq(struct uart_port *port)serial8250_tx_chars (up); up->lsr_saved_flags = orig_lsr; -uart_unlock_and_check_sysrq (&up->port, flags); + +uart_unlock_and_check_sysrq (&up->port); +return 1 ; }EXPORT_SYMBOL_GPL (fsl8250_handle_irq);
drivers/tty/serial/8250/8250_omap.c +3 -3
@@ -1143,7 +1143,6 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)struct uart_8250_port *up =up_to_u8250p (port);struct omap8250_priv *priv = up->port.private_data;unsigned char status; -unsigned long flags;u8 iir;serial8250_rpm_get (up);
@@ -1154,7 +1153,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)return IRQ_HANDLED ; } -spin_lock_irqsave (&port->lock, flags); +spin_lock (&port->lock); status =serial_port_in (port,UART_LSR );
@@ -1179,7 +1178,8 @@ static int omap_8250_dma_handle_irq(struct uart_port *port) } } -uart_unlock_and_check_sysrq (port, flags); +uart_unlock_and_check_sysrq (port); +serial8250_rpm_put (up);return 1 ; }
drivers/tty/serial/8250/8250_port.c +3 -3
@@ -1879,14 +1879,13 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)int serial8250_handle_irq (struct uart_port *port,unsigned int iir) {unsigned char status; -unsigned long flags;struct uart_8250_port *up =up_to_u8250p (port);bool skip_rx = false;if (iir &UART_IIR_NO_INT )return 0 ; -spin_lock_irqsave (&port->lock, flags); +spin_lock (&port->lock); status =serial_port_in (port,UART_LSR );
@@ -1912,7 +1911,8 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir) (up->ier & UART_IER_THRI))serial8250_tx_chars (up); -uart_unlock_and_check_sysrq (port, flags); +uart_unlock_and_check_sysrq (port); +return 1 ; }EXPORT_SYMBOL_GPL (serial8250_handle_irq);
drivers/tty/serial/qcom_geni_serial.c +3 -3
@@ -818,7 +818,6 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)u32 s_irq_status;u32 geni_status;struct uart_port *uport = dev; -unsigned long flags;bool drop_rx = false;struct tty_port *tport = &uport->state->port;struct qcom_geni_serial_port *port =to_dev_port (uport, uport);
@@ -826,7 +825,8 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)if (uport->suspended)return IRQ_NONE ; -spin_lock_irqsave (&uport->lock, flags); +spin_lock (&uport->lock); + m_irq_status =readl (uport->membase +SE_GENI_M_IRQ_STATUS ); s_irq_status =readl (uport->membase +SE_GENI_S_IRQ_STATUS ); geni_status =readl (uport->membase +SE_GENI_STATUS );
@@ -861,7 +861,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)qcom_geni_serial_handle_rx (uport, drop_rx); out_unlock: -uart_unlock_and_check_sysrq (uport, flags); +uart_unlock_and_check_sysrq (uport);return IRQ_HANDLED ; }
include/linux/serial_core.h +5 -5
@@ -500,19 +500,19 @@ static inline int uart_prepare_sysrq_char(struct uart_port *port, unsigned int creturn 0 ; } -static inline void uart_unlock_and_check_sysrq (struct uart_port *port,unsigned long irqflags) +static inline void uart_unlock_and_check_sysrq (struct uart_port *port) {int sysrq_ch;if (!port->has_sysrq) { -spin_unlock_irqrestore (&port->lock, irqflags); +spin_unlock (&port->lock);return ; } sysrq_ch = port->sysrq_ch; port->sysrq_ch =0 ; -spin_unlock_irqrestore (&port->lock, irqflags); +spin_unlock (&port->lock);if (sysrq_ch)handle_sysrq (sysrq_ch);
@@ -526,9 +526,9 @@ static inline int uart_prepare_sysrq_char(struct uart_port *port, unsigned int c {return 0 ; } -static inline void uart_unlock_and_check_sysrq (struct uart_port *port,unsigned long irqflags) +static inline void uart_unlock_and_check_sysrq (struct uart_port *port) { -spin_unlock_irqrestore (&port->lock, irqflags); +spin_unlock (&port->lock); }#endif /* CONFIG_MAGIC_SYSRQ_SERIAL */
Signature · 1 line
2.26.3
PATCH 2/3 serial: stm32: fix threaded interrupt handling
In reply to Johan HovoldWhen DMA is enabled the receive handler runs in a threaded handler, but the primary handler up until very recently neither disabled interrupts in the device or used IRQF_ONESHOT. This would lead to a deadlock if an interrupt comes in while the threaded receive handler is running under the port lock.
Commit ad7676812437 ("serial: stm32: fix a deadlock condition with wakeup event") claimed to fix an unrelated deadlock, but unfortunately also disabled interrupts in the threaded handler. While this prevents the deadlock mentioned in the previous paragraph it also defeats the purpose of using a threaded handler in the first place.
Fix this by making the interrupt one-shot and not disabling interrupts in the threaded handler.
Note that (receive) DMA must not be used for a console port as the threaded handler could be interrupted while holding the port lock, something which could lead to a deadlock in case an interrupt handler ends up calling printk.
--- drivers/tty/serial/stm32-usart.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
drivers/tty/serial/stm32-usart.c +12 -10
@@ -214,14 +214,11 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)struct tty_port *tport = &port->state->port;struct stm32_port *stm32_port =to_stm32_port (port);const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs; -unsigned long c, flags; +unsigned long c;u32 sr;char flag; -if (threaded) -spin_lock_irqsave (&port->lock, flags); -else -spin_lock (&port->lock); +spin_lock (&port->lock);while (stm32_usart_pending_rx (port, &sr, &stm32_port->last_res, threaded)) {
@@ -278,10 +275,7 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)uart_insert_char (port, sr,USART_SR_ORE , c, flag); } -if (threaded) -spin_unlock_irqrestore (&port->lock, flags); -else -spin_unlock (&port->lock); +spin_unlock (&port->lock);tty_flip_buffer_push (tport); }
@@ -667,7 +661,8 @@ static int stm32_usart_startup(struct uart_port *port) ret =request_threaded_irq (port->irq, stm32_usart_interrupt, stm32_usart_threaded_interrupt, -IRQF_NO_SUSPEND , name, port); +IRQF_ONESHOT |IRQF_NO_SUSPEND , + name, port);if (ret)return ret;
@@ -1156,6 +1151,13 @@ static int stm32_usart_of_dma_rx_probe(struct stm32_port *stm32port,struct dma_async_tx_descriptor *desc =NULL ;int ret; +/* +* Using DMA and threaded handler for the console could lead to +* deadlocks. +*/ +if (uart_console (port)) +return -ENODEV ; +/* Request DMA RX channel */ stm32port->rx_ch =dma_request_slave_channel (dev,"rx" );if (!stm32port->rx_ch) {
Signature · 1 line
2.26.3
Re: [PATCH 2/3] serial: stm32: fix threaded interrupt handling
Quoted 2 lines · Johan Hovold
When DMA is enabled the receive handler runs in a threaded handler, but the primary handler up until very recently neither disabled interrupts
Scratch the "up until very recently" bit here since the driver still doesn't disable interrupt in the device (it just disables all interrupts in the threaded handler). The rest stands as is.
Quoted 10 lines
in the device or used
IRQF_ONESHOT. This would lead to a deadlock if an interrupt comes in while the threaded receive handler is running under the port lock.Commit
ad7676812437("serial: stm32: fix a deadlock condition with wakeup event") claimed to fix an unrelated deadlock, but unfortunately also disabled interrupts in the threaded handler. While this prevents the deadlock mentioned in the previous paragraph it also defeats the purpose of using a threaded handler in the first place.
Quoted 7 lines
Fix this by making the interrupt one-shot and not disabling interrupts in the threaded handler.
Note that (receive) DMA must not be used for a console port as the threaded handler could be interrupted while holding the port lock, something which could lead to a deadlock in case an interrupt handler ends up calling printk.
Johan
A message that is not in the archive (sent off-list or not yet received): CAL9mu0KwgOFQfa8ft4rB6+F=KLd1gZLYDvwpAW72zPAFntehVw@mail.gmail.com
Re: [PATCH 2/3] serial: stm32: fix threaded interrupt handling
Quoted 22 lines · dillon min
Hi Johan
Thanks for share your patch.
Quoted 6 lines · Johan Hovold <[email protected]>于2021年4月16日 周五22:11写道:
When DMA is enabled the receive handler runs in a threaded handler, but the primary handler up until very recently neither disabled interrupts in the device or used
IRQF_ONESHOT. This would lead to a deadlock if an interrupt comes in while the threaded receive handler is running under the port lock.Greg told me there was a patch fixed this case. In case hard irq &
threaded_fnboth offered. Thelocal_irq_save()will be executed before call driver’s threaded handler.Post the original mail from Greg
Please see
81e2073c175b("genirq: Disable interrupts for force threaded handlers") for when threaded irq handlers have irqs disabled, isn't that the case you are trying to "protect" from here?
Quoted 5 lines
Why is the "threaded" flag used at all? The driver should not care.
Also see
9baedb7baeda("serial: imx: drop workaround for forced irq threading") in linux-next for an example of how this was fixed up in a serial driver.
Neither of these commits are (directly) related to the problem this patch addresses (they are about force-threaded handlers, this is about a normal threaded handler which run with interrupts enabled).
Johan
Re: [PATCH 2/3] serial: stm32: fix threaded interrupt handling
In reply to Johan HovoldHi, Johan
Quoted 20 lines · Johan Hovold
When DMA is enabled the receive handler runs in a threaded handler, but the primary handler up until very recently neither disabled interrupts in the device or used
IRQF_ONESHOT. This would lead to a deadlock if an interrupt comes in while the threaded receive handler is running under the port lock.Commit
ad7676812437("serial: stm32: fix a deadlock condition with wakeup event") claimed to fix an unrelated deadlock, but unfortunately also disabled interrupts in the threaded handler. While this prevents the deadlock mentioned in the previous paragraph it also defeats the purpose of using a threaded handler in the first place.Fix this by making the interrupt one-shot and not disabling interrupts in the threaded handler.
Note that (receive) DMA must not be used for a console port as the threaded handler could be interrupted while holding the port lock, something which could lead to a deadlock in case an interrupt handler ends up calling printk.
Quoted 6 lines
Fixes ad7676812437 ("serial: stm32: fix a deadlock condition with wakeup event")Fixes 3489187204eb ("serial: stm32: adding dma support")Cc [email protected] # 4.9Cc Alexandre TORGUE <[email protected]>Cc Gerald Baeza <[email protected]>Signed-off-by Johan Hovold <[email protected]>
Quoted 61 lines
--- drivers/tty/serial/stm32-usart.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)drivers/tty/serial/stm32-usart.c +12
-10@@ -214,14 +214,11 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)struct tty_port *tport = &port->state->port;struct stm32_port *stm32_port =to_stm32_port (port);const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs; -unsigned long c, flags; +unsigned long c;u32 sr;char flag; -if (threaded) -spin_lock_irqsave (&port->lock, flags); -else -spin_lock (&port->lock); +spin_lock (&port->lock);while (stm32_usart_pending_rx (port, &sr, &stm32_port->last_res, threaded)) {@@ -278,10 +275,7 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)uart_insert_char (port, sr,USART_SR_ORE , c, flag); } -if (threaded) -spin_unlock_irqrestore (&port->lock, flags); -else -spin_unlock (&port->lock); +spin_unlock (&port->lock);tty_flip_buffer_push (tport); }@@ -667,7 +661,8 @@ static int stm32_usart_startup(struct uart_port *port) ret =request_threaded_irq (port->irq, stm32_usart_interrupt, stm32_usart_threaded_interrupt, -IRQF_NO_SUSPEND , name, port); +IRQF_ONESHOT |IRQF_NO_SUSPEND , + name, port);if (ret)return ret;@@ -1156,6 +1151,13 @@ static int stm32_usart_of_dma_rx_probe(struct stm32_port *stm32port,struct dma_async_tx_descriptor *desc =NULL ;int ret; +/* +* Using DMA and threaded handler for the console could lead to +* deadlocks. +*/ +if (uart_console (port)) +return -ENODEV ; +/* Request DMA RX channel */ stm32port->rx_ch =dma_request_slave_channel (dev,"rx" );if (!stm32port->rx_ch) {
PATCH 3/3 serial: stm32: defer sysrq processing
In reply to Johan HovoldUse the uart_unlock_and_check_sysrq() helper to defer sysrq processing until receive processing is done and the port lock has been released.
This allows cleaning up the console_write() implementation by not having to work around the recursive sysrq case (by dropping locking completely) and also makes the console code work with PREEMPT_RT by no longer relying on local_irq_save().
--- drivers/tty/serial/stm32-usart.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/tty/serial/stm32-usart.c +6 -10
@@ -270,12 +270,12 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded) } } -if (uart_handle_sysrq_char (port, c)) +if (uart_prepare_sysrq_char (port, c))continue ;uart_insert_char (port, sr,USART_SR_ORE , c, flag); } -spin_unlock (&port->lock); +uart_unlock_and_check_sysrq (port);tty_flip_buffer_push (tport); }
@@ -1430,13 +1430,10 @@ static void stm32_usart_console_write(struct console *co, const char *s,u32 old_cr1, new_cr1;int locked =1 ; -local_irq_save (flags); -if (port->sysrq) - locked =0 ; -else if (oops_in_progress) - locked =spin_trylock (&port->lock); +if (oops_in_progress) + locked =spin_trylock_irqsave (&port->lock, flags);else -spin_lock (&port->lock); +spin_lock_irqsave (&port->lock, flags);/* Save and disable interrupts, enable the transmitter */ old_cr1 =readl_relaxed (port->membase + ofs->cr1);
@@ -1450,8 +1447,7 @@ static void stm32_usart_console_write(struct console *co, const char *s,writel_relaxed (old_cr1, port->membase + ofs->cr1);if (locked) -spin_unlock (&port->lock); -local_irq_restore (flags); +spin_unlock_irqrestore (&port->lock, flags); }static int stm32_usart_console_setup (struct console *co,char *options)
Signature · 1 line
2.26.3
Re: [PATCH 3/3] serial: stm32: defer sysrq processing
Hi,
Quoted 9 lines · Johan Hovold
Use the
uart_unlock_and_check_sysrq()helper to defer sysrq processing until receive processing is done and the port lock has been released.This allows cleaning up the
console_write()implementation by not having to work around the recursive sysrq case (by dropping locking completely) and also makes the console code work withPREEMPT_RTby no longer relying onlocal_irq_save().Signed-off-by Johan Hovold <[email protected]>
Quoted 50 lines
--- drivers/tty/serial/stm32-usart.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)drivers/tty/serial/stm32-usart.c +6
-10@@ -270,12 +270,12 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded) } } -if (uart_handle_sysrq_char (port, c)) +if (uart_prepare_sysrq_char (port, c))continue ;uart_insert_char (port, sr,USART_SR_ORE , c, flag); } -spin_unlock (&port->lock); +uart_unlock_and_check_sysrq (port);tty_flip_buffer_push (tport); }@@ -1430,13 +1430,10 @@ static void stm32_usart_console_write(struct console *co, const char *s,u32 old_cr1, new_cr1;int locked =1 ; -local_irq_save (flags); -if (port->sysrq) - locked =0 ; -else if (oops_in_progress) - locked =spin_trylock (&port->lock); +if (oops_in_progress) + locked =spin_trylock_irqsave (&port->lock, flags);else -spin_lock (&port->lock); +spin_lock_irqsave (&port->lock, flags);/* Save and disable interrupts, enable the transmitter */ old_cr1 =readl_relaxed (port->membase + ofs->cr1);@@ -1450,8 +1447,7 @@ static void stm32_usart_console_write(struct console *co, const char *s,writel_relaxed (old_cr1, port->membase + ofs->cr1);if (locked) -spin_unlock (&port->lock); -local_irq_restore (flags); +spin_unlock_irqrestore (&port->lock, flags); }static int stm32_usart_console_setup (struct console *co,char *options)