| Message ID | 1349432276-22919-4-git-send-email-inderpal.singh@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote: > Since peripheral channel resources are not being allocated at probe, > no need to flush the channels and free the resources in remove function. > In case, the channel is in use by some client, return EBUSY. > > Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> > --- > drivers/dma/pl330.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index bf71ff7..4b7a34d 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -3009,18 +3009,21 @@ static int __devexit pl330_remove(struct amba_device *adev) > if (!pdmac) > return 0; > > + /* check if any client is using any channel */ > + list_for_each_entry(pch, &pdmac->ddma.channels, > + chan.device_node) { > + > + if (pch->chan.client_count) > + return -EBUSY; > + } > + > while (!list_empty(&pdmac->desc_pool)) { Did you get this code executed? I think No. The dmaengine holds the reference to channels, so if they are in use and not freed by client your remove wont be called. So this check is redundant
Hi Vinod, On 24 October 2012 09:44, Vinod Koul <vkoul@infradead.org> wrote: > On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote: >> Since peripheral channel resources are not being allocated at probe, >> no need to flush the channels and free the resources in remove function. >> In case, the channel is in use by some client, return EBUSY. >> >> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> >> --- >> drivers/dma/pl330.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >> index bf71ff7..4b7a34d 100644 >> --- a/drivers/dma/pl330.c >> +++ b/drivers/dma/pl330.c >> @@ -3009,18 +3009,21 @@ static int __devexit pl330_remove(struct amba_device *adev) >> if (!pdmac) >> return 0; >> >> + /* check if any client is using any channel */ >> + list_for_each_entry(pch, &pdmac->ddma.channels, >> + chan.device_node) { >> + >> + if (pch->chan.client_count) >> + return -EBUSY; >> + } >> + >> while (!list_empty(&pdmac->desc_pool)) { > > Did you get this code executed? > I think No. > > The dmaengine holds the reference to channels, so if they are in use and > not freed by client your remove wont be called. So this check is > redundant > This code will get executed only in case of force removal of the module which was discussed in the first version of the patch at [1]. Now, if we do not have to think about force removal then this patch will go back to the first version. Let me know your view. [1] https://patchwork.kernel.org/patch/1503171/ Regards, Inder > -- > Vinod Koul > Intel Corp. >
On Thu, 2012-10-25 at 16:53 +0530, Inderpal Singh wrote: > > This code will get executed only in case of force removal of the > module which was discussed in the first version of the patch at [1]. > Now, if we do not have to think about force removal then this patch > will go back to the first version. But why are you doing force removal of driver even when client is holding a reference to you. What happens when client finally tries to free the channel? What is the problem you are trying to solve? > > Let me know your view. > > [1] https://patchwork.kernel.org/patch/1503171/ >
Hi Vinod, On 26 October 2012 10:15, Vinod Koul <vkoul@infradead.org> wrote: > On Thu, 2012-10-25 at 16:53 +0530, Inderpal Singh wrote: >> >> This code will get executed only in case of force removal of the >> module which was discussed in the first version of the patch at [1]. >> Now, if we do not have to think about force removal then this patch >> will go back to the first version. > But why are you doing force removal of driver even when client is > holding a reference to you. > > What happens when client finally tries to free the channel? Since we return EBUSY so forced removal won't succeed. Client can free the channel eventually. > > What is the problem you are trying to solve? >> There was a long discussion about it in the first version of the patch. Allow me to explain it to you. The existing driver does DMA_TERMINATE_ALL and frees resources for all the channels in the _remove function. The first version of patch removed this flushing and freeing of channel resources because they are not getting allocated in the probe. Jassi pointed out that manual flushing is needed if a force removal happens and some client is queued. Then it was agreed that flushing is not needed, instead we should return EBUSY if client is queued on some channel (this will happen only in force removal case). Hence this additional check in v2 version so that force removal does not succeeds if any client is queued. If you think force removal is not a practical scenario and we should not be bothering about it, this check can be removed and the patch will go back to first version which just removes flushing and freeing of channels beacues they are not getting allocated in probe. Let me know your view. Regards, Inder >> Let me know your view. >> >> [1] https://patchwork.kernel.org/patch/1503171/ >> > > > -- > Vinod Koul > Intel Corp. >
On Sat, 2012-10-27 at 15:50 +0530, Inderpal Singh wrote: > Hi Vinod, > > On 26 October 2012 10:15, Vinod Koul <vkoul@infradead.org> wrote: > > On Thu, 2012-10-25 at 16:53 +0530, Inderpal Singh wrote: > >> > >> This code will get executed only in case of force removal of the > >> module which was discussed in the first version of the patch at [1]. > >> Now, if we do not have to think about force removal then this patch > >> will go back to the first version. > > But why are you doing force removal of driver even when client is > > holding a reference to you. > > > > What happens when client finally tries to free the channel? > Since we return EBUSY so forced removal won't succeed. Client can free > the channel eventually. And that is my concern. You have forcefully removed the dma module. What happens then? How will the free calll get executed, wont you hit a panic. > > > > > What is the problem you are trying to solve? > >> > > There was a long discussion about it in the first version of the > patch. Allow me to explain it to you. > > The existing driver does DMA_TERMINATE_ALL and frees resources for all > the channels in the _remove function. Which for starters may not be right thing to do. Shouldn't you first make sure client has freed all references to your driver and then only remove. Freeing resources in .remove without keeping client in sync doesn't sound to be good idea to me. > The first version of patch > removed this flushing and freeing of channel resources because they > are not getting allocated in the probe. Jassi pointed out that manual > flushing is needed if a force removal happens and some client is > queued. Then it was agreed that flushing is not needed, instead we > should return EBUSY if client is queued on some channel (this will > happen only in force removal case). Hence this additional check in v2 > version so that force removal does not succeeds if any client is > queued. > > If you think force removal is not a practical scenario and we should > not be bothering about it, this check can be removed and the patch > will go back to first version which just removes flushing and freeing > of channels beacues they are not getting allocated in probe. > > Let me know your view. > > Regards, > Inder > > > >> Let me know your view. > >> > >> [1] https://patchwork.kernel.org/patch/1503171/ > >> > > > > > > -- > > Vinod Koul > > Intel Corp. > >
Hi Vinod, On 29 October 2012 10:15, Vinod Koul <vkoul@infradead.org> wrote: > On Sat, 2012-10-27 at 15:50 +0530, Inderpal Singh wrote: >> Hi Vinod, >> >> On 26 October 2012 10:15, Vinod Koul <vkoul@infradead.org> wrote: >> > On Thu, 2012-10-25 at 16:53 +0530, Inderpal Singh wrote: >> >> >> >> This code will get executed only in case of force removal of the >> >> module which was discussed in the first version of the patch at [1]. >> >> Now, if we do not have to think about force removal then this patch >> >> will go back to the first version. >> > But why are you doing force removal of driver even when client is >> > holding a reference to you. >> > >> > What happens when client finally tries to free the channel? >> Since we return EBUSY so forced removal won't succeed. Client can free >> the channel eventually. > And that is my concern. You have forcefully removed the dma module. > What happens then? How will the free calll get executed, wont you hit a > panic. Yes, you are correct, It will hit a panic. The return value from remove is not being checked in __device_release_driver because of which dma module is forcefully removed even if we return EBUSY from driver's remove. Hence returning error from .remove is not useful at all. >> >> > >> > What is the problem you are trying to solve? >> >> >> >> There was a long discussion about it in the first version of the >> patch. Allow me to explain it to you. >> >> The existing driver does DMA_TERMINATE_ALL and frees resources for all >> the channels in the _remove function. > Which for starters may not be right thing to do. Please consider v1 patch which removes DMA_TERMINATE_ALL and freeing of resources from .remove function because in normal scenario if remove is reached it is sure that no client is holding any reference to the driver hence no need to flush and free the channels. > Shouldn't you first > make sure client has freed all references to your driver and then only > remove. Freeing resources in .remove without keeping client in sync > doesn't sound to be good idea to me. > >> The first version of patch >> removed this flushing and freeing of channel resources because they >> are not getting allocated in the probe. Jassi pointed out that manual >> flushing is needed if a force removal happens and some client is >> queued. Then it was agreed that flushing is not needed, instead we >> should return EBUSY if client is queued on some channel (this will >> happen only in force removal case). Hence this additional check in v2 >> version so that force removal does not succeeds if any client is >> queued. >> >> If you think force removal is not a practical scenario and we should >> not be bothering about it, this check can be removed and the patch >> will go back to first version which just removes flushing and freeing >> of channels beacues they are not getting allocated in probe. >> >> Let me know your view. >> >> Regards, >> Inder >> >> >> >> Let me know your view. >> >> >> >> [1] https://patchwork.kernel.org/patch/1503171/ >> >> >> > >> > >> > -- >> > Vinod Koul >> > Intel Corp. >> > > > > -- > Vinod Koul > Intel Corp. >
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index bf71ff7..4b7a34d 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -3009,18 +3009,21 @@ static int __devexit pl330_remove(struct amba_device *adev) if (!pdmac) return 0; + /* check if any client is using any channel */ + list_for_each_entry(pch, &pdmac->ddma.channels, + chan.device_node) { + + if (pch->chan.client_count) + return -EBUSY; + } + amba_set_drvdata(adev, NULL); - /* Idle the DMAC */ list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels, chan.device_node) { /* Remove the channel */ list_del(&pch->chan.device_node); - - /* Flush the channel */ - pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0); - pl330_free_chan_resources(&pch->chan); } while (!list_empty(&pdmac->desc_pool)) {
Since peripheral channel resources are not being allocated at probe, no need to flush the channels and free the resources in remove function. In case, the channel is in use by some client, return EBUSY. Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> --- drivers/dma/pl330.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)