Re: [PATCH 2/2] tty: serial: samsung: do not re-arm RX DMA once RX has been stopped

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tty: serial: samsung: s3c24xx_serial_rx_chars_dma() ignores rx_enabled
--

commit 487da6105041ec489b81c069edfdd379610e15fb
Author: Samarth Narang <samarth.colleges@gmail.com>

tty: serial: samsung: do not re-arm RX DMA once RX has been stopped

This commit prevents s3c24xx_serial_rx_dma_complete() from re-arming RX DMA
after ourport->rx_enabled has been cleared, and clears ourport->rx_enabled
under the port lock during s3c64xx_serial_shutdown().

Link: https://lore.kernel.org/r/20261030162334.3000737-1-samarth.colleges@gmail.com

> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 2f6c69beccb74..3f2346f4fd0a9 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -622,7 +622,8 @@ static void s3c24xx_serial_rx_dma_complete(void *args)
>  		tty_kref_put(tty);
>  	}
>  
> -	s3c64xx_start_rx_dma(ourport);
> +	if (ourport->rx_enabled)
> +		s3c64xx_start_rx_dma(ourport);
>  
>  	uart_port_unlock_irqrestore(port, flags);
>  }
> @@ -1165,10 +1166,13 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
>  static void s3c64xx_serial_shutdown(struct uart_port *port)
>  {
>  	struct s3c24xx_uart_port *ourport = to_ourport(port);
> +	unsigned long flags;
>  
> +	uart_port_lock_irqsave(port, &flags);
>  	ourport->tx_enabled = 0;
>  	ourport->tx_mode = 0;
>  	ourport->rx_enabled = 0;
> +	uart_port_unlock_irqrestore(port, flags);

[Severity: High]
Does s3c24xx_serial_rx_chars_dma() also need to check ourport->rx_enabled
after acquiring the port lock?

In drivers/tty/serial/samsung_tty.c, s3c24xx_serial_rx_chars_dma() reads
S3C2410_UTRSTAT before acquiring uart_port_lock(port), and then proceeds
without checking whether ourport->rx_enabled is still set:

drivers/tty/serial/samsung_tty.c:s3c24xx_serial_rx_chars_dma() {
	utrstat = rd_regl(port, S3C2410_UTRSTAT);
	rd_regl(port, S3C2410_UFSTAT);

	uart_port_lock(port);

	if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
		s3c64xx_start_rx_dma(ourport);
		if (ourport->rx_mode == S3C24XX_RX_PIO)
			enable_rx_dma(ourport);
		goto finish;
	}

	if (ourport->rx_mode == S3C24XX_RX_DMA) {
		dmaengine_pause(dma->rx_chan);
		dmaengine_tx_status(dma->rx_chan, dma->rx_cookie, &state);
		dmaengine_terminate_all(dma->rx_chan);
		received = dma->rx_bytes_requested - state.residue;
		s3c24xx_uart_copy_rx_to_tty(ourport, t, received);

		enable_rx_pio(ourport);
	}
}

If an RX interrupt fires on another CPU and spins on port->lock while
s3c24xx_serial_stop_rx() or s3c64xx_serial_shutdown() clears
ourport->rx_enabled (especially during s3c64xx_serial_shutdown(), where
S3C64XX_UINTM is only masked after free_irq()), s3c24xx_serial_rx_chars_dma()
can enter the critical section after ourport->rx_enabled is 0.

When S3C2410_UTRSTAT_TIMEOUT is not set, s3c24xx_serial_rx_chars_dma() will
call s3c64xx_start_rx_dma() and enable_rx_dma(), restarting RX DMA after
s3c24xx_serial_stop_rx() has already stopped reception and terminated the
channel.

When S3C2410_UTRSTAT_TIMEOUT is set, neither s3c24xx_serial_stop_rx() nor
s3c24xx_serial_rx_dma_complete() (when skipping s3c64xx_start_rx_dma()
because ourport->rx_enabled is 0) resets ourport->rx_mode from
S3C24XX_RX_DMA. Could s3c24xx_serial_rx_chars_dma() then query
dmaengine_tx_status() on a dma->rx_cookie that was already completed or
terminated (where state.residue is 0) and copy dma->rx_bytes_requested
(PAGE_SIZE) bytes of stale data from dma->rx_buf into the tty flip buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031004217.3340610-1-samarth.colleges@gmail.com?part=2