[PATCH v4] mailbox: mtk-cmdq: fix runtime PM usage counter leak in cmdq_mbox_flush()

[PATCH v4] mailbox: mtk-cmdq: fix runtime PM usage counter leak in cmdq_mbox_flush()

[Manush Prajwal]

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.

Verified the merged exit path with a standalone model of the four
possible outcomes (initial get failure, "out:" success, "wait:"
success, "wait:" timeout): the runtime PM refcount returns to its
starting value and the return code is correct on every path.

Signed-off-by: Manush Prajwal <manushprajwal555@gmail.com>
---
v4:
- Add parentheses to the function name in the summary phrase
  (cmdq_mbox_flush -> cmdq_mbox_flush()), per Markus Elfring's review.

- Reformat this changelog so each version identifier stands on its
  own line and blocks are separated by blank lines, per Markus
  Elfring's review.

- No functional change from v3. Still no Fixes tag: I don't have a
  git history for this file available in my current environment to
  identify the commit that introduced this exit path with confidence,
  and would rather leave the tag off than guess a SHA. Still happy to
  add it in a v5 if Markus or anyone else on Cc can point at the right
  commit.

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

Re: [PATCH v4] mailbox: mtk-cmdq: fix runtime PM usage counter leak in cmdq_mbox_flush()

[Markus Elfring]

…
> ---
> v4:
…
> - No functional change from v3. Still no Fixes tag: I don't have a
>   git history for this file available in my current environment to

How can this happen?

Are you still looking for more convenient user interfaces?

See also:
* https://git-scm.com/docs/git-blame
* https://www.geeksforgeeks.org/git/how-to-use-git-blame/#an-alternate-option-to-the-command-line-gui-based
* https://stackoverflow.com/questions/31203001/what-does-git-blame-do


>   identify the commit that introduced this exit path with confidence,

I assume that corresponding patching experiences can grow.


>   and would rather leave the tag off than guess a SHA. Still happy to
>   add it in a v5 if Markus or anyone else on Cc can point at the right
>   commit.

It would be nice if further advice would be added accordingly.


…
>  drivers/mailbox/mtk-cmdq-mailbox.c | 15 +++++++--------…


Regards,
Markus