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

[Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>]

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@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.

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;