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@xxxxxxx>
> Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxxxx>
>
> ---
> .../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
>
