From: Stefan Wahren <wahrenst@gmx.net>
To: Umang Jain <umang.jain@ideasonboard.com>, linux-staging@lists.linux.dev
Cc: Dan Carpenter <error27@gmail.com>,
Kieran Bingham <kieran.bingham@ideasonboard.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Dave Stevenson <dave.stevenson@raspberrypi.com>,
Phil Elwell <phil@raspberrypi.com>, Greg KH <greg@kroah.com>
Subject: Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
Date: Wed, 27 Mar 2024 21:40:38 +0100 [thread overview]
Message-ID: <7fc1488b-a0d8-468e-a13d-92ee6e23d657@gmx.net> (raw)
In-Reply-To: <f4b729c7-744e-4c26-bdc0-23880a3bc4f5@ideasonboard.com>
Hi Umang,
Am 27.03.24 um 21:00 schrieb Umang Jain:
> 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@ideasonboard.com>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> ---
>>> 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.
sorry, i don't understand the point behind this. As i mention it's clear
that we cannot call kthread_stop() from vchiq_shutdown_internal because
this function is also used by different drivers. The calls to create the
kthreads should be symmetric to the stop calls. From my understanding
the kthreads are created from vchiq_probe(), so the kthreads should
stopped directly or indirectly from vchiq_remove().
This has nothing to do with Raspberry Pi 3 vs 4 or 32 bit vs 64 bit. It
was just luck that it worked in your case.
>
> 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
>
next prev parent reply other threads:[~2024-03-27 20:40 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-21 13:07 [PATCH v2 0/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
2024-03-21 13:07 ` [PATCH v2 1/5] staging: vc04_services: Remove unused function declarations Umang Jain
2024-03-21 13:07 ` [PATCH v2 2/5] staging: vc04_services: vchiq_arm: Use appropriate dev_* log helpers Umang Jain
2024-03-21 13:07 ` [PATCH v2 3/5] staging: vc04_services: Do not log error on kzalloc() Umang Jain
2024-03-21 17:41 ` Kieran Bingham
2024-03-21 13:07 ` [PATCH v2 4/5] staging: vc04_services: Implement vchiq_bus .remove Umang Jain
2024-03-21 17:18 ` Kieran Bingham
2024-03-22 4:21 ` Umang Jain
2024-03-21 13:07 ` [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
2024-03-26 21:22 ` Stefan Wahren
2024-03-27 2:52 ` Umang Jain
2024-03-27 18:19 ` Stefan Wahren
2024-03-27 20:00 ` Umang Jain
2024-03-27 20:40 ` Stefan Wahren [this message]
2024-03-28 5:18 ` Umang Jain
2024-03-28 10:45 ` Dave Stevenson
2024-03-28 14:53 ` Mark Brown
2024-03-28 15:29 ` Stefan Wahren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7fc1488b-a0d8-468e-a13d-92ee6e23d657@gmx.net \
--to=wahrenst@gmx.net \
--cc=dave.stevenson@raspberrypi.com \
--cc=error27@gmail.com \
--cc=greg@kroah.com \
--cc=kieran.bingham@ideasonboard.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-staging@lists.linux.dev \
--cc=phil@raspberrypi.com \
--cc=umang.jain@ideasonboard.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).