[PATCH 0/8] serial: tegra: various fixes

[PATCH 0/8] serial: tegra: various fixes

[Jon Hunter]

Various fixes for the tegra hsuart driver.

Tested on tegra124 nyan-big by opening a serial console on ttyTHS0
and performing simple z-modem transfers.

Jon Hunter (6):
  serial: tegra: Correct delay after TX flush
  serial: tegra: Add delay after enabling FIFO mode
  serial: tegra: Use unsigned types for RX and TX byte counts
  serial: tegra: Fix cookie used by TX channel
  serial: tegra: Correct shutdown of UARTs
  serial: tegra: Correct error handling on DMA setup

Shardar Shariff Md (2):
  serial: tegra: check the count and read if any from dma
  serial: tegra: handle race condition on uart rx side

 drivers/tty/serial/serial-tegra.c | 128 ++++++++++++++++++++++++++------------
 1 file changed, 87 insertions(+), 41 deletions(-)

-- 
1.9.1

[PATCH 0/8] serial: tegra: various fixes

[Jon Hunter]

Various fixes for the tegra hsuart driver.

Tested on tegra124 nyan-big by opening a serial console on ttyTHS0
and performing simple z-modem transfers.

Jon Hunter (6):
  serial: tegra: Correct delay after TX flush
  serial: tegra: Add delay after enabling FIFO mode
  serial: tegra: Use unsigned types for RX and TX byte counts
  serial: tegra: Fix cookie used by TX channel
  serial: tegra: Correct shutdown of UARTs
  serial: tegra: Correct error handling on DMA setup

Shardar Shariff Md (2):
  serial: tegra: check the count and read if any from dma
  serial: tegra: handle race condition on uart rx side

 drivers/tty/serial/serial-tegra.c | 128 ++++++++++++++++++++++++++------------
 1 file changed, 87 insertions(+), 41 deletions(-)

-- 
1.9.1

[PATCH 1/8] serial: tegra: Correct delay after TX flush

[Jon Hunter]

For all tegra devices (up to t210), there is a hardware issue that
requires software to wait for 32 UART clock periods for the flush
to propagate otherwise TX data could be post. Add a helper function
to wait for N UART clock periods and update delay following FIFO
flush to be 32 UART clock cycles.

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 1d5ea3964ee5..9e08d3f07509 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -234,6 +234,22 @@ static void tegra_uart_break_ctl(struct uart_port *u, int break_ctl)
 	tup->lcr_shadow = lcr;
 }
 
+/**
+ * tegra_uart_wait_cycle_time: Wait for N UART clock periods
+ *
+ * @tup:	Tegra serial port data structure.
+ * @cycles:	Number of clock periods to wait.
+ *
+ * Tegra UARTs are clocked at 16X the baud/bit rate and hence the UART
+ * clock speed is 16X the current baud rate.
+ */
+static void tegra_uart_wait_cycle_time(struct tegra_uart_port *tup,
+				       unsigned int cycles)
+{
+	if (tup->current_baud)
+		udelay(DIV_ROUND_UP(cycles * 1000000, tup->current_baud * 16));
+}
+
 /* Wait for a symbol-time. */
 static void tegra_uart_wait_sym_time(struct tegra_uart_port *tup,
 		unsigned int syms)
@@ -263,8 +279,12 @@ static void tegra_uart_fifo_reset(struct tegra_uart_port *tup, u8 fcr_bits)
 	/* Dummy read to ensure the write is posted */
 	tegra_uart_read(tup, UART_SCR);
 
-	/* Wait for the flush to propagate. */
-	tegra_uart_wait_sym_time(tup, 1);
+	/*
+	 * For all tegra devices (up to t210), there is a hardware issue that
+	 * requires software to wait for 32 UART clock periods for the flush
+	 * to propagate, otherwise data could be lost.
+	 */
+	tegra_uart_wait_cycle_time(tup, 32);
 }
 
 static int tegra_set_baudrate(struct tegra_uart_port *tup, unsigned int baud)
-- 
1.9.1

[PATCH 1/8] serial: tegra: Correct delay after TX flush

[Jon Hunter]

For all tegra devices (up to t210), there is a hardware issue that
requires software to wait for 32 UART clock periods for the flush
to propagate otherwise TX data could be post. Add a helper function
to wait for N UART clock periods and update delay following FIFO
flush to be 32 UART clock cycles.

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 1d5ea3964ee5..9e08d3f07509 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -234,6 +234,22 @@ static void tegra_uart_break_ctl(struct uart_port *u, int break_ctl)
 	tup->lcr_shadow = lcr;
 }
 
+/**
+ * tegra_uart_wait_cycle_time: Wait for N UART clock periods
+ *
+ * @tup:	Tegra serial port data structure.
+ * @cycles:	Number of clock periods to wait.
+ *
+ * Tegra UARTs are clocked at 16X the baud/bit rate and hence the UART
+ * clock speed is 16X the current baud rate.
+ */
+static void tegra_uart_wait_cycle_time(struct tegra_uart_port *tup,
+				       unsigned int cycles)
+{
+	if (tup->current_baud)
+		udelay(DIV_ROUND_UP(cycles * 1000000, tup->current_baud * 16));
+}
+
 /* Wait for a symbol-time. */
 static void tegra_uart_wait_sym_time(struct tegra_uart_port *tup,
 		unsigned int syms)
@@ -263,8 +279,12 @@ static void tegra_uart_fifo_reset(struct tegra_uart_port *tup, u8 fcr_bits)
 	/* Dummy read to ensure the write is posted */
 	tegra_uart_read(tup, UART_SCR);
 
-	/* Wait for the flush to propagate. */
-	tegra_uart_wait_sym_time(tup, 1);
+	/*
+	 * For all tegra devices (up to t210), there is a hardware issue that
+	 * requires software to wait for 32 UART clock periods for the flush
+	 * to propagate, otherwise data could be lost.
+	 */
+	tegra_uart_wait_cycle_time(tup, 32);
 }
 
 static int tegra_set_baudrate(struct tegra_uart_port *tup, unsigned int baud)
-- 
1.9.1

[PATCH 2/8] serial: tegra: Add delay after enabling FIFO mode

[Jon Hunter]

For all tegra devices (up to t210), there is a hardware issue that
requires software to wait for 3 UART clock periods after enabling
the TX fifo, otherwise data could be lost.

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 9e08d3f07509..0d9d7ceb1dbb 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -885,6 +885,16 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
 	tup->fcr_shadow |= TEGRA_UART_TX_TRIG_16B;
 	tegra_uart_write(tup, tup->fcr_shadow, UART_FCR);
 
+	/* Dummy read to ensure the write is posted */
+	tegra_uart_read(tup, UART_SCR);
+
+	/*
+	 * For all tegra devices (up to t210), there is a hardware issue that
+	 * requires software to wait for 3 UART clock periods after enabling
+	 * the TX fifo, otherwise data could be lost.
+	 */
+	tegra_uart_wait_cycle_time(tup, 3);
+
 	/*
 	 * Initialize the UART with default configuration
 	 * (115200, N, 8, 1) so that the receive DMA buffer may be
-- 
1.9.1

[PATCH 2/8] serial: tegra: Add delay after enabling FIFO mode

[Jon Hunter]

For all tegra devices (up to t210), there is a hardware issue that
requires software to wait for 3 UART clock periods after enabling
the TX fifo, otherwise data could be lost.

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 9e08d3f07509..0d9d7ceb1dbb 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -885,6 +885,16 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
 	tup->fcr_shadow |= TEGRA_UART_TX_TRIG_16B;
 	tegra_uart_write(tup, tup->fcr_shadow, UART_FCR);
 
+	/* Dummy read to ensure the write is posted */
+	tegra_uart_read(tup, UART_SCR);
+
+	/*
+	 * For all tegra devices (up to t210), there is a hardware issue that
+	 * requires software to wait for 3 UART clock periods after enabling
+	 * the TX fifo, otherwise data could be lost.
+	 */
+	tegra_uart_wait_cycle_time(tup, 3);
+
 	/*
 	 * Initialize the UART with default configuration
 	 * (115200, N, 8, 1) so that the receive DMA buffer may be
-- 
1.9.1

[PATCH 3/8] serial: tegra: check the count and read if any from dma

[Jon Hunter]

From: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

It is only necessary to read data from the dma buffer when the count
value is non-zero and hence, tegra_uart_copy_rx_to_tty() so only be
called when this is the case.

Although, this was being tested for in two places, there is a third
place where this was not tested. However, instead of adding another
if-statement prior to calling tegra_uart_copy_rx_to_tty(), move the test
inside the function.

Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org: Re-worked patch to move the check for the count
 value inside the function tegra_uart_copy_rx_to_tty(). Updated
 changelog with more commentary.]
Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/tty/serial/serial-tegra.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 0d9d7ceb1dbb..a53899c47e60 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -554,6 +554,10 @@ static void tegra_uart_copy_rx_to_tty(struct tegra_uart_port *tup,
 {
 	int copied;
 
+	/* If count is zero, then there is no data to be copied */
+	if (!count)
+		return;
+
 	tup->uport.icount.rx += count;
 	if (!tty) {
 		dev_err(tup->uport.dev, "No tty port\n");
@@ -588,8 +592,7 @@ static void tegra_uart_rx_dma_complete(void *args)
 		set_rts(tup, false);
 
 	/* If we are here, DMA is stopped */
-	if (count)
-		tegra_uart_copy_rx_to_tty(tup, port, count);
+	tegra_uart_copy_rx_to_tty(tup, port, count);
 
 	tegra_uart_handle_rx_pio(tup, port);
 	if (tty) {
@@ -626,8 +629,7 @@ static void tegra_uart_handle_rx_dma(struct tegra_uart_port *tup,
 	count = tup->rx_bytes_requested - state.residue;
 
 	/* If we are here, DMA is stopped */
-	if (count)
-		tegra_uart_copy_rx_to_tty(tup, port, count);
+	tegra_uart_copy_rx_to_tty(tup, port, count);
 
 	tegra_uart_handle_rx_pio(tup, port);
 	if (tty) {
-- 
1.9.1

[PATCH 3/8] serial: tegra: check the count and read if any from dma

[Jon Hunter]

From: Shardar Shariff Md <smohammed@nvidia.com>

It is only necessary to read data from the dma buffer when the count
value is non-zero and hence, tegra_uart_copy_rx_to_tty() so only be
called when this is the case.

Although, this was being tested for in two places, there is a third
place where this was not tested. However, instead of adding another
if-statement prior to calling tegra_uart_copy_rx_to_tty(), move the test
inside the function.

Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
[jonathanh@nvidia.com: Re-worked patch to move the check for the count
 value inside the function tegra_uart_copy_rx_to_tty(). Updated
 changelog with more commentary.]
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/tty/serial/serial-tegra.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 0d9d7ceb1dbb..a53899c47e60 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -554,6 +554,10 @@ static void tegra_uart_copy_rx_to_tty(struct tegra_uart_port *tup,
 {
 	int copied;
 
+	/* If count is zero, then there is no data to be copied */
+	if (!count)
+		return;
+
 	tup->uport.icount.rx += count;
 	if (!tty) {
 		dev_err(tup->uport.dev, "No tty port\n");
@@ -588,8 +592,7 @@ static void tegra_uart_rx_dma_complete(void *args)
 		set_rts(tup, false);
 
 	/* If we are here, DMA is stopped */
-	if (count)
-		tegra_uart_copy_rx_to_tty(tup, port, count);
+	tegra_uart_copy_rx_to_tty(tup, port, count);
 
 	tegra_uart_handle_rx_pio(tup, port);
 	if (tty) {
@@ -626,8 +629,7 @@ static void tegra_uart_handle_rx_dma(struct tegra_uart_port *tup,
 	count = tup->rx_bytes_requested - state.residue;
 
 	/* If we are here, DMA is stopped */
-	if (count)
-		tegra_uart_copy_rx_to_tty(tup, port, count);
+	tegra_uart_copy_rx_to_tty(tup, port, count);
 
 	tegra_uart_handle_rx_pio(tup, port);
 	if (tty) {
-- 
1.9.1

[PATCH 4/8] serial: tegra: handle race condition on uart rx side

[Jon Hunter]

From: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The tegra serial driver has two paths through which receive data is
copied up to the tty layer. These are:
1. DMA completion callback
2. UART RX interrupt

A UART RX interrupt occurs for either RX_TIMEOUT (data has been sitting
in the Rx FIFO for more than 4 character times without being read
because there is not enough data to reach the trigger level), End of
Receive Data event (receiver detects that data stops coming in for more
than 4 character times) or a receive error.

In the RX interrupt path, the following happens ...
- All RX DMA transfers are stopped
- Any data in the DMA buffer and RX FIFO are copied up to the tty layer.
- DMA is restarted/primed for the RX path

In the DMA completion callback, the DMA buffer is copied up to the tty
layer but there is no check to see if the RX interrupt could have
occurred between the DMA interrupt firing the the DMA callback running.
Hence, if a RX interrupt was to occur shortly after the DMA completion
interrupt, it is possible that the RX interrupt path has already copied
the DMA buffer before the DMA callback has been called. Therefore, when
the DMA callback is called, if the DMA is already in-progress, then this
indicates that the UART RX interrupt has already occurred and there is
nothing to do in the DMA callback. This race condition can cause
duplicated data to be received.

Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org: Moved async_tx_ack() call to after check to see
 if DMA has completed because if the DMA is in progress we do not need
 to ACK yet. Changed the print from dev_info to dev_debug. Updated
 changelog to add more commentary on the race condition based upon
 feedback from author.]
Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/tty/serial/serial-tegra.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index a53899c47e60..17d8a08b047b 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -583,10 +583,20 @@ static void tegra_uart_rx_dma_complete(void *args)
 	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;
 
-	async_tx_ack(tup->rx_dma_desc);
 	spin_lock_irqsave(&u->lock, flags);
 
+	status = dmaengine_tx_status(tup->rx_dma_chan, tup->rx_cookie, &state);
+
+	if (status == DMA_IN_PROGRESS) {
+		dev_dbg(tup->uport.dev, "RX DMA is in progress\n");
+		goto done;
+	}
+
+	async_tx_ack(tup->rx_dma_desc);
+
 	/* Deactivate flow control to stop sender */
 	if (tup->rts_active)
 		set_rts(tup, false);
@@ -607,6 +617,7 @@ static void tegra_uart_rx_dma_complete(void *args)
 	if (tup->rts_active)
 		set_rts(tup, true);
 
+done:
 	spin_unlock_irqrestore(&u->lock, flags);
 }
 
-- 
1.9.1

[PATCH 4/8] serial: tegra: handle race condition on uart rx side

[Jon Hunter]

From: Shardar Shariff Md <smohammed@nvidia.com>

The tegra serial driver has two paths through which receive data is
copied up to the tty layer. These are:
1. DMA completion callback
2. UART RX interrupt

A UART RX interrupt occurs for either RX_TIMEOUT (data has been sitting
in the Rx FIFO for more than 4 character times without being read
because there is not enough data to reach the trigger level), End of
Receive Data event (receiver detects that data stops coming in for more
than 4 character times) or a receive error.

In the RX interrupt path, the following happens ...
- All RX DMA transfers are stopped
- Any data in the DMA buffer and RX FIFO are copied up to the tty layer.
- DMA is restarted/primed for the RX path

In the DMA completion callback, the DMA buffer is copied up to the tty
layer but there is no check to see if the RX interrupt could have
occurred between the DMA interrupt firing the the DMA callback running.
Hence, if a RX interrupt was to occur shortly after the DMA completion
interrupt, it is possible that the RX interrupt path has already copied
the DMA buffer before the DMA callback has been called. Therefore, when
the DMA callback is called, if the DMA is already in-progress, then this
indicates that the UART RX interrupt has already occurred and there is
nothing to do in the DMA callback. This race condition can cause
duplicated data to be received.

Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
[jonathanh@nvidia.com: Moved async_tx_ack() call to after check to see
 if DMA has completed because if the DMA is in progress we do not need
 to ACK yet. Changed the print from dev_info to dev_debug. Updated
 changelog to add more commentary on the race condition based upon
 feedback from author.]
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/tty/serial/serial-tegra.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index a53899c47e60..17d8a08b047b 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -583,10 +583,20 @@ static void tegra_uart_rx_dma_complete(void *args)
 	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;
 
-	async_tx_ack(tup->rx_dma_desc);
 	spin_lock_irqsave(&u->lock, flags);
 
+	status = dmaengine_tx_status(tup->rx_dma_chan, tup->rx_cookie, &state);
+
+	if (status == DMA_IN_PROGRESS) {
+		dev_dbg(tup->uport.dev, "RX DMA is in progress\n");
+		goto done;
+	}
+
+	async_tx_ack(tup->rx_dma_desc);
+
 	/* Deactivate flow control to stop sender */
 	if (tup->rts_active)
 		set_rts(tup, false);
@@ -607,6 +617,7 @@ static void tegra_uart_rx_dma_complete(void *args)
 	if (tup->rts_active)
 		set_rts(tup, true);
 
+done:
 	spin_unlock_irqrestore(&u->lock, flags);
 }
 
-- 
1.9.1

[PATCH 5/8] serial: tegra: Use unsigned types for RX and TX byte counts

[Jon Hunter]

The function tty_insert_flip_string() takes an argument "size" which is
of type size_t. This is an unsigned type. Update the count,
rx_bytes_requested and tx_bytes_requested in the tegra serial driver to
be unsigned integers so that an unsigned type is passed to
tty_insert_flip_string().

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 17d8a08b047b..a5312e4b6393 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -131,8 +131,8 @@ struct tegra_uart_port {
 	struct dma_async_tx_descriptor		*rx_dma_desc;
 	dma_cookie_t				tx_cookie;
 	dma_cookie_t				rx_cookie;
-	int					tx_bytes_requested;
-	int					rx_bytes_requested;
+	unsigned int				tx_bytes_requested;
+	unsigned int				rx_bytes_requested;
 };
 
 static void tegra_uart_start_next_tx(struct tegra_uart_port *tup);
@@ -408,7 +408,7 @@ static void tegra_uart_tx_dma_complete(void *args)
 	struct circ_buf *xmit = &tup->uport.state->xmit;
 	struct dma_tx_state state;
 	unsigned long flags;
-	int count;
+	unsigned int count;
 
 	dmaengine_tx_status(tup->tx_dma_chan, tup->rx_cookie, &state);
 	count = tup->tx_bytes_requested - state.residue;
@@ -500,7 +500,7 @@ static void tegra_uart_stop_tx(struct uart_port *u)
 	struct tegra_uart_port *tup = to_tegra_uport(u);
 	struct circ_buf *xmit = &tup->uport.state->xmit;
 	struct dma_tx_state state;
-	int count;
+	unsigned int count;
 
 	if (tup->tx_in_progress != TEGRA_UART_TX_DMA)
 		return;
@@ -550,7 +550,8 @@ static void tegra_uart_handle_rx_pio(struct tegra_uart_port *tup,
 }
 
 static void tegra_uart_copy_rx_to_tty(struct tegra_uart_port *tup,
-		struct tty_port *tty, int count)
+				      struct tty_port *tty,
+				      unsigned int count)
 {
 	int copied;
 
@@ -579,7 +580,7 @@ static void tegra_uart_rx_dma_complete(void *args)
 {
 	struct tegra_uart_port *tup = args;
 	struct uart_port *u = &tup->uport;
-	int count = tup->rx_bytes_requested;
+	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;
@@ -628,7 +629,7 @@ static void tegra_uart_handle_rx_dma(struct tegra_uart_port *tup,
 	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;
-	int count;
+	unsigned int count;
 
 	/* Deactivate flow control to stop sender */
 	if (tup->rts_active)
-- 
1.9.1

[PATCH 5/8] serial: tegra: Use unsigned types for RX and TX byte counts

[Jon Hunter]

The function tty_insert_flip_string() takes an argument "size" which is
of type size_t. This is an unsigned type. Update the count,
rx_bytes_requested and tx_bytes_requested in the tegra serial driver to
be unsigned integers so that an unsigned type is passed to
tty_insert_flip_string().

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 17d8a08b047b..a5312e4b6393 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -131,8 +131,8 @@ struct tegra_uart_port {
 	struct dma_async_tx_descriptor		*rx_dma_desc;
 	dma_cookie_t				tx_cookie;
 	dma_cookie_t				rx_cookie;
-	int					tx_bytes_requested;
-	int					rx_bytes_requested;
+	unsigned int				tx_bytes_requested;
+	unsigned int				rx_bytes_requested;
 };
 
 static void tegra_uart_start_next_tx(struct tegra_uart_port *tup);
@@ -408,7 +408,7 @@ static void tegra_uart_tx_dma_complete(void *args)
 	struct circ_buf *xmit = &tup->uport.state->xmit;
 	struct dma_tx_state state;
 	unsigned long flags;
-	int count;
+	unsigned int count;
 
 	dmaengine_tx_status(tup->tx_dma_chan, tup->rx_cookie, &state);
 	count = tup->tx_bytes_requested - state.residue;
@@ -500,7 +500,7 @@ static void tegra_uart_stop_tx(struct uart_port *u)
 	struct tegra_uart_port *tup = to_tegra_uport(u);
 	struct circ_buf *xmit = &tup->uport.state->xmit;
 	struct dma_tx_state state;
-	int count;
+	unsigned int count;
 
 	if (tup->tx_in_progress != TEGRA_UART_TX_DMA)
 		return;
@@ -550,7 +550,8 @@ static void tegra_uart_handle_rx_pio(struct tegra_uart_port *tup,
 }
 
 static void tegra_uart_copy_rx_to_tty(struct tegra_uart_port *tup,
-		struct tty_port *tty, int count)
+				      struct tty_port *tty,
+				      unsigned int count)
 {
 	int copied;
 
@@ -579,7 +580,7 @@ static void tegra_uart_rx_dma_complete(void *args)
 {
 	struct tegra_uart_port *tup = args;
 	struct uart_port *u = &tup->uport;
-	int count = tup->rx_bytes_requested;
+	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;
@@ -628,7 +629,7 @@ static void tegra_uart_handle_rx_dma(struct tegra_uart_port *tup,
 	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;
-	int count;
+	unsigned int count;
 
 	/* Deactivate flow control to stop sender */
 	if (tup->rts_active)
-- 
1.9.1

[PATCH 6/8] serial: tegra: Fix cookie used by TX channel

[Jon Hunter]

The DMA cookie for the RX channel is being used by the TX channel.
Therefore, fix driver to use the correct DMA cookie for the TX channel.

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index a5312e4b6393..987c05665e62 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -410,7 +410,7 @@ static void tegra_uart_tx_dma_complete(void *args)
 	unsigned long flags;
 	unsigned int count;
 
-	dmaengine_tx_status(tup->tx_dma_chan, tup->rx_cookie, &state);
+	dmaengine_tx_status(tup->tx_dma_chan, tup->tx_cookie, &state);
 	count = tup->tx_bytes_requested - state.residue;
 	async_tx_ack(tup->tx_dma_desc);
 	spin_lock_irqsave(&tup->uport.lock, flags);
-- 
1.9.1

[PATCH 6/8] serial: tegra: Fix cookie used by TX channel

[Jon Hunter]

The DMA cookie for the RX channel is being used by the TX channel.
Therefore, fix driver to use the correct DMA cookie for the TX channel.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/tty/serial/serial-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index a5312e4b6393..987c05665e62 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -410,7 +410,7 @@ static void tegra_uart_tx_dma_complete(void *args)
 	unsigned long flags;
 	unsigned int count;
 
-	dmaengine_tx_status(tup->tx_dma_chan, tup->rx_cookie, &state);
+	dmaengine_tx_status(tup->tx_dma_chan, tup->tx_cookie, &state);
 	count = tup->tx_bytes_requested - state.residue;
 	async_tx_ack(tup->tx_dma_desc);
 	spin_lock_irqsave(&tup->uport.lock, flags);
-- 
1.9.1

[PATCH 7/8] serial: tegra: Correct shutdown of UARTs

[Jon Hunter]

There are two issues in the shutdown path of the UARTs which are:
1. The function tegra_uart_shutdown() calls tegra_uart_flush_buffer()
   to stop DMA TX transfers. However, tegra_uart_flush_buffer() is
   called after the DMA channels have already been freed and so actually
   does nothing.
2. The function that frees the DMA channels
   (tegra_uart_dma_channel_free()), unmaps the dma buffer before
   freeing the DMA channel and does not ensure the DMA has been
   stopped.

Resolve this by fixing the code in tegra_uart_dma_channel_free() to
ensure the DMA is stopped, free the DMA channel and then unmap the DMA
buffer. Finally, remove the unnecessary call to tegra_uart_flush_buffer().

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 987c05665e62..96378da9aefc 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -1020,24 +1020,23 @@ scrub:
 static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
 		bool dma_to_memory)
 {
-	struct dma_chan *dma_chan;
-
 	if (dma_to_memory) {
+		dmaengine_terminate_all(tup->rx_dma_chan);
+		dma_release_channel(tup->rx_dma_chan);
 		dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
 				tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
-		dma_chan = tup->rx_dma_chan;
 		tup->rx_dma_chan = NULL;
 		tup->rx_dma_buf_phys = 0;
 		tup->rx_dma_buf_virt = NULL;
 	} else {
+		dmaengine_terminate_all(tup->tx_dma_chan);
+		dma_release_channel(tup->tx_dma_chan);
 		dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
 			UART_XMIT_SIZE, DMA_TO_DEVICE);
-		dma_chan = tup->tx_dma_chan;
 		tup->tx_dma_chan = NULL;
 		tup->tx_dma_buf_phys = 0;
 		tup->tx_dma_buf_virt = NULL;
 	}
-	dma_release_channel(dma_chan);
 }
 
 static int tegra_uart_startup(struct uart_port *u)
@@ -1104,8 +1103,6 @@ static void tegra_uart_shutdown(struct uart_port *u)
 	tegra_uart_dma_channel_free(tup, true);
 	tegra_uart_dma_channel_free(tup, false);
 	free_irq(u->irq, tup);
-
-	tegra_uart_flush_buffer(u);
 }
 
 static void tegra_uart_enable_ms(struct uart_port *u)
-- 
1.9.1

[PATCH 7/8] serial: tegra: Correct shutdown of UARTs

[Jon Hunter]

There are two issues in the shutdown path of the UARTs which are:
1. The function tegra_uart_shutdown() calls tegra_uart_flush_buffer()
   to stop DMA TX transfers. However, tegra_uart_flush_buffer() is
   called after the DMA channels have already been freed and so actually
   does nothing.
2. The function that frees the DMA channels
   (tegra_uart_dma_channel_free()), unmaps the dma buffer before
   freeing the DMA channel and does not ensure the DMA has been
   stopped.

Resolve this by fixing the code in tegra_uart_dma_channel_free() to
ensure the DMA is stopped, free the DMA channel and then unmap the DMA
buffer. Finally, remove the unnecessary call to tegra_uart_flush_buffer().

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 987c05665e62..96378da9aefc 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -1020,24 +1020,23 @@ scrub:
 static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
 		bool dma_to_memory)
 {
-	struct dma_chan *dma_chan;
-
 	if (dma_to_memory) {
+		dmaengine_terminate_all(tup->rx_dma_chan);
+		dma_release_channel(tup->rx_dma_chan);
 		dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
 				tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
-		dma_chan = tup->rx_dma_chan;
 		tup->rx_dma_chan = NULL;
 		tup->rx_dma_buf_phys = 0;
 		tup->rx_dma_buf_virt = NULL;
 	} else {
+		dmaengine_terminate_all(tup->tx_dma_chan);
+		dma_release_channel(tup->tx_dma_chan);
 		dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
 			UART_XMIT_SIZE, DMA_TO_DEVICE);
-		dma_chan = tup->tx_dma_chan;
 		tup->tx_dma_chan = NULL;
 		tup->tx_dma_buf_phys = 0;
 		tup->tx_dma_buf_virt = NULL;
 	}
-	dma_release_channel(dma_chan);
 }
 
 static int tegra_uart_startup(struct uart_port *u)
@@ -1104,8 +1103,6 @@ static void tegra_uart_shutdown(struct uart_port *u)
 	tegra_uart_dma_channel_free(tup, true);
 	tegra_uart_dma_channel_free(tup, false);
 	free_irq(u->irq, tup);
-
-	tegra_uart_flush_buffer(u);
 }
 
 static void tegra_uart_enable_ms(struct uart_port *u)
-- 
1.9.1

[PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Jon Hunter]

Function tegra_uart_dma_channel_allocate() does not check that
dma_map_single() mapped the DMA buffer correctly. Add a check for this
and appropriate error handling.

Furthermore, if dmaengine_slave_config() (called by
tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
instead of just dma_release_channel() if  dmaengine_slave_config() fails.

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 96378da9aefc..3b63f103f0c9 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
 	return 0;
 }
 
+static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
+		bool dma_to_memory)
+{
+	if (dma_to_memory) {
+		dmaengine_terminate_all(tup->rx_dma_chan);
+		dma_release_channel(tup->rx_dma_chan);
+		dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
+				tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
+		tup->rx_dma_chan = NULL;
+		tup->rx_dma_buf_phys = 0;
+		tup->rx_dma_buf_virt = NULL;
+	} else {
+		dmaengine_terminate_all(tup->tx_dma_chan);
+		dma_release_channel(tup->tx_dma_chan);
+		dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
+			UART_XMIT_SIZE, DMA_TO_DEVICE);
+		tup->tx_dma_chan = NULL;
+		tup->tx_dma_buf_phys = 0;
+		tup->tx_dma_buf_virt = NULL;
+	}
+}
+
 static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
 			bool dma_to_memory)
 {
@@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
 		dma_phys = dma_map_single(tup->uport.dev,
 			tup->uport.state->xmit.buf, UART_XMIT_SIZE,
 			DMA_TO_DEVICE);
+		if (dma_mapping_error(tup->uport.dev, dma_phys)) {
+			dev_err(tup->uport.dev, "dma_map_single tx failed\n");
+			dma_release_channel(dma_chan);
+			return -ENOMEM;
+		}
 		dma_buf = tup->uport.state->xmit.buf;
 	}
 
@@ -1013,32 +1040,10 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
 	return 0;
 
 scrub:
-	dma_release_channel(dma_chan);
+	tegra_uart_dma_channel_free(tup, dma_to_memory);
 	return ret;
 }
 
-static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
-		bool dma_to_memory)
-{
-	if (dma_to_memory) {
-		dmaengine_terminate_all(tup->rx_dma_chan);
-		dma_release_channel(tup->rx_dma_chan);
-		dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
-				tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
-		tup->rx_dma_chan = NULL;
-		tup->rx_dma_buf_phys = 0;
-		tup->rx_dma_buf_virt = NULL;
-	} else {
-		dmaengine_terminate_all(tup->tx_dma_chan);
-		dma_release_channel(tup->tx_dma_chan);
-		dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
-			UART_XMIT_SIZE, DMA_TO_DEVICE);
-		tup->tx_dma_chan = NULL;
-		tup->tx_dma_buf_phys = 0;
-		tup->tx_dma_buf_virt = NULL;
-	}
-}
-
 static int tegra_uart_startup(struct uart_port *u)
 {
 	struct tegra_uart_port *tup = to_tegra_uport(u);
-- 
1.9.1

[PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Jon Hunter]

Function tegra_uart_dma_channel_allocate() does not check that
dma_map_single() mapped the DMA buffer correctly. Add a check for this
and appropriate error handling.

Furthermore, if dmaengine_slave_config() (called by
tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
instead of just dma_release_channel() if  dmaengine_slave_config() fails.

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

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 96378da9aefc..3b63f103f0c9 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
 	return 0;
 }
 
+static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
+		bool dma_to_memory)
+{
+	if (dma_to_memory) {
+		dmaengine_terminate_all(tup->rx_dma_chan);
+		dma_release_channel(tup->rx_dma_chan);
+		dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
+				tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
+		tup->rx_dma_chan = NULL;
+		tup->rx_dma_buf_phys = 0;
+		tup->rx_dma_buf_virt = NULL;
+	} else {
+		dmaengine_terminate_all(tup->tx_dma_chan);
+		dma_release_channel(tup->tx_dma_chan);
+		dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
+			UART_XMIT_SIZE, DMA_TO_DEVICE);
+		tup->tx_dma_chan = NULL;
+		tup->tx_dma_buf_phys = 0;
+		tup->tx_dma_buf_virt = NULL;
+	}
+}
+
 static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
 			bool dma_to_memory)
 {
@@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
 		dma_phys = dma_map_single(tup->uport.dev,
 			tup->uport.state->xmit.buf, UART_XMIT_SIZE,
 			DMA_TO_DEVICE);
+		if (dma_mapping_error(tup->uport.dev, dma_phys)) {
+			dev_err(tup->uport.dev, "dma_map_single tx failed\n");
+			dma_release_channel(dma_chan);
+			return -ENOMEM;
+		}
 		dma_buf = tup->uport.state->xmit.buf;
 	}
 
@@ -1013,32 +1040,10 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
 	return 0;
 
 scrub:
-	dma_release_channel(dma_chan);
+	tegra_uart_dma_channel_free(tup, dma_to_memory);
 	return ret;
 }
 
-static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
-		bool dma_to_memory)
-{
-	if (dma_to_memory) {
-		dmaengine_terminate_all(tup->rx_dma_chan);
-		dma_release_channel(tup->rx_dma_chan);
-		dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
-				tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
-		tup->rx_dma_chan = NULL;
-		tup->rx_dma_buf_phys = 0;
-		tup->rx_dma_buf_virt = NULL;
-	} else {
-		dmaengine_terminate_all(tup->tx_dma_chan);
-		dma_release_channel(tup->tx_dma_chan);
-		dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
-			UART_XMIT_SIZE, DMA_TO_DEVICE);
-		tup->tx_dma_chan = NULL;
-		tup->tx_dma_buf_phys = 0;
-		tup->tx_dma_buf_virt = NULL;
-	}
-}
-
 static int tegra_uart_startup(struct uart_port *u)
 {
 	struct tegra_uart_port *tup = to_tegra_uport(u);
-- 
1.9.1

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Alexandre Courbot]

On Tue, May 5, 2015 at 11:17 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
> Function tegra_uart_dma_channel_allocate() does not check that
> dma_map_single() mapped the DMA buffer correctly. Add a check for this
> and appropriate error handling.
>
> Furthermore, if dmaengine_slave_config() (called by
> tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
> is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
> instead of just dma_release_channel() if  dmaengine_slave_config() fails.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/tty/serial/serial-tegra.c | 51 +++++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
> index 96378da9aefc..3b63f103f0c9 100644
> --- a/drivers/tty/serial/serial-tegra.c
> +++ b/drivers/tty/serial/serial-tegra.c
> @@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
>         return 0;
>  }
>
> +static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
> +               bool dma_to_memory)
> +{
> +       if (dma_to_memory) {
> +               dmaengine_terminate_all(tup->rx_dma_chan);
> +               dma_release_channel(tup->rx_dma_chan);
> +               dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
> +                               tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
> +               tup->rx_dma_chan = NULL;
> +               tup->rx_dma_buf_phys = 0;
> +               tup->rx_dma_buf_virt = NULL;
> +       } else {
> +               dmaengine_terminate_all(tup->tx_dma_chan);
> +               dma_release_channel(tup->tx_dma_chan);
> +               dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
> +                       UART_XMIT_SIZE, DMA_TO_DEVICE);
> +               tup->tx_dma_chan = NULL;
> +               tup->tx_dma_buf_phys = 0;
> +               tup->tx_dma_buf_virt = NULL;
> +       }
> +}
> +
>  static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>                         bool dma_to_memory)
>  {
> @@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>                 dma_phys = dma_map_single(tup->uport.dev,
>                         tup->uport.state->xmit.buf, UART_XMIT_SIZE,
>                         DMA_TO_DEVICE);
> +               if (dma_mapping_error(tup->uport.dev, dma_phys)) {
> +                       dev_err(tup->uport.dev, "dma_map_single tx failed\n");
> +                       dma_release_channel(dma_chan);
> +                       return -ENOMEM;

Is -ENOMEM the error code we want to return here?

IIUC dma_buf will be leaked if an error occurs here because it has not
been assigned to your structure and will therefore be ignored when
tegra_uart_dma_channel_free() is called.

Since we have a "scrub" label at the end of this function, I think I'd
also prefer if this block and the one before could jump to error
labels as well for consistency.

Otherwise, pretty nice series, comprehensive and easy to read.

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Jon Hunter]

On 12/05/15 09:39, Alexandre Courbot wrote:
> On Tue, May 5, 2015 at 11:17 PM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> Function tegra_uart_dma_channel_allocate() does not check that
>> dma_map_single() mapped the DMA buffer correctly. Add a check for this
>> and appropriate error handling.
>>
>> Furthermore, if dmaengine_slave_config() (called by
>> tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
>> is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
>> instead of just dma_release_channel() if  dmaengine_slave_config() fails.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/tty/serial/serial-tegra.c | 51 +++++++++++++++++++++------------------
>>  1 file changed, 28 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
>> index 96378da9aefc..3b63f103f0c9 100644
>> --- a/drivers/tty/serial/serial-tegra.c
>> +++ b/drivers/tty/serial/serial-tegra.c
>> @@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
>>         return 0;
>>  }
>>
>> +static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
>> +               bool dma_to_memory)
>> +{
>> +       if (dma_to_memory) {
>> +               dmaengine_terminate_all(tup->rx_dma_chan);
>> +               dma_release_channel(tup->rx_dma_chan);
>> +               dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
>> +                               tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
>> +               tup->rx_dma_chan = NULL;
>> +               tup->rx_dma_buf_phys = 0;
>> +               tup->rx_dma_buf_virt = NULL;
>> +       } else {
>> +               dmaengine_terminate_all(tup->tx_dma_chan);
>> +               dma_release_channel(tup->tx_dma_chan);
>> +               dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
>> +                       UART_XMIT_SIZE, DMA_TO_DEVICE);
>> +               tup->tx_dma_chan = NULL;
>> +               tup->tx_dma_buf_phys = 0;
>> +               tup->tx_dma_buf_virt = NULL;
>> +       }
>> +}
>> +
>>  static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>                         bool dma_to_memory)
>>  {
>> @@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>                 dma_phys = dma_map_single(tup->uport.dev,
>>                         tup->uport.state->xmit.buf, UART_XMIT_SIZE,
>>                         DMA_TO_DEVICE);
>> +               if (dma_mapping_error(tup->uport.dev, dma_phys)) {
>> +                       dev_err(tup->uport.dev, "dma_map_single tx failed\n");
>> +                       dma_release_channel(dma_chan);
>> +                       return -ENOMEM;
> 
> Is -ENOMEM the error code we want to return here?

I think that it is appropriate as we are unable to map the memory we are
requesting. I did look at a few other drivers and several return -ENOMEM
here. I saw others return -EFAULT, but given this is memory related,
seems ok, unless you have a better suggestion.

> IIUC dma_buf will be leaked if an error occurs here because it has not
> been assigned to your structure and will therefore be ignored when
> tegra_uart_dma_channel_free() is called.

In the original code, if dmaengine_slave_config() failed, then yes there
would be a memory leak. That should no longer be the case.

> Since we have a "scrub" label at the end of this function, I think I'd
> also prefer if this block and the one before could jump to error
> labels as well for consistency.

Yes I see. I wondered if it would be better to just get rid of the
"scrub" label since it is only used in one place instead?

By the way, I got a notification from Greg that these are now queued in
his tty-testing branch [1]. Assuming these are ok, may be I could fix
that up in a follow-up patch?

> Otherwise, pretty nice series, comprehensive and easy to read.

Thanks!
Jon

[1]
http://git.kernel.org/cgit/linux/kernel/git/gregkh/tty.git/log/?h=tty-testing

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Jon Hunter]

On 12/05/15 09:39, Alexandre Courbot wrote:
> On Tue, May 5, 2015 at 11:17 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Function tegra_uart_dma_channel_allocate() does not check that
>> dma_map_single() mapped the DMA buffer correctly. Add a check for this
>> and appropriate error handling.
>>
>> Furthermore, if dmaengine_slave_config() (called by
>> tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
>> is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
>> instead of just dma_release_channel() if  dmaengine_slave_config() fails.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/tty/serial/serial-tegra.c | 51 +++++++++++++++++++++------------------
>>  1 file changed, 28 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
>> index 96378da9aefc..3b63f103f0c9 100644
>> --- a/drivers/tty/serial/serial-tegra.c
>> +++ b/drivers/tty/serial/serial-tegra.c
>> @@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
>>         return 0;
>>  }
>>
>> +static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
>> +               bool dma_to_memory)
>> +{
>> +       if (dma_to_memory) {
>> +               dmaengine_terminate_all(tup->rx_dma_chan);
>> +               dma_release_channel(tup->rx_dma_chan);
>> +               dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
>> +                               tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
>> +               tup->rx_dma_chan = NULL;
>> +               tup->rx_dma_buf_phys = 0;
>> +               tup->rx_dma_buf_virt = NULL;
>> +       } else {
>> +               dmaengine_terminate_all(tup->tx_dma_chan);
>> +               dma_release_channel(tup->tx_dma_chan);
>> +               dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
>> +                       UART_XMIT_SIZE, DMA_TO_DEVICE);
>> +               tup->tx_dma_chan = NULL;
>> +               tup->tx_dma_buf_phys = 0;
>> +               tup->tx_dma_buf_virt = NULL;
>> +       }
>> +}
>> +
>>  static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>                         bool dma_to_memory)
>>  {
>> @@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>                 dma_phys = dma_map_single(tup->uport.dev,
>>                         tup->uport.state->xmit.buf, UART_XMIT_SIZE,
>>                         DMA_TO_DEVICE);
>> +               if (dma_mapping_error(tup->uport.dev, dma_phys)) {
>> +                       dev_err(tup->uport.dev, "dma_map_single tx failed\n");
>> +                       dma_release_channel(dma_chan);
>> +                       return -ENOMEM;
> 
> Is -ENOMEM the error code we want to return here?

I think that it is appropriate as we are unable to map the memory we are
requesting. I did look at a few other drivers and several return -ENOMEM
here. I saw others return -EFAULT, but given this is memory related,
seems ok, unless you have a better suggestion.

> IIUC dma_buf will be leaked if an error occurs here because it has not
> been assigned to your structure and will therefore be ignored when
> tegra_uart_dma_channel_free() is called.

In the original code, if dmaengine_slave_config() failed, then yes there
would be a memory leak. That should no longer be the case.

> Since we have a "scrub" label at the end of this function, I think I'd
> also prefer if this block and the one before could jump to error
> labels as well for consistency.

Yes I see. I wondered if it would be better to just get rid of the
"scrub" label since it is only used in one place instead?

By the way, I got a notification from Greg that these are now queued in
his tty-testing branch [1]. Assuming these are ok, may be I could fix
that up in a follow-up patch?

> Otherwise, pretty nice series, comprehensive and easy to read.

Thanks!
Jon

[1]
http://git.kernel.org/cgit/linux/kernel/git/gregkh/tty.git/log/?h=tty-testing

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Alexandre Courbot]

On Tue, May 12, 2015 at 6:51 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 12/05/15 09:39, Alexandre Courbot wrote:
>> On Tue, May 5, 2015 at 11:17 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> Function tegra_uart_dma_channel_allocate() does not check that
>>> dma_map_single() mapped the DMA buffer correctly. Add a check for this
>>> and appropriate error handling.
>>>
>>> Furthermore, if dmaengine_slave_config() (called by
>>> tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
>>> is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
>>> instead of just dma_release_channel() if  dmaengine_slave_config() fails.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  drivers/tty/serial/serial-tegra.c | 51 +++++++++++++++++++++------------------
>>>  1 file changed, 28 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
>>> index 96378da9aefc..3b63f103f0c9 100644
>>> --- a/drivers/tty/serial/serial-tegra.c
>>> +++ b/drivers/tty/serial/serial-tegra.c
>>> @@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
>>>         return 0;
>>>  }
>>>
>>> +static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
>>> +               bool dma_to_memory)
>>> +{
>>> +       if (dma_to_memory) {
>>> +               dmaengine_terminate_all(tup->rx_dma_chan);
>>> +               dma_release_channel(tup->rx_dma_chan);
>>> +               dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
>>> +                               tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
>>> +               tup->rx_dma_chan = NULL;
>>> +               tup->rx_dma_buf_phys = 0;
>>> +               tup->rx_dma_buf_virt = NULL;
>>> +       } else {
>>> +               dmaengine_terminate_all(tup->tx_dma_chan);
>>> +               dma_release_channel(tup->tx_dma_chan);
>>> +               dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
>>> +                       UART_XMIT_SIZE, DMA_TO_DEVICE);
>>> +               tup->tx_dma_chan = NULL;
>>> +               tup->tx_dma_buf_phys = 0;
>>> +               tup->tx_dma_buf_virt = NULL;
>>> +       }
>>> +}
>>> +
>>>  static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>>                         bool dma_to_memory)
>>>  {
>>> @@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>>                 dma_phys = dma_map_single(tup->uport.dev,
>>>                         tup->uport.state->xmit.buf, UART_XMIT_SIZE,
>>>                         DMA_TO_DEVICE);
>>> +               if (dma_mapping_error(tup->uport.dev, dma_phys)) {
>>> +                       dev_err(tup->uport.dev, "dma_map_single tx failed\n");
>>> +                       dma_release_channel(dma_chan);
>>> +                       return -ENOMEM;
>>
>> Is -ENOMEM the error code we want to return here?
>
> I think that it is appropriate as we are unable to map the memory we are
> requesting. I did look at a few other drivers and several return -ENOMEM
> here. I saw others return -EFAULT, but given this is memory related,
> seems ok, unless you have a better suggestion.
>
>> IIUC dma_buf will be leaked if an error occurs here because it has not
>> been assigned to your structure and will therefore be ignored when
>> tegra_uart_dma_channel_free() is called.
>
> In the original code, if dmaengine_slave_config() failed, then yes there
> would be a memory leak. That should no longer be the case.

Mmm I am pretty sure that even after your patch the memory allocated
through the DMA API will not be freed if we hit an error there,
because dma_buf/dma_phys are not yet affected to your tegra_uart_port
structure when you call dma_release_channel(). Or maybe I am missing
something?

>
>> Since we have a "scrub" label at the end of this function, I think I'd
>> also prefer if this block and the one before could jump to error
>> labels as well for consistency.
>
> Yes I see. I wondered if it would be better to just get rid of the
> "scrub" label since it is only used in one place instead?

I am fine with whichever makes the most sense, although I am biased
towards having all error handing at the end of the function. But your
call.

>
> By the way, I got a notification from Greg that these are now queued in
> his tty-testing branch [1]. Assuming these are ok, may be I could fix
> that up in a follow-up patch?

Sounds good!

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Jon Hunter]

On 13/05/15 05:56, Alexandre Courbot wrote:
> On Tue, May 12, 2015 at 6:51 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 12/05/15 09:39, Alexandre Courbot wrote:
>>> On Tue, May 5, 2015 at 11:17 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>> Function tegra_uart_dma_channel_allocate() does not check that
>>>> dma_map_single() mapped the DMA buffer correctly. Add a check for this
>>>> and appropriate error handling.
>>>>
>>>> Furthermore, if dmaengine_slave_config() (called by
>>>> tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
>>>> is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
>>>> instead of just dma_release_channel() if  dmaengine_slave_config() fails.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>> ---
>>>>  drivers/tty/serial/serial-tegra.c | 51 +++++++++++++++++++++------------------
>>>>  1 file changed, 28 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
>>>> index 96378da9aefc..3b63f103f0c9 100644
>>>> --- a/drivers/tty/serial/serial-tegra.c
>>>> +++ b/drivers/tty/serial/serial-tegra.c
>>>> @@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
>>>>         return 0;
>>>>  }
>>>>
>>>> +static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
>>>> +               bool dma_to_memory)
>>>> +{
>>>> +       if (dma_to_memory) {
>>>> +               dmaengine_terminate_all(tup->rx_dma_chan);
>>>> +               dma_release_channel(tup->rx_dma_chan);
>>>> +               dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
>>>> +                               tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
>>>> +               tup->rx_dma_chan = NULL;
>>>> +               tup->rx_dma_buf_phys = 0;
>>>> +               tup->rx_dma_buf_virt = NULL;
>>>> +       } else {
>>>> +               dmaengine_terminate_all(tup->tx_dma_chan);
>>>> +               dma_release_channel(tup->tx_dma_chan);
>>>> +               dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
>>>> +                       UART_XMIT_SIZE, DMA_TO_DEVICE);
>>>> +               tup->tx_dma_chan = NULL;
>>>> +               tup->tx_dma_buf_phys = 0;
>>>> +               tup->tx_dma_buf_virt = NULL;
>>>> +       }
>>>> +}
>>>> +
>>>>  static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>>>                         bool dma_to_memory)
>>>>  {
>>>> @@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>>>                 dma_phys = dma_map_single(tup->uport.dev,
>>>>                         tup->uport.state->xmit.buf, UART_XMIT_SIZE,
>>>>                         DMA_TO_DEVICE);
>>>> +               if (dma_mapping_error(tup->uport.dev, dma_phys)) {
>>>> +                       dev_err(tup->uport.dev, "dma_map_single tx failed\n");
>>>> +                       dma_release_channel(dma_chan);
>>>> +                       return -ENOMEM;
>>>
>>> Is -ENOMEM the error code we want to return here?
>>
>> I think that it is appropriate as we are unable to map the memory we are
>> requesting. I did look at a few other drivers and several return -ENOMEM
>> here. I saw others return -EFAULT, but given this is memory related,
>> seems ok, unless you have a better suggestion.
>>
>>> IIUC dma_buf will be leaked if an error occurs here because it has not
>>> been assigned to your structure and will therefore be ignored when
>>> tegra_uart_dma_channel_free() is called.
>>
>> In the original code, if dmaengine_slave_config() failed, then yes there
>> would be a memory leak. That should no longer be the case.
> 
> Mmm I am pretty sure that even after your patch the memory allocated
> through the DMA API will not be freed if we hit an error there,
> because dma_buf/dma_phys are not yet affected to your tegra_uart_port
> structure when you call dma_release_channel(). Or maybe I am missing
> something?

So there are two paths through the tegra_uart_dma_channel_allocate()
function, one for RX and one for TX. In the RX case, a buffer is
allocated via dma_alloc_coherent() and if this fails, then we simply
call dma_release_channel(). So there should not be any memory leaked in
this path and we should not need to worry about dma_buf/dma_phys here.

In the TX case, the xmit.buf (allocated by the serial_core driver) is
mapped to physical space for DMA. If the mapping fails, the xmit.buf is
not freed here and we simply call dma_release_channel().

If you are concerned about the xmit.buf, then this is part serial core
and allocated when uart_open() is called. It uart_open() fails, because
the tegra_uart_dma_channel_allocate() fails, then uart_close() will be
called (according the to kernel-doc for uart_open) and should be freed
when uart_shutdown() is called. So I don't see a problem here.

Let me know if I am misunderstanding you.

>>
>>> Since we have a "scrub" label at the end of this function, I think I'd
>>> also prefer if this block and the one before could jump to error
>>> labels as well for consistency.
>>
>> Yes I see. I wondered if it would be better to just get rid of the
>> "scrub" label since it is only used in one place instead?
> 
> I am fine with whichever makes the most sense, although I am biased
> towards having all error handing at the end of the function. But your
> call.

Ok.

Cheers
Jon

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Jon Hunter]

On 13/05/15 05:56, Alexandre Courbot wrote:
> On Tue, May 12, 2015 at 6:51 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 12/05/15 09:39, Alexandre Courbot wrote:
>>> On Tue, May 5, 2015 at 11:17 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>> Function tegra_uart_dma_channel_allocate() does not check that
>>>> dma_map_single() mapped the DMA buffer correctly. Add a check for this
>>>> and appropriate error handling.
>>>>
>>>> Furthermore, if dmaengine_slave_config() (called by
>>>> tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
>>>> is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
>>>> instead of just dma_release_channel() if  dmaengine_slave_config() fails.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>> ---
>>>>  drivers/tty/serial/serial-tegra.c | 51 +++++++++++++++++++++------------------
>>>>  1 file changed, 28 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
>>>> index 96378da9aefc..3b63f103f0c9 100644
>>>> --- a/drivers/tty/serial/serial-tegra.c
>>>> +++ b/drivers/tty/serial/serial-tegra.c
>>>> @@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
>>>>         return 0;
>>>>  }
>>>>
>>>> +static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
>>>> +               bool dma_to_memory)
>>>> +{
>>>> +       if (dma_to_memory) {
>>>> +               dmaengine_terminate_all(tup->rx_dma_chan);
>>>> +               dma_release_channel(tup->rx_dma_chan);
>>>> +               dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
>>>> +                               tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
>>>> +               tup->rx_dma_chan = NULL;
>>>> +               tup->rx_dma_buf_phys = 0;
>>>> +               tup->rx_dma_buf_virt = NULL;
>>>> +       } else {
>>>> +               dmaengine_terminate_all(tup->tx_dma_chan);
>>>> +               dma_release_channel(tup->tx_dma_chan);
>>>> +               dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
>>>> +                       UART_XMIT_SIZE, DMA_TO_DEVICE);
>>>> +               tup->tx_dma_chan = NULL;
>>>> +               tup->tx_dma_buf_phys = 0;
>>>> +               tup->tx_dma_buf_virt = NULL;
>>>> +       }
>>>> +}
>>>> +
>>>>  static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>>>                         bool dma_to_memory)
>>>>  {
>>>> @@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>>>                 dma_phys = dma_map_single(tup->uport.dev,
>>>>                         tup->uport.state->xmit.buf, UART_XMIT_SIZE,
>>>>                         DMA_TO_DEVICE);
>>>> +               if (dma_mapping_error(tup->uport.dev, dma_phys)) {
>>>> +                       dev_err(tup->uport.dev, "dma_map_single tx failed\n");
>>>> +                       dma_release_channel(dma_chan);
>>>> +                       return -ENOMEM;
>>>
>>> Is -ENOMEM the error code we want to return here?
>>
>> I think that it is appropriate as we are unable to map the memory we are
>> requesting. I did look at a few other drivers and several return -ENOMEM
>> here. I saw others return -EFAULT, but given this is memory related,
>> seems ok, unless you have a better suggestion.
>>
>>> IIUC dma_buf will be leaked if an error occurs here because it has not
>>> been assigned to your structure and will therefore be ignored when
>>> tegra_uart_dma_channel_free() is called.
>>
>> In the original code, if dmaengine_slave_config() failed, then yes there
>> would be a memory leak. That should no longer be the case.
> 
> Mmm I am pretty sure that even after your patch the memory allocated
> through the DMA API will not be freed if we hit an error there,
> because dma_buf/dma_phys are not yet affected to your tegra_uart_port
> structure when you call dma_release_channel(). Or maybe I am missing
> something?

So there are two paths through the tegra_uart_dma_channel_allocate()
function, one for RX and one for TX. In the RX case, a buffer is
allocated via dma_alloc_coherent() and if this fails, then we simply
call dma_release_channel(). So there should not be any memory leaked in
this path and we should not need to worry about dma_buf/dma_phys here.

In the TX case, the xmit.buf (allocated by the serial_core driver) is
mapped to physical space for DMA. If the mapping fails, the xmit.buf is
not freed here and we simply call dma_release_channel().

If you are concerned about the xmit.buf, then this is part serial core
and allocated when uart_open() is called. It uart_open() fails, because
the tegra_uart_dma_channel_allocate() fails, then uart_close() will be
called (according the to kernel-doc for uart_open) and should be freed
when uart_shutdown() is called. So I don't see a problem here.

Let me know if I am misunderstanding you.

>>
>>> Since we have a "scrub" label at the end of this function, I think I'd
>>> also prefer if this block and the one before could jump to error
>>> labels as well for consistency.
>>
>> Yes I see. I wondered if it would be better to just get rid of the
>> "scrub" label since it is only used in one place instead?
> 
> I am fine with whichever makes the most sense, although I am biased
> towards having all error handing at the end of the function. But your
> call.

Ok.

Cheers
Jon

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Alexandre Courbot]

On Wed, May 13, 2015 at 5:23 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 13/05/15 05:56, Alexandre Courbot wrote:
>> On Tue, May 12, 2015 at 6:51 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>
>>> On 12/05/15 09:39, Alexandre Courbot wrote:
>>>> On Tue, May 5, 2015 at 11:17 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>> Function tegra_uart_dma_channel_allocate() does not check that
>>>>> dma_map_single() mapped the DMA buffer correctly. Add a check for this
>>>>> and appropriate error handling.
>>>>>
>>>>> Furthermore, if dmaengine_slave_config() (called by
>>>>> tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
>>>>> is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
>>>>> instead of just dma_release_channel() if  dmaengine_slave_config() fails.
>>>>>
>>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>>> ---
>>>>>  drivers/tty/serial/serial-tegra.c | 51 +++++++++++++++++++++------------------
>>>>>  1 file changed, 28 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
>>>>> index 96378da9aefc..3b63f103f0c9 100644
>>>>> --- a/drivers/tty/serial/serial-tegra.c
>>>>> +++ b/drivers/tty/serial/serial-tegra.c
>>>>> @@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
>>>>>         return 0;
>>>>>  }
>>>>>
>>>>> +static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
>>>>> +               bool dma_to_memory)
>>>>> +{
>>>>> +       if (dma_to_memory) {
>>>>> +               dmaengine_terminate_all(tup->rx_dma_chan);
>>>>> +               dma_release_channel(tup->rx_dma_chan);
>>>>> +               dma_free_coherent(tup->uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
>>>>> +                               tup->rx_dma_buf_virt, tup->rx_dma_buf_phys);
>>>>> +               tup->rx_dma_chan = NULL;
>>>>> +               tup->rx_dma_buf_phys = 0;
>>>>> +               tup->rx_dma_buf_virt = NULL;
>>>>> +       } else {
>>>>> +               dmaengine_terminate_all(tup->tx_dma_chan);
>>>>> +               dma_release_channel(tup->tx_dma_chan);
>>>>> +               dma_unmap_single(tup->uport.dev, tup->tx_dma_buf_phys,
>>>>> +                       UART_XMIT_SIZE, DMA_TO_DEVICE);
>>>>> +               tup->tx_dma_chan = NULL;
>>>>> +               tup->tx_dma_buf_phys = 0;
>>>>> +               tup->tx_dma_buf_virt = NULL;
>>>>> +       }
>>>>> +}
>>>>> +
>>>>>  static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>>>>                         bool dma_to_memory)
>>>>>  {
>>>>> @@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>>>>>                 dma_phys = dma_map_single(tup->uport.dev,
>>>>>                         tup->uport.state->xmit.buf, UART_XMIT_SIZE,
>>>>>                         DMA_TO_DEVICE);
>>>>> +               if (dma_mapping_error(tup->uport.dev, dma_phys)) {
>>>>> +                       dev_err(tup->uport.dev, "dma_map_single tx failed\n");
>>>>> +                       dma_release_channel(dma_chan);
>>>>> +                       return -ENOMEM;
>>>>
>>>> Is -ENOMEM the error code we want to return here?
>>>
>>> I think that it is appropriate as we are unable to map the memory we are
>>> requesting. I did look at a few other drivers and several return -ENOMEM
>>> here. I saw others return -EFAULT, but given this is memory related,
>>> seems ok, unless you have a better suggestion.
>>>
>>>> IIUC dma_buf will be leaked if an error occurs here because it has not
>>>> been assigned to your structure and will therefore be ignored when
>>>> tegra_uart_dma_channel_free() is called.
>>>
>>> In the original code, if dmaengine_slave_config() failed, then yes there
>>> would be a memory leak. That should no longer be the case.
>>
>> Mmm I am pretty sure that even after your patch the memory allocated
>> through the DMA API will not be freed if we hit an error there,
>> because dma_buf/dma_phys are not yet affected to your tegra_uart_port
>> structure when you call dma_release_channel(). Or maybe I am missing
>> something?
>
> So there are two paths through the tegra_uart_dma_channel_allocate()
> function, one for RX and one for TX. In the RX case, a buffer is
> allocated via dma_alloc_coherent() and if this fails, then we simply
> call dma_release_channel(). So there should not be any memory leaked in
> this path and we should not need to worry about dma_buf/dma_phys here.
>
> In the TX case, the xmit.buf (allocated by the serial_core driver) is
> mapped to physical space for DMA. If the mapping fails, the xmit.buf is
> not freed here and we simply call dma_release_channel().
>
> If you are concerned about the xmit.buf, then this is part serial core
> and allocated when uart_open() is called. It uart_open() fails, because
> the tegra_uart_dma_channel_allocate() fails, then uart_close() will be
> called (according the to kernel-doc for uart_open) and should be freed
> when uart_shutdown() is called. So I don't see a problem here.
>
> Let me know if I am misunderstanding you.

You are right - I overlooked the fact there were RX and TX paths.

There may still be a leak (that is not related to your patch) in the
RX path though:

    dma_buf = dma_alloc_coherent(...);
    ret = dmaengine_slave_config(...);
    if (ret < 0) {
        ...
        goto scrub;
    }

    tup->rx_dma_buf_virt = dma_buf;
    tup->rx_dma_buf_phys = dma_phys;

scrub:
    dma_release_channel(dma_chan);
    return ret;

It seems that if dmaengine_slave_config() fails, then the result of
dma_alloc_coherent() remains purely local to the function and is never
freed. Or am I missing something again?

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Jon Hunter]

On 20/05/15 04:57, Alexandre Courbot wrote:

[snip]

> There may still be a leak (that is not related to your patch) in the
> RX path though:
> 
>     dma_buf = dma_alloc_coherent(...);
>     ret = dmaengine_slave_config(...);
>     if (ret < 0) {
>         ...
>         goto scrub;
>     }
> 
>     tup->rx_dma_buf_virt = dma_buf;
>     tup->rx_dma_buf_phys = dma_phys;
> 
> scrub:
>     dma_release_channel(dma_chan);
>     return ret;
> 
> It seems that if dmaengine_slave_config() fails, then the result of
> dma_alloc_coherent() remains purely local to the function and is never
> freed. Or am I missing something again?

Yes, you are right here. Actually, this would be applicable to
both RX and TX paths because in the TX path the buffer would
not be unmapped. I will generated a separate patch for fix
this. How does the following look?

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 3b63f103f0c9..cf0133ae762d 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -999,6 +999,12 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
                        dma_release_channel(dma_chan);
                        return -ENOMEM;
                }
+               dma_sconfig.src_addr = tup->uport.mapbase;
+               dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+               dma_sconfig.src_maxburst = 4;
+               tup->rx_dma_chan = dma_chan;
+               tup->rx_dma_buf_virt = dma_buf;
+               tup->rx_dma_buf_phys = dma_phys;
        } else {
                dma_phys = dma_map_single(tup->uport.dev,
                        tup->uport.state->xmit.buf, UART_XMIT_SIZE,
@@ -1009,39 +1015,23 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
                        return -ENOMEM;
                }
                dma_buf = tup->uport.state->xmit.buf;
-       }
-
-       if (dma_to_memory) {
-               dma_sconfig.src_addr = tup->uport.mapbase;
-               dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
-               dma_sconfig.src_maxburst = 4;
-       } else {
                dma_sconfig.dst_addr = tup->uport.mapbase;
                dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
                dma_sconfig.dst_maxburst = 16;
+               tup->tx_dma_chan = dma_chan;
+               tup->tx_dma_buf_virt = dma_buf;
+               tup->tx_dma_buf_phys = dma_phys;
        }
 
        ret = dmaengine_slave_config(dma_chan, &dma_sconfig);
        if (ret < 0) {
                dev_err(tup->uport.dev,
                        "Dma slave config failed, err = %d\n", ret);
-               goto scrub;
+               tegra_uart_dma_channel_free(tup, dma_to_memory);
+               return ret;
        }
 
-       if (dma_to_memory) {
-               tup->rx_dma_chan = dma_chan;
-               tup->rx_dma_buf_virt = dma_buf;
-               tup->rx_dma_buf_phys = dma_phys;
-       } else {
-               tup->tx_dma_chan = dma_chan;
-               tup->tx_dma_buf_virt = dma_buf;
-               tup->tx_dma_buf_phys = dma_phys;
-       }
        return 0;
-
-scrub:
-       tegra_uart_dma_channel_free(tup, dma_to_memory);
-       return ret;
 }
 
Cheers
Jon

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Jon Hunter]

On 20/05/15 04:57, Alexandre Courbot wrote:

[snip]

> There may still be a leak (that is not related to your patch) in the
> RX path though:
> 
>     dma_buf = dma_alloc_coherent(...);
>     ret = dmaengine_slave_config(...);
>     if (ret < 0) {
>         ...
>         goto scrub;
>     }
> 
>     tup->rx_dma_buf_virt = dma_buf;
>     tup->rx_dma_buf_phys = dma_phys;
> 
> scrub:
>     dma_release_channel(dma_chan);
>     return ret;
> 
> It seems that if dmaengine_slave_config() fails, then the result of
> dma_alloc_coherent() remains purely local to the function and is never
> freed. Or am I missing something again?

Yes, you are right here. Actually, this would be applicable to
both RX and TX paths because in the TX path the buffer would
not be unmapped. I will generated a separate patch for fix
this. How does the following look?

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 3b63f103f0c9..cf0133ae762d 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -999,6 +999,12 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
                        dma_release_channel(dma_chan);
                        return -ENOMEM;
                }
+               dma_sconfig.src_addr = tup->uport.mapbase;
+               dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+               dma_sconfig.src_maxburst = 4;
+               tup->rx_dma_chan = dma_chan;
+               tup->rx_dma_buf_virt = dma_buf;
+               tup->rx_dma_buf_phys = dma_phys;
        } else {
                dma_phys = dma_map_single(tup->uport.dev,
                        tup->uport.state->xmit.buf, UART_XMIT_SIZE,
@@ -1009,39 +1015,23 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
                        return -ENOMEM;
                }
                dma_buf = tup->uport.state->xmit.buf;
-       }
-
-       if (dma_to_memory) {
-               dma_sconfig.src_addr = tup->uport.mapbase;
-               dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
-               dma_sconfig.src_maxburst = 4;
-       } else {
                dma_sconfig.dst_addr = tup->uport.mapbase;
                dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
                dma_sconfig.dst_maxburst = 16;
+               tup->tx_dma_chan = dma_chan;
+               tup->tx_dma_buf_virt = dma_buf;
+               tup->tx_dma_buf_phys = dma_phys;
        }
 
        ret = dmaengine_slave_config(dma_chan, &dma_sconfig);
        if (ret < 0) {
                dev_err(tup->uport.dev,
                        "Dma slave config failed, err = %d\n", ret);
-               goto scrub;
+               tegra_uart_dma_channel_free(tup, dma_to_memory);
+               return ret;
        }
 
-       if (dma_to_memory) {
-               tup->rx_dma_chan = dma_chan;
-               tup->rx_dma_buf_virt = dma_buf;
-               tup->rx_dma_buf_phys = dma_phys;
-       } else {
-               tup->tx_dma_chan = dma_chan;
-               tup->tx_dma_buf_virt = dma_buf;
-               tup->tx_dma_buf_phys = dma_phys;
-       }
        return 0;
-
-scrub:
-       tegra_uart_dma_channel_free(tup, dma_to_memory);
-       return ret;
 }
 
Cheers
Jon

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Jon Hunter]

On 20/05/15 10:51, Jon Hunter wrote:
> 
> On 20/05/15 04:57, Alexandre Courbot wrote:
> 
> [snip]
> 
>> There may still be a leak (that is not related to your patch) in the
>> RX path though:
>>
>>     dma_buf = dma_alloc_coherent(...);
>>     ret = dmaengine_slave_config(...);
>>     if (ret < 0) {
>>         ...
>>         goto scrub;
>>     }
>>
>>     tup->rx_dma_buf_virt = dma_buf;
>>     tup->rx_dma_buf_phys = dma_phys;
>>
>> scrub:
>>     dma_release_channel(dma_chan);
>>     return ret;
>>
>> It seems that if dmaengine_slave_config() fails, then the result of
>> dma_alloc_coherent() remains purely local to the function and is never
>> freed. Or am I missing something again?
> 
> Yes, you are right here. Actually, this would be applicable to
> both RX and TX paths because in the TX path the buffer would
> not be unmapped. I will generated a separate patch for fix
> this. How does the following look?
> 
> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
> index 3b63f103f0c9..cf0133ae762d 100644
> --- a/drivers/tty/serial/serial-tegra.c
> +++ b/drivers/tty/serial/serial-tegra.c
> @@ -999,6 +999,12 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>                         dma_release_channel(dma_chan);
>                         return -ENOMEM;
>                 }
> +               dma_sconfig.src_addr = tup->uport.mapbase;
> +               dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +               dma_sconfig.src_maxburst = 4;
> +               tup->rx_dma_chan = dma_chan;
> +               tup->rx_dma_buf_virt = dma_buf;
> +               tup->rx_dma_buf_phys = dma_phys;
>         } else {
>                 dma_phys = dma_map_single(tup->uport.dev,
>                         tup->uport.state->xmit.buf, UART_XMIT_SIZE,
> @@ -1009,39 +1015,23 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>                         return -ENOMEM;
>                 }
>                 dma_buf = tup->uport.state->xmit.buf;
> -       }
> -
> -       if (dma_to_memory) {
> -               dma_sconfig.src_addr = tup->uport.mapbase;
> -               dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> -               dma_sconfig.src_maxburst = 4;
> -       } else {
>                 dma_sconfig.dst_addr = tup->uport.mapbase;
>                 dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>                 dma_sconfig.dst_maxburst = 16;
> +               tup->tx_dma_chan = dma_chan;
> +               tup->tx_dma_buf_virt = dma_buf;
> +               tup->tx_dma_buf_phys = dma_phys;
>         }
>  
>         ret = dmaengine_slave_config(dma_chan, &dma_sconfig);
>         if (ret < 0) {
>                 dev_err(tup->uport.dev,
>                         "Dma slave config failed, err = %d\n", ret);
> -               goto scrub;
> +               tegra_uart_dma_channel_free(tup, dma_to_memory);
> +               return ret;
>         }
>  
> -       if (dma_to_memory) {
> -               tup->rx_dma_chan = dma_chan;
> -               tup->rx_dma_buf_virt = dma_buf;
> -               tup->rx_dma_buf_phys = dma_phys;
> -       } else {
> -               tup->tx_dma_chan = dma_chan;
> -               tup->tx_dma_buf_virt = dma_buf;
> -               tup->tx_dma_buf_phys = dma_phys;
> -       }
>         return 0;
> -
> -scrub:
> -       tegra_uart_dma_channel_free(tup, dma_to_memory);
> -       return ret;
>  }

Posted the patch here [1].

Jon

[1] https://lkml.org/lkml/2015/5/20/347

Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup

[Jon Hunter]

On 20/05/15 10:51, Jon Hunter wrote:
> 
> On 20/05/15 04:57, Alexandre Courbot wrote:
> 
> [snip]
> 
>> There may still be a leak (that is not related to your patch) in the
>> RX path though:
>>
>>     dma_buf = dma_alloc_coherent(...);
>>     ret = dmaengine_slave_config(...);
>>     if (ret < 0) {
>>         ...
>>         goto scrub;
>>     }
>>
>>     tup->rx_dma_buf_virt = dma_buf;
>>     tup->rx_dma_buf_phys = dma_phys;
>>
>> scrub:
>>     dma_release_channel(dma_chan);
>>     return ret;
>>
>> It seems that if dmaengine_slave_config() fails, then the result of
>> dma_alloc_coherent() remains purely local to the function and is never
>> freed. Or am I missing something again?
> 
> Yes, you are right here. Actually, this would be applicable to
> both RX and TX paths because in the TX path the buffer would
> not be unmapped. I will generated a separate patch for fix
> this. How does the following look?
> 
> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
> index 3b63f103f0c9..cf0133ae762d 100644
> --- a/drivers/tty/serial/serial-tegra.c
> +++ b/drivers/tty/serial/serial-tegra.c
> @@ -999,6 +999,12 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>                         dma_release_channel(dma_chan);
>                         return -ENOMEM;
>                 }
> +               dma_sconfig.src_addr = tup->uport.mapbase;
> +               dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +               dma_sconfig.src_maxburst = 4;
> +               tup->rx_dma_chan = dma_chan;
> +               tup->rx_dma_buf_virt = dma_buf;
> +               tup->rx_dma_buf_phys = dma_phys;
>         } else {
>                 dma_phys = dma_map_single(tup->uport.dev,
>                         tup->uport.state->xmit.buf, UART_XMIT_SIZE,
> @@ -1009,39 +1015,23 @@ static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
>                         return -ENOMEM;
>                 }
>                 dma_buf = tup->uport.state->xmit.buf;
> -       }
> -
> -       if (dma_to_memory) {
> -               dma_sconfig.src_addr = tup->uport.mapbase;
> -               dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> -               dma_sconfig.src_maxburst = 4;
> -       } else {
>                 dma_sconfig.dst_addr = tup->uport.mapbase;
>                 dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>                 dma_sconfig.dst_maxburst = 16;
> +               tup->tx_dma_chan = dma_chan;
> +               tup->tx_dma_buf_virt = dma_buf;
> +               tup->tx_dma_buf_phys = dma_phys;
>         }
>  
>         ret = dmaengine_slave_config(dma_chan, &dma_sconfig);
>         if (ret < 0) {
>                 dev_err(tup->uport.dev,
>                         "Dma slave config failed, err = %d\n", ret);
> -               goto scrub;
> +               tegra_uart_dma_channel_free(tup, dma_to_memory);
> +               return ret;
>         }
>  
> -       if (dma_to_memory) {
> -               tup->rx_dma_chan = dma_chan;
> -               tup->rx_dma_buf_virt = dma_buf;
> -               tup->rx_dma_buf_phys = dma_phys;
> -       } else {
> -               tup->tx_dma_chan = dma_chan;
> -               tup->tx_dma_buf_virt = dma_buf;
> -               tup->tx_dma_buf_phys = dma_phys;
> -       }
>         return 0;
> -
> -scrub:
> -       tegra_uart_dma_channel_free(tup, dma_to_memory);
> -       return ret;
>  }

Posted the patch here [1].

Jon

[1] https://lkml.org/lkml/2015/5/20/347