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

[Umang Jain <umang.jain@ideasonboard.com>]

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