| Message ID | 1481884751-21411-1-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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
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);
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(+)