From: Stefan Wahren <wahrenst@gmx.net>
To: Phil Elwell <phil@raspberrypi.com>,
Umang Jain <umang.jain@ideasonboard.com>
Cc: linux-staging@lists.linux.dev,
Stefan Wahren <stefan.wahren@i2se.com>,
Dan Carpenter <error27@gmail.com>,
Kieran Bingham <kieran.bingham@ideasonboard.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Dave Stevenson <dave.stevenson@raspberrypi.com>,
Greg KH <greg@kroah.com>, Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH] staging: vc04_services: Stop kthreads on .remove
Date: Fri, 29 Mar 2024 10:53:19 +0100 [thread overview]
Message-ID: <b03c9b02-b2c7-4a09-961a-9708456ad181@gmx.net> (raw)
In-Reply-To: <CAMEGJJ2V1G+JNGeR-=eL32WZVbcME0X_KGgGs8bxJp6+aY4GSA@mail.gmail.com>
Am 28.03.24 um 23:43 schrieb Phil Elwell:
> Hi Umang,
>
> On Thu, 28 Mar 2024 at 16:54, Umang Jain <umang.jain@ideasonboard.com> wrote:
>>
>>
>> On 28/03/24 10:19 pm, Stefan Wahren wrote:
>>> Hi Umang,
>>>
>>> Am 28.03.24 um 06:16 schrieb Umang Jain:
>>>> 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.
>>> i tested the patch on my Raspberry Pi 3B+ with multi_v7_defconfig and
>>>
>>> CONFIG_BCM_VIDEOCORE=y
>>> CONFIG_BCM2835_VCHIQ=m
>>> CONFIG_VCHIQ_CDEV=y
>>>
>>> Now X came up, but if i run
>>>
>>> modprobe -r vchiq
>>>
>>> but i'm still getting
>>>
>>> [ 146.730014] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
>>> unexpected msgid 0@10b01b8d,0
>>>
>>> so it seems to me stopping those kthreads isn't that trivial (which i
>>> never expected). Maybe we need to care about the order or an even more
>>> complex synchronization mechanism between VideoCore firmware and the
>>> kthreads.
>> Dave, Phil - would you happen to have any opinions on this behavior ?
>>> I won't have the time for further investigations during the eastern
>>> holidays. Maybe we should revert d9c60badccc1 and take the necessary
>>> time for proper testing.
>>>
> It's hard to be sure, but I would guess that stopping the thread is
> causing the remote_event_wait in sync_func to terminate early. The
> following code assumes that the message is valid, when it fact it will
> have a message type of VCHIQ_MSG_PADDING.
>
> If that's the case, you can either find some way to spot that the wait
> has actually failed, or handle VCHIQ_MSG_PADDING explicitly in the
> switch.
Thanks. It seems the return value of remote_event_wait() is ignored here
and should be used.
>
> Phil
prev parent reply other threads:[~2024-03-29 9:53 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
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 [this message]
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=b03c9b02-b2c7-4a09-961a-9708456ad181@gmx.net \
--to=wahrenst@gmx.net \
--cc=broonie@kernel.org \
--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=stefan.wahren@i2se.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).