Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1243385 > unrolled thread

[PATCH 1/4] serial: tegra: Handle another RX race condition

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/4] serial: tegra: Handle another RX race condition Jon Hunter <jonathanh@nvidia.com> - 2015-10-09 16:00 +0200
    Re: [PATCH 1/4] serial: tegra: Handle another RX race condition Jon Hunter <jonathanh@nvidia.com> - 2015-10-09 16:00 +0200

#1243385 — [PATCH 1/4] serial: tegra: Handle another RX race condition

Commit 853a699739fe ("serial: tegra: handle race condition on uart rx
side") attempted to fix a race condition between the RX end of
transmission interrupt and RX DMA completion callback. Despite this
fix there is still another case where these two paths can race and
result in duplicated data. The race condition is as follows:

1. DMA completion interrupt occurs and schedules tasklet to call DMA
   callback.
2. DMA callback for the UART driver starts to execute. This will copy
   the data from the DMA buffer and restart the DMA. This is done under
   uart port spinlock.
3. During the callback, UART interrupt is raised for end of receive. The
   UART ISR runs and waits to acquire port spinlock held by the DMA
   callback.
4. DMA callback gives up spinlock after copying the data, but before
   restarting DMA.
5. UART ISR acquires the spin lock and reads the same DMA buffer because
   DMA has not been restarted yet.

The release of the spinlock during the DMA callback was introduced by
commit 9b88748b362c ("tty: serial: tegra: drop uart_port->lock before
calling tty_flip_buffer_push()") to fix a spinlock lock-up issue when
calling tty_flip_buffer_push(). However, since then commit a9c3f68f3cd8
("tty: Fix low_latency BUG") migrated tty_flip_buffer_push() to always
use a workqueue, allowing tty_flip_buffer_push() to be called from
within atomic sections. Therefore, we can remove the unlocking of the
spinlock from the DMA callback and UART ISR and this will ensure that
the race condition no longer occurs.

Reported-by: Christopher Freeman <cfreeman@nvidia.com>
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/tty/serial/serial-tegra.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index cf0133ae762d..38b49f447bd7 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -607,9 +607,7 @@ static void tegra_uart_rx_dma_complete(void *args)
 
 	tegra_uart_handle_rx_pio(tup, port);
 	if (tty) {
-		spin_unlock_irqrestore(&u->lock, flags);
 		tty_flip_buffer_push(port);
-		spin_lock_irqsave(&u->lock, flags);
 		tty_kref_put(tty);
 	}
 	tegra_uart_start_rx_dma(tup);
@@ -622,13 +620,11 @@ done:
 	spin_unlock_irqrestore(&u->lock, flags);
 }
 
-static void tegra_uart_handle_rx_dma(struct tegra_uart_port *tup,
-		unsigned long *flags)
+static void tegra_uart_handle_rx_dma(struct tegra_uart_port *tup)
 {
 	struct dma_tx_state state;
 	struct tty_struct *tty = tty_port_tty_get(&tup->uport.state->port);
 	struct tty_port *port = &tup->uport.state->port;
-	struct uart_port *u = &tup->uport;
 	unsigned int count;
 
 	/* Deactivate flow control to stop sender */
@@ -645,9 +641,7 @@ static void tegra_uart_handle_rx_dma(struct tegra_uart_port *tup,
 
 	tegra_uart_handle_rx_pio(tup, port);
 	if (tty) {
-		spin_unlock_irqrestore(&u->lock, *flags);
 		tty_flip_buffer_push(port);
-		spin_lock_irqsave(&u->lock, *flags);
 		tty_kref_put(tty);
 	}
 	tegra_uart_start_rx_dma(tup);
@@ -714,7 +708,7 @@ static irqreturn_t tegra_uart_isr(int irq, void *data)
 		iir = tegra_uart_read(tup, UART_IIR);
 		if (iir & UART_IIR_NO_INT) {
 			if (is_rx_int) {
-				tegra_uart_handle_rx_dma(tup, &flags);
+				tegra_uart_handle_rx_dma(tup);
 				if (tup->rx_in_progress) {
 					ier = tup->ier_shadow;
 					ier |= (UART_IER_RLSI | UART_IER_RTOIE |
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1243386

Adding Chris to CC.

Jon

On 09/10/15 14:49, Jon Hunter wrote:
> Commit 853a699739fe ("serial: tegra: handle race condition on uart rx
> side") attempted to fix a race condition between the RX end of
> transmission interrupt and RX DMA completion callback. Despite this
> fix there is still another case where these two paths can race and
> result in duplicated data. The race condition is as follows:
> 
> 1. DMA completion interrupt occurs and schedules tasklet to call DMA
>    callback.
> 2. DMA callback for the UART driver starts to execute. This will copy
>    the data from the DMA buffer and restart the DMA. This is done under
>    uart port spinlock.
> 3. During the callback, UART interrupt is raised for end of receive. The
>    UART ISR runs and waits to acquire port spinlock held by the DMA
>    callback.
> 4. DMA callback gives up spinlock after copying the data, but before
>    restarting DMA.
> 5. UART ISR acquires the spin lock and reads the same DMA buffer because
>    DMA has not been restarted yet.
> 
> The release of the spinlock during the DMA callback was introduced by
> commit 9b88748b362c ("tty: serial: tegra: drop uart_port->lock before
> calling tty_flip_buffer_push()") to fix a spinlock lock-up issue when
> calling tty_flip_buffer_push(). However, since then commit a9c3f68f3cd8
> ("tty: Fix low_latency BUG") migrated tty_flip_buffer_push() to always
> use a workqueue, allowing tty_flip_buffer_push() to be called from
> within atomic sections. Therefore, we can remove the unlocking of the
> spinlock from the DMA callback and UART ISR and this will ensure that
> the race condition no longer occurs.
> 
> Reported-by: Christopher Freeman <cfreeman@nvidia.com>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/tty/serial/serial-tegra.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
> index cf0133ae762d..38b49f447bd7 100644
> --- a/drivers/tty/serial/serial-tegra.c
> +++ b/drivers/tty/serial/serial-tegra.c
> @@ -607,9 +607,7 @@ static void tegra_uart_rx_dma_complete(void *args)
>  
>  	tegra_uart_handle_rx_pio(tup, port);
>  	if (tty) {
> -		spin_unlock_irqrestore(&u->lock, flags);
>  		tty_flip_buffer_push(port);
> -		spin_lock_irqsave(&u->lock, flags);
>  		tty_kref_put(tty);
>  	}
>  	tegra_uart_start_rx_dma(tup);
> @@ -622,13 +620,11 @@ done:
>  	spin_unlock_irqrestore(&u->lock, flags);
>  }
>  
> -static void tegra_uart_handle_rx_dma(struct tegra_uart_port *tup,
> -		unsigned long *flags)
> +static void tegra_uart_handle_rx_dma(struct tegra_uart_port *tup)
>  {
>  	struct dma_tx_state state;
>  	struct tty_struct *tty = tty_port_tty_get(&tup->uport.state->port);
>  	struct tty_port *port = &tup->uport.state->port;
> -	struct uart_port *u = &tup->uport;
>  	unsigned int count;
>  
>  	/* Deactivate flow control to stop sender */
> @@ -645,9 +641,7 @@ static void tegra_uart_handle_rx_dma(struct tegra_uart_port *tup,
>  
>  	tegra_uart_handle_rx_pio(tup, port);
>  	if (tty) {
> -		spin_unlock_irqrestore(&u->lock, *flags);
>  		tty_flip_buffer_push(port);
> -		spin_lock_irqsave(&u->lock, *flags);
>  		tty_kref_put(tty);
>  	}
>  	tegra_uart_start_rx_dma(tup);
> @@ -714,7 +708,7 @@ static irqreturn_t tegra_uart_isr(int irq, void *data)
>  		iir = tegra_uart_read(tup, UART_IIR);
>  		if (iir & UART_IIR_NO_INT) {
>  			if (is_rx_int) {
> -				tegra_uart_handle_rx_dma(tup, &flags);
> +				tegra_uart_handle_rx_dma(tup);
>  				if (tup->rx_in_progress) {
>  					ier = tup->ier_shadow;
>  					ier |= (UART_IER_RLSI | UART_IER_RTOIE |
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web