* [PATCH v2 0/2] staging: vchiq_core: Stop kthreads on module unload
@ 2024-07-03 13:10 Umang Jain
2024-07-03 13:10 ` [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value Umang Jain
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Umang Jain @ 2024-07-03 13:10 UTC (permalink / raw)
To: linux-staging
Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson,
Phil Elwell, Greg Kroah-Hartman, Stefan Wahren, 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
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value 2024-07-03 13:10 [PATCH v2 0/2] staging: vchiq_core: Stop kthreads on module unload Umang Jain @ 2024-07-03 13:10 ` Umang Jain 2024-07-03 16:18 ` Stefan Wahren 2024-07-03 13:10 ` [PATCH v2 2/2] staging: vc04_services: vchiq_core: Stop kthreads on vchiq module unload Umang Jain 2024-07-06 8:50 ` [PATCH v2 0/2] staging: vchiq_core: Stop kthreads on " Stefan Wahren 2 siblings, 1 reply; 8+ messages in thread From: Umang Jain @ 2024-07-03 13:10 UTC (permalink / raw) To: linux-staging Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson, Phil Elwell, Greg Kroah-Hartman, Stefan Wahren, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value 2024-07-03 13:10 ` [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value Umang Jain @ 2024-07-03 16:18 ` Stefan Wahren 2024-07-03 16:54 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Stefan Wahren @ 2024-07-03 16:18 UTC (permalink / raw) To: Umang Jain, linux-staging Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson, Phil Elwell, Greg Kroah-Hartman 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(); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value 2024-07-03 16:18 ` Stefan Wahren @ 2024-07-03 16:54 ` Dan Carpenter 0 siblings, 0 replies; 8+ messages in thread From: Dan Carpenter @ 2024-07-03 16:54 UTC (permalink / raw) To: Stefan Wahren Cc: Umang Jain, linux-staging, Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson, Phil Elwell, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] staging: vc04_services: vchiq_core: Stop kthreads on vchiq module unload 2024-07-03 13:10 [PATCH v2 0/2] staging: vchiq_core: Stop kthreads on module unload Umang Jain 2024-07-03 13:10 ` [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value Umang Jain @ 2024-07-03 13:10 ` Umang Jain 2024-07-03 16:31 ` Stefan Wahren 2024-07-06 8:50 ` [PATCH v2 0/2] staging: vchiq_core: Stop kthreads on " Stefan Wahren 2 siblings, 1 reply; 8+ messages in thread From: Umang Jain @ 2024-07-03 13:10 UTC (permalink / raw) To: linux-staging Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson, Phil Elwell, Greg Kroah-Hartman, Stefan Wahren, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] staging: vc04_services: vchiq_core: Stop kthreads on vchiq module unload 2024-07-03 13:10 ` [PATCH v2 2/2] staging: vc04_services: vchiq_core: Stop kthreads on vchiq module unload Umang Jain @ 2024-07-03 16:31 ` Stefan Wahren 2024-07-03 16:44 ` Umang Jain 0 siblings, 1 reply; 8+ messages in thread From: Stefan Wahren @ 2024-07-03 16:31 UTC (permalink / raw) To: Umang Jain, linux-staging Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson, Phil Elwell, Greg Kroah-Hartman 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; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] staging: vc04_services: vchiq_core: Stop kthreads on vchiq module unload 2024-07-03 16:31 ` Stefan Wahren @ 2024-07-03 16:44 ` Umang Jain 0 siblings, 0 replies; 8+ messages in thread From: Umang Jain @ 2024-07-03 16:44 UTC (permalink / raw) To: Stefan Wahren, linux-staging Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson, Phil Elwell, Greg Kroah-Hartman 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; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/2] staging: vchiq_core: Stop kthreads on module unload 2024-07-03 13:10 [PATCH v2 0/2] staging: vchiq_core: Stop kthreads on module unload Umang Jain 2024-07-03 13:10 ` [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value Umang Jain 2024-07-03 13:10 ` [PATCH v2 2/2] staging: vc04_services: vchiq_core: Stop kthreads on vchiq module unload Umang Jain @ 2024-07-06 8:50 ` Stefan Wahren 2 siblings, 0 replies; 8+ messages in thread From: Stefan Wahren @ 2024-07-06 8:50 UTC (permalink / raw) To: Umang Jain, linux-staging Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson, Phil Elwell, Greg Kroah-Hartman 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-07-06 8:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-07-03 13:10 [PATCH v2 0/2] staging: vchiq_core: Stop kthreads on module unload Umang Jain 2024-07-03 13:10 ` [PATCH v2 1/2] staging: vchiq_core: Bubble up wait_event_interruptible() return value Umang Jain 2024-07-03 16:18 ` Stefan Wahren 2024-07-03 16:54 ` Dan Carpenter 2024-07-03 13:10 ` [PATCH v2 2/2] staging: vc04_services: vchiq_core: Stop kthreads on vchiq module unload Umang Jain 2024-07-03 16:31 ` Stefan Wahren 2024-07-03 16:44 ` Umang Jain 2024-07-06 8:50 ` [PATCH v2 0/2] staging: vchiq_core: Stop kthreads on " Stefan Wahren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox