From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754009AbbJNOYh (ORCPT ); Wed, 14 Oct 2015 10:24:37 -0400 Received: from mail-wi0-f172.google.com ([209.85.212.172]:37485 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753612AbbJNOYg (ORCPT ); Wed, 14 Oct 2015 10:24:36 -0400 Subject: Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver To: "M'boumba Cedric Madianga" References: <1444745127-1105-1-git-send-email-cedric.madianga@gmail.com> <1444745127-1105-3-git-send-email-cedric.madianga@gmail.com> <561D1689.90703@linaro.org> <561E58AC.80207@linaro.org> Cc: Maxime Coquelin , robh+dt@kernel.org, pawel.moll@arm.com, Mark Rutland , ijc+devicetree@hellion.org.uk, Kumar Gala , linux@arm.linux.org.uk, Vinod Koul , linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org From: Daniel Thompson Message-ID: <561E658D.1070900@linaro.org> Date: Wed, 14 Oct 2015 15:24:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 14/10/15 14:41, M'boumba Cedric Madianga wrote: > 2015-10-14 15:29 GMT+02:00 Daniel Thompson : >> On 14/10/15 14:17, M'boumba Cedric Madianga wrote: >>> >>> Hi Daniel, >>> >>>>>> + >>>>>> +static int stm32_dma_remove(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct stm32_dma_device *dmadev = platform_get_drvdata(pdev); >>>>>> + >>>>>> + of_dma_controller_free(pdev->dev.of_node); >>>>>> + >>>>>> + dma_async_device_unregister(&dmadev->ddev); >>>>>> + >>>>>> + clk_disable_unprepare(dmadev->clk); >>>>> >>>>> >>>>> >>>>> What is the purpose of disabling/unpreparing the clock here? >>>>> stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources() >>>>> should >>>>> pair up and the clock should already be stopped. >>> >>> >>> stm32_dma_remove() could be called during an on-going transfer during >>> module unload. >>> So in that case, it seems that disabling/unpreparing the clock is needed. >> >> >> Really? >> >> I think we need to be sure any on-going transfers are stopped. >> >> There are multiple reasons for this, not least the risk of executing a >> callback that has been freed, but the one related to my point is that a >> single clk_disable_unprepare() will remain broken because if you don't know >> that the transfers have stopped then you don't know how many on-going >> transfers there are. > > In the next version, I am going to stop the DMA, free all interrupts > and kill tasklet in stm32_dma_remove. > In that way, all on-going transfers will be lost as we don't have to > wait the end of remaining transfer in order to execute this function > as quickly as possible. Hmnnn... The dmaengine framework will WARN_ONCE() if an dmaengine is removed whilst it is active and also works hard to ensure dmaengine modules are not removed whilst there are active drivers using the framework. How do we get into this function whilst there is still an active DMA channels? > But even with this improvement, I think I have to disable the clock here. As above, I think the dmaengine framework work to protect you from this sort problem. However even if I am wrong about that then unconditionally calling clk_disable_unprepare() can not be used to reliably manage the clocks. You don't know what the counts are! Daniel.