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

[PATCH v2 0/4] 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 last 2 patches balance the module's remove function.

This series depends on "61c6e7531d3b66b3 ........DMA: PL330: Check the
pointer returned by kzalloc" which is on slave-dma's "fixes" branch. 
Hence slave-dma tree's "next" branch was merged with "fixes" and 
applied patch at [1] to fix the build error.

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

Changes since v1:
 - Protect only list_add_tail with spin_locks
 - Return EBUSY from remove if channel is in use
 - unregister dma_device in remove
 
Inderpal Singh (4):
  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
  DMA: PL330: unregister dma_device in module's remove function

 drivers/dma/pl330.c |   53 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 15 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/4] 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 v2 1/4] DMA: PL330: Free memory allocated for peripheral channels

[Vinod Koul]

On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh 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;
>  	}
>  
>  	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;

This looks fine, but if you use devm_ functions then you dont need to do
all this.
Can you do that conversion instead?

-- 
Vinod Koul
Intel Corp.

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

[Inderpal Singh]

Hi Vinod,

Thanks for reviewing.

On 24 October 2012 09:35, Vinod Koul <vkoul@infradead.org> wrote:
> On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh 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;
>>       }
>>
>>       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;
>
> This looks fine, but if you use devm_ functions then you dont need to do
> all this.
> Can you do that conversion instead?
>

Good point.
I will do the conversion and send it again.

Regards,
Inder

> --
> Vinod Koul
> Intel Corp.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/4] 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 |   35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 10c6b6a..bf71ff7 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2541,20 +2541,20 @@ 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;
+	for (i = 0; i < count; i++) {
+		desc = kmalloc(sizeof(*desc), flg);
+		if (!desc)
+			break;
+		_init_desc(desc);
 
-	spin_lock_irqsave(&pdmac->pool_lock, flags);
+		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);
-	}
+		list_add_tail(&desc->node, &pdmac->desc_pool);
 
-	spin_unlock_irqrestore(&pdmac->pool_lock, flags);
+		spin_unlock_irqrestore(&pdmac->pool_lock, flags);
+	}
 
-	return count;
+	return i;
 }
 
 static struct dma_pl330_desc *
@@ -2857,6 +2857,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 +2979,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 +3001,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 +3023,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 v2 2/4] DMA: PL330: Change allocation method to properly free DMA descriptors

[Vinod Koul]

On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh 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
For probe, again you have cleaner code by using devm_kzalloc.

On 1, if we use the devm_kzalloc then we don't need to allocate in
chunks right?

> 
> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
> ---
>  drivers/dma/pl330.c |   35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 10c6b6a..bf71ff7 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2541,20 +2541,20 @@ 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;
> +	for (i = 0; i < count; i++) {
> +		desc = kmalloc(sizeof(*desc), flg);
> +		if (!desc)
> +			break;
> +		_init_desc(desc);
>  
> -	spin_lock_irqsave(&pdmac->pool_lock, flags);
> +		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);
> -	}
> +		list_add_tail(&desc->node, &pdmac->desc_pool);
>  
> -	spin_unlock_irqrestore(&pdmac->pool_lock, flags);
> +		spin_unlock_irqrestore(&pdmac->pool_lock, flags);
> +	}
>  
> -	return count;
> +	return i;
>  }
>  
>  static struct dma_pl330_desc *
> @@ -2857,6 +2857,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 +2979,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 +3001,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 +3023,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);


-- 
Vinod Koul
Intel Corp.

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

[Inderpal Singh]

On 24 October 2012 09:40, Vinod Koul <vkoul@infradead.org> wrote:
> On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh 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
> For probe, again you have cleaner code by using devm_kzalloc.
>
> On 1, if we use the devm_kzalloc then we don't need to allocate in
> chunks right?
>

Yes, if we use devm_kzalloc we wont have to allocate in chunks.
I will update this patch to use devm_kzalloc and send it again.


>>
>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
>> ---
>>  drivers/dma/pl330.c |   35 +++++++++++++++++++++++++----------
>>  1 file changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 10c6b6a..bf71ff7 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -2541,20 +2541,20 @@ 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;
>> +     for (i = 0; i < count; i++) {
>> +             desc = kmalloc(sizeof(*desc), flg);
>> +             if (!desc)
>> +                     break;
>> +             _init_desc(desc);
>>
>> -     spin_lock_irqsave(&pdmac->pool_lock, flags);
>> +             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);
>> -     }
>> +             list_add_tail(&desc->node, &pdmac->desc_pool);
>>
>> -     spin_unlock_irqrestore(&pdmac->pool_lock, flags);
>> +             spin_unlock_irqrestore(&pdmac->pool_lock, flags);
>> +     }
>>
>> -     return count;
>> +     return i;
>>  }
>>
>>  static struct dma_pl330_desc *
>> @@ -2857,6 +2857,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 +2979,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 +3001,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 +3023,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);
>
>
> --
> Vinod Koul
> Intel Corp.
>
Regards,
Inder

[PATCH v2 3/4] 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.
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;
+	}
+
 	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)) {
-- 
1.7.9.5

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

[Vinod Koul]

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

-- 
Vinod Koul
Intel Corp.

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

[Inderpal Singh]

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.
>

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

[Vinod Koul]

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/
> 


-- 
Vinod Koul
Intel Corp.

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

[Inderpal Singh]

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.
>

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

[Vinod Koul]

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.
> >


-- 
Vinod Koul
Intel Corp.

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

[Inderpal Singh]

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.
>

[PATCH v2 4/4] DMA: PL330: unregister dma_device in module's remove function

[Inderpal Singh]

unregister dma_device in module's remove function.

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

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4b7a34d..e7dc040 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3017,6 +3017,8 @@ static int __devexit pl330_remove(struct amba_device *adev)
 			return -EBUSY;
 	}
 
+	dma_async_device_unregister(&pdmac->ddma);
+
 	amba_set_drvdata(adev, NULL);
 
 	list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels,
-- 
1.7.9.5

Re: [PATCH v2 4/4] DMA: PL330: unregister dma_device in module's remove function

[Vinod Koul]

On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
> unregister dma_device in module's remove function.
> 
> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
> ---
>  drivers/dma/pl330.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 4b7a34d..e7dc040 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -3017,6 +3017,8 @@ static int __devexit pl330_remove(struct amba_device *adev)
>  			return -EBUSY;
>  	}
>  
> +	dma_async_device_unregister(&pdmac->ddma);
> +
>  	amba_set_drvdata(adev, NULL);
>  
>  	list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels,

Ok with this one :)

Tried applying but didn't work out. You would need to regenerate this
one.

Thanks
-- 
Vinod Koul
Intel Corp.

Re: [PATCH v2 4/4] DMA: PL330: unregister dma_device in module's remove function

[Inderpal Singh]

On 24 October 2012 09:49, Vinod Koul <vkoul@infradead.org> wrote:
> On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
>> unregister dma_device in module's remove function.
>>
>> Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org>
>> ---
>>  drivers/dma/pl330.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 4b7a34d..e7dc040 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -3017,6 +3017,8 @@ static int __devexit pl330_remove(struct amba_device *adev)
>>                       return -EBUSY;
>>       }
>>
>> +     dma_async_device_unregister(&pdmac->ddma);
>> +
>>       amba_set_drvdata(adev, NULL);
>>
>>       list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels,
>
> Ok with this one :)
>
> Tried applying but didn't work out. You would need to regenerate this
> one.
>

I will regenerate this along with other patches and resend.

With Regards,
Inder

> Thanks
> --
> Vinod Koul
> Intel Corp.
>

Re: [PATCH v2 0/4] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

Hello,

On 5 October 2012 06:17, Inderpal Singh <inderpal.singh@linaro.org> wrote:
> 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 last 2 patches balance the module's remove function.
>
> This series depends on "61c6e7531d3b66b3 ........DMA: PL330: Check the
> pointer returned by kzalloc" which is on slave-dma's "fixes" branch.
> Hence slave-dma tree's "next" branch was merged with "fixes" and
> applied patch at [1] to fix the build error.
>
> [1] http://permalink.gmane.org/gmane.linux.kernel.next/24274
>
> Changes since v1:
>  - Protect only list_add_tail with spin_locks
>  - Return EBUSY from remove if channel is in use
>  - unregister dma_device in remove
>
> Inderpal Singh (4):
>   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
>   DMA: PL330: unregister dma_device in module's remove function
>
>  drivers/dma/pl330.c |   53 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 15 deletions(-)
>

Any comments on this v2 version of the patchset?

Thanks,
Inder

> --
> 1.7.9.5
>

Re: [PATCH v2 0/4] DMA: PL330: Fix mem leaks and balance probe/remove

[Jassi Brar]

On Fri, Oct 5, 2012 at 3:47 PM, Inderpal Singh
<inderpal.singh@linaro.org> wrote:
> 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 last 2 patches balance the module's remove function.
>
> This series depends on "61c6e7531d3b66b3 ........DMA: PL330: Check the
> pointer returned by kzalloc" which is on slave-dma's "fixes" branch.
> Hence slave-dma tree's "next" branch was merged with "fixes" and
> applied patch at [1] to fix the build error.
>
> [1] http://permalink.gmane.org/gmane.linux.kernel.next/24274
>
> Changes since v1:
>  - Protect only list_add_tail with spin_locks
>  - Return EBUSY from remove if channel is in use
>  - unregister dma_device in remove
>
> Inderpal Singh (4):
>   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
>   DMA: PL330: unregister dma_device in module's remove function
>
>  drivers/dma/pl330.c |   53 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 15 deletions(-)
>
All seem fine.
Acked-by: Jassi Brar <jassisinghbrar@gmail.com>

Thanks.

Re: [PATCH v2 0/4] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

On 13 October 2012 16:33, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Fri, Oct 5, 2012 at 3:47 PM, Inderpal Singh
> <inderpal.singh@linaro.org> wrote:
>> 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 last 2 patches balance the module's remove function.
>>
>> This series depends on "61c6e7531d3b66b3 ........DMA: PL330: Check the
>> pointer returned by kzalloc" which is on slave-dma's "fixes" branch.
>> Hence slave-dma tree's "next" branch was merged with "fixes" and
>> applied patch at [1] to fix the build error.
>>
>> [1] http://permalink.gmane.org/gmane.linux.kernel.next/24274
>>
>> Changes since v1:
>>  - Protect only list_add_tail with spin_locks
>>  - Return EBUSY from remove if channel is in use
>>  - unregister dma_device in remove
>>
>> Inderpal Singh (4):
>>   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
>>   DMA: PL330: unregister dma_device in module's remove function
>>
>>  drivers/dma/pl330.c |   53 ++++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 38 insertions(+), 15 deletions(-)
>>
> All seem fine.
> Acked-by: Jassi Brar <jassisinghbrar@gmail.com>
>
Thanks Jassi.

Vinod,
I have tested this series against your latest slave-dma ->fixes branch
which is based on 3.7-rc1.
The patchset applies cleanly and works fine.

Thanks,
Inder

> Thanks.