[PATCH] dmaengine: pl330: Fix runtime PM support for terminated transfers

[Marek Szyprowski]

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