[PATCH] mailbox: mtk-cmdq: Add unregister mailbox controller in cmdq_remove()

[PATCH] mailbox: mtk-cmdq: Add unregister mailbox controller in cmdq_remove()

[Jason-JH.Lin]

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@mediatek.com>
---
 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);
 
-- 
2.18.0

Re: [PATCH] mailbox: mtk-cmdq: Add unregister mailbox controller in cmdq_remove()

[AngeloGioacchino Del Regno]

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@mediatek.com>

Hello,

I think you forgot about...

https://lore.kernel.org/all/6fcd48b14e865c25e6db7559fe6b946537bfa0ce.camel@mediatek.com/

...as that would also resolve this one without any hacks.

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);
>   

Re: [PATCH] mailbox: mtk-cmdq: Add unregister mailbox controller in cmdq_remove()

[Jason-JH Lin (林睿祥)]

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@mediatek.com>
> 
> Hello,
> 
> I think you forgot about...
> 
> 
https://lore.kernel.org/all/6fcd48b14e865c25e6db7559fe6b946537bfa0ce.camel@mediatek.com/
> 

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.

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);
> >   
> 
> 

Re: [PATCH] mailbox: mtk-cmdq: Add unregister mailbox controller in cmdq_remove()

[Jason-JH Lin (林睿祥)]

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@mediatek.com>
> > 
> > Hello,
> > 
> > I think you forgot about...
> > 
> > 
> 
> 
https://lore.kernel.org/all/6fcd48b14e865c25e6db7559fe6b946537bfa0ce.camel@mediatek.com/
> > 
> 
> 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().

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);
> > >   
> > 
> > 

Re: [PATCH] mailbox: mtk-cmdq: Add unregister mailbox controller in cmdq_remove()

[AngeloGioacchino Del Regno]

Il 18/06/24 05:28, Jason-JH Lin (林睿祥) ha scritto:
> 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@mediatek.com>
>>>
>>> Hello,
>>>
>>> I think you forgot about...
>>>
>>>
>>
>>
> https://lore.kernel.org/all/6fcd48b14e865c25e6db7559fe6b946537bfa0ce.camel@mediatek.com/
>>>
>>
>> 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.

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);
>>>>    
>>>
>>>

Re: [PATCH] mailbox: mtk-cmdq: Add unregister mailbox controller in cmdq_remove()

[Jason-JH Lin (林睿祥)]

Hi Angelo,

[snip]

> > 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 added dump_stack() to the first line of cmdq_remove() and reproduced
the crash issue.

Here is the log:
[  191.058651] CPU: 4 PID: 4408 Comm: bash Not tainted 6.6.32-02970-g5212e5e959db-dirty #1 ba5ec8409e01b005e537787edcf4515f18ba6ad7[  191.070213] Hardware name: Google Rauru board (DT)
[  191.074997] Call trace:
[  191.077435]  dump_backtrace+0xe8/0x110
[  191.081184]  show_stack+0x1c/0x40
[  191.084494]  dump_stack_lvl+0x80/0xa8
[  191.088155]  dump_stack+0x14/0x28
[  191.091466]  cmdq_remove+0x1c/0x60
[  191.094867]  platform_remove+0x3c/0x78
[  191.098616]  device_remove+0x4c/0x78
[  191.102188]  device_release_driver_internal+0xc4/0x140
[  191.107320]  device_driver_detach+0x1c/0x30
[  191.111498]  unbind_store+0x74/0xc0
[  191.114980]  drv_attr_store+0x28/0x40
[  191.118635]  sysfs_kf_write+0x40/0x58
[  191.122293]  kernfs_fop_write_iter+0xf0/0x190
[  191.126644]  vfs_write+0x234/0x320
[  191.130046]  ksys_write+0x7c/0xf0
[  191.133357]  __arm64_sys_write+0x20/0x30
[  191.137277]  invoke_syscall+0x70/0xf8
[  191.140939]  el0_svc_common+0x8c/0xf0
[  191.144598]  do_el0_svc+0x24/0x38
[  191.147911]  el0_svc+0x34/0x98
[  191.150964]  el0t_64_sync_handler+0x40/0xe8
[  191.155144]  el0t_64_sync+0x180/0x188
[  191.158975] ------------[ cut here ]------------
[  191.163584] WARNING: CPU: 4 PID: 4408 at drivers/mailbox/mtk-cmdq-
mailbox.c:534 cmdq_mbox_shutdown+0x190/0x1b0
[  191.173583] Modules linked in: dm_integrity async_xor xor xor_neon
async_tx lz4 lz4_compress zstd zstd_compress zram zsmalloc uinput
rfcomm veth fuse uvcvideo videobuf2_vmalloc uvc mediatek_cpufreq_hw
mtk_jpeg videobuf2_dma_contig videobuf2_memops mtk_jpeg_enc_hw
mtk_jpeg_dec_hw v4l2_mem2mem videobuf2_v4l2 videobuf2_common
ip6table_nat xt_MASQUERADE xt_cgroup 8021q mt7921e mt7921_common
mt792x_lib mt76_connac_lib mt76 mac80211 ax88796b iio_trig_sysfs
algif_hash algif_skcipher cros_ec_light_prox cros_ec_sensors af_alg
cros_ec_sensors_core cfg80211 industrialio_triggered_buffer kfifo_buf
asix phylink cros_ec_sensorhub usbnet mii btusb btmtk btbcm btintel
btrtl bluetooth ecdh_generic ecc joydev
[  191.234890] CPU: 4 PID: 4408 Comm: bash Not tainted 6.6.32-02970-
g5212e5e959db-dirty #1 ba5ec8409e01b005e537787edcf4515f18ba6ad7
[  191.246442] Hardware name: Google Rauru board (DT)
[  191.251225] pstate: 23400009 (nzCv daif +PAN -UAO +TCO +DIT -SSBS
BTYPE=--)
[  191.258178] pc : cmdq_mbox_shutdown+0x190/0x1b0
[  191.262707] lr : cmdq_mbox_shutdown+0x4c/0x1b0
[  191.267148] sp : ffffffc086f73a70
[  191.270456] x29: ffffffc086f73a90 x28: ffffff80f9282e80 x27:
0000000000000000
[  191.277586] x26: 0000000000000000 x25: 0000000000000000 x24:
0000000000000000
[  191.284716] x23: ffffff808089c800 x22: ffffffd6426d11c7 x21:
0000000000000000
[  191.291845] x20: ffffff808089c880 x19: ffffff80802d7480 x18:
00000000000001b7
[  191.298975] x17: 000000000000000e x16: 00000000000001b7 x15:
0000000000000004
[  191.306104] x14: fffffffffffffffe x13: 0000000000080800 x12:
0000000000000001
[  191.313234] x11: 00000000fffffff3 x10: dead000000000122 x9 :
0000000100000000
[  191.320363] x8 : 0000000100000000 x7 : 7f7f7f7f7f7f7f7f x6 :
fefefefefefefeff
[  191.327493] x5 : 0000000000000008 x4 : ffffffd64268784d x3 :
ffffff8083fa09c0
[  191.334621] x2 : ffffffd6426d11c7 x1 : 0000000000000000 x0 :
00000000fffffff3
[  191.341749] Call trace:
[  191.344189]  cmdq_mbox_shutdown+0x190/0x1b0
[  191.348370]  mbox_free_channel+0x40/0xa8
[  191.352290]  mbox_controller_unregister+0x8c/0xe0
[  191.356991]  __devm_mbox_controller_unregister+0x18/0x28
[  191.362300]  release_nodes+0x5c/0x90
[  191.365875]  devres_release_all+0x8c/0xd8
[  191.369883]  device_unbind_cleanup+0x1c/0x70
[  191.374146]  device_release_driver_internal+0xe4/0x140
[  191.379277]  device_driver_detach+0x1c/0x30
[  191.383454]  unbind_store+0x74/0xc0
[  191.386937]  drv_attr_store+0x28/0x40
[  191.390593]  sysfs_kf_write+0x40/0x58
[  191.394249]  kernfs_fop_write_iter+0xf0/0x190
[  191.398599]  vfs_write+0x234/0x320
[  191.401999]  ksys_write+0x7c/0xf0
[  191.405311]  __arm64_sys_write+0x20/0x30
[  191.409231]  invoke_syscall+0x70/0xf8
[  191.412892]  el0_svc_common+0x8c/0xf0
[  191.416552]  do_el0_svc+0x24/0x38
[  191.419865]  el0_svc+0x34/0x98
[  191.422916]  el0t_64_sync_handler+0x40/0xe8
[  191.427097]  el0t_64_sync+0x180/0x188
[  191.430751] ---[ end trace 0000000000000000 ]---

Regards,
Jason-JH Lin

> 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.
> 
> Thanks!
> Angelo

Re: [PATCH] mailbox: mtk-cmdq: Add unregister mailbox controller in cmdq_remove()

[Jason-JH Lin (林睿祥)]

On Tue, 2024-06-18 at 13:47 +0200, AngeloGioacchino Del Regno wrote:
> Il 18/06/24 05:28, Jason-JH Lin (林睿祥) ha scritto:
> > 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@mediatek.com>
> > > > 
> > > > Hello,
> > > > 
> > > > I think you forgot about...
> > > > 
> > > > 
> > > 
> > > 
> > 
> > 
https://lore.kernel.org/all/6fcd48b14e865c25e6db7559fe6b946537bfa0ce.camel@mediatek.com/
> > > > 
> > > 
> > > 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);
> > > > >    
> > > > 
> > > > 
> 
> 

Re: [PATCH] mailbox: mtk-cmdq: Add unregister mailbox controller in cmdq_remove()

[AngeloGioacchino Del Regno]

Il 17/07/24 05:44, Jason-JH Lin (林睿祥) ha scritto:
> On Tue, 2024-06-18 at 13:47 +0200, AngeloGioacchino Del Regno wrote:
>> Il 18/06/24 05:28, Jason-JH Lin (林睿祥) ha scritto:
>>> 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@mediatek.com>
>>>>>
>>>>> Hello,
>>>>>
>>>>> I think you forgot about...
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
> https://lore.kernel.org/all/6fcd48b14e865c25e6db7559fe6b946537bfa0ce.camel@mediatek.com/
>>>>>
>>>>
>>>> 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()
> 

Hey.

That's a nice conclusion here!
If the first one has no issues, go for the first one: that's just about
moving a call upwards, noiseless and pretty.

Cheers!
Angelo

> 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);
>>>>>>     
>>>>>
>>>>>
>>
>>