On 13/05/15 05:56, Alexandre Courbot wrote: > On Tue, May 12, 2015 at 6:51 PM, Jon Hunter <jonathanh@xxxxxxxxxx> wrote: >> >> On 12/05/15 09:39, Alexandre Courbot wrote: >>> On Tue, May 5, 2015 at 11:17 PM, Jon Hunter <jonathanh@xxxxxxxxxx> 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@xxxxxxxxxx> >>>> --- >>>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html