[PATCH 0/3] DMA: PL330: Fix mem leaks and balance probe/remove

[PATCH 0/3] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

The first 2 patches of this series fix memory leaks because the memory
allocated for peripheral channels and DMA descriptors were not getting
freed.

The third patch balances the module's remove function.

This patchset is based on slave-dma tree's "next" branch merged with
"fixes" branch and applied patch at [1] to fix the build error.

[1] http://permalink.gmane.org/gmane.linux.kernel.next/24274

Inderpal Singh (3):
  DMA: PL330: Free memory allocated for peripheral channels
  DMA: PL330: Change allocation method to properly free DMA descriptors
  DMA: PL330: Balance module remove function with probe

 drivers/dma/pl330.c |   37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] DMA: PL330: Free memory allocated for peripheral channels

[Inderpal Singh]

The allocated memory for peripheral channels is not being freed upon
failure in probe and in module's remove funtion. It will lead to memory
leakage. Hence free the allocated memory.

Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
---
 drivers/dma/pl330.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 2ebd4cd..10c6b6a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	ret = dma_async_device_register(pd);
 	if (ret) {
 		dev_err(&adev->dev, "unable to register DMAC\n");
-		goto probe_err4;
+		goto probe_err5;
 	}
 
 	dev_info(&adev->dev,
@@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	return 0;
 
+probe_err5:
+	kfree(pdmac->peripherals);
 probe_err4:
 	pl330_del(pi);
 probe_err3:
@@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device *adev)
 	res = &adev->res;
 	release_mem_region(res->start, resource_size(res));
 
+	kfree(pdmac->peripherals);
 	kfree(pdmac);
 
 	return 0;
-- 
1.7.9.5

Re: [PATCH 1/3] DMA: PL330: Free memory allocated for peripheral channels

[Jassi Brar]

On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
<inderpal.singh@linaro.org> wrote:
> The allocated memory for peripheral channels is not being freed upon
> failure in probe and in module's remove funtion. It will lead to memory
> leakage. Hence free the allocated memory.
>
> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
> ---
>  drivers/dma/pl330.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 2ebd4cd..10c6b6a 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>         ret = dma_async_device_register(pd);
>         if (ret) {
>                 dev_err(&adev->dev, "unable to register DMAC\n");
> -               goto probe_err4;
> +               goto probe_err5;
>         }
>
Sorry this patch seems malformed. Against which tree did you prepare it ?

-jassi



>         dev_info(&adev->dev,
> @@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>
>         return 0;
>
> +probe_err5:
> +       kfree(pdmac->peripherals);
>  probe_err4:
>         pl330_del(pi);
>  probe_err3:
> @@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device *adev)
>         res = &adev->res;
>         release_mem_region(res->start, resource_size(res));
>
> +       kfree(pdmac->peripherals);
>         kfree(pdmac);
>
>         return 0;
> --
> 1.7.9.5
>

Re: [PATCH 1/3] DMA: PL330: Free memory allocated for peripheral channels

[Inderpal Singh]

On 25 September 2012 18:17, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
> <inderpal.singh@linaro.org> wrote:
>> The allocated memory for peripheral channels is not being freed upon
>> failure in probe and in module's remove funtion. It will lead to memory
>> leakage. Hence free the allocated memory.
>>
>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
>> ---
>>  drivers/dma/pl330.c |    5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 2ebd4cd..10c6b6a 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>         ret = dma_async_device_register(pd);
>>         if (ret) {
>>                 dev_err(&adev->dev, "unable to register DMAC\n");
>> -               goto probe_err4;
>> +               goto probe_err5;
>>         }
>>
> Sorry this patch seems malformed. Against which tree did you prepare it ?
>
This patch depends on "61c6e7531d3b66b3 ........DMA: PL330: Check the
pointer returned by kzalloc" which is on vinod's slave-dma's "fixes"
branch. So I merged slave-dma's "next" and "fixes" branches. Now after
merging, build error occurs due to some conflict so I had to apply the
patch sent by Sachin at [1]

Same had been mentioned in the cover letter.

[1] http://permalink.gmane.org/gmane.linux.kernel.next/24274

Thanks,
Inder

> -jassi
>
>
>
>>         dev_info(&adev->dev,
>> @@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>
>>         return 0;
>>
>> +probe_err5:
>> +       kfree(pdmac->peripherals);
>>  probe_err4:
>>         pl330_del(pi);
>>  probe_err3:
>> @@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device *adev)
>>         res = &adev->res;
>>         release_mem_region(res->start, resource_size(res));
>>
>> +       kfree(pdmac->peripherals);
>>         kfree(pdmac);
>>
>>         return 0;
>> --
>> 1.7.9.5
>>

[PATCH 2/3] DMA: PL330: Change allocation method to properly free DMA descriptors

[Inderpal Singh]

In probe, memory for multiple DMA descriptors were being allocated at once
and then it was being split and added into DMA pool one by one. The address
of this memory allocation is not being saved anywhere. To free this memory,
the address is required. Initially the first node of the pool will be
pointed by this address but as we use this pool the descs will shuffle and
hence we will lose the track of the address.

This patch does following:

1. Allocates DMA descs one by one and then adds them to pool so that all
   descs can be fetched and memory freed one by one. This way runtime
   added descs can also be freed.
2. Free DMA descs in case of error in probe and in module's remove function

Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
---
 drivers/dma/pl330.c |   28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 10c6b6a..04d83e6 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2541,20 +2541,19 @@ static int add_desc(struct dma_pl330_dmac *pdmac, gfp_t flg, int count)
 	if (!pdmac)
 		return 0;
 
-	desc = kmalloc(count * sizeof(*desc), flg);
-	if (!desc)
-		return 0;
-
 	spin_lock_irqsave(&pdmac->pool_lock, flags);
 
 	for (i = 0; i < count; i++) {
-		_init_desc(&desc[i]);
-		list_add_tail(&desc[i].node, &pdmac->desc_pool);
+		desc = kmalloc(sizeof(*desc), flg);
+		if (!desc)
+			break;
+		_init_desc(desc);
+		list_add_tail(&desc->node, &pdmac->desc_pool);
 	}
 
 	spin_unlock_irqrestore(&pdmac->pool_lock, flags);
 
-	return count;
+	return i;
 }
 
 static struct dma_pl330_desc *
@@ -2857,6 +2856,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	struct dma_pl330_platdata *pdat;
 	struct dma_pl330_dmac *pdmac;
 	struct dma_pl330_chan *pch;
+	struct dma_pl330_desc *desc;
 	struct pl330_info *pi;
 	struct dma_device *pd;
 	struct resource *res;
@@ -2978,6 +2978,12 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 probe_err5:
 	kfree(pdmac->peripherals);
 probe_err4:
+	while (!list_empty(&pdmac->desc_pool)) {
+		desc = list_entry(pdmac->desc_pool.next,
+				struct dma_pl330_desc, node);
+		list_del(&desc->node);
+		kfree(desc);
+	}
 	pl330_del(pi);
 probe_err3:
 	free_irq(irq, pi);
@@ -2994,6 +3000,7 @@ static int __devexit pl330_remove(struct amba_device *adev)
 {
 	struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
 	struct dma_pl330_chan *pch, *_p;
+	struct dma_pl330_desc *desc;
 	struct pl330_info *pi;
 	struct resource *res;
 	int irq;
@@ -3015,6 +3022,13 @@ static int __devexit pl330_remove(struct amba_device *adev)
 		pl330_free_chan_resources(&pch->chan);
 	}
 
+	while (!list_empty(&pdmac->desc_pool)) {
+		desc = list_entry(pdmac->desc_pool.next,
+				struct dma_pl330_desc, node);
+		list_del(&desc->node);
+		kfree(desc);
+	}
+
 	pi = &pdmac->pif;
 
 	pl330_del(pi);
-- 
1.7.9.5

Re: [PATCH 2/3] DMA: PL330: Change allocation method to properly free DMA descriptors

[Jassi Brar]

On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
<inderpal.singh@linaro.org> wrote:
> In probe, memory for multiple DMA descriptors were being allocated at once
> and then it was being split and added into DMA pool one by one. The address
> of this memory allocation is not being saved anywhere. To free this memory,
> the address is required. Initially the first node of the pool will be
> pointed by this address but as we use this pool the descs will shuffle and
> hence we will lose the track of the address.
>
> This patch does following:
>
> 1. Allocates DMA descs one by one and then adds them to pool so that all
>    descs can be fetched and memory freed one by one. This way runtime
>    added descs can also be freed.
> 2. Free DMA descs in case of error in probe and in module's remove function
>
> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
> ---
>  drivers/dma/pl330.c |   28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 10c6b6a..04d83e6 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2541,20 +2541,19 @@ static int add_desc(struct dma_pl330_dmac *pdmac, gfp_t flg, int count)
>         if (!pdmac)
>                 return 0;
>
> -       desc = kmalloc(count * sizeof(*desc), flg);
> -       if (!desc)
> -               return 0;
> -
>         spin_lock_irqsave(&pdmac->pool_lock, flags);
>
>         for (i = 0; i < count; i++) {
> -               _init_desc(&desc[i]);
> -               list_add_tail(&desc[i].node, &pdmac->desc_pool);
> +               desc = kmalloc(sizeof(*desc), flg);
> +               if (!desc)
> +                       break;
> +               _init_desc(desc);
> +               list_add_tail(&desc->node, &pdmac->desc_pool);
>         }
>
>         spin_unlock_irqrestore(&pdmac->pool_lock, flags);
>
> -       return count;
> +       return i;
>  }
>
OK, but the kmalloc shouldn't be done with irqs disabled. Please
protect only the list_add_tail()

thanks.

>  static struct dma_pl330_desc *
> @@ -2857,6 +2856,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>         struct dma_pl330_platdata *pdat;
>         struct dma_pl330_dmac *pdmac;
>         struct dma_pl330_chan *pch;
> +       struct dma_pl330_desc *desc;
>         struct pl330_info *pi;
>         struct dma_device *pd;
>         struct resource *res;
> @@ -2978,6 +2978,12 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>  probe_err5:
>         kfree(pdmac->peripherals);
>  probe_err4:
> +       while (!list_empty(&pdmac->desc_pool)) {
> +               desc = list_entry(pdmac->desc_pool.next,
> +                               struct dma_pl330_desc, node);
> +               list_del(&desc->node);
> +               kfree(desc);
> +       }
>         pl330_del(pi);
>  probe_err3:
>         free_irq(irq, pi);
> @@ -2994,6 +3000,7 @@ static int __devexit pl330_remove(struct amba_device *adev)
>  {
>         struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
>         struct dma_pl330_chan *pch, *_p;
> +       struct dma_pl330_desc *desc;
>         struct pl330_info *pi;
>         struct resource *res;
>         int irq;
> @@ -3015,6 +3022,13 @@ static int __devexit pl330_remove(struct amba_device *adev)
>                 pl330_free_chan_resources(&pch->chan);
>         }
>
> +       while (!list_empty(&pdmac->desc_pool)) {
> +               desc = list_entry(pdmac->desc_pool.next,
> +                               struct dma_pl330_desc, node);
> +               list_del(&desc->node);
> +               kfree(desc);
> +       }
> +
>         pi = &pdmac->pif;
>
>         pl330_del(pi);
> --
> 1.7.9.5
>

Re: [PATCH 2/3] DMA: PL330: Change allocation method to properly free DMA descriptors

[Inderpal Singh]

On 25 September 2012 18:39, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
> <inderpal.singh@linaro.org> wrote:
>> In probe, memory for multiple DMA descriptors were being allocated at once
>> and then it was being split and added into DMA pool one by one. The address
>> of this memory allocation is not being saved anywhere. To free this memory,
>> the address is required. Initially the first node of the pool will be
>> pointed by this address but as we use this pool the descs will shuffle and
>> hence we will lose the track of the address.
>>
>> This patch does following:
>>
>> 1. Allocates DMA descs one by one and then adds them to pool so that all
>>    descs can be fetched and memory freed one by one. This way runtime
>>    added descs can also be freed.
>> 2. Free DMA descs in case of error in probe and in module's remove function
>>
>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
>> ---
>>  drivers/dma/pl330.c |   28 +++++++++++++++++++++-------
>>  1 file changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 10c6b6a..04d83e6 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -2541,20 +2541,19 @@ static int add_desc(struct dma_pl330_dmac *pdmac, gfp_t flg, int count)
>>         if (!pdmac)
>>                 return 0;
>>
>> -       desc = kmalloc(count * sizeof(*desc), flg);
>> -       if (!desc)
>> -               return 0;
>> -
>>         spin_lock_irqsave(&pdmac->pool_lock, flags);
>>
>>         for (i = 0; i < count; i++) {
>> -               _init_desc(&desc[i]);
>> -               list_add_tail(&desc[i].node, &pdmac->desc_pool);
>> +               desc = kmalloc(sizeof(*desc), flg);
>> +               if (!desc)
>> +                       break;
>> +               _init_desc(desc);
>> +               list_add_tail(&desc->node, &pdmac->desc_pool);
>>         }
>>
>>         spin_unlock_irqrestore(&pdmac->pool_lock, flags);
>>
>> -       return count;
>> +       return i;
>>  }
>>
> OK, but the kmalloc shouldn't be done with irqs disabled. Please
> protect only the list_add_tail()
>
Yes, I missed it. I will update and send it again.

Thanks,
Inder

> thanks.
>
>>  static struct dma_pl330_desc *
>> @@ -2857,6 +2856,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>         struct dma_pl330_platdata *pdat;
>>         struct dma_pl330_dmac *pdmac;
>>         struct dma_pl330_chan *pch;
>> +       struct dma_pl330_desc *desc;
>>         struct pl330_info *pi;
>>         struct dma_device *pd;
>>         struct resource *res;
>> @@ -2978,6 +2978,12 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>  probe_err5:
>>         kfree(pdmac->peripherals);
>>  probe_err4:
>> +       while (!list_empty(&pdmac->desc_pool)) {
>> +               desc = list_entry(pdmac->desc_pool.next,
>> +                               struct dma_pl330_desc, node);
>> +               list_del(&desc->node);
>> +               kfree(desc);
>> +       }
>>         pl330_del(pi);
>>  probe_err3:
>>         free_irq(irq, pi);
>> @@ -2994,6 +3000,7 @@ static int __devexit pl330_remove(struct amba_device *adev)
>>  {
>>         struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
>>         struct dma_pl330_chan *pch, *_p;
>> +       struct dma_pl330_desc *desc;
>>         struct pl330_info *pi;
>>         struct resource *res;
>>         int irq;
>> @@ -3015,6 +3022,13 @@ static int __devexit pl330_remove(struct amba_device *adev)
>>                 pl330_free_chan_resources(&pch->chan);
>>         }
>>
>> +       while (!list_empty(&pdmac->desc_pool)) {
>> +               desc = list_entry(pdmac->desc_pool.next,
>> +                               struct dma_pl330_desc, node);
>> +               list_del(&desc->node);
>> +               kfree(desc);
>> +       }
>> +
>>         pi = &pdmac->pif;
>>
>>         pl330_del(pi);
>> --
>> 1.7.9.5
>>

[PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

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);
-- 
1.7.9.5

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Jassi Brar]

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

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

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

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Jassi Brar]

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.

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

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

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Jassi Brar]

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.

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

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.

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Jassi Brar]

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.

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

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

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Jassi Brar]

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 ?

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Vinod Koul]

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?

-- 
~Vinod

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

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
>

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Jassi Brar]

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.

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

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

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Jassi Brar]

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.

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

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