2021-07-14 08:08:23

by Johan Hovold

[permalink] [raw]
Subject: [PATCH] serial: 8250: fix handle_irq locking

The 8250 handle_irq callback is not just called from the interrupt
handler but also from a timer callback when polling (e.g. for ports
without an interrupt line). Consequently the callback must explicitly
disable interrupts to avoid a potential deadlock with another interrupt
in polled mode.

Add back an irqrestore-version of the sysrq port-unlock helper and use
it in the 8250 callbacks that need it.

Fixes: 75f4e830fa9c ("serial: do not restore interrupt state in sysrq helper")
Reported-by: kernel test robot <[email protected]>
Cc: [email protected] # 5.13
Cc: Joel Stanley <[email protected]>
Cc: Andrew Jeffery <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/tty/serial/8250/8250_aspeed_vuart.c | 5 +++--
drivers/tty/serial/8250/8250_fsl.c | 5 +++--
drivers/tty/serial/8250/8250_port.c | 5 +++--
include/linux/serial_core.h | 24 +++++++++++++++++++++
4 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 4caab8714e2c..2350fb3bb5e4 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -329,6 +329,7 @@ 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;
unsigned int space, count;

iir = serial_port_in(port, UART_IIR);
@@ -336,7 +337,7 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)
if (iir & UART_IIR_NO_INT)
return 0;

- spin_lock(&port->lock);
+ spin_lock_irqsave(&port->lock, flags);

lsr = serial_port_in(port, UART_LSR);

@@ -370,7 +371,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);
+ uart_unlock_and_check_sysrq_irqrestore(port, flags);

return 1;
}
diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 4e75d2e4f87c..fc65a2293ce9 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -30,10 +30,11 @@ 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(&up->port.lock);
+ spin_lock_irqsave(&up->port.lock, flags);

iir = port->serial_in(port, UART_IIR);
if (iir & UART_IIR_NO_INT) {
@@ -82,7 +83,7 @@ int fsl8250_handle_irq(struct uart_port *port)

up->lsr_saved_flags = orig_lsr;

- uart_unlock_and_check_sysrq(&up->port);
+ uart_unlock_and_check_sysrq_irqrestore(&up->port, flags);

return 1;
}
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 2164290cbd31..d65778c4e4ca 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1893,11 +1893,12 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
unsigned char status;
struct uart_8250_port *up = up_to_u8250p(port);
bool skip_rx = false;
+ unsigned long flags;

if (iir & UART_IIR_NO_INT)
return 0;

- spin_lock(&port->lock);
+ spin_lock_irqsave(&port->lock, flags);

status = serial_port_in(port, UART_LSR);

@@ -1923,7 +1924,7 @@ 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);
+ uart_unlock_and_check_sysrq_irqrestore(port, flags);

return 1;
}
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 52d7fb92a69d..c58cc142d23f 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -518,6 +518,25 @@ static inline void uart_unlock_and_check_sysrq(struct uart_port *port)
if (sysrq_ch)
handle_sysrq(sysrq_ch);
}
+
+static inline void uart_unlock_and_check_sysrq_irqrestore(struct uart_port *port,
+ unsigned long flags)
+{
+ int sysrq_ch;
+
+ if (!port->has_sysrq) {
+ spin_unlock_irqrestore(&port->lock, flags);
+ return;
+ }
+
+ sysrq_ch = port->sysrq_ch;
+ port->sysrq_ch = 0;
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ if (sysrq_ch)
+ handle_sysrq(sysrq_ch);
+}
#else /* CONFIG_MAGIC_SYSRQ_SERIAL */
static inline int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
{
@@ -531,6 +550,11 @@ static inline void uart_unlock_and_check_sysrq(struct uart_port *port)
{
spin_unlock(&port->lock);
}
+static inline void uart_unlock_and_check_sysrq_irqrestore(struct uart_port *port,
+ unsigned long flags)
+{
+ spin_unlock_irqrestore(&port->lock, flags);
+}
#endif /* CONFIG_MAGIC_SYSRQ_SERIAL */

/*
--
2.31.1