diff mbox

dmaengine: pl330: Fix runtime PM support for terminated transfers

Message ID 1481884751-21411-1-git-send-email-m.szyprowski@samsung.com
State New
Headers show

Commit Message

Marek Szyprowski Dec. 16, 2016, 10:39 a.m. UTC 
PL330 DMA engine driver is leaking a runtime reference after any terminated
DMA transactions. This patch fixes this issue by tracking runtime PM state
of the device and making additional call to pm_runtime_put() in terminate_all
callback if needed.

Fixes: ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 drivers/dma/pl330.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

-- 
1.9.1

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

Comments

Krzysztof Kozlowski Dec. 23, 2016, 2:33 a.m. UTC | #1 
On Fri, Dec 16, 2016 at 11:39:11AM +0100, Marek Szyprowski wrote:
> PL330 DMA engine driver is leaking a runtime reference after any terminated

> DMA transactions. This patch fixes this issue by tracking runtime PM state

> of the device and making additional call to pm_runtime_put() in terminate_all

> callback if needed.

> 

> Fixes: ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  drivers/dma/pl330.c | 11 +++++++++++

>  1 file changed, 11 insertions(+)

> 

> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c

> index 030fe05ed43b..9f3dbc8c63d2 100644

> --- a/drivers/dma/pl330.c

> +++ b/drivers/dma/pl330.c

> @@ -448,6 +448,9 @@ struct dma_pl330_chan {

>  

>  	/* for cyclic capability */

>  	bool cyclic;

> +

> +	/* for runtime pm tracking */

> +	bool active;

>  };

>  

>  struct pl330_dmac {

> @@ -2031,6 +2034,7 @@ static void pl330_tasklet(unsigned long data)

>  		_stop(pch->thread);

>  		spin_unlock(&pch->thread->dmac->lock);

>  		power_down = true;

> +		pch->active = false;

>  	} else {

>  		/* Make sure the PL330 Channel thread is active */

>  		spin_lock(&pch->thread->dmac->lock);

> @@ -2050,6 +2054,7 @@ static void pl330_tasklet(unsigned long data)

>  			desc->status = PREP;

>  			list_move_tail(&desc->node, &pch->work_list);

>  			if (power_down) {

> +				pch->active = true;


It's been a while since I was playign with the driver so I don't
remember everything... but I can't get the logic behind this.

The device is marked as inactive and scheduled to power down. But you
mark chanel as active.

Maybe it is correct but for me it is unreadable.

I understand that you wanted to mark the device as active at some point,
in case transfer was terminated?

Best regards,
Krzysztof

>  				spin_lock(&pch->thread->dmac->lock);

>  				_start(pch->thread);

>  				spin_unlock(&pch->thread->dmac->lock);

> @@ -2164,6 +2169,7 @@ static int pl330_terminate_all(struct dma_chan *chan)

>  	unsigned long flags;

>  	struct pl330_dmac *pl330 = pch->dmac;

>  	LIST_HEAD(list);

> +	bool power_down = false;

>  

>  	pm_runtime_get_sync(pl330->ddma.dev);

>  	spin_lock_irqsave(&pch->lock, flags);

> @@ -2174,6 +2180,8 @@ static int pl330_terminate_all(struct dma_chan *chan)

>  	pch->thread->req[0].desc = NULL;

>  	pch->thread->req[1].desc = NULL;

>  	pch->thread->req_running = -1;

> +	power_down = pch->active;

> +	pch->active = false;

>  

>  	/* Mark all desc done */

>  	list_for_each_entry(desc, &pch->submitted_list, node) {

> @@ -2191,6 +2199,8 @@ static int pl330_terminate_all(struct dma_chan *chan)

>  	list_splice_tail_init(&pch->completed_list, &pl330->desc_pool);

>  	spin_unlock_irqrestore(&pch->lock, flags);

>  	pm_runtime_mark_last_busy(pl330->ddma.dev);

> +	if (power_down)

> +		pm_runtime_put_autosuspend(pl330->ddma.dev);

>  	pm_runtime_put_autosuspend(pl330->ddma.dev);

>  

>  	return 0;

> @@ -2350,6 +2360,7 @@ static void pl330_issue_pending(struct dma_chan *chan)

>  		 * updated on work_list emptiness status.

>  		 */

>  		WARN_ON(list_empty(&pch->submitted_list));

> +		pch->active = true;

>  		pm_runtime_get_sync(pch->dmac->ddma.dev);

>  	}

>  	list_splice_tail_init(&pch->submitted_list, &pch->work_list);

> -- 

> 1.9.1

> 

--
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
Marek Szyprowski Dec. 23, 2016, 9:52 a.m. UTC | #2 
Hi Krzysztof,


On 2016-12-23 03:33, Krzysztof Kozlowski wrote:
> On Fri, Dec 16, 2016 at 11:39:11AM +0100, Marek Szyprowski wrote:

>> PL330 DMA engine driver is leaking a runtime reference after any terminated

>> DMA transactions. This patch fixes this issue by tracking runtime PM state

>> of the device and making additional call to pm_runtime_put() in terminate_all

>> callback if needed.

>>

>> Fixes: ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>> ---

>>   drivers/dma/pl330.c | 11 +++++++++++

>>   1 file changed, 11 insertions(+)

>>

>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c

>> index 030fe05ed43b..9f3dbc8c63d2 100644

>> --- a/drivers/dma/pl330.c

>> +++ b/drivers/dma/pl330.c

>> @@ -448,6 +448,9 @@ struct dma_pl330_chan {

>>   

>>   	/* for cyclic capability */

>>   	bool cyclic;

>> +

>> +	/* for runtime pm tracking */

>> +	bool active;

>>   };

>>   

>>   struct pl330_dmac {

>> @@ -2031,6 +2034,7 @@ static void pl330_tasklet(unsigned long data)

>>   		_stop(pch->thread);

>>   		spin_unlock(&pch->thread->dmac->lock);

>>   		power_down = true;

>> +		pch->active = false;

>>   	} else {

>>   		/* Make sure the PL330 Channel thread is active */

>>   		spin_lock(&pch->thread->dmac->lock);

>> @@ -2050,6 +2054,7 @@ static void pl330_tasklet(unsigned long data)

>>   			desc->status = PREP;

>>   			list_move_tail(&desc->node, &pch->work_list);

>>   			if (power_down) {

>> +				pch->active = true;

> It's been a while since I was playign with the driver so I don't

> remember everything... but I can't get the logic behind this.

>

> The device is marked as inactive and scheduled to power down. But you

> mark chanel as active.


Please look 3 lines further. The channel is started again (because this
is cyclic request), so setting active to true is justified. Even power_down
is then set to false.

> Maybe it is correct but for me it is unreadable.

>

> I understand that you wanted to mark the device as active at some point,

> in case transfer was terminated?


Frankly, I have no idea how to explain it more clear. Cyclic transfers are
automatically restarted, so the above logic simply keeps pch->active in sync
with hardware operations.

>

> Best regards,

> Krzysztof

>

>>   				spin_lock(&pch->thread->dmac->lock);

>>   				_start(pch->thread);

>>   				spin_unlock(&pch->thread->dmac->lock);

>> @@ -2164,6 +2169,7 @@ static int pl330_terminate_all(struct dma_chan *chan)

>>   	unsigned long flags;

>>   	struct pl330_dmac *pl330 = pch->dmac;

>>   	LIST_HEAD(list);

>> +	bool power_down = false;

>>   

>>   	pm_runtime_get_sync(pl330->ddma.dev);

>>   	spin_lock_irqsave(&pch->lock, flags);

>> @@ -2174,6 +2180,8 @@ static int pl330_terminate_all(struct dma_chan *chan)

>>   	pch->thread->req[0].desc = NULL;

>>   	pch->thread->req[1].desc = NULL;

>>   	pch->thread->req_running = -1;

>> +	power_down = pch->active;

>> +	pch->active = false;

>>   

>>   	/* Mark all desc done */

>>   	list_for_each_entry(desc, &pch->submitted_list, node) {

>> @@ -2191,6 +2199,8 @@ static int pl330_terminate_all(struct dma_chan *chan)

>>   	list_splice_tail_init(&pch->completed_list, &pl330->desc_pool);

>>   	spin_unlock_irqrestore(&pch->lock, flags);

>>   	pm_runtime_mark_last_busy(pl330->ddma.dev);

>> +	if (power_down)

>> +		pm_runtime_put_autosuspend(pl330->ddma.dev);

>>   	pm_runtime_put_autosuspend(pl330->ddma.dev);

>>   

>>   	return 0;

>> @@ -2350,6 +2360,7 @@ static void pl330_issue_pending(struct dma_chan *chan)

>>   		 * updated on work_list emptiness status.

>>   		 */

>>   		WARN_ON(list_empty(&pch->submitted_list));

>> +		pch->active = true;

>>   		pm_runtime_get_sync(pch->dmac->ddma.dev);

>>   	}

>>   	list_splice_tail_init(&pch->submitted_list, &pch->work_list);

>> -- 

>> 1.9.1

>>


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
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
Krzysztof Kozlowski Dec. 24, 2016, 11:13 a.m. UTC | #3 
On Fri, Dec 23, 2016 at 10:52:28AM +0100, Marek Szyprowski wrote:
> Hi Krzysztof,

> 

> 

> On 2016-12-23 03:33, Krzysztof Kozlowski wrote:

> >On Fri, Dec 16, 2016 at 11:39:11AM +0100, Marek Szyprowski wrote:

> >>PL330 DMA engine driver is leaking a runtime reference after any terminated

> >>DMA transactions. This patch fixes this issue by tracking runtime PM state

> >>of the device and making additional call to pm_runtime_put() in terminate_all

> >>callback if needed.

> >>

> >>Fixes: ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")

> >>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> >>---

> >>  drivers/dma/pl330.c | 11 +++++++++++

> >>  1 file changed, 11 insertions(+)

> >>

> >>diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c

> >>index 030fe05ed43b..9f3dbc8c63d2 100644

> >>--- a/drivers/dma/pl330.c

> >>+++ b/drivers/dma/pl330.c

> >>@@ -448,6 +448,9 @@ struct dma_pl330_chan {

> >>  	/* for cyclic capability */

> >>  	bool cyclic;

> >>+

> >>+	/* for runtime pm tracking */

> >>+	bool active;

> >>  };

> >>  struct pl330_dmac {

> >>@@ -2031,6 +2034,7 @@ static void pl330_tasklet(unsigned long data)

> >>  		_stop(pch->thread);

> >>  		spin_unlock(&pch->thread->dmac->lock);

> >>  		power_down = true;

> >>+		pch->active = false;

> >>  	} else {

> >>  		/* Make sure the PL330 Channel thread is active */

> >>  		spin_lock(&pch->thread->dmac->lock);

> >>@@ -2050,6 +2054,7 @@ static void pl330_tasklet(unsigned long data)

> >>  			desc->status = PREP;

> >>  			list_move_tail(&desc->node, &pch->work_list);

> >>  			if (power_down) {

> >>+				pch->active = true;

> >It's been a while since I was playign with the driver so I don't

> >remember everything... but I can't get the logic behind this.

> >

> >The device is marked as inactive and scheduled to power down. But you

> >mark chanel as active.

> 

> Please look 3 lines further. The channel is started again (because this

> is cyclic request), so setting active to true is justified. Even power_down

> is then set to false.


Ahhh, damn the missing context. I looked at full source and it makes
sense now.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>


Best regards,
Krzysztof

--
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
Vinod Koul Jan. 3, 2017, 3:49 a.m. UTC | #4 
On Fri, Dec 16, 2016 at 11:39:11AM +0100, Marek Szyprowski wrote:
> PL330 DMA engine driver is leaking a runtime reference after any terminated

> DMA transactions. This patch fixes this issue by tracking runtime PM state

> of the device and making additional call to pm_runtime_put() in terminate_all

> callback if needed.


Applied now

-- 
~Vinod
--
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
diff mbox

Patch

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 030fe05ed43b..9f3dbc8c63d2 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -448,6 +448,9 @@  struct dma_pl330_chan {
 
 	/* for cyclic capability */
 	bool cyclic;
+
+	/* for runtime pm tracking */
+	bool active;
 };
 
 struct pl330_dmac {
@@ -2031,6 +2034,7 @@  static void pl330_tasklet(unsigned long data)
 		_stop(pch->thread);
 		spin_unlock(&pch->thread->dmac->lock);
 		power_down = true;
+		pch->active = false;
 	} else {
 		/* Make sure the PL330 Channel thread is active */
 		spin_lock(&pch->thread->dmac->lock);
@@ -2050,6 +2054,7 @@  static void pl330_tasklet(unsigned long data)
 			desc->status = PREP;
 			list_move_tail(&desc->node, &pch->work_list);
 			if (power_down) {
+				pch->active = true;
 				spin_lock(&pch->thread->dmac->lock);
 				_start(pch->thread);
 				spin_unlock(&pch->thread->dmac->lock);
@@ -2164,6 +2169,7 @@  static int pl330_terminate_all(struct dma_chan *chan)
 	unsigned long flags;
 	struct pl330_dmac *pl330 = pch->dmac;
 	LIST_HEAD(list);
+	bool power_down = false;
 
 	pm_runtime_get_sync(pl330->ddma.dev);
 	spin_lock_irqsave(&pch->lock, flags);
@@ -2174,6 +2180,8 @@  static int pl330_terminate_all(struct dma_chan *chan)
 	pch->thread->req[0].desc = NULL;
 	pch->thread->req[1].desc = NULL;
 	pch->thread->req_running = -1;
+	power_down = pch->active;
+	pch->active = false;
 
 	/* Mark all desc done */
 	list_for_each_entry(desc, &pch->submitted_list, node) {
@@ -2191,6 +2199,8 @@  static int pl330_terminate_all(struct dma_chan *chan)
 	list_splice_tail_init(&pch->completed_list, &pl330->desc_pool);
 	spin_unlock_irqrestore(&pch->lock, flags);
 	pm_runtime_mark_last_busy(pl330->ddma.dev);
+	if (power_down)
+		pm_runtime_put_autosuspend(pl330->ddma.dev);
 	pm_runtime_put_autosuspend(pl330->ddma.dev);
 
 	return 0;
@@ -2350,6 +2360,7 @@  static void pl330_issue_pending(struct dma_chan *chan)
 		 * updated on work_list emptiness status.
 		 */
 		WARN_ON(list_empty(&pch->submitted_list));
+		pch->active = true;
 		pm_runtime_get_sync(pch->dmac->ddma.dev);
 	}
 	list_splice_tail_init(&pch->submitted_list, &pch->work_list);