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

[sashiko-bot@kernel.org]

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