serial: tegra: various fixes

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 <[email protected]>

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

drivers/tty/serial/serial-tegra.c +22 -0

index 96378da9aefc..3b63f103f0c9 100644
@@ -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)
 {

Quoted 49 lines · 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 <[email protected]>

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

drivers/tty/serial/serial-tegra.c +22 -0

index 96378da9aefc..3b63f103f0c9 100644
@@ -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,

drivers/tty/serial/serial-tegra.c static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup, · line 1009

Quoted 6 lines

drivers/tty/serial/serial-tegra.c +4 -0

                        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.

Quoted 50 lines · Alexandre Courbot

Quoted 49 lines · 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 <[email protected]>

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

drivers/tty/serial/serial-tegra.c +22 -0

index 96378da9aefc..3b63f103f0c9 100644
@@ -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,

Quoted 8 lines

drivers/tty/serial/serial-tegra.c static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup, · line 1009

Quoted 6 lines

drivers/tty/serial/serial-tegra.c +4 -0

                        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.

Quoted 3 lines

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.

Quoted 4 lines

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?

Quoted 70 lines · Jon Hunter

Quoted 50 lines · Alexandre Courbot

Quoted 49 lines · 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 <[email protected]>

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

drivers/tty/serial/serial-tegra.c +22 -0

index 96378da9aefc..3b63f103f0c9 100644
@@ -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,

Quoted 8 lines

drivers/tty/serial/serial-tegra.c static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup, · line 1009

Quoted 6 lines

drivers/tty/serial/serial-tegra.c +4 -0

                        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.

Quoted 3 lines

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.

Quoted 2 lines

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?

Quoted 7 lines

Quoted 4 lines

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.

Quoted 71 lines · Alexandre Courbot

Quoted 70 lines · Jon Hunter

Quoted 50 lines · Alexandre Courbot

Quoted 49 lines · 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 <[email protected]>

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

drivers/tty/serial/serial-tegra.c +22 -0

index 96378da9aefc..3b63f103f0c9 100644
@@ -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,

Quoted 8 lines

drivers/tty/serial/serial-tegra.c static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup, · line 1009

Quoted 6 lines

drivers/tty/serial/serial-tegra.c +4 -0

                        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.

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

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?

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

[snip]

Quoted 13 lines

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;

Quoted 7 lines

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?

drivers/tty/serial/serial-tegra.c +11 -13

index 3b63f103f0c9..cf0133ae762d 100644
@@ -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 {