[PATCH 0/4] serial: tegra: One fix and a few clean-ups

[PATCH 0/4] serial: tegra: One fix and a few clean-ups

[Jon Hunter]

Fix a race condition in the receive path and simplify/clean-up the
serial-tegra driver.

This has been tested on a tegra124 by using zmodem protocol to transfer a
file from PC to the tegra device and verifying the downloaded file matches
the original.

Jon Hunter (4):
  serial: tegra: Handle another RX race condition
  serial: tegra: Remove unnecessary return statements
  serial: tegra: Remove redundant code and check in tegra_uart_stop_rx()
  serial: tegra: Add helper function for handling RX buffer

 drivers/tty/serial/serial-tegra.c | 94 ++++++++++++---------------------------
 1 file changed, 28 insertions(+), 66 deletions(-)

-- 
2.1.4

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

[Jon Hunter]

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

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

[Jon Hunter]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  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 |
> 

[PATCH 2/4] serial: tegra: Remove unnecessary return statements

[Jon Hunter]

Some functions in the serial-tegra driver have unnecessary return
statements at the end of a void function and so remove them.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/tty/serial/serial-tegra.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 38b49f447bd7..42583484d4b2 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -186,7 +186,6 @@ static void set_rts(struct tegra_uart_port *tup, bool active)
 		tegra_uart_write(tup, mcr, UART_MCR);
 		tup->mcr_shadow = mcr;
 	}
-	return;
 }
 
 static void set_dtr(struct tegra_uart_port *tup, bool active)
@@ -202,7 +201,6 @@ static void set_dtr(struct tegra_uart_port *tup, bool active)
 		tegra_uart_write(tup, mcr, UART_MCR);
 		tup->mcr_shadow = mcr;
 	}
-	return;
 }
 
 static void tegra_uart_set_mctrl(struct uart_port *u, unsigned int mctrl)
@@ -217,7 +215,6 @@ static void tegra_uart_set_mctrl(struct uart_port *u, unsigned int mctrl)
 
 	dtr_enable = !!(mctrl & TIOCM_DTR);
 	set_dtr(tup, dtr_enable);
-	return;
 }
 
 static void tegra_uart_break_ctl(struct uart_port *u, int break_ctl)
@@ -511,7 +508,6 @@ static void tegra_uart_stop_tx(struct uart_port *u)
 	async_tx_ack(tup->tx_dma_desc);
 	xmit->tail = (xmit->tail + count) & (UART_XMIT_SIZE - 1);
 	tup->tx_in_progress = 0;
-	return;
 }
 
 static void tegra_uart_handle_tx_pio(struct tegra_uart_port *tup)
@@ -523,7 +519,6 @@ static void tegra_uart_handle_tx_pio(struct tegra_uart_port *tup)
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&tup->uport);
 	tegra_uart_start_next_tx(tup);
-	return;
 }
 
 static void tegra_uart_handle_rx_pio(struct tegra_uart_port *tup,
@@ -545,8 +540,6 @@ static void tegra_uart_handle_rx_pio(struct tegra_uart_port *tup,
 		if (!uart_handle_sysrq_char(&tup->uport, ch) && tty)
 			tty_insert_flip_char(tty, ch, flag);
 	} while (1);
-
-	return;
 }
 
 static void tegra_uart_copy_rx_to_tty(struct tegra_uart_port *tup,
@@ -691,7 +684,6 @@ static void tegra_uart_handle_modem_signal_change(struct uart_port *u)
 	/* Will start/stop_tx accordingly */
 	if (msr & UART_MSR_DCTS)
 		uart_handle_cts_change(&tup->uport, msr & UART_MSR_CTS);
-	return;
 }
 
 static irqreturn_t tegra_uart_isr(int irq, void *data)
@@ -799,7 +791,6 @@ static void tegra_uart_stop_rx(struct uart_port *u)
 		tty_flip_buffer_push(port);
 		tty_kref_put(tty);
 	}
-	return;
 }
 
 static void tegra_uart_hw_deinit(struct tegra_uart_port *tup)
@@ -1077,7 +1068,6 @@ static void tegra_uart_flush_buffer(struct uart_port *u)
 	tup->tx_bytes = 0;
 	if (tup->tx_dma_chan)
 		dmaengine_terminate_all(tup->tx_dma_chan);
-	return;
 }
 
 static void tegra_uart_shutdown(struct uart_port *u)
@@ -1217,7 +1207,6 @@ static void tegra_uart_set_termios(struct uart_port *u,
 	tegra_uart_read(tup, UART_IER);
 
 	spin_unlock_irqrestore(&u->lock, flags);
-	return;
 }
 
 static const char *tegra_uart_type(struct uart_port *u)
-- 
2.1.4

[PATCH 3/4] serial: tegra: Remove redundant code and check in tegra_uart_stop_rx()

[Jon Hunter]

The serial-tegra driver always uses DMA and hence the driver always
allocates DMA channels. Therefore, the test to see if the RX DMA channel
is initialised in tegra_uart_stop_rx() is unnecessary and so remove
the test and the code that corresponds to the case where the RX DMA
channel is not initialised. Please note that the call to
tegra_uart_stop_rx() should always be before the call to
tegra_uart_shutdown() which will uninitialise the RX DMA channel.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/tty/serial/serial-tegra.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 42583484d4b2..11aa5e1e3705 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -777,16 +777,13 @@ static void tegra_uart_stop_rx(struct uart_port *u)
 	tup->ier_shadow = ier;
 	tegra_uart_write(tup, ier, UART_IER);
 	tup->rx_in_progress = 0;
-	if (tup->rx_dma_chan) {
-		dmaengine_terminate_all(tup->rx_dma_chan);
-		dmaengine_tx_status(tup->rx_dma_chan, tup->rx_cookie, &state);
-		async_tx_ack(tup->rx_dma_desc);
-		count = tup->rx_bytes_requested - state.residue;
-		tegra_uart_copy_rx_to_tty(tup, port, count);
-		tegra_uart_handle_rx_pio(tup, port);
-	} else {
-		tegra_uart_handle_rx_pio(tup, port);
-	}
+	dmaengine_terminate_all(tup->rx_dma_chan);
+	dmaengine_tx_status(tup->rx_dma_chan, tup->rx_cookie, &state);
+	async_tx_ack(tup->rx_dma_desc);
+	count = tup->rx_bytes_requested - state.residue;
+	tegra_uart_copy_rx_to_tty(tup, port, count);
+	tegra_uart_handle_rx_pio(tup, port);
+
 	if (tty) {
 		tty_flip_buffer_push(port);
 		tty_kref_put(tty);
-- 
2.1.4

[PATCH 4/4] serial: tegra: Add helper function for handling RX buffer

[Jon Hunter]

In the tegra UART driver there are three places where the RX DMA buffer
is handled and pushed up to the tty layer. In all three instances the
same functions are called and so instead of duplicating the code in three
places, move this code to a new helper function and use this new function.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/tty/serial/serial-tegra.c | 66 ++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 42 deletions(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 11aa5e1e3705..1d6fc60ed013 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -569,13 +569,30 @@ static void tegra_uart_copy_rx_to_tty(struct tegra_uart_port *tup,
 				TEGRA_UART_RX_DMA_BUFFER_SIZE, DMA_TO_DEVICE);
 }
 
+static void tegra_uart_rx_buffer_push(struct tegra_uart_port *tup,
+				      unsigned int residue)
+{
+	struct tty_port *port = &tup->uport.state->port;
+	struct tty_struct *tty = tty_port_tty_get(port);
+	unsigned int count;
+
+	async_tx_ack(tup->rx_dma_desc);
+	count = tup->rx_bytes_requested - residue;
+
+	/* If we are here, DMA is stopped */
+	tegra_uart_copy_rx_to_tty(tup, port, count);
+
+	tegra_uart_handle_rx_pio(tup, port);
+	if (tty) {
+		tty_flip_buffer_push(port);
+		tty_kref_put(tty);
+	}
+}
+
 static void tegra_uart_rx_dma_complete(void *args)
 {
 	struct tegra_uart_port *tup = args;
 	struct uart_port *u = &tup->uport;
-	unsigned int count = tup->rx_bytes_requested;
-	struct tty_struct *tty = tty_port_tty_get(&tup->uport.state->port);
-	struct tty_port *port = &u->state->port;
 	unsigned long flags;
 	struct dma_tx_state state;
 	enum dma_status status;
@@ -589,20 +606,11 @@ static void tegra_uart_rx_dma_complete(void *args)
 		goto done;
 	}
 
-	async_tx_ack(tup->rx_dma_desc);
-
 	/* Deactivate flow control to stop sender */
 	if (tup->rts_active)
 		set_rts(tup, false);
 
-	/* If we are here, DMA is stopped */
-	tegra_uart_copy_rx_to_tty(tup, port, count);
-
-	tegra_uart_handle_rx_pio(tup, port);
-	if (tty) {
-		tty_flip_buffer_push(port);
-		tty_kref_put(tty);
-	}
+	tegra_uart_rx_buffer_push(tup, 0);
 	tegra_uart_start_rx_dma(tup);
 
 	/* Activate flow control to start transfer */
@@ -616,27 +624,14 @@ done:
 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;
-	unsigned int count;
 
 	/* Deactivate flow control to stop sender */
 	if (tup->rts_active)
 		set_rts(tup, false);
 
 	dmaengine_terminate_all(tup->rx_dma_chan);
-	dmaengine_tx_status(tup->rx_dma_chan,  tup->rx_cookie, &state);
-	async_tx_ack(tup->rx_dma_desc);
-	count = tup->rx_bytes_requested - state.residue;
-
-	/* If we are here, DMA is stopped */
-	tegra_uart_copy_rx_to_tty(tup, port, count);
-
-	tegra_uart_handle_rx_pio(tup, port);
-	if (tty) {
-		tty_flip_buffer_push(port);
-		tty_kref_put(tty);
-	}
+	dmaengine_tx_status(tup->rx_dma_chan, tup->rx_cookie, &state);
+	tegra_uart_rx_buffer_push(tup, state.residue);
 	tegra_uart_start_rx_dma(tup);
 
 	if (tup->rts_active)
@@ -755,11 +750,8 @@ static irqreturn_t tegra_uart_isr(int irq, void *data)
 static void tegra_uart_stop_rx(struct uart_port *u)
 {
 	struct tegra_uart_port *tup = to_tegra_uport(u);
-	struct tty_struct *tty;
-	struct tty_port *port = &u->state->port;
 	struct dma_tx_state state;
 	unsigned long ier;
-	int count;
 
 	if (tup->rts_active)
 		set_rts(tup, false);
@@ -767,8 +759,6 @@ static void tegra_uart_stop_rx(struct uart_port *u)
 	if (!tup->rx_in_progress)
 		return;
 
-	tty = tty_port_tty_get(&tup->uport.state->port);
-
 	tegra_uart_wait_sym_time(tup, 1); /* wait a character interval */
 
 	ier = tup->ier_shadow;
@@ -779,15 +769,7 @@ static void tegra_uart_stop_rx(struct uart_port *u)
 	tup->rx_in_progress = 0;
 	dmaengine_terminate_all(tup->rx_dma_chan);
 	dmaengine_tx_status(tup->rx_dma_chan, tup->rx_cookie, &state);
-	async_tx_ack(tup->rx_dma_desc);
-	count = tup->rx_bytes_requested - state.residue;
-	tegra_uart_copy_rx_to_tty(tup, port, count);
-	tegra_uart_handle_rx_pio(tup, port);
-
-	if (tty) {
-		tty_flip_buffer_push(port);
-		tty_kref_put(tty);
-	}
+	tegra_uart_rx_buffer_push(tup, state.residue);
 }
 
 static void tegra_uart_hw_deinit(struct tegra_uart_port *tup)
-- 
2.1.4