From: Kieran Bingham <kieran.bingham@ideasonboard.com>
To: Umang Jain <umang.jain@ideasonboard.com>, linux-staging@lists.linux.dev
Cc: Stefan Wahren <stefan.wahren@i2se.com>,
Dan Carpenter <error27@gmail.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Dave Stevenson <dave.stevenson@raspberrypi.com>,
Phil Elwell <phil@raspberrypi.com>, Greg KH <greg@kroah.com>,
Umang Jain <umang.jain@ideasonboard.com>,
Stefan Wahren <wahrenst@gmx.net>
Subject: Re: [PATCH] staging: vc04_services: Stop kthreads on .remove
Date: Thu, 28 Mar 2024 08:37:50 +0000 [thread overview]
Message-ID: <171161507013.3072637.12125782507523919379@ping.linuxembedded.co.uk> (raw)
In-Reply-To: <20240328051615.618908-1-umang.jain@ideasonboard.com>
Quoting Umang Jain (2024-03-28 05:16:15)
> It turns out that stopping kthreads on vchiq_shutdown_internal()
> corrupts the vchiq firmware during probe.
>
> [ 11.005324] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf unexpected msgid 0@b18db30a,0
>
> Since the kthreads are created during vchiq_probe(), symmetrically
> they should be stopped on vchiq_remove().
>
> Also, vchiq_shutdown_internal() is called by vchiq_shutdown() which
> is a exported function. Hence, modules can take indirectly shut the
> vchiq interface by stopping the kthreads.
>
> Fix it by stopping the kthreads in .remove instead.
>
> Fixes: d9c60badccc1 ("staging: vc04_services: vchiq_core: Stop kthreads on shutdown")
> Reported-by: Stefan Wahren <wahrenst@gmx.net>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>
> Reproduced on rpi-3-b 32-bit kernel with multi_v7_defconfig and all
> vchiq configs options. Patch seems to fix the error mentioned in the
> commit message.
Yikes, I wish I'd spotted that asymmetry in my review of the previous
patch, but certainly keeping this symmetrical here sounds correct and
looks right to me. And if it fixes the bug ...
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> ---
> .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 6 ++++++
> .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 4 ----
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> 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 1d21f9cfbfe5..9af77d17a1e8 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1771,10 +1771,16 @@ static int vchiq_probe(struct platform_device *pdev)
>
> static void vchiq_remove(struct platform_device *pdev)
> {
> + struct vchiq_state *state = vchiq_get_state();
> +
> vchiq_device_unregister(bcm2835_audio);
> vchiq_device_unregister(bcm2835_camera);
> vchiq_debugfs_deinit();
> vchiq_deregister_chrdev();
> +
> + kthread_stop(state->sync_thread);
> + kthread_stop(state->recycle_thread);
> + kthread_stop(state->slot_handler_thread);
> }
>
> static struct platform_driver vchiq_driver = {
> 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 953ccd87f425..658d19f1e7e8 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -2844,10 +2844,6 @@ 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);
> }
>
> int
> --
> 2.43.0
>
next prev parent reply other threads:[~2024-03-28 8:37 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-28 5:16 [PATCH] staging: vc04_services: Stop kthreads on .remove Umang Jain
2024-03-28 8:37 ` Kieran Bingham [this message]
2024-03-28 16:49 ` Stefan Wahren
2024-03-28 16:53 ` Umang Jain
2024-03-28 19:06 ` Dave Stevenson
2024-03-28 22:43 ` Phil Elwell
2024-03-29 9:53 ` 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=171161507013.3072637.12125782507523919379@ping.linuxembedded.co.uk \
--to=kieran.bingham@ideasonboard.com \
--cc=dave.stevenson@raspberrypi.com \
--cc=error27@gmail.com \
--cc=greg@kroah.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-staging@lists.linux.dev \
--cc=phil@raspberrypi.com \
--cc=stefan.wahren@i2se.com \
--cc=umang.jain@ideasonboard.com \
--cc=wahrenst@gmx.net \
/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).