[PATCH v2 0/2] staging: vchiq_core: Stop kthreads on module unload

[PATCH v2 0/2] staging: vchiq_core: Stop kthreads on module unload

[Umang Jain]

This is a re-attempt of [1] where we noticed corruption of vc04 firmware
on stopping the kthread.

After investigation, I found that the case where
wait_event_interruptible() can return early(wait failed) with
-ERESTARTSYS, is something not handling in remote_event_wait(). Once we
bubble up the that return/err code and handle it - the issue is resolved
correctly and kthreads are stopped as expected.

Patch 1/2 handles the returning of the return value from
wait_event_interruptible()

Patch 2/2 handles stopping of kthreads.

---
Testing with
+CONFIG_BCM_VIDEOCORE=y
+CONFIG_BCM2835_VCHIQ=m
+CONFIG_VCHIQ_CDEV=y
+CONFIG_BCM2835_VCHIQ_MMAL=m
+CONFIG_VIDEO_BCM2835=m

and as per
drivers/staging/vc04_services/interface/TESTING

Entire test sequence:
https://paste.debian.net/plain/1322185
---

[1]: https://lore.kernel.org/linux-staging/20240403052100.2794-1-umang.jain@ideasonboard.com/ 

Changes in v2:
- Return 0 as success value for remote_wait_event() in 1/2
- Fix stopping of 'keep-alive' kthread in 2/2. 

Umang Jain (2):
  staging: vchiq_core: Bubble up wait_event_interruptible() return value
  staging: vc04_services: vchiq_core: Stop kthreads on vchiq module
    unload

 drivers/staging/vc04_services/interface/TODO  |  7 ----
 .../interface/vchiq_arm/vchiq_arm.c           | 10 ++++-
 .../interface/vchiq_arm/vchiq_core.c          | 37 ++++++++++++++-----
 3 files changed, 36 insertions(+), 18 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value

[Umang Jain]

wait_event_interruptible() returns if the condition evaluates to true
it receives a signal. However, the current code always assume that the
wait_event_interruptible() returns only when the event is fired.
This should not be the case as wait_event_interruptible() can
return on receiving a signal (with -ERESTARTSYS as return value).

We should consider this and bubble up the return value of
wait_event_interruptible() to exactly know if the wait has failed
and error out. This will also help to properly stop kthreads in the
subsequent patch.

Meanwhile at it, remote_wait_event() is modified to return 0 on success,
and an error code (from wait_event_interruptible()) on failure. The
return value is now checked for remote_wait_event() calls.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_core.c          | 31 ++++++++++++++-----
 1 file changed, 24 insertions(+), 7 deletions(-)

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 4f65e4021c4d..dd70f2881eca 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -501,16 +501,21 @@ remote_event_create(wait_queue_head_t *wq, struct remote_event *event)
  * routines where switched to the "interruptible" family of functions, as the
  * former was deemed unjustified and the use "killable" set all VCHIQ's
  * threads in D state.
+ *
+ * Returns: 0 on success, a negative error code on failure
  */
 static inline int
 remote_event_wait(wait_queue_head_t *wq, struct remote_event *event)
 {
+	int ret = 0;
+
 	if (!event->fired) {
 		event->armed = 1;
 		dsb(sy);
-		if (wait_event_interruptible(*wq, event->fired)) {
+		ret = wait_event_interruptible(*wq, event->fired);
+		if (ret) {
 			event->armed = 0;
-			return 0;
+			return ret;
 		}
 		event->armed = 0;
 		/* Ensure that the peer sees that we are not waiting (armed == 0). */
@@ -518,7 +523,7 @@ remote_event_wait(wait_queue_head_t *wq, struct remote_event *event)
 	}
 
 	event->fired = 0;
-	return 1;
+	return ret;
 }
 
 /*
@@ -1140,6 +1145,7 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 	struct vchiq_header *header;
 	ssize_t callback_result;
 	int svc_fourcc;
+	int ret;
 
 	local = state->local;
 
@@ -1147,7 +1153,9 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 	    mutex_lock_killable(&state->sync_mutex))
 		return -EAGAIN;
 
-	remote_event_wait(&state->sync_release_event, &local->sync_release);
+	ret = remote_event_wait(&state->sync_release_event, &local->sync_release);
+	if (ret)
+		return ret;
 
 	/* Ensure that reads don't overtake the remote_event_wait. */
 	rmb();
@@ -1929,13 +1937,16 @@ slot_handler_func(void *v)
 {
 	struct vchiq_state *state = v;
 	struct vchiq_shared_state *local = state->local;
+	int ret;
 
 	DEBUG_INITIALISE(local);
 
 	while (1) {
 		DEBUG_COUNT(SLOT_HANDLER_COUNT);
 		DEBUG_TRACE(SLOT_HANDLER_LINE);
-		remote_event_wait(&state->trigger_event, &local->trigger);
+		ret = remote_event_wait(&state->trigger_event, &local->trigger);
+		if (ret)
+			return ret;
 
 		/* Ensure that reads don't overtake the remote_event_wait. */
 		rmb();
@@ -1966,6 +1977,7 @@ recycle_func(void *v)
 	struct vchiq_shared_state *local = state->local;
 	u32 *found;
 	size_t length;
+	int ret;
 
 	length = sizeof(*found) * BITSET_SIZE(VCHIQ_MAX_SERVICES);
 
@@ -1975,7 +1987,9 @@ recycle_func(void *v)
 		return -ENOMEM;
 
 	while (1) {
-		remote_event_wait(&state->recycle_event, &local->recycle);
+		ret = remote_event_wait(&state->recycle_event, &local->recycle);
+		if (ret)
+			return ret;
 
 		process_free_queue(state, found, length);
 	}
@@ -1992,6 +2006,7 @@ sync_func(void *v)
 		(struct vchiq_header *)SLOT_DATA_FROM_INDEX(state,
 			state->remote->slot_sync);
 	int svc_fourcc;
+	int ret;
 
 	while (1) {
 		struct vchiq_service *service;
@@ -1999,7 +2014,9 @@ sync_func(void *v)
 		int type;
 		unsigned int localport, remoteport;
 
-		remote_event_wait(&state->sync_trigger_event, &local->sync_trigger);
+		ret = remote_event_wait(&state->sync_trigger_event, &local->sync_trigger);
+		if (ret)
+			return ret;
 
 		/* Ensure that reads don't overtake the remote_event_wait. */
 		rmb();
-- 
2.43.0

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

[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@ideasonboard.com>
---
 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);
+
+	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;
-- 
2.43.0

Re: [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value

[Stefan Wahren]

Hi Umang,

Am 03.07.24 um 15:10 schrieb Umang Jain:
> wait_event_interruptible() returns if the condition evaluates to true
> it receives a signal. However, the current code always assume that the
> wait_event_interruptible() returns only when the event is fired.
> This should not be the case as wait_event_interruptible() can
> return on receiving a signal (with -ERESTARTSYS as return value).
>
> We should consider this and bubble up the return value of
> wait_event_interruptible() to exactly know if the wait has failed
> and error out. This will also help to properly stop kthreads in the
> subsequent patch.
>
> Meanwhile at it, remote_wait_event() is modified to return 0 on success,
> and an error code (from wait_event_interruptible()) on failure. The
> return value is now checked for remote_wait_event() calls.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../interface/vchiq_arm/vchiq_core.c          | 31 ++++++++++++++-----
>   1 file changed, 24 insertions(+), 7 deletions(-)
>
> 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 4f65e4021c4d..dd70f2881eca 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -501,16 +501,21 @@ remote_event_create(wait_queue_head_t *wq, struct remote_event *event)
>    * routines where switched to the "interruptible" family of functions, as the
>    * former was deemed unjustified and the use "killable" set all VCHIQ's
>    * threads in D state.
> + *
> + * Returns: 0 on success, a negative error code on failure
>    */
>   static inline int
>   remote_event_wait(wait_queue_head_t *wq, struct remote_event *event)
>   {
> +	int ret = 0;
> +
>   	if (!event->fired) {
>   		event->armed = 1;
>   		dsb(sy);
> -		if (wait_event_interruptible(*wq, event->fired)) {
> +		ret = wait_event_interruptible(*wq, event->fired);
> +		if (ret) {
>   			event->armed = 0;
> -			return 0;
> +			return ret;
>   		}
>   		event->armed = 0;
>   		/* Ensure that the peer sees that we are not waiting (armed == 0). */
> @@ -518,7 +523,7 @@ remote_event_wait(wait_queue_head_t *wq, struct remote_event *event)
>   	}
>
>   	event->fired = 0;
> -	return 1;
> +	return ret;
in general this patch looks good to me. But maybe we better return 0
directly and reduce the scope of ret.
>   }
>
>   /*
> @@ -1140,6 +1145,7 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
>   	struct vchiq_header *header;
>   	ssize_t callback_result;
>   	int svc_fourcc;
> +	int ret;
>
>   	local = state->local;
>
> @@ -1147,7 +1153,9 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
>   	    mutex_lock_killable(&state->sync_mutex))
>   		return -EAGAIN;
>
> -	remote_event_wait(&state->sync_release_event, &local->sync_release);
> +	ret = remote_event_wait(&state->sync_release_event, &local->sync_release);
> +	if (ret)
> +		return ret;
>
>   	/* Ensure that reads don't overtake the remote_event_wait. */
>   	rmb();
> @@ -1929,13 +1937,16 @@ slot_handler_func(void *v)
>   {
>   	struct vchiq_state *state = v;
>   	struct vchiq_shared_state *local = state->local;
> +	int ret;
>
>   	DEBUG_INITIALISE(local);
>
>   	while (1) {
>   		DEBUG_COUNT(SLOT_HANDLER_COUNT);
>   		DEBUG_TRACE(SLOT_HANDLER_LINE);
> -		remote_event_wait(&state->trigger_event, &local->trigger);
> +		ret = remote_event_wait(&state->trigger_event, &local->trigger);
> +		if (ret)
> +			return ret;
>
>   		/* Ensure that reads don't overtake the remote_event_wait. */
>   		rmb();
> @@ -1966,6 +1977,7 @@ recycle_func(void *v)
>   	struct vchiq_shared_state *local = state->local;
>   	u32 *found;
>   	size_t length;
> +	int ret;
>
>   	length = sizeof(*found) * BITSET_SIZE(VCHIQ_MAX_SERVICES);
>
> @@ -1975,7 +1987,9 @@ recycle_func(void *v)
>   		return -ENOMEM;
>
>   	while (1) {
> -		remote_event_wait(&state->recycle_event, &local->recycle);
> +		ret = remote_event_wait(&state->recycle_event, &local->recycle);
> +		if (ret)
> +			return ret;
>
>   		process_free_queue(state, found, length);
>   	}
> @@ -1992,6 +2006,7 @@ sync_func(void *v)
>   		(struct vchiq_header *)SLOT_DATA_FROM_INDEX(state,
>   			state->remote->slot_sync);
>   	int svc_fourcc;
> +	int ret;
>
>   	while (1) {
>   		struct vchiq_service *service;
> @@ -1999,7 +2014,9 @@ sync_func(void *v)
>   		int type;
>   		unsigned int localport, remoteport;
>
> -		remote_event_wait(&state->sync_trigger_event, &local->sync_trigger);
> +		ret = remote_event_wait(&state->sync_trigger_event, &local->sync_trigger);
> +		if (ret)
> +			return ret;
>
>   		/* Ensure that reads don't overtake the remote_event_wait. */
>   		rmb();

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

[Stefan Wahren]

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@ideasonboard.com>
> ---
>   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;

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

[Umang Jain]

Hi Stefan

On 03/07/24 10:01 pm, Stefan Wahren wrote:
> 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@ideasonboard.com>
>> ---
>>   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.

I think what you are asking here is a entire de-init() sequence on 
probe() failure. It can be done (I haven't taken a look in deep yet) but 
it can also be done on top. cleaning up kthreads would be a part of it - 
but I suspect there might be other parts for that sequence

my preference here is to fix the issue we saw earlier on module unload

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

Re: [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value

[Dan Carpenter]

On Wed, Jul 03, 2024 at 06:18:19PM +0200, Stefan Wahren wrote:
> > + *
> > + * Returns: 0 on success, a negative error code on failure

Probably not even worth adding this comment.  It's assumed.

> >    */
> >   static inline int
> >   remote_event_wait(wait_queue_head_t *wq, struct remote_event *event)
> >   {
> > +	int ret = 0;
> > +
> >   	if (!event->fired) {
> >   		event->armed = 1;
> >   		dsb(sy);
> > -		if (wait_event_interruptible(*wq, event->fired)) {
> > +		ret = wait_event_interruptible(*wq, event->fired);
> > +		if (ret) {
> >   			event->armed = 0;
> > -			return 0;
> > +			return ret;
> >   		}
> >   		event->armed = 0;
> >   		/* Ensure that the peer sees that we are not waiting (armed == 0). */
> > @@ -518,7 +523,7 @@ remote_event_wait(wait_queue_head_t *wq, struct remote_event *event)
> >   	}
> > 
> >   	event->fired = 0;
> > -	return 1;
> > +	return ret;
> in general this patch looks good to me. But maybe we better return 0
> directly and reduce the scope of ret.

Heh.  If I could have found some more things to complain about then I
was going to ask that this be changed to "return 0;" as well.  But I
always feel like "ret" should be function scope.  Otherwise you can get
multiple ret declarations in a function and it leads to not setting the
correct error code.

regards,
dan carpenter

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

[Stefan Wahren]

Am 03.07.24 um 15:10 schrieb Umang Jain:
> This is a re-attempt of [1] where we noticed corruption of vc04 firmware
> on stopping the kthread.
>
> After investigation, I found that the case where
> wait_event_interruptible() can return early(wait failed) with
> -ERESTARTSYS, is something not handling in remote_event_wait(). Once we
> bubble up the that return/err code and handle it - the issue is resolved
> correctly and kthreads are stopped as expected.
>
> Patch 1/2 handles the returning of the return value from
> wait_event_interruptible()
>
> Patch 2/2 handles stopping of kthreads.
>
The whole series is:

Tested-by: Stefan Wahren <wahrenst@gmx.net>