| Message ID | 1348563456-30569-4-git-send-email-inderpal.singh@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh <inderpal.singh@linaro.org> wrote: > Since peripheral channel resources are not being allocated at probe, > no need to flush the channels and free the resources in remove function. > > Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> > --- > drivers/dma/pl330.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 04d83e6..6f06080 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -3012,16 +3012,10 @@ static int __devexit pl330_remove(struct amba_device *adev) > > /* Idle the DMAC */ > list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels, > - chan.device_node) { > - > + 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)) { > desc = list_entry(pdmac->desc_pool.next, > struct dma_pl330_desc, node); I am not sure about this patch. The DMA_TERMINATE_ALL is only called by the client and if the pl330 module is forced unloaded while some client is queued, we have to manually do DMA_TERMINATE_ALL. A better option could be to simply fail pl330_remove if some client is queued on the DMAC. -jassi
On 25 September 2012 18:47, Jassi Brar <jassisinghbrar@gmail.com> wrote: > On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh > <inderpal.singh@linaro.org> wrote: >> Since peripheral channel resources are not being allocated at probe, >> no need to flush the channels and free the resources in remove function. >> >> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> >> --- >> drivers/dma/pl330.c | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >> index 04d83e6..6f06080 100644 >> --- a/drivers/dma/pl330.c >> +++ b/drivers/dma/pl330.c >> @@ -3012,16 +3012,10 @@ static int __devexit pl330_remove(struct amba_device *adev) >> >> /* Idle the DMAC */ >> list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels, >> - chan.device_node) { >> - >> + 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)) { >> desc = list_entry(pdmac->desc_pool.next, >> struct dma_pl330_desc, node); > > I am not sure about this patch. The DMA_TERMINATE_ALL is only called > by the client and if the pl330 module is forced unloaded while some > client is queued, we have to manually do DMA_TERMINATE_ALL. > A better option could be to simply fail pl330_remove if some client is > queued on the DMAC. > If we fail pl330_remove while some client is queued, the force unload will fail and the force unload will lose its purpose. How about conditionally DMA_TERMINATE_ALL and free resources like below ? @@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct amba_device *adev) /* 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); + if (pch->chan.client_count != 0) { + /* Flush the channel */ + pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0); + pl330_free_chan_resources(&pch->chan); + } } while (!list_empty(&pdmac->desc_pool)) { Thanks, Inder > -jassi
On Wed, Sep 26, 2012 at 12:11 PM, Inderpal Singh <inderpal.singh@linaro.org> wrote: > How about conditionally DMA_TERMINATE_ALL and free resources like below ? > > @@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct > amba_device *adev) > /* 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); > + if (pch->chan.client_count != 0) { > + /* Flush the channel */ > + pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0); > + pl330_free_chan_resources(&pch->chan); > + } > } > It is perfectly safe to flush the channels that have client_count == 0 as well. Which is already the case.
On 26 September 2012 15:02, Jassi Brar <jassisinghbrar@gmail.com> wrote: > On Wed, Sep 26, 2012 at 12:11 PM, Inderpal Singh > <inderpal.singh@linaro.org> wrote: > >> How about conditionally DMA_TERMINATE_ALL and free resources like below ? >> >> @@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct >> amba_device *adev) >> /* 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); >> + if (pch->chan.client_count != 0) { >> + /* Flush the channel */ >> + pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0); >> + pl330_free_chan_resources(&pch->chan); >> + } >> } >> > It is perfectly safe to flush the channels that have client_count == 0 > as well. Which is already the case. As per my understanding, if client_count ==0, It may mean following: 1. This channel was never allocated to any client. Hence alloc_chan_resources was not done for it. So, no need to flush and free resources if it was never allocated at the first place. If we free_chan_resources, it will not be balanced against alloc_chan_resources. Scenario: insmod and then rmmod. Or, 2. The channel was allocated to some client, alloc_chan_resource was done, the work for the client is completed and free_chan_resources was performed. Now since resources have already been freed, no need to flush and free_chan_resources again in remove. The whole idea of this patch is to balance alloc_chan_resources and free_chan_resources. Thanks, Inder
On Wed, Sep 26, 2012 at 4:25 PM, Inderpal Singh <inderpal.singh@linaro.org> wrote: > On 26 September 2012 15:02, Jassi Brar <jassisinghbrar@gmail.com> wrote: >> On Wed, Sep 26, 2012 at 12:11 PM, Inderpal Singh >> <inderpal.singh@linaro.org> wrote: >> >>> How about conditionally DMA_TERMINATE_ALL and free resources like below ? >>> >>> @@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct >>> amba_device *adev) >>> /* 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); >>> + if (pch->chan.client_count != 0) { >>> + /* Flush the channel */ >>> + pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0); >>> + pl330_free_chan_resources(&pch->chan); >>> + } >>> } >>> >> It is perfectly safe to flush the channels that have client_count == 0 >> as well. Which is already the case. > > As per my understanding, if client_count ==0, It may mean following: > > 1. This channel was never allocated to any client. Hence > alloc_chan_resources was not done for it. > So, no need to flush and free resources if it was never allocated at > the first place. If we free_chan_resources, it will not be balanced > against alloc_chan_resources. Scenario: insmod and then rmmod. > > Or, > > 2. The channel was allocated to some client, alloc_chan_resource was > done, the work for the client is completed and free_chan_resources was > performed. Now since resources have already been freed, no need to > flush and free_chan_resources again in remove. > > The whole idea of this patch is to balance alloc_chan_resources and > free_chan_resources. > Do you see any issue with channels that have zero client_count ? AFAIU there are already enough checks in the path to weed out unused channels, so let us please not uglify the code by adding new unnecessary checks. thanks.
On 26 September 2012 22:19, Jassi Brar <jassisinghbrar@gmail.com> wrote: > On Wed, Sep 26, 2012 at 4:25 PM, Inderpal Singh > <inderpal.singh@linaro.org> wrote: >> On 26 September 2012 15:02, Jassi Brar <jassisinghbrar@gmail.com> wrote: >>> On Wed, Sep 26, 2012 at 12:11 PM, Inderpal Singh >>> <inderpal.singh@linaro.org> wrote: >>> >>>> How about conditionally DMA_TERMINATE_ALL and free resources like below ? >>>> >>>> @@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct >>>> amba_device *adev) >>>> /* 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); >>>> + if (pch->chan.client_count != 0) { >>>> + /* Flush the channel */ >>>> + pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0); >>>> + pl330_free_chan_resources(&pch->chan); >>>> + } >>>> } >>>> >>> It is perfectly safe to flush the channels that have client_count == 0 >>> as well. Which is already the case. >> >> As per my understanding, if client_count ==0, It may mean following: >> >> 1. This channel was never allocated to any client. Hence >> alloc_chan_resources was not done for it. >> So, no need to flush and free resources if it was never allocated at >> the first place. If we free_chan_resources, it will not be balanced >> against alloc_chan_resources. Scenario: insmod and then rmmod. >> >> Or, >> >> 2. The channel was allocated to some client, alloc_chan_resource was >> done, the work for the client is completed and free_chan_resources was >> performed. Now since resources have already been freed, no need to >> flush and free_chan_resources again in remove. >> >> The whole idea of this patch is to balance alloc_chan_resources and >> free_chan_resources. >> > Do you see any issue with channels that have zero client_count ? As I explained in my previous mail, If the client_count is zero, either the alloc_chan_resources is not done(or failed) for that channel or the free_chan_resource have already been done. Please refer below code from dmaengine.c static void dma_chan_put(struct dma_chan *chan) { if (!chan->client_count) return; /* this channel failed alloc_chan_resources */ chan->client_count--; module_put(dma_chan_to_owner(chan)); if (chan->client_count == 0) chan->device->device_free_chan_resources(chan); } As you mentioned channel flush is needed if some client is queued and force unload happens. I am not against flushing and freeing channels. I am __only__ suggesting to flush and free channels for which alloc_chan_resource was successful, which can be decided by "chan->client_count" as being done in above function. Don't you think free_chan_resource should be done __only if__ alloc_chan_resource was successful ? Thanks, Inder > AFAIU there are already enough checks in the path to weed out unused > channels, so let us please not uglify the code by adding new > unnecessary checks. > > thanks.
On Thu, Sep 27, 2012 at 9:43 AM, Inderpal Singh <inderpal.singh@linaro.org> wrote: > > Don't you think free_chan_resource should be done __only if__ > alloc_chan_resource was successful ? > No, I don't think so. Thanks.
On 27 September 2012 10:35, Jassi Brar <jassisinghbrar@gmail.com> wrote: > On Thu, Sep 27, 2012 at 9:43 AM, Inderpal Singh > <inderpal.singh@linaro.org> wrote: >> >> Don't you think free_chan_resource should be done __only if__ >> alloc_chan_resource was successful ? >> > No, I don't think so. Thanks. Thanks for quick response. Please elaborate more on this as I find it against the general rule and against the dmaengine implementation which checks on the same condition before proceeding for free_chan_resouces in dma_chan_put function. Thanks, Inder
On Thu, Sep 27, 2012 at 11:00 AM, Inderpal Singh <inderpal.singh@linaro.org> wrote: > On 27 September 2012 10:35, Jassi Brar <jassisinghbrar@gmail.com> wrote: >> On Thu, Sep 27, 2012 at 9:43 AM, Inderpal Singh >> <inderpal.singh@linaro.org> wrote: >>> >>> Don't you think free_chan_resource should be done __only if__ >>> alloc_chan_resource was successful ? >>> >> No, I don't think so. Thanks. > > Thanks for quick response. > Please elaborate more on this as I find it against the general rule > and against the dmaengine implementation which checks on the same > condition before proceeding for free_chan_resouces in dma_chan_put > function. > I thought I already explained it, but here is the summary. Calling pl330_free_chan_resources() for channels that have zero client is already safe. Preventing the call by checking !client_count only increases LOC making it uglier. ** If the new check provides any more security, please let me know. ** Food for thought : we never check for NULL before passing a pointer to kfree(). Why ?
On Wed, 2012-09-26 at 12:11 +0530, Inderpal Singh wrote: > If we fail pl330_remove while some client is queued, the force unload > will fail and the > force unload will lose its purpose. > How about conditionally DMA_TERMINATE_ALL and free resources like > below ? Why would you want to remove the driver when it is doing something useful? You have to ensure driver is not doing anything. What is point here?
On 27 September 2012 15:18, Vinod Koul <vinod.koul@linux.intel.com> wrote: > On Wed, 2012-09-26 at 12:11 +0530, Inderpal Singh wrote: >> If we fail pl330_remove while some client is queued, the force unload >> will fail and the >> force unload will lose its purpose. >> How about conditionally DMA_TERMINATE_ALL and free resources like >> below ? > Why would you want to remove the driver when it is doing something > useful? You have to ensure driver is not doing anything. > > What is point here? > As mentioned by jassi, if the pl330 module is forced unloaded while some client is queued, we have to manually do DMA_TERMINATE_ALL. If failing remove is a better option in case some client is queued, we can do away with DMA_TERMINATE_ALL and free_chan_resources and simply return a suitable error code from remove. Kindly suggest. Thanks, Inder > -- > ~Vinod >
On Thu, Sep 27, 2012 at 9:11 PM, Inderpal Singh <inderpal.singh@linaro.org> wrote: > On 27 September 2012 15:18, Vinod Koul <vinod.koul@linux.intel.com> wrote: >> On Wed, 2012-09-26 at 12:11 +0530, Inderpal Singh wrote: >>> If we fail pl330_remove while some client is queued, the force unload >>> will fail and the >>> force unload will lose its purpose. >>> How about conditionally DMA_TERMINATE_ALL and free resources like >>> below ? >> Why would you want to remove the driver when it is doing something >> useful? You have to ensure driver is not doing anything. >> >> What is point here? >> > As mentioned by jassi, if the pl330 module is forced unloaded while > some client is queued, we have to manually do DMA_TERMINATE_ALL. > I meant that in the current situation. Not ideally. > If failing remove is a better option in case some client is queued, we > can do away with DMA_TERMINATE_ALL and free_chan_resources and simply > return a suitable error code from remove. > That was exactly what I suggested as an alternative.
On 27 September 2012 21:36, Jassi Brar <jassisinghbrar@gmail.com> wrote: > On Thu, Sep 27, 2012 at 9:11 PM, Inderpal Singh > <inderpal.singh@linaro.org> wrote: >> On 27 September 2012 15:18, Vinod Koul <vinod.koul@linux.intel.com> wrote: >>> On Wed, 2012-09-26 at 12:11 +0530, Inderpal Singh wrote: >>>> If we fail pl330_remove while some client is queued, the force unload >>>> will fail and the >>>> force unload will lose its purpose. >>>> How about conditionally DMA_TERMINATE_ALL and free resources like >>>> below ? >>> Why would you want to remove the driver when it is doing something >>> useful? You have to ensure driver is not doing anything. >>> >>> What is point here? >>> >> As mentioned by jassi, if the pl330 module is forced unloaded while >> some client is queued, we have to manually do DMA_TERMINATE_ALL. >> > I meant that in the current situation. Not ideally. > >> If failing remove is a better option in case some client is queued, we >> can do away with DMA_TERMINATE_ALL and free_chan_resources and simply >> return a suitable error code from remove. >> > That was exactly what I suggested as an alternative. Yes, but our discussion went about continue doing DMA_TERMINATE_ALL and freeing. Now, if we have to check if any client is using the channel and then decide. We will have to traverse the channel list twice once to check the usage and second time to delete the nodes from the list if we go ahead with remove. The remove will look like below: @@ -3008,18 +3008,19 @@ 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_safe(pch, _p, &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); } Please suggest if there is any better way to do it. Thanks, Inder
On Fri, Sep 28, 2012 at 10:03 AM, Inderpal Singh <inderpal.singh@linaro.org> wrote: > > Now, if we have to check if any client is using the channel and then > decide. We will have to traverse the channel list twice once to check > the usage and second time to delete the nodes from the list if we go > ahead with remove. > The remove will look like below: > > @@ -3008,18 +3008,19 @@ 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_safe(pch, _p, &pdmac->ddma.channels, > + chan.device_node) { > + > + if (pch->chan.client_count) > + return -EBUSY; > + } > + The iteration doesn't have to be the 'safe' version here. Other than that it seems OK.
On 28 September 2012 16:28, Jassi Brar <jassisinghbrar@gmail.com> wrote: > On Fri, Sep 28, 2012 at 10:03 AM, Inderpal Singh > <inderpal.singh@linaro.org> wrote: >> >> Now, if we have to check if any client is using the channel and then >> decide. We will have to traverse the channel list twice once to check >> the usage and second time to delete the nodes from the list if we go >> ahead with remove. >> The remove will look like below: >> >> @@ -3008,18 +3008,19 @@ 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_safe(pch, _p, &pdmac->ddma.channels, >> + chan.device_node) { >> + >> + if (pch->chan.client_count) >> + return -EBUSY; >> + } >> + > The iteration doesn't have to be the 'safe' version here. Other than > that it seems OK. Ok. I will update the patch and send again. Thanks, Inder
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 04d83e6..6f06080 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -3012,16 +3012,10 @@ static int __devexit pl330_remove(struct amba_device *adev) /* Idle the DMAC */ list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels, - chan.device_node) { - + 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)) { desc = list_entry(pdmac->desc_pool.next, struct dma_pl330_desc, node);
Since peripheral channel resources are not being allocated at probe, no need to flush the channels and free the resources in remove function. Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> --- drivers/dma/pl330.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)