Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933523AbbEMIX2 (ORCPT ); Wed, 13 May 2015 04:23:28 -0400 Received: from hqemgate14.nvidia.com ([216.228.121.143]:13676 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932438AbbEMIXY (ORCPT ); Wed, 13 May 2015 04:23:24 -0400 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Wed, 13 May 2015 01:21:41 -0700 Message-ID: <555309F4.1080306@nvidia.com> Date: Wed, 13 May 2015 09:23:16 +0100 From: Jon Hunter User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Alexandre Courbot CC: Laxman Dewangan , Greg Kroah-Hartman , Jiri Slaby , , "linux-tegra@vger.kernel.org" , Linux Kernel Mailing List Subject: Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup References: <1430835479-6613-1-git-send-email-jonathanh@nvidia.com> <1430835479-6613-9-git-send-email-jonathanh@nvidia.com> <5551CD25.4030206@nvidia.com> In-Reply-To: X-Originating-IP: [10.21.134.107] X-ClientProxiedBy: UKMAIL101.nvidia.com (10.26.138.13) To UKMAIL101.nvidia.com (10.26.138.13) Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5642 Lines: 121 On 13/05/15 05:56, Alexandre Courbot wrote: > On Tue, May 12, 2015 at 6:51 PM, Jon Hunter wrote: >> >> On 12/05/15 09:39, Alexandre Courbot wrote: >>> On Tue, May 5, 2015 at 11:17 PM, Jon Hunter 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 >>>> --- >>>> 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/