[PATCH 0/2] tty: serial: samsung: RX DMA fixes in stop_rx and the RX completion

[PATCH 0/2] tty: serial: samsung: RX DMA fixes in stop_rx and the RX completion

[Samarth Narang]

This series addresses two RX DMA issues previously raised under the
release_dma patch [1] and subsequently confirmed by Marek Szyprowski.

The first fixes s3c24xx_serial_stop_rx(), which incorrectly pauses the
TX channel instead of the RX channel before reading the RX residue. The
second prevents s3c24xx_serial_rx_dma_complete() from re-arming RX DMA
after reception has been stopped or during shutdown, which could
otherwise start a new transfer after the channel has been terminated.

The series applies on top of [1] and has only been compile-tested with
arm64 defconfig and W=1, with no new warnings. It has not been tested on
hardware.

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

Samarth Narang (2):
  tty: serial: samsung: pause the RX DMA channel in stop_rx
  tty: serial: samsung: do not re-arm RX DMA once RX has been stopped

 drivers/tty/serial/samsung_tty.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


base-commit: 83f32c773255720a661aa45d83e36278135f23f5
prerequisite-patch-id: cfa1c1db9d23de3ae1cc6f0476a8ac611d09c7a8
-- 
2.34.1

[PATCH 1/2] tty: serial: samsung: pause the RX DMA channel in stop_rx

[Samarth Narang]

In s3c24xx_serial_stop_rx(), the driver pauses dma->tx_chan before
reading the residue of the RX transfer on dma->rx_chan to determine how
many bytes have been received. Since pausing the TX channel does not
stop RX DMA, additional bytes may arrive before the RX channel is
terminated, causing them to be omitted from the data passed to the tty.
This also unnecessarily pauses an active TX transfer, which the driver
does not subsequently resume.

Pause the RX channel instead, following the existing handling in
s3c24xx_serial_rx_chars_dma() on the RX timeout path.

Fixes: b543c301c2c5 ("serial: samsung: add DMA support for RX")
Link: https://lore.kernel.org/r/20261030162334.3000737-1-samarth.colleges@gmail.com
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
 drivers/tty/serial/samsung_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 4eeef149b..2f6c69bec 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -553,7 +553,7 @@ static void s3c24xx_serial_stop_rx(struct uart_port *port)
 		ourport->rx_enabled = 0;
 	}
 	if (dma && dma->rx_chan) {
-		dmaengine_pause(dma->tx_chan);
+		dmaengine_pause(dma->rx_chan);
 		dma_status = dmaengine_tx_status(dma->rx_chan,
 				dma->rx_cookie, &state);
 		if (dma_status == DMA_IN_PROGRESS ||
-- 
2.34.1

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

[Samarth Narang]

s3c24xx_serial_rx_dma_complete() unconditionally submits a new RX
descriptor after each completion, even when reception has been stopped
or the port is shutting down. A completion racing with
s3c24xx_serial_stop_rx() or shutdown may therefore restart DMA after the
channel has been terminated, potentially allowing it to access the RX
buffer after s3c24xx_serial_release_dma() has unmapped and freed it.

Only re-arm RX DMA while rx_enabled is set, and clear the flag under the
port lock during shutdown, since the hangup path can reach
s3c64xx_serial_shutdown() without calling stop_rx(). Because the
completion callback also holds the port lock, any descriptor submitted
before rx_enabled is cleared will be covered by the subsequent channel
termination.

Fixes: b543c301c2c5 ("serial: samsung: add DMA support for RX")
Link: https://lore.kernel.org/r/20261030162334.3000737-1-samarth.colleges@gmail.com
Assisted-by: LLM
Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>
---
 drivers/tty/serial/samsung_tty.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 2f6c69bec..3f2346f4f 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);
 
 	free_irq(port->irq, ourport);
 
-- 
2.34.1

Re: [PATCH 1/2] tty: serial: samsung: pause the RX DMA channel in stop_rx

[sashiko-bot]

> In s3c24xx_serial_stop_rx(), the driver pauses dma->tx_chan before
> reading the residue of the RX transfer on dma->rx_chan to determine how
> many bytes have been received. Since pausing the TX channel does not
> stop RX DMA, additional bytes may arrive before the RX channel is
> terminated, causing them to be omitted from the data passed to the tty.
> This also unnecessarily pauses an active TX transfer, which the driver
> does not subsequently resume.
> 
> Pause the RX channel instead, following the existing handling in
> s3c24xx_serial_rx_chars_dma() on the RX timeout path.
> 
> Fixes: b543c301c2c5 ("serial: samsung: add DMA support for RX")
> Link: https://lore.kernel.org/r/20261030162334.3000737-1-samarth.colleges@gmail.com
> Assisted-by: LLM
> Signed-off-by: Samarth Narang <samarth.colleges@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

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

[sashiko-bot]

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