[PATCH v3] mailbox: mtk-cmdq: fix runtime PM usage counter leak in cmdq_mbox_flush
"Manush Prajwal" <[email protected]>
| Newsgroups | org.infradead.lists.linux-mediatek,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
cmdq_mbox_flush() calls pm_runtime_get_sync() and returns its error code directly on failure. Per Documentation/power/runtime_pm.rst, pm_runtime_get_sync() does not drop the usage counter on error, so the caller is expected to release the reference itself. The early return here skipped that, leaking a runtime PM usage count on cmdq->mbox.dev on every failed resume. v2 fixed this by switching to pm_runtime_resume_and_get(), which performs the get-and-put internally on failure. Markus Elfring additionally pointed out that the "out:" and "wait:" labels duplicate the same pm_runtime_mark_last_busy() + pm_runtime_put_autosuspend() + return sequence. That duplication hides a second, separate leak: the "wait:" path's timeout branch (readl_poll_timeout_atomic() failing) returns -EFAULT directly, without ever calling pm_runtime_mark_last_busy() / pm_runtime_put_autosuspend(), so the pm_runtime reference taken at function entry is leaked on every polling timeout too. Fix both by merging the two exit sequences into one common "out_pm:" label reached by both "out:" and "wait:", using "ret" to carry the return value. "ret" is explicitly reset to 0 on both success paths before reaching "out_pm:", since pm_runtime_resume_and_get() can return a positive value (e.g. 1) on success, not just 0, and that stale value must not leak into the function's return value. Signed-off-by: Manush Prajwal <[email protected]> --- v3: Fix the pm_runtime leak on the "wait:" timeout path found while addressing Markus Elfring's duplicate-code comment, by merging the "out:" and "wait:" exit sequences into a single "out_pm:" label. Add Matthias Brugger and AngeloGioacchino Del Regno plus the linux-mediatek and linux-arm-kernel lists to Cc, per MAINTAINERS' "ARM/Mediatek SoC support" entry (matches any drivers/mailbox/mtk-*.c filename), per Markus Elfring's review. v2: Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync() plus a manual pm_runtime_put_noidle() on the error path, per Markus Elfring's review. drivers/mailbox/mtk-cmdq-mailbox.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c index e523c84b4..d4e5f6a7b 100644 --- a/drivers/mailbox/mtk-cmdq-mailbox.c +++ b/drivers/mailbox/mtk-cmdq-mailbox.c @@ -565,7 +565,7 @@ static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long timeout) int ret; - ret = pm_runtime_get_sync(cmdq->mbox.dev); + ret = pm_runtime_resume_and_get(cmdq->mbox.dev); if (ret < 0) return ret; @@ -590,22 +590,21 @@ static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long timeout) cmdq_thread_disable(cmdq, thread); out: spin_unlock_irqrestore(&thread->chan->lock, flags); - pm_runtime_mark_last_busy(cmdq->mbox.dev); - pm_runtime_put_autosuspend(cmdq->mbox.dev); - - return 0; + ret = 0; + goto out_pm; wait: cmdq_thread_resume(thread); spin_unlock_irqrestore(&thread->chan->lock, flags); + ret = 0; if (readl_poll_timeout_atomic(thread->base + CMDQ_THR_ENABLE_TASK, enable, enable == 0, 1, timeout)) { dev_err(cmdq->mbox.dev, "Fail to wait GCE thread 0x%x done\n", (u32)(thread->base - cmdq->base)); - - return -EFAULT; + ret = -EFAULT; } + +out_pm: pm_runtime_mark_last_busy(cmdq->mbox.dev); pm_runtime_put_autosuspend(cmdq->mbox.dev); - return 0; + return ret; } -- 2.46.2.windows.1