| Message ID | 20200113215506.13329-7-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vimc: race condition fixes | expand |
Hi Dafna, On 1/13/20 7:55 PM, Dafna Hirschfeld wrote: > After a successful media_device_register call, call v4l2_device_get(). > and set the media_devnode release callback to a function that > calls v4l2_device_put(). > That should ensure that the v4l2_device's release callback is called > when the very last user of any of the registered device nodes has > closed its fh. This patch shouldn't be required if you accept my suggestion in patch 3/6, since the release function in v4l2_dev wouldn't be used. Let me know what you think. Regards, Helen > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/media/platform/vimc/vimc-core.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c > index 9d4e8bc89620..0f03e9cec075 100644 > --- a/drivers/media/platform/vimc/vimc-core.c > +++ b/drivers/media/platform/vimc/vimc-core.c > @@ -214,6 +214,14 @@ static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > kfree(vimc); > } > > +static void vimc_media_device_release(struct media_devnode *devnode) > +{ > + struct media_device *mdev = devnode->media_dev; > + struct vimc_device *vimc = container_of(mdev, struct vimc_device, mdev); > + > + v4l2_device_put(&vimc->v4l2_dev); > +} > + > static int vimc_register_devices(struct vimc_device *vimc) > { > int ret; > @@ -252,6 +260,8 @@ static int vimc_register_devices(struct vimc_device *vimc) > goto err_rm_subdevs; > } > > + v4l2_device_get(&vimc->v4l2_dev); > + vimc->mdev.devnode->release = vimc_media_device_release; > /* Expose all subdev's nodes*/ > ret = v4l2_device_register_subdev_nodes(&vimc->v4l2_dev); > if (ret) { >
Hi Dafna, On 1/13/20 10:55 PM, Dafna Hirschfeld wrote: > After a successful media_device_register call, call v4l2_device_get(). > and set the media_devnode release callback to a function that > calls v4l2_device_put(). > That should ensure that the v4l2_device's release callback is called > when the very last user of any of the registered device nodes has > closed its fh. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/media/platform/vimc/vimc-core.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c > index 9d4e8bc89620..0f03e9cec075 100644 > --- a/drivers/media/platform/vimc/vimc-core.c > +++ b/drivers/media/platform/vimc/vimc-core.c > @@ -214,6 +214,14 @@ static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > kfree(vimc); > } > > +static void vimc_media_device_release(struct media_devnode *devnode) > +{ > + struct media_device *mdev = devnode->media_dev; > + struct vimc_device *vimc = container_of(mdev, struct vimc_device, mdev); > + > + v4l2_device_put(&vimc->v4l2_dev); > +} > + > static int vimc_register_devices(struct vimc_device *vimc) > { > int ret; > @@ -252,6 +260,8 @@ static int vimc_register_devices(struct vimc_device *vimc) > goto err_rm_subdevs; > } > > + v4l2_device_get(&vimc->v4l2_dev); > + vimc->mdev.devnode->release = vimc_media_device_release; > /* Expose all subdev's nodes*/ > ret = v4l2_device_register_subdev_nodes(&vimc->v4l2_dev); > if (ret) { > I like the idea, but I think the roles of v4l2_device and media_device should be swapped. Logically the media device is the top-level device, and the v4l2_device sits below it. So rather than cleaning everything up in the v4l2_device release callback, that should be done in the mdev.devnode->release callback. So during the probe you need a call to get_device(&mdev.devnode->dev) and in the v4l2_device release callback you call put_device(&mdev.devnode->dev). And the mdev.devnode->release() callback is then used to clean everything up. I think it is a good idea to add helper functions media_device_get/put that take a media_device pointer as argument, but I'd do that as a new last patch, replacing any get/put_device() calls for mdev.devnode in one go. There may be some discussion about that, so having this as the last patch makes it easier to postpone merging it if needed. Regards, Hans
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c index 9d4e8bc89620..0f03e9cec075 100644 --- a/drivers/media/platform/vimc/vimc-core.c +++ b/drivers/media/platform/vimc/vimc-core.c @@ -214,6 +214,14 @@ static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) kfree(vimc); } +static void vimc_media_device_release(struct media_devnode *devnode) +{ + struct media_device *mdev = devnode->media_dev; + struct vimc_device *vimc = container_of(mdev, struct vimc_device, mdev); + + v4l2_device_put(&vimc->v4l2_dev); +} + static int vimc_register_devices(struct vimc_device *vimc) { int ret; @@ -252,6 +260,8 @@ static int vimc_register_devices(struct vimc_device *vimc) goto err_rm_subdevs; } + v4l2_device_get(&vimc->v4l2_dev); + vimc->mdev.devnode->release = vimc_media_device_release; /* Expose all subdev's nodes*/ ret = v4l2_device_register_subdev_nodes(&vimc->v4l2_dev); if (ret) {
After a successful media_device_register call, call v4l2_device_get(). and set the media_devnode release callback to a function that calls v4l2_device_put(). That should ensure that the v4l2_device's release callback is called when the very last user of any of the registered device nodes has closed its fh. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/media/platform/vimc/vimc-core.c | 10 ++++++++++ 1 file changed, 10 insertions(+)