Skip to contentSkip to conversation

dmaengine: pl330: Fix runtime PM support for terminated transfers

PATCH New Marek Szyprowski 3 participants 5 messages Dec 16, 2016 – Jan 3, 2017

Subsystems (2)
Arm Samsung Exynos dmaengine
lore
Share & tools
lore

Marek Szyprowski

author
#0 lore raw

PATCH 1/1 dmaengine: pl330: Fix runtime PM support for terminated transfers

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 <[email protected]>
---
 drivers/dma/pl330.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/dma/pl330.c +11 -0
index 030fe05ed43b..9f3dbc8c63d2 100644
@@ -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);
Signature · 1 line
1.9.1
maintainer
#1 lore raw

PATCH 1/1 dmaengine: pl330: Fix runtime PM support for terminated transfers

Quoted 34 lines · Marek Szyprowski

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 <[email protected]>
---
 drivers/dma/pl330.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/dma/pl330.c +4 -0
index 030fe05ed43b..9f3dbc8c63d2 100644
@@ -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);

drivers/dma/pl330.c static void pl330_tasklet(unsigned long data) · line 2057

Quoted 4 lines
drivers/dma/pl330.c +1 -0
@@ -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

Quoted 40 lines
drivers/dma/pl330.c +6 -0
 				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);
Signature · 1 line
1.9.1
author
#2 lore raw

PATCH 1/1 dmaengine: pl330: Fix runtime PM support for terminated transfers

Hi Krzysztof,

Quoted 42 lines · Krzysztof Kozlowski
Quoted 34 lines · Marek Szyprowski

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 <[email protected]>
---
  drivers/dma/pl330.c | 11 +++++++++++
  1 file changed, 11 insertions(+)
drivers/dma/pl330.c +4 -0
index 030fe05ed43b..9f3dbc8c63d2 100644
@@ -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);

drivers/dma/pl330.c static void pl330_tasklet(unsigned long data) · line 2057

Quoted 4 lines
drivers/dma/pl330.c +1 -0
@@ -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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 32 lines

Best regards,
Krzysztof

Quoted 28 lines
drivers/dma/pl330.c +5 -0
  				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);
Quoted 12 lines
Quoted 12 lines
drivers/dma/pl330.c +1 -0
  	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);
Signature · 1 line
1.9.1

Best regards

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
maintainer
#3 lore raw

PATCH 1/1 dmaengine: pl330: Fix runtime PM support for terminated transfers

Quoted 44 lines · Marek Szyprowski

Hi Krzysztof,

Quoted 40 lines · Krzysztof Kozlowski
Quoted 32 lines · Marek Szyprowski

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 <[email protected]>
---
 drivers/dma/pl330.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/dma/pl330.c +4 -0
index 030fe05ed43b..9f3dbc8c63d2 100644
@@ -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);

drivers/dma/pl330.c static void pl330_tasklet(unsigned long data) · line 2057

Quoted 4 lines
drivers/dma/pl330.c +1 -0
@@ -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.

Quoted 6 lines
Quoted 2 lines

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 <[email protected]>

Best regards,
Krzysztof

#4 lore raw

PATCH 1/1 dmaengine: pl330: Fix runtime PM support for terminated transfers

In reply to Marek Szyprowski
Quoted 4 lines · Marek Szyprowski

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

Signature · 1 line
~Vinod