[PATCH] serial: qcom-geni: Clear pending IRQs while suspended

[PATCH] serial: qcom-geni: Clear pending IRQs while suspended

[Praveen Talari]

When uart_suspend_port() marks a port suspended, an interrupt may still
be pending before TX and RX are stopped. Returning IRQ_NONE before taking
the port lock leaves those interrupt status bits uncleared and can cause
the interrupt to be retriggered.

Take the UART port lock and clear the relevant interrupt status first,
then skip interrupt processing while the port is suspended.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/tty/serial/qcom_geni_serial.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index bfdc155c128d..d8323d6b5286 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1210,16 +1210,26 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 	struct tty_port *tport = &uport->state->port;
 	struct qcom_geni_serial_port *port = to_dev_port(uport);
 
-	if (uport->suspended)
-		return IRQ_NONE;
-
 	uart_port_lock(uport);
 
+	m_irq_status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
 	s_irq_status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
+	dma_tx_status = readl(uport->membase + SE_DMA_TX_IRQ_STAT);
 	dma_rx_status = readl(uport->membase + SE_DMA_RX_IRQ_STAT);
+	geni_status = readl(uport->membase + SE_GENI_STATUS);
+	dma = readl(uport->membase + SE_GENI_DMA_MODE_EN);
+	m_irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
+	writel(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
 	writel(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
+	writel(dma_tx_status, uport->membase + SE_DMA_TX_IRQ_CLR);
 	writel(dma_rx_status, uport->membase + SE_DMA_RX_IRQ_CLR);
 
+	trace_geni_serial_irq(uport->dev, m_irq_status, s_irq_status,
+			      dma_tx_status, dma_rx_status);
+
+	if (uport->suspended)
+		goto out_unlock;
+
 	if (s_irq_status & S_RX_FIFO_WR_ERR_EN) {
 		uport->icount.overrun++;
 		tty_insert_flip_char(tport, 0, TTY_OVERRUN);
@@ -1234,18 +1244,6 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 		port->brk = true;
 	}
 
-	m_irq_status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
-	dma_tx_status = readl(uport->membase + SE_DMA_TX_IRQ_STAT);
-	geni_status = readl(uport->membase + SE_GENI_STATUS);
-	dma = readl(uport->membase + SE_GENI_DMA_MODE_EN);
-	m_irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
-
-	trace_geni_serial_irq(uport->dev, m_irq_status, s_irq_status,
-			      dma_tx_status, dma_rx_status);
-
-	writel(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
-	writel(dma_tx_status, uport->membase + SE_DMA_TX_IRQ_CLR);
-
 	if (WARN_ON(m_irq_status & M_ILLEGAL_CMD_EN))
 		goto handle_rx;
 

---
base-commit: 2efb7a11f1aa4d2dc285fe2680cca22cb9f72f64
change-id: 20261019-ack-interrupts-on-suspended-port-0c1da05a8407

Best regards,
--  
Praveen Talari <praveen.talari@oss.qualcomm.com>

Re: [PATCH] serial: qcom-geni: Clear pending IRQs while suspended

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: qcom-geni: interrupt storm in qcom_geni_serial_isr() on suspend
- [Medium] serial: qcom-geni: clearing DMA IRQs when suspended desynchronizes DMA state
--

commit 80a526c2aa218ac3d5d987a8838881b22d009fa9
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

serial: qcom-geni: Clear pending IRQs while suspended

This commit moves the interrupt status register reads and clears to the start
of qcom_geni_serial_isr() under the UART port lock before checking
uport->suspended. When the port is suspended, the handler now clears the
interrupt status bits, jumps to out_unlock, and returns IRQ_HANDLED.

> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index bfdc155c128da..d8323d6b52869 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1210,16 +1210,26 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
>  	struct tty_port *tport = &uport->state->port;
>  	struct qcom_geni_serial_port *port = to_dev_port(uport);
>  
> -	if (uport->suspended)
> -		return IRQ_NONE;
> -
>  	uart_port_lock(uport);
>  
> +	m_irq_status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
>  	s_irq_status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
> +	dma_tx_status = readl(uport->membase + SE_DMA_TX_IRQ_STAT);
>  	dma_rx_status = readl(uport->membase + SE_DMA_RX_IRQ_STAT);
> +	geni_status = readl(uport->membase + SE_GENI_STATUS);
> +	dma = readl(uport->membase + SE_GENI_DMA_MODE_EN);
> +	m_irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> +	writel(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
>  	writel(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
> +	writel(dma_tx_status, uport->membase + SE_DMA_TX_IRQ_CLR);
>  	writel(dma_rx_status, uport->membase + SE_DMA_RX_IRQ_CLR);
>  
> +	trace_geni_serial_irq(uport->dev, m_irq_status, s_irq_status,
> +			      dma_tx_status, dma_rx_status);
> +
> +	if (uport->suspended)
> +		goto out_unlock;

[Severity: High]
Can this cause a hardirq storm and CPU livelock during system suspend on
FIFO-mode UART ports?

In drivers/tty/serial/serial_core.c:uart_suspend_port(), uport->suspended is
set with local interrupts enabled and without holding uport->lock before
acquiring the port lock to stop TX and RX:

drivers/tty/serial/serial_core.c:uart_suspend_port() {
	uport->suspended = 1;

	if (tty_port_initialized(port)) {
		...
		scoped_guard(uart_port_lock_irq, uport) {
			ops->stop_tx(uport);
			...
			ops->stop_rx(uport);
		}
}

If a FIFO watermark interrupt (M_TX_FIFO_WATERMARK_EN or
S_RX_FIFO_WATERMARK_EN) fires in that window before uart_port_lock_irq() is
reached, the watermark interrupts are still enabled in SE_GENI_M_IRQ_EN and
SE_GENI_S_IRQ_EN.

As noted in drivers/tty/serial/qcom_geni_serial.c:
qcom_geni_serial_handle_tx_fifo(), the GENI FIFO watermark interrupts are
level-triggered and latched in hardware:

	/*
	 * The tx fifo watermark is level triggered and latched. Though we had
	 * cleared it in qcom_geni_serial_isr it will have already reasserted
	 * so we must clear it again here after our writes.
	 */
	writel(M_TX_FIFO_WATERMARK_EN,
			uport->membase + SE_GENI_M_IRQ_CLEAR);

When qcom_geni_serial_isr() writes SE_GENI_M_IRQ_CLEAR and
SE_GENI_S_IRQ_CLEAR and immediately jumps to out_unlock without servicing the
FIFOs, clearing SE_GENI_TX_WATERMARK_REG, or masking the watermark bits in
SE_GENI_M_IRQ_EN and SE_GENI_S_IRQ_EN, the status bit immediately re-latches
while the FIFO level still satisfies the watermark threshold. Because
qcom_geni_serial_isr() returns IRQ_HANDLED, won't the CPU immediately
re-enter the ISR on IRQ exit and starve uart_suspend_port() before it can
call ops->stop_tx() and ops->stop_rx()?

[Severity: Medium]
What happens on DMA-mode UART ports if a TX or RX DMA completion interrupt
fires in this same suspend window after uport->suspended is set and before
ops->stop_tx() and ops->stop_rx() acquire uport->lock?

In drivers/tty/serial/qcom_geni_serial.c:qcom_geni_serial_isr(), clearing
SE_DMA_TX_IRQ_CLR and SE_DMA_RX_IRQ_CLR and jumping to out_unlock skips
qcom_geni_serial_handle_tx_dma() and qcom_geni_serial_handle_rx_dma().

For TX DMA, qcom_geni_serial_handle_tx_dma() normally advances the xmit FIFO
and clears port->tx_dma_addr upon TX_DMA_DONE:

drivers/tty/serial/qcom_geni_serial.c:qcom_geni_serial_handle_tx_dma() {
	...
	if (!tx_dma_stale && fifo_len >= port->tx_remaining)
		uart_xmit_advance(uport, port->tx_remaining);

	geni_se_tx_dma_unprep(&port->se, port->tx_dma_addr, port->tx_remaining);
	port->tx_dma_addr = 0;
	port->tx_remaining = 0;
	port->tx_dma_stale = false;
}

When TX_DMA_DONE is cleared without running qcom_geni_serial_handle_tx_dma(),
port->tx_dma_addr remains non-zero. Later in suspend,
qcom_geni_serial_stop_tx_dma() resets the TX DMA FSM and unmaps
port->tx_dma_addr without advancing tport->xmit_fifo, which will cause
uart_resume_port() -> qcom_geni_serial_start_tx_dma() to re-transmit the
already-sent port->tx_remaining bytes.

Similarly, for RX DMA, clearing RX_EOT or RX_DMA_DONE in SE_DMA_RX_IRQ_CLR
without calling qcom_geni_serial_handle_rx_dma() drops the received buffer
and skips re-arming RX DMA via geni_se_rx_init_dma() while S_GENI_CMD_ACTIVE
remains asserted. When qcom_geni_serial_stop_rx_dma() then runs:

drivers/tty/serial/qcom_geni_serial.c:qcom_geni_serial_stop_rx_dma() {
	...
	if (!qcom_geni_serial_secondary_active(uport))
		return;

	geni_se_cancel_s_cmd(&port->se);
	done = qcom_geni_serial_poll_bit(uport, SE_DMA_RX_IRQ_STAT,
			RX_EOT, true);
}

won't polling SE_DMA_RX_IRQ_STAT for RX_EOT busy-wait with interrupts
disabled until poll_timeout_us expires before falling back to an RX abort and
FSM reset?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-ack-interrupts-on-suspended-port-v1-1-2747a58ca7cb@oss.qualcomm.com?part=1