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(-)
--
2.26.3
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.
Fixes: ad7676812437 ("serial: stm32: fix a deadlock condition with wakeup event")
Fixes: 3489187204eb ("serial: stm32: adding dma support")
Cc: [email protected] # 4.9
Cc: Alexandre TORGUE <[email protected]>
Cc: Gerald Baeza <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/tty/serial/stm32-usart.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 4d277804c63e..3524ed2c0c73 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -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) {
--
2.26.3
On Fri, Apr 16, 2021 at 10:35:25PM +0800, dillon min wrote:
> Hi Johan
>
> Thanks for share your patch.
>
> 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_fn both offered. The local_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?
>
> 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
Hi, Johan
On 4/16/21 4:05 PM, Johan Hovold wrote:
> 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.
>
> Fixes: ad7676812437 ("serial: stm32: fix a deadlock condition with wakeup event")
> Fixes: 3489187204eb ("serial: stm32: adding dma support")
> Cc: [email protected] # 4.9
> Cc: Alexandre TORGUE <[email protected]>
> Cc: Gerald Baeza <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
Reviewed-by: Valentin Caron<[email protected]>
> ---
> drivers/tty/serial/stm32-usart.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 4d277804c63e..3524ed2c0c73 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -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) {