On Tue, 2024-06-18 at 13:47 +0200, AngeloGioacchino Del Regno wrote:
Il 18/06/24 05:28, Jason-JH Lin (林睿祥) ha scritto:https://lore.kernel.org/all/6fcd48b14e865c25e6db7559fe6b946537bfa0ce.camel@xxxxxxxxxxxx/
Hi Angelo,
On Fri, 2024-06-14 at 00:52 +0800, Jason-JH.Lin wrote:
Hi Angelo,
On Thu, 2024-06-13 at 17:10 +0200, AngeloGioacchino Del Regno
wrote:
Il 13/06/24 17:06, Jason-JH.Lin ha scritto:
Add unregister mailbox controller in cmdq_remove to fix cmdq
unbind
WARN_ON message from pm_runtime_get_sync() in
cmdq_mbox_shutdown().
Fixes: 623a6143a845 ("mailbox: mediatek: Add Mediatek CMDQ
driver")
Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>
Hello,
I think you forgot about...
I'll send this series next week after testing it.
...as that would also resolve this one without any hacks.
I thought it was another problem, so I sent this patch.
After looking to the kerneldoc of
devm_mbox_controller_unregister(),
I
found that it's not necessary to call this anywhere.
I'll drop this patch. Thanks for the review.
I found that the series of "Move pm_runimte_get and put to
mbox_chan_ops API" can not fix this unbind crash issue.
It seems they are 2 different issues.
So I think calling devm_mbox_controller_unregister() in
cmdq_remove()
can ensure the CMDQ device is not removed and be paired to
cmdq_probe().
Can you please paste the stack trace of that warning that you're
seeing when
calling cmdq_remove()?
I'm not convinced that this is the best solution - it might be, but I
have
a hunch that there might be a better way to address this issue.
After tracing the stack trace again, I found this call trace warning is
caused in WARN_ON(pm_runtime_get_sync(cmdq->mbox.dev) < 0). The return
value of pm_runtime_get_sync() is -13(-EACCESS) that's caused by
calling pm_runtime_disable() before calling pm_runtime_get_sync().
CMDQ driver uses devm_mbox_controller_register() in cmdq_probe() to
bind the cmdq device to the mbox_controller, so
devm_mbox_controller_unregister() will automatically unregister the
device bound to the mailbox controller when the device-managed resource
is removed. That means devm_mbox_controller_unregister() and
cmdq_mbox_shoutdown() will be called after cmdq_remove().
CMDQ driver also uses devm_pm_runtime_enable() in cmdq_probe() after
devm_mbox_controller_register(), so that devm_pm_runtime_disable() will
be called after cmdq_remove(), but before
devm_mbox_controller_unregister().
To fix this problem, we need to make devm_pm_runtime_disable() be
called after devm_mbox_controller_unregister().
I've tried 2 ways can fix this problem:
- Swap the sequence of devm_mbox_controller_register() and
devm_pm_runtime_enable() in cmdq_probe()
- Change to use mbox_controller_register() in cmdq_probe() and use
mbox_controller_unregister() in cmdq_probe()
Which one do you think is better?
Regards,
Jason-JH.Lin
Thanks!
Angelo
Regards,
Jason-JH.Lin
Regards,
Jason-JH.Lin
Cheers,
Angelo
---
drivers/mailbox/mtk-cmdq-mailbox.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
b/drivers/mailbox/mtk-cmdq-mailbox.c
index 4aa394e91109..1399e18a39a4 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -371,6 +371,8 @@ static void cmdq_remove(struct
platform_device
*pdev)
{
struct cmdq *cmdq = platform_get_drvdata(pdev);
+ devm_mbox_controller_unregister(&pdev->dev, &cmdq-mbox);+
if (cmdq->pdata->sw_ddr_en)
cmdq_sw_ddr_enable(cmdq, false);