Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown

[Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>]

Hi Stefan

On 27/03/24 11:49 pm, Stefan Wahren wrote:
> Hi Umang,
>
> Am 21.03.24 um 14:07 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 on vchiq_shutdown().
> >
> > This also address the following TODO item:
> >   * Fix kernel module support
> >
> > hence drop it from the TODO item list.
> >
> > Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/staging/vc04_services/interface/TODO           |  7 -------
> >   .../vc04_services/interface/vchiq_arm/vchiq_arm.c      |  8 ++++++--
> >   .../vc04_services/interface/vchiq_arm/vchiq_core.c     | 10 +++++++---
> >   3 files changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/interface/TODO 
> > b/drivers/staging/vc04_services/interface/TODO
> > index 05eb5140d096..57a2d6761f9b 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 ad506016fc93..1d21f9cfbfe5 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -726,8 +726,9 @@ void free_bulk_waiter(struct vchiq_instance 
> > *instance)
> >
> >   int vchiq_shutdown(struct vchiq_instance *instance)
> >   {
> > -    int status = 0;
> >       struct vchiq_state *state = instance->state;
> > +    struct vchiq_arm_state *arm_state;
> > +    int status = 0;
> >
> >       if (mutex_lock_killable(&state->mutex))
> >           return -EAGAIN;
> > @@ -739,6 +740,9 @@ int vchiq_shutdown(struct vchiq_instance *instance)
> >
> >       dev_dbg(state->dev, "core: (%p): returning %d\n", instance, 
> > status);
> >
> > +    arm_state = vchiq_platform_get_arm_state(state);
> > +    kthread_stop(arm_state->ka_thread);
> > +
> >       free_bulk_waiter(instance);
> >       kfree(instance);
> >
> > @@ -1310,7 +1314,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)) {
> > 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 76c27778154a..953ccd87f425 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > @@ -1936,7 +1936,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);
> >           remote_event_wait(&state->trigger_event, &local->trigger);
> > @@ -1978,7 +1978,7 @@ recycle_func(void *v)
> >       if (!found)
> >           return -ENOMEM;
> >
> > -    while (1) {
> > +    while (!kthread_should_stop()) {
> >           remote_event_wait(&state->recycle_event, &local->recycle);
> >
> >           process_free_queue(state, found, length);
> > @@ -1997,7 +1997,7 @@ sync_func(void *v)
> >               state->remote->slot_sync);
> >       int svc_fourcc;
> >
> > -    while (1) {
> > +    while (!kthread_should_stop()) {
> >           struct vchiq_service *service;
> >           int msgid, size;
> >           int type;
> > @@ -2844,6 +2844,10 @@ 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);
>
> i had a little bit time to look at the corrupt issue, i think this
> caused by this change. This function is called by vchiq_shutdown which
> is exported and used by VCHIQ user drivers like bcm2835-audio or
> mmal-vchiq. So this is absolutely not the right place to stop the kernel
> threads and the issue is not specific to the Raspberry Pi 3.

Can you try building with ARCH=64-bit and testing it please? Please 
refer to the cover section where I have tested 64-bit on rpi4.

Regarding 32-bit build for my Pi4 - everything gets broken for me RPi4 
(if I try to test via SD-card) ... and I am also having problems with 
setting up a NFS-mounted 32-bit rootfs today so can't boot the pi 
through NFS with 32-bit kernel :(

My Pi3 power cable/microUSB went missing, so I have ordered a new one. 
It will take a couple of days to arrive before  I can test it there.


> >   }
> >
> >   int