[PATCH] staging: vc04_services: Stop kthreads on .remove

[PATCH] staging: vc04_services: Stop kthreads on .remove

[Umang Jain]

It turns out that stopping kthreads on vchiq_shutdown_internal()
corrupts the vchiq firmware during probe.

[   11.005324] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf unexpected msgid 0@b18db30a,0

Since the kthreads are created during vchiq_probe(), symmetrically
they should be stopped on vchiq_remove().

Also, vchiq_shutdown_internal() is called by vchiq_shutdown() which
is a exported function. Hence, modules can take indirectly shut the
vchiq interface by stopping the kthreads.

Fix it by stopping the kthreads in .remove instead.

Fixes: d9c60badccc1 ("staging: vc04_services: vchiq_core: Stop kthreads on shutdown")
Reported-by: Stefan Wahren <wahrenst@gmx.net>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---

Reproduced on rpi-3-b 32-bit kernel with multi_v7_defconfig and all
vchiq configs options. Patch seems to fix the error mentioned in the
commit message.

---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c   | 6 ++++++
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c  | 4 ----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 1d21f9cfbfe5..9af77d17a1e8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1771,10 +1771,16 @@ static int vchiq_probe(struct platform_device *pdev)
 
 static void vchiq_remove(struct platform_device *pdev)
 {
+	struct vchiq_state *state = vchiq_get_state();
+
 	vchiq_device_unregister(bcm2835_audio);
 	vchiq_device_unregister(bcm2835_camera);
 	vchiq_debugfs_deinit();
 	vchiq_deregister_chrdev();
+
+	kthread_stop(state->sync_thread);
+	kthread_stop(state->recycle_thread);
+	kthread_stop(state->slot_handler_thread);
 }
 
 static struct platform_driver vchiq_driver = {
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 953ccd87f425..658d19f1e7e8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2844,10 +2844,6 @@ vchiq_shutdown_internal(struct vchiq_state *state, struct vchiq_instance *instan
 		(void)vchiq_remove_service(instance, service->handle);
 		vchiq_service_put(service);
 	}
-
-	kthread_stop(state->sync_thread);
-	kthread_stop(state->recycle_thread);
-	kthread_stop(state->slot_handler_thread);
 }
 
 int
-- 
2.43.0

Re: [PATCH] staging: vc04_services: Stop kthreads on .remove

[Kieran Bingham]

Quoting Umang Jain (2024-03-28 05:16:15)
> It turns out that stopping kthreads on vchiq_shutdown_internal()
> corrupts the vchiq firmware during probe.
> 
> [   11.005324] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf unexpected msgid 0@b18db30a,0
> 
> Since the kthreads are created during vchiq_probe(), symmetrically
> they should be stopped on vchiq_remove().
> 
> Also, vchiq_shutdown_internal() is called by vchiq_shutdown() which
> is a exported function. Hence, modules can take indirectly shut the
> vchiq interface by stopping the kthreads.
> 
> Fix it by stopping the kthreads in .remove instead.
> 
> Fixes: d9c60badccc1 ("staging: vc04_services: vchiq_core: Stop kthreads on shutdown")
> Reported-by: Stefan Wahren <wahrenst@gmx.net>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
> 
> Reproduced on rpi-3-b 32-bit kernel with multi_v7_defconfig and all
> vchiq configs options. Patch seems to fix the error mentioned in the
> commit message.

Yikes, I wish I'd spotted that asymmetry in my review of the previous
patch, but certainly keeping this symmetrical here sounds correct and
looks right to me. And if it fixes the bug ...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> ---
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c   | 6 ++++++
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c  | 4 ----
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 1d21f9cfbfe5..9af77d17a1e8 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1771,10 +1771,16 @@ static int vchiq_probe(struct platform_device *pdev)
>  
>  static void vchiq_remove(struct platform_device *pdev)
>  {
> +       struct vchiq_state *state = vchiq_get_state();
> +
>         vchiq_device_unregister(bcm2835_audio);
>         vchiq_device_unregister(bcm2835_camera);
>         vchiq_debugfs_deinit();
>         vchiq_deregister_chrdev();
> +
> +       kthread_stop(state->sync_thread);
> +       kthread_stop(state->recycle_thread);
> +       kthread_stop(state->slot_handler_thread);
>  }
>  
>  static struct platform_driver vchiq_driver = {
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> index 953ccd87f425..658d19f1e7e8 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -2844,10 +2844,6 @@ vchiq_shutdown_internal(struct vchiq_state *state, struct vchiq_instance *instan
>                 (void)vchiq_remove_service(instance, service->handle);
>                 vchiq_service_put(service);
>         }
> -
> -       kthread_stop(state->sync_thread);
> -       kthread_stop(state->recycle_thread);
> -       kthread_stop(state->slot_handler_thread);
>  }
>  
>  int
> -- 
> 2.43.0
>

Re: [PATCH] staging: vc04_services: Stop kthreads on .remove

[Stefan Wahren]

Hi Umang,

Am 28.03.24 um 06:16 schrieb Umang Jain:
> It turns out that stopping kthreads on vchiq_shutdown_internal()
> corrupts the vchiq firmware during probe.
>
> [   11.005324] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf unexpected msgid 0@b18db30a,0
>
> Since the kthreads are created during vchiq_probe(), symmetrically
> they should be stopped on vchiq_remove().
>
> Also, vchiq_shutdown_internal() is called by vchiq_shutdown() which
> is a exported function. Hence, modules can take indirectly shut the
> vchiq interface by stopping the kthreads.
>
> Fix it by stopping the kthreads in .remove instead.
>
> Fixes: d9c60badccc1 ("staging: vc04_services: vchiq_core: Stop kthreads on shutdown")
> Reported-by: Stefan Wahren <wahrenst@gmx.net>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>
> Reproduced on rpi-3-b 32-bit kernel with multi_v7_defconfig and all
> vchiq configs options. Patch seems to fix the error mentioned in the
> commit message.
i tested the patch on my Raspberry Pi 3B+ with multi_v7_defconfig and

CONFIG_BCM_VIDEOCORE=y
CONFIG_BCM2835_VCHIQ=m
CONFIG_VCHIQ_CDEV=y

Now X came up, but if i run

modprobe -r vchiq

but i'm still getting

[  146.730014] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
unexpected msgid 0@10b01b8d,0

so it seems to me stopping those kthreads isn't that trivial (which i
never expected). Maybe we need to care about the order or an even more
complex synchronization mechanism between VideoCore firmware and the
kthreads.

I won't have the time for further investigations during the eastern
holidays. Maybe we should revert d9c60badccc1 and take the necessary
time for proper testing.

Best regards

Re: [PATCH] staging: vc04_services: Stop kthreads on .remove

[Umang Jain]

On 28/03/24 10:19 pm, Stefan Wahren wrote:
> Hi Umang,
>
> Am 28.03.24 um 06:16 schrieb Umang Jain:
>> It turns out that stopping kthreads on vchiq_shutdown_internal()
>> corrupts the vchiq firmware during probe.
>>
>> [   11.005324] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf 
>> unexpected msgid 0@b18db30a,0
>>
>> Since the kthreads are created during vchiq_probe(), symmetrically
>> they should be stopped on vchiq_remove().
>>
>> Also, vchiq_shutdown_internal() is called by vchiq_shutdown() which
>> is a exported function. Hence, modules can take indirectly shut the
>> vchiq interface by stopping the kthreads.
>>
>> Fix it by stopping the kthreads in .remove instead.
>>
>> Fixes: d9c60badccc1 ("staging: vc04_services: vchiq_core: Stop 
>> kthreads on shutdown")
>> Reported-by: Stefan Wahren <wahrenst@gmx.net>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>
>> Reproduced on rpi-3-b 32-bit kernel with multi_v7_defconfig and all
>> vchiq configs options. Patch seems to fix the error mentioned in the
>> commit message.
> i tested the patch on my Raspberry Pi 3B+ with multi_v7_defconfig and
>
> CONFIG_BCM_VIDEOCORE=y
> CONFIG_BCM2835_VCHIQ=m
> CONFIG_VCHIQ_CDEV=y
>
> Now X came up, but if i run
>
> modprobe -r vchiq
>
> but i'm still getting
>
> [  146.730014] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
> unexpected msgid 0@10b01b8d,0
>
> so it seems to me stopping those kthreads isn't that trivial (which i
> never expected). Maybe we need to care about the order or an even more
> complex synchronization mechanism between VideoCore firmware and the
> kthreads.

Dave, Phil - would you happen to have any opinions on this behavior ?
>
> I won't have the time for further investigations during the eastern
> holidays. Maybe we should revert d9c60badccc1 and take the necessary
> time for proper testing.
>
> Best regards

Re: [PATCH] staging: vc04_services: Stop kthreads on .remove

[Dave Stevenson]

Hi Umang

On Thu, 28 Mar 2024 at 16:54, Umang Jain <umang.jain@ideasonboard.com> wrote:
>
>
>
> On 28/03/24 10:19 pm, Stefan Wahren wrote:
> > Hi Umang,
> >
> > Am 28.03.24 um 06:16 schrieb Umang Jain:
> >> It turns out that stopping kthreads on vchiq_shutdown_internal()
> >> corrupts the vchiq firmware during probe.
> >>
> >> [   11.005324] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
> >> unexpected msgid 0@b18db30a,0
> >>
> >> Since the kthreads are created during vchiq_probe(), symmetrically
> >> they should be stopped on vchiq_remove().
> >>
> >> Also, vchiq_shutdown_internal() is called by vchiq_shutdown() which
> >> is a exported function. Hence, modules can take indirectly shut the
> >> vchiq interface by stopping the kthreads.
> >>
> >> Fix it by stopping the kthreads in .remove instead.
> >>
> >> Fixes: d9c60badccc1 ("staging: vc04_services: vchiq_core: Stop
> >> kthreads on shutdown")
> >> Reported-by: Stefan Wahren <wahrenst@gmx.net>
> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >> ---
> >>
> >> Reproduced on rpi-3-b 32-bit kernel with multi_v7_defconfig and all
> >> vchiq configs options. Patch seems to fix the error mentioned in the
> >> commit message.
> > i tested the patch on my Raspberry Pi 3B+ with multi_v7_defconfig and
> >
> > CONFIG_BCM_VIDEOCORE=y
> > CONFIG_BCM2835_VCHIQ=m
> > CONFIG_VCHIQ_CDEV=y
> >
> > Now X came up, but if i run
> >
> > modprobe -r vchiq
> >
> > but i'm still getting
> >
> > [  146.730014] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
> > unexpected msgid 0@10b01b8d,0
> >
> > so it seems to me stopping those kthreads isn't that trivial (which i
> > never expected). Maybe we need to care about the order or an even more
> > complex synchronization mechanism between VideoCore firmware and the
> > kthreads.
>
> Dave, Phil - would you happen to have any opinions on this behavior ?

Sorry, that's not code I know at all.

  Dave

> >
> > I won't have the time for further investigations during the eastern
> > holidays. Maybe we should revert d9c60badccc1 and take the necessary
> > time for proper testing.
> >
> > Best regards
>

Re: [PATCH] staging: vc04_services: Stop kthreads on .remove

[Phil Elwell]

Hi Umang,

On Thu, 28 Mar 2024 at 16:54, Umang Jain <umang.jain@ideasonboard.com> wrote:
>
>
>
> On 28/03/24 10:19 pm, Stefan Wahren wrote:
> > Hi Umang,
> >
> > Am 28.03.24 um 06:16 schrieb Umang Jain:
> >> It turns out that stopping kthreads on vchiq_shutdown_internal()
> >> corrupts the vchiq firmware during probe.
> >>
> >> [   11.005324] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
> >> unexpected msgid 0@b18db30a,0
> >>
> >> Since the kthreads are created during vchiq_probe(), symmetrically
> >> they should be stopped on vchiq_remove().
> >>
> >> Also, vchiq_shutdown_internal() is called by vchiq_shutdown() which
> >> is a exported function. Hence, modules can take indirectly shut the
> >> vchiq interface by stopping the kthreads.
> >>
> >> Fix it by stopping the kthreads in .remove instead.
> >>
> >> Fixes: d9c60badccc1 ("staging: vc04_services: vchiq_core: Stop
> >> kthreads on shutdown")
> >> Reported-by: Stefan Wahren <wahrenst@gmx.net>
> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >> ---
> >>
> >> Reproduced on rpi-3-b 32-bit kernel with multi_v7_defconfig and all
> >> vchiq configs options. Patch seems to fix the error mentioned in the
> >> commit message.
> > i tested the patch on my Raspberry Pi 3B+ with multi_v7_defconfig and
> >
> > CONFIG_BCM_VIDEOCORE=y
> > CONFIG_BCM2835_VCHIQ=m
> > CONFIG_VCHIQ_CDEV=y
> >
> > Now X came up, but if i run
> >
> > modprobe -r vchiq
> >
> > but i'm still getting
> >
> > [  146.730014] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
> > unexpected msgid 0@10b01b8d,0
> >
> > so it seems to me stopping those kthreads isn't that trivial (which i
> > never expected). Maybe we need to care about the order or an even more
> > complex synchronization mechanism between VideoCore firmware and the
> > kthreads.
>
> Dave, Phil - would you happen to have any opinions on this behavior ?
> >
> > I won't have the time for further investigations during the eastern
> > holidays. Maybe we should revert d9c60badccc1 and take the necessary
> > time for proper testing.
> >

It's hard to be sure, but I would guess that stopping the thread is
causing the remote_event_wait in sync_func to terminate early. The
following code assumes that the message is valid, when it fact it will
have a message type of VCHIQ_MSG_PADDING.

If that's the case, you can either find some way to spot that the wait
has actually failed, or handle VCHIQ_MSG_PADDING explicitly in the
switch.

Phil

Re: [PATCH] staging: vc04_services: Stop kthreads on .remove

[Stefan Wahren]

Am 28.03.24 um 23:43 schrieb Phil Elwell:
> Hi Umang,
>
> On Thu, 28 Mar 2024 at 16:54, Umang Jain <umang.jain@ideasonboard.com> wrote:
>>
>>
>> On 28/03/24 10:19 pm, Stefan Wahren wrote:
>>> Hi Umang,
>>>
>>> Am 28.03.24 um 06:16 schrieb Umang Jain:
>>>> It turns out that stopping kthreads on vchiq_shutdown_internal()
>>>> corrupts the vchiq firmware during probe.
>>>>
>>>> [   11.005324] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
>>>> unexpected msgid 0@b18db30a,0
>>>>
>>>> Since the kthreads are created during vchiq_probe(), symmetrically
>>>> they should be stopped on vchiq_remove().
>>>>
>>>> Also, vchiq_shutdown_internal() is called by vchiq_shutdown() which
>>>> is a exported function. Hence, modules can take indirectly shut the
>>>> vchiq interface by stopping the kthreads.
>>>>
>>>> Fix it by stopping the kthreads in .remove instead.
>>>>
>>>> Fixes: d9c60badccc1 ("staging: vc04_services: vchiq_core: Stop
>>>> kthreads on shutdown")
>>>> Reported-by: Stefan Wahren <wahrenst@gmx.net>
>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>>> ---
>>>>
>>>> Reproduced on rpi-3-b 32-bit kernel with multi_v7_defconfig and all
>>>> vchiq configs options. Patch seems to fix the error mentioned in the
>>>> commit message.
>>> i tested the patch on my Raspberry Pi 3B+ with multi_v7_defconfig and
>>>
>>> CONFIG_BCM_VIDEOCORE=y
>>> CONFIG_BCM2835_VCHIQ=m
>>> CONFIG_VCHIQ_CDEV=y
>>>
>>> Now X came up, but if i run
>>>
>>> modprobe -r vchiq
>>>
>>> but i'm still getting
>>>
>>> [  146.730014] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
>>> unexpected msgid 0@10b01b8d,0
>>>
>>> so it seems to me stopping those kthreads isn't that trivial (which i
>>> never expected). Maybe we need to care about the order or an even more
>>> complex synchronization mechanism between VideoCore firmware and the
>>> kthreads.
>> Dave, Phil - would you happen to have any opinions on this behavior ?
>>> I won't have the time for further investigations during the eastern
>>> holidays. Maybe we should revert d9c60badccc1 and take the necessary
>>> time for proper testing.
>>>
> It's hard to be sure, but I would guess that stopping the thread is
> causing the remote_event_wait in sync_func to terminate early. The
> following code assumes that the message is valid, when it fact it will
> have a message type of VCHIQ_MSG_PADDING.
>
> If that's the case, you can either find some way to spot that the wait
> has actually failed, or handle VCHIQ_MSG_PADDING explicitly in the
> switch.
Thanks. It seems the return value of remote_event_wait() is ignored here
and should be used.
>
> Phil