Skip to contentSkip to conversation

tty: xilinx_uartps: split sysrq handling

PATCH In review Sean Anderson 2 participants 2 messages Jan 10, 2025 – Jan 11, 2025

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 tty: xilinx_uartps: split sysrq handling

lockdep detects the following circular locking dependency:

CPU 0 CPU 1
========================== ============================
cdns_uart_isr() printk()
uart_port_lock(port) console_lock()

			     cdns_uart_console_write()
                               if (!port->sysrq)
                                 uart_port_lock(port)
  uart_handle_break()
    port->sysrq = ...

uart_handle_sysrq_char()
printk()
console_lock()

The fixed commit attempts to avoid this situation by only taking the port lock in cdns_uart_console_write if port->sysrq unset. However, if (as shown above) cdns_uart_console_write runs before port->sysrq is set, then it will try to take the port lock anyway. This may result in a deadlock.

Fix this by splitting sysrq handling into two parts. We use the prepare helper under the port lock and defer handling until we release the lock.

Fixes 74ea66d4ca06 ("tty: xuartps: Improve sysrq handling")
Cc <[email protected]> # c980248179d: serial: xilinx_uartps: Use port lock wrappers

---

 drivers/tty/serial/xilinx_uartps.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/tty/serial/xilinx_uartps.c +3 -5
index beb151be4d32..92ec51870d1d 100644
@@ -287,7 +287,7 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
 				continue;
 		}
 
-		if (uart_handle_sysrq_char(port, data))
+		if (uart_prepare_sysrq_char(port, data))
 			continue;
 
 		if (is_rxbs_support) {
@@ -495,7 +495,7 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 	    !(readl(port->membase + CDNS_UART_CR) & CDNS_UART_CR_RX_DIS))
 		cdns_uart_handle_rx(dev_id, isrstatus);
 
-	uart_port_unlock(port);
+	uart_unlock_and_check_sysrq(port);
 	return IRQ_HANDLED;
 }
 
@@ -1380,9 +1380,7 @@ static void cdns_uart_console_write(struct console *co, const char *s,
 	unsigned int imr, ctrl;
 	int locked = 1;
 
-	if (port->sysrq)
-		locked = 0;
-	else if (oops_in_progress)
+	if (oops_in_progress)
 		locked = uart_port_trylock_irqsave(port, &flags);
 	else
 		uart_port_lock_irqsave(port, &flags);
Signature · 1 line
2.35.1.1320.gc452695387.dirty
Acked-by on v1 1/1
#1 lore raw
Quoted 2 lines · Sean Anderson

Fix this by splitting sysrq handling into two parts. We use the prepare helper under the port lock and defer handling until we release the lock.

Note that this fix is only necessary because this console driver is using the legacy console API. For the NBCON API it is allowed to call printk() while holding the port lock.

But since code already exists to allow deferring the sysrq execution until the port lock is not held, this patch is probably a good idea anyway because it can reduce port lock contention. AFAIK there are no sysrq actions that require port lock synchronization.