[v3,0/3] media: vimc: release vimc in release cb of v4l2_device

Message

Dafna Hirschfeld Nov. 26, 2019, 12:08 p.m. UTC

This patchset solves a crash that happens when unbinding the vimc
device while it is streaming. Currently when the device is unbounded
the vimc entities are released imidiettaly, this cause a crash
if the streaming thread or the capture device dereference them
after the release. The patchset solves this by deferring the release
to the release callback of v4l2_device. This ensures that
the vimc entities will be released after the last fh is closed
and so the streaming terminates before.

- The first patch replaces the usage of vimc_device.pdev.dev
with vimc.mdev.dev
- The second patch allocates the vimc_device dynamically.
This is needed since the release of the device is deferred
and might run after the device is initialized again.
- The third patch moves the release of the vimc_device
and all the vimc entities to the release callback of
v4l2_device.

Changes from v2:
in patch 3, in case of failure in the probe function the memory is released
directly from the probe function, and only on success path the release callback
of v4l2_device is assigned

Dafna Hirschfeld (3):
  media: vimc: replace vimc->pdev.dev with vimc->mdev.dev
  media: vimc: allocate vimc_device dynamically
  media: vimc: crash fix - release vimc in the v4l_device release

 drivers/media/platform/vimc/vimc-capture.c |  18 ++--
 drivers/media/platform/vimc/vimc-common.c  |   2 -
 drivers/media/platform/vimc/vimc-common.h  |  30 +++---
 drivers/media/platform/vimc/vimc-core.c    | 101 ++++++++++++++-------
 drivers/media/platform/vimc/vimc-debayer.c |  19 ++--
 drivers/media/platform/vimc/vimc-scaler.c  |  19 ++--
 drivers/media/platform/vimc/vimc-sensor.c  |  18 ++--
 7 files changed, 115 insertions(+), 92 deletions(-)

Comments

Shuah Khan Nov. 26, 2019, 7 p.m. UTC | #1

On 11/26/19 5:08 AM, Dafna Hirschfeld wrote:
> This patchset solves a crash that happens when unbinding the vimc
> device while it is streaming. Currently when the device is unbounded
> the vimc entities are released imidiettaly, this cause a crash
> if the streaming thread or the capture device dereference them
> after the release. The patchset solves this by deferring the release
> to the release callback of v4l2_device. This ensures that
> the vimc entities will be released after the last fh is closed
> and so the streaming terminates before.
> 
> - The first patch replaces the usage of vimc_device.pdev.dev
> with vimc.mdev.dev
> - The second patch allocates the vimc_device dynamically.
> This is needed since the release of the device is deferred
> and might run after the device is initialized again.
> - The third patch moves the release of the vimc_device
> and all the vimc entities to the release callback of
> v4l2_device.
> 
> Changes from v2:
> in patch 3, in case of failure in the probe function the memory is released
> directly from the probe function, and only on success path the release callback
> of v4l2_device is assigned
> 

You might be able to fix the streaming crash with this change,
however, I don't think this will work in all cases.

Please test these changes with tools/testing/selftests/media_tests

I am curious what happens when you bind/unbind the driver while
running media ioctls e.g: MEDIA_IOC_DEVICE_INFO.

thanks,
-- Shuah