If the call to dmaengine_slave_config() fails, then the DMA buffer will
not be freed/unmapped. Fix this by moving the code that stores the
address of the buffer in the tegra_uart_port structure to before the
call to dmaengine_slave_config().
Reported-by: Alexandre Courbot <[email protected]>
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/tty/serial/serial-tegra.c | 32 +++++++++++---------------------
1 file changed, 11 insertions(+), 21 deletions(-)
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;
}
static int tegra_uart_startup(struct uart_port *u)
--
1.9.1
On 20/05/15 12:21, Jon Hunter wrote:
> If the call to dmaengine_slave_config() fails, then the DMA buffer will
> not be freed/unmapped. Fix this by moving the code that stores the
> address of the buffer in the tegra_uart_port structure to before the
> call to dmaengine_slave_config().
By the way, just to be clear, I did try to fix this before [1], but
failed :-(
Thanks to Alex for pointing this out.
Cheers
Jon
[1] https://lkml.org/lkml/2015/5/5/802
> Reported-by: Alexandre Courbot <[email protected]>
> Signed-off-by: Jon Hunter <[email protected]>
> ---
> drivers/tty/serial/serial-tegra.c | 32 +++++++++++---------------------
> 1 file changed, 11 insertions(+), 21 deletions(-)
>
> 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;
> }
>
> static int tegra_uart_startup(struct uart_port *u)
>
On 20/05/15 12:25, Jon Hunter wrote:
>
> On 20/05/15 12:21, Jon Hunter wrote:
>> If the call to dmaengine_slave_config() fails, then the DMA buffer will
>> not be freed/unmapped. Fix this by moving the code that stores the
>> address of the buffer in the tegra_uart_port structure to before the
>> call to dmaengine_slave_config().
>
> By the way, just to be clear, I did try to fix this before [1], but
> failed :-(
To be doubly clear, this is targeted to be applied on top of the
previous patch [1] which is now in linux-next.
Jon
> [1] https://lkml.org/lkml/2015/5/5/802
On Wed, May 20, 2015 at 8:21 PM, Jon Hunter <[email protected]> wrote:
> If the call to dmaengine_slave_config() fails, then the DMA buffer will
> not be freed/unmapped. Fix this by moving the code that stores the
> address of the buffer in the tegra_uart_port structure to before the
> call to dmaengine_slave_config().
>
> Reported-by: Alexandre Courbot <[email protected]>
> Signed-off-by: Jon Hunter <[email protected]>
Looks good, we had the same if/else condition appearing three times in
this function for no real reason anyway. This considerably simplifies
the code.
> drivers/tty/serial/serial-tegra.c | 32 +++++++++++---------------------
> 1 file changed, 11 insertions(+), 21 deletions(-)
Negative number of lines, another good point for this patch!
Reviewed-by: Alexandre Courbot <[email protected]>