From: Helen Koike <helen.koike@collabora.com>
To: "Shuah Khan" <skhan@linuxfoundation.org>,
"André Almeida" <andrealmeid@collabora.com>,
mchehab@kernel.org, hverkuil@xs4all.nl,
laurent.pinchart@ideasonboard.com
Cc: linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
kernel@collabora.com
Subject: Re: [PATCH 0/3] Collapse vimc into single monolithic driver
Date: Mon, 12 Aug 2019 20:41:33 -0300 [thread overview]
Message-ID: <e81df4a5-2393-e341-258c-abf55babe519@collabora.com> (raw)
In-Reply-To: <03b51b1e-3d78-72e6-3b3e-210b4411c897@linuxfoundation.org>
Hi Shuah,
On 8/12/19 7:14 PM, Shuah Khan wrote:
> On 8/12/19 1:10 PM, Shuah Khan wrote:
>> On 8/12/19 12:52 PM, André Almeida wrote:
>>> Hi Shuah,
>>>
>>> On 8/12/19 11:08 AM, Shuah Khan wrote:
>>>> On 8/9/19 9:51 PM, Helen Koike wrote:
>>>>> Hi Andre,
>>>>>
>>>>> Thanks for testing this.
>>>>>
>>>>> On 8/9/19 9:24 PM, André Almeida wrote:
>>>>>> On 8/9/19 9:17 PM, Shuah Khan wrote:
>>>>>>> Hi Andre,
>>>>>>>
>>>>>>> On 8/9/19 5:52 PM, André Almeida wrote:
>>>>>>>> Hello Shuah,
>>>>>>>>
>>>>>>>> Thanks for the patch, I did some comments below.
>>>>>>>>
>>>>>>>> On 8/9/19 6:45 PM, Shuah Khan wrote:
>>>>>>>>> vimc uses Component API to split the driver into functional
>>>>>>>>> components.
>>>>>>>>> The real hardware resembles a monolith structure than component and
>>>>>>>>> component structure added a level of complexity making it hard to
>>>>>>>>> maintain without adding any real benefit.
>>>>>>>>> The sensor is one vimc component that would makes sense to be a
>>>>>>>>> separate
>>>>>>>>> module to closely align with the real hardware. It would be easier to
>>>>>>>>> collapse vimc into single monolithic driver first and then split the
>>>>>>>>> sensor off as a separate module.
>>>>>>>>>
>>>>>>>>> This patch series emoves the component API and makes minimal
>>>>>>>>> changes to
>>>>>>>>> the code base preserving the functional division of the code
>>>>>>>>> structure.
>>>>>>>>> Preserving the functional structure allows us to split the sensor off
>>>>>>>>> as a separate module in the future.
>>>>>>>>>
>>>>>>>>> Major design elements in this change are:
>>>>>>>>> - Use existing struct vimc_ent_config and struct
>>>>>>>>> vimc_pipeline_config
>>>>>>>>> to drive the initialization of the functional components.
>>>>>>>>> - Make vimc_ent_config global by moving it to vimc.h
>>>>>>>>> - Add two new hooks add and rm to initialize and register,
>>>>>>>>> unregister
>>>>>>>>> and free subdevs.
>>>>>>>>> - All component API is now gone and bind and unbind hooks are
>>>>>>>>> modified
>>>>>>>>> to do "add" and "rm" with minimal changes to just add and rm
>>>>>>>>> subdevs.
>>>>>>>>> - vimc-core's bind and unbind are now register and unregister.
>>>>>>>>> - vimc-core invokes "add" hooks from its
>>>>>>>>> vimc_register_devices().
>>>>>>>>> The "add" hooks remain the same and register subdevs. They
>>>>>>>>> don't
>>>>>>>>> create platform devices of their own and use vimc's
>>>>>>>>> pdev.dev as
>>>>>>>>> their reference device. The "add" hooks save their
>>>>>>>>> vimc_ent_device(s)
>>>>>>>>> in the corresponding vimc_ent_config.
>>>>>>>>> - vimc-core invokes "rm" hooks from its unregister to
>>>>>>>>> unregister
>>>>>>>>> subdevs
>>>>>>>>> and cleanup.
>>>>>>>>> - vimc-core invokes "add" and "rm" hooks with pointer to struct
>>>>>>>>> vimc_device
>>>>>>>>> and the corresponding struct vimc_ent_config pointer.
>>>>>>>>> The following configure and stream test works on all devices.
>>>>>>>>> media-ctl -d platform:vimc -V '"Sensor
>>>>>>>>> A":0[fmt:SBGGR8_1X8/640x480]'
>>>>>>>>> media-ctl -d platform:vimc -V '"Debayer
>>>>>>>>> A":0[fmt:SBGGR8_1X8/640x480]'
>>>>>>>>> media-ctl -d platform:vimc -V '"Sensor
>>>>>>>>> B":0[fmt:SBGGR8_1X8/640x480]'
>>>>>>>>> media-ctl -d platform:vimc -V '"Debayer
>>>>>>>>> B":0[fmt:SBGGR8_1X8/640x480]'
>>>>>>>>> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v
>>>>>>>>> width=1920,height=1440
>>>>>>>>> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v
>>>>>>>>> pixelformat=BA81
>>>>>>>>> v4l2-ctl -z platform:vimc -d "Raw Capture 1" -v
>>>>>>>>> pixelformat=BA81
>>>>>>>>> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video1
>>>>>>>>> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2
>>>>>>>>> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video3
>>>>>>>>>
>>>>>>>>> The third patch in the series fixes a general protection fault found
>>>>>>>>> when rmmod is done while stream is active.
>>>>>>>>
>>>>>>>> I applied your patch on top of media_tree/master and I did some
>>>>>>>> testing.
>>>>>>>> Not sure if I did something wrong, but just adding and removing the
>>>>>>>> module generated a kernel panic:
>>>>>>>
>>>>>>> Thanks for testing.
>>>>>>>
>>>>>>> Odd. I tested modprobe and rmmod both.I was working on Linux 5.3-rc2.
>>>>>>> I will apply these to media latest and work from there. I have to
>>>>>>> rebase these on top of the reverts from Lucas and Helen
>>>>>>
>>>>>> Ok, please let me know if I succeeded to reproduce.
>>>>>>
>>>>>>>>
>>>>>>>> ~# modprobe vimc
>>>>>>>> ~# rmmod vimc
>>>>>>>> [ 16.452974] stack segment: 0000 [#1] SMP PTI
>>>>>>>> [ 16.453688] CPU: 0 PID: 2038 Comm: rmmod Not tainted 5.3.0-rc2+ #36
>>>>>>>> [ 16.454678] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>>>>>>>> BIOS 1.12.0-20181126_142135-anatol 04/01/2014
>>>>>>>> [ 16.456191] RIP: 0010:kfree+0x4d/0x240
>>>>>>>>
>>>>>>>> <registers values...>
>>>>>>>>
>>>>>>>> [ 16.469188] Call Trace:
>>>>>>>> [ 16.469666] vimc_remove+0x35/0x90 [vimc]
>>>>>>>> [ 16.470436] platform_drv_remove+0x1f/0x40
>>>>>>>> [ 16.471233] device_release_driver_internal+0xd3/0x1b0
>>>>>>>> [ 16.472184] driver_detach+0x37/0x6b
>>>>>>>> [ 16.472882] bus_remove_driver+0x50/0xc1
>>>>>>>> [ 16.473569] vimc_exit+0xc/0xca0 [vimc]
>>>>>>>> [ 16.474231] __x64_sys_delete_module+0x18d/0x240
>>>>>>>> [ 16.475036] do_syscall_64+0x43/0x110
>>>>>>>> [ 16.475656] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>>>>>> [ 16.476504] RIP: 0033:0x7fceb8dafa4b
>>>>>>>>
>>>>>>>> <registers values...>
>>>>>>>>
>>>>>>>> [ 16.484853] Modules linked in: vimc(-) videobuf2_vmalloc
>>>>>>>> videobuf2_memops v4l2_tpg videobuf2_v4l2 videobuf2_common
>>>>>>>> [ 16.486187] ---[ end trace 91e5e0894e254d49 ]---
>>>>>>>> [ 16.486758] RIP: 0010:kfree+0x4d/0x240
>>>>>>>>
>>>>>>>> <registers values...>
>>>>>>>>
>>>>>>>> fish: “rmmod vimc” terminated by signal SIGSEGV (Address boundary
>>>>>>>> error)
>>>>>>>>
>>>>>>>> I just added the module after booting, no other action was made.
>>>>>>>> Here is
>>>>>>>> how my `git log --oneline` looks like:
>>>>>>>>
>>>>>>>> 897d708e922b media: vimc: Fix gpf in rmmod path when stream is active
>>>>>>>> 2e4a5ad8ad6d media: vimc: Collapse component structure into a single
>>>>>>>> monolithic driver
>>>>>>>> 7c8da1687e92 media: vimc: move private defines to a common header
>>>>>>>> 97299a303532 media: Remove dev_err() usage after platform_get_irq()
>>>>>>>> 25a3d6bac6b9 media: adv7511/cobalt: rename driver name to adv7511-v4l2
>>>>>
>>>>> I couldn't reproduce the error, my tree looks the same:
>>>>>
>>>>> [I] koike@floko ~/m/o/linux> git log --oneline
>>>>> e3345155c8ed (HEAD) media: vimc: Fix gpf in rmmod path when stream is
>>>>> active
>>>>> 43e9e2fe761f media: vimc: Collapse component structure into a single
>>>>> monolithic driver
>>>>> 8a6d0b9adde0 media: vimc: move private defines to a common header
>>>>> 97299a303532 (media/master) media: Remove dev_err() usage after
>>>>> platform_get_irq()
>>>>> 25a3d6bac6b9 media: adv7511/cobalt: rename driver name to adv7511-v4l2
>>>>
>>>> Thanks Helen for trying to reproduce and sharing the result.
>>>
>>> Me and Helen found out what is the problem. If you follow this call trace:
>>>
>>> vimc_ent_sd_unregister()
>>> v4l2_device_unregister_subdev()
>>> v4l2_subdev_release()
>>>
>>> You'll notice that this last function calls the `release` callback
>>> implementation of the subdevice. For instance, the `release` of
>>> vimc-sensor is this one:
>>>
>>> static void vimc_sen_release(struct v4l2_subdev *sd)
>>> {
>>> struct vimc_sen_device *vsen =
>>> container_of(sd, struct vimc_sen_device, sd);
>>>
>>> v4l2_ctrl_handler_free(&vsen->hdl);
>>> tpg_free(&vsen->tpg);
>>> kfree(vsen);
>>> }
>>>
>>> And then you can see that `vsen` has been freed. Back to
>>> vimc_ent_sd_unregister(), after v4l2_device_unregister_subdev(), the
>>> function will call vimc_pads_cleanup(). This is basically a
>>> kfree(ved->pads), but `ved` has just been freed at
>>> v4l2_subdev_release(), producing a memory fault.
>>>
>>> To fix that, we found two options:
>>>
>>> - place the kfree(ved->pads) inside the release callback of each
>>> subdevice and removing vimc_pads_cleanup() from
>>> vimc_ent_sd_unregister()
>>> - use a auxiliary variable to hold the address of the pads, for instance:
>>>
>>> void vimc_ent_sd_unregister(...)
>>> {
>>> struct media_pad *pads = ved->pads;
>>> ...
>>> vimc_pads_cleanup(pads);
>>> }
>>>
>>>
>>
>> I fixed a problem in the thirds patch. vimc-capture uses the first
>> approach - placing the kfree(ved->pads) inside the release callback.
>>
>> I am debugging another such problem in unbind path while streaming.
>> I am working on v2 and I will look for the rmmod problem and fix it.
>>
>> thanks again for testing and finding the root cause.
>> -- Shuah
>
> Hi Andre,
>
> Here is what's happening.
>
> Before this change, you can't really do rmmod vimc, because vimc is in
> use by other component drivers. With the collapse, now you can actually
> do rmmod on vimc and this problem in vimc_ent_sd_unregister() that frees
> pads first and the does v4l2_device_unregister_subdev().
>
> I fixed this in the 3/3 patch. I can reproduce the problem with patches 1 and 2, and patch 3 fixes it.
>
> Did you test with the third patch in this series?
yes, we tested with 3/3, but the new problem now is when doing the following
in this order:
v4l2_device_unregister_subdev(sd);
vimc_pads_cleanup(ved->pads);
v4l2_device_unregister_subdev() calls the release function of the subdevice that
frees the ved object, so ved->pads is not valid anymore. That is why André suggested
a temporary variable to hold ved->pads and to be able to free it later:
struct media_pad *pads = ved->pads;
v4l2_device_unregister_subdev(sd);
vimc_pads_cleanup(pads); // So we don't use the ved object here anymore.
Regards,
Helen
>
> thanks,
> -- Shuah
next prev parent reply other threads:[~2019-08-12 23:41 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-09 21:45 [PATCH 0/3] Collapse vimc into single monolithic driver Shuah Khan
2019-08-09 21:45 ` [PATCH 1/3] media: vimc: move private defines to a common header Shuah Khan
2019-08-10 3:15 ` Helen Koike
2019-08-12 14:03 ` Shuah Khan
2019-08-10 14:14 ` Laurent Pinchart
2019-08-12 14:19 ` Shuah Khan
2019-08-12 14:24 ` Laurent Pinchart
2019-08-12 14:27 ` Shuah Khan
2019-08-09 21:45 ` [PATCH 2/3] media: vimc: Collapse component structure into a single monolithic driver Shuah Khan
2019-08-10 4:12 ` Helen Koike
2019-08-12 14:12 ` Shuah Khan
2019-08-09 21:45 ` [PATCH 3/3] media: vimc: Fix gpf in rmmod path when stream is active Shuah Khan
2019-08-09 23:52 ` [PATCH 0/3] Collapse vimc into single monolithic driver André Almeida
2019-08-10 0:17 ` Shuah Khan
2019-08-10 0:24 ` André Almeida
2019-08-10 0:48 ` Shuah Khan
2019-08-10 3:51 ` Helen Koike
2019-08-12 14:08 ` Shuah Khan
2019-08-12 18:52 ` André Almeida
2019-08-12 19:10 ` Shuah Khan
2019-08-12 22:14 ` Shuah Khan
2019-08-12 23:41 ` Helen Koike [this message]
2019-08-13 0:58 ` Shuah Khan
2019-08-13 9:56 ` Laurent Pinchart
2019-08-13 12:25 ` Helen Koike
2019-08-13 12:36 ` Laurent Pinchart
2019-08-13 23:22 ` Shuah Khan
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=e81df4a5-2393-e341-258c-abf55babe519@collabora.com \
--to=helen.koike@collabora.com \
--cc=andrealmeid@collabora.com \
--cc=hverkuil@xs4all.nl \
--cc=kernel@collabora.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=skhan@linuxfoundation.org \
/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 NNTP newsgroup(s).