Re: [PATCH v2 2/2] staging: vc04_services: vchiq_core: Stop kthreads on vchiq module unload

[Stefan Wahren <wahrenst@xxxxxxx>]

Hi Umang,

Am 03.07.24 um 15:10 schrieb Umang Jain:
> The various kthreads thread functions (slot_handler_func, sync_func,
> recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
> vchiq_arm should be stopped when the module is unloaded.
>
> Previous attempt were made to address this but later reverted [1]
> due to VC04 firmware corruption. The issue around
> wait_event_interruptible() handling on stopping a kthread has been
> handled in the previous commit. Hence, it is now safe to stop kthreads
> on module unload, without any firmware corruption.
>
> This also completes the "Fix kernel module support" TODO item, hence
> drop it from the list.
>
> [1] commit ebee9ca2f59e ("Revert "staging: vc04_services: vchiq_core: Stop kthreads on shutdown"")
>
> Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> ---
>   drivers/staging/vc04_services/interface/TODO           |  7 -------
>   .../vc04_services/interface/vchiq_arm/vchiq_arm.c      | 10 +++++++++-
>   .../vc04_services/interface/vchiq_arm/vchiq_core.c     |  6 +++---
>   3 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
> index 05f129c0c254..dfb1ee49633f 100644
> --- a/drivers/staging/vc04_services/interface/TODO
> +++ b/drivers/staging/vc04_services/interface/TODO
> @@ -16,13 +16,6 @@ some of the ones we want:
>     to manage these buffers as dmabufs so that we can zero-copy import
>     camera images into vc4 for rendering/display.
>
> -* Fix kernel module support
> -
> -Even the VPU firmware doesn't support a VCHI re-connect, the driver
> -should properly handle a module unload. This also includes that all
> -resources must be freed (kthreads, debugfs entries, ...) and global
> -variables avoided.
> -
>   * Documentation
>
>   A short top-down description of this driver's architecture (function of
> 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 9e6102c43e00..c4d97dbf6ba8 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1315,7 +1315,7 @@ vchiq_keepalive_thread_func(void *v)
>   		goto shutdown;
>   	}
>
> -	while (1) {
> +	while (!kthread_should_stop()) {
>   		long rc = 0, uc = 0;
>
>   		if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
> @@ -1776,12 +1776,20 @@ static int vchiq_probe(struct platform_device *pdev)
>   static void vchiq_remove(struct platform_device *pdev)
>   {
>   	struct vchiq_drv_mgmt *mgmt = dev_get_drvdata(&pdev->dev);
> +	struct vchiq_arm_state *arm_state;
>
>   	vchiq_device_unregister(bcm2835_audio);
>   	vchiq_device_unregister(bcm2835_camera);
>   	vchiq_debugfs_deinit();
>   	vchiq_deregister_chrdev();
>
> +	kthread_stop(mgmt->state.sync_thread);
> +	kthread_stop(mgmt->state.recycle_thread);
> +	kthread_stop(mgmt->state.slot_handler_thread);
This is fine as long as vchiq_probe() is successful. But we also need to
cleanup the kthreads during vchiq_probe() in case something went wrong
after vchiq_init_state(). I think we should introduce a helper to
cleanup them up.

Finally the subject needs to be adjusted too.
> +
> +	arm_state = vchiq_platform_get_arm_state(&mgmt->state);
> +	kthread_stop(arm_state->ka_thread);
> +
>   	kfree(mgmt);
>   }
>
> 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 dd70f2881eca..50af04b217f4 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -1941,7 +1941,7 @@ slot_handler_func(void *v)
>
>   	DEBUG_INITIALISE(local);
>
> -	while (1) {
> +	while (!kthread_should_stop()) {
>   		DEBUG_COUNT(SLOT_HANDLER_COUNT);
>   		DEBUG_TRACE(SLOT_HANDLER_LINE);
>   		ret = remote_event_wait(&state->trigger_event, &local->trigger);
> @@ -1986,7 +1986,7 @@ recycle_func(void *v)
>   	if (!found)
>   		return -ENOMEM;
>
> -	while (1) {
> +	while (!kthread_should_stop()) {
>   		ret = remote_event_wait(&state->recycle_event, &local->recycle);
>   		if (ret)
>   			return ret;
> @@ -2008,7 +2008,7 @@ sync_func(void *v)
>   	int svc_fourcc;
>   	int ret;
>
> -	while (1) {
> +	while (!kthread_should_stop()) {
>   		struct vchiq_service *service;
>   		int msgid, size;
>   		int type;