| Message ID | 526256F5.1060404@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Sylwester On Sat, Oct 19, 2013 at 11:55 AM, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote: > Hi Ricardo, > > > On 10/14/2013 09:41 AM, Ricardo Ribalda Delgado wrote: >> >> vb2_fop_relase does not held the lock although it is modifying the >> queue->owner field. > > [...] > >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >> b/drivers/media/v4l2-core/videobuf2-core.c >> index 9fc4bab..3a961ee 100644 >> --- a/drivers/media/v4l2-core/videobuf2-core.c >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >> @@ -2588,8 +2588,15 @@ int vb2_fop_release(struct file *file) >> struct video_device *vdev = video_devdata(file); >> >> if (file->private_data == vdev->queue->owner) { >> + struct mutex *lock; >> + >> + lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock; >> + if (lock) >> + mutex_lock(lock); >> vb2_queue_release(vdev->queue); >> vdev->queue->owner = NULL; >> + if (lock) >> + mutex_unlock(lock); >> } >> return v4l2_fh_release(file); >> } > > > It seems you didn't inspect all users of vb2_fop_release(). There are 3 > drivers > that don't assign vb2_fop_release() to struct v4l2_file_operations directly > but > instead call it from within its own release() handler. Two of them do call > vb2_fop_release() with the video queue lock already held. > > $ git grep -n vb2_fop_rel -- drivers/media/ > > drivers/media/platform/exynos4-is/fimc-capture.c:552: ret = > vb2_fop_release(file); > drivers/media/platform/exynos4-is/fimc-lite.c:549: vb2_fop_release(file); > Very good catch, thanks! > A rather ugly solution would be to open code the vb2_fop_release() function > in those driver, like in below patch (untested). Unless there are better > proposals I would queue the patch as below together with the $subject patch > upstream. IMHO this will lead to the same type of mistakes in the future. What about creating a function __vb2_fop_release that does exactly the same as the original function but with an extra parameter bool lock_held vb2_fop_release will be a wrapper for that funtion with lock_held== false drivers that overload the fop_release and need to hold the lock will call the __ function with lock_held= true What do you think? Thanks! > > > From 3617684d759bb021e3cf1d862a91cb6e18d12052 Mon Sep 17 00:00:00 2001 > From: Sylwester Nawrocki <s.nawrocki@samsung.com> > Date: Sat, 19 Oct 2013 11:48:10 +0200 > Subject: [PATCH] exynos4-is: Do not call vb2_fop_release() with queue lock > held > > Currently vb2_fop_release() function doesn't take the queue lock, > but it is going to change and then there would happen a deadlock > in fimc_capture_release() and fimc_lite_release(), since these > function take the video queue lock prior to calling vb2_fop_release(). > > To avoid a deadlock open code the vb2_fop_release() function in those > drivers. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > drivers/media/platform/exynos4-is/fimc-capture.c | 11 ++++++++--- > drivers/media/platform/exynos4-is/fimc-lite.c | 8 +++++++- > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c > b/drivers/media/platform/exynos4-is/fimc-capture.c > index fb27ff7..e9a5c90 100644 > --- a/drivers/media/platform/exynos4-is/fimc-capture.c > +++ b/drivers/media/platform/exynos4-is/fimc-capture.c > @@ -537,6 +537,7 @@ static int fimc_capture_release(struct file *file) > { > struct fimc_dev *fimc = video_drvdata(file); > struct fimc_vid_cap *vc = &fimc->vid_cap; > + struct video_device *vdev = &vc->ve.vdev; > bool close = v4l2_fh_is_singular_file(file); > int ret; > > @@ -545,11 +546,15 @@ static int fimc_capture_release(struct file *file) > mutex_lock(&fimc->lock); > > if (close && vc->streaming) { > - media_entity_pipeline_stop(&vc->ve.vdev.entity); > + media_entity_pipeline_stop(&vdev->entity); > vc->streaming = false; > } > > - ret = vb2_fop_release(file); > + if (file->private_data == vdev->queue->owner) { > + vb2_queue_release(vdev->queue); > + vdev->queue->owner = NULL; > + } > + ret = v4l2_fh_release(file); > > if (close) { > clear_bit(ST_CAPT_BUSY, &fimc->state); > @@ -557,7 +562,7 @@ static int fimc_capture_release(struct file *file) > clear_bit(ST_CAPT_SUSPENDED, &fimc->state); > > fimc_md_graph_lock(&vc->ve); > - vc->ve.vdev.entity.use_count--; > + vdev->entity.use_count--; > fimc_md_graph_unlock(&vc->ve); > } > > diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c > b/drivers/media/platform/exynos4-is/fimc-lite.c > index e5798f7..182db3c 100644 > --- a/drivers/media/platform/exynos4-is/fimc-lite.c > +++ b/drivers/media/platform/exynos4-is/fimc-lite.c > @@ -528,6 +528,7 @@ static int fimc_lite_release(struct file *file) > { > struct fimc_lite *fimc = video_drvdata(file); > struct media_entity *entity = &fimc->ve.vdev.entity; > + struct video_device *vdev = &fimc->ve.vdev; > > mutex_lock(&fimc->lock); > > @@ -546,7 +547,12 @@ static int fimc_lite_release(struct file *file) > mutex_unlock(&entity->parent->graph_mutex); > } > > - vb2_fop_release(file); > + if (file->private_data == vdev->queue->owner) { > + vb2_queue_release(vdev->queue); > + vdev->queue->owner = NULL; > + } > + v4l2_fh_release(file); > + > pm_runtime_put(&fimc->pdev->dev); > clear_bit(ST_FLITE_SUSPENDED, &fimc->state); > > -- > 1.7.4.1
On 10/19/2013 12:22 PM, Ricardo Ribalda Delgado wrote: > On Sat, Oct 19, 2013 at 11:55 AM, Sylwester Nawrocki > <sylvester.nawrocki@gmail.com> wrote: >> > On 10/14/2013 09:41 AM, Ricardo Ribalda Delgado wrote: >>> >> >>> >> vb2_fop_relase does not held the lock although it is modifying the >>> >> queue->owner field. >> > [...] >>> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >>> >> b/drivers/media/v4l2-core/videobuf2-core.c >>> >> index 9fc4bab..3a961ee 100644 >>> >> --- a/drivers/media/v4l2-core/videobuf2-core.c >>> >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>> >> @@ -2588,8 +2588,15 @@ int vb2_fop_release(struct file *file) >>> >> struct video_device *vdev = video_devdata(file); >>> >> >>> >> if (file->private_data == vdev->queue->owner) { >>> >> + struct mutex *lock; >>> >> + >>> >> + lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock; >>> >> + if (lock) >>> >> + mutex_lock(lock); >>> >> vb2_queue_release(vdev->queue); >>> >> vdev->queue->owner = NULL; >>> >> + if (lock) >>> >> + mutex_unlock(lock); >>> >> } >>> >> return v4l2_fh_release(file); >>> >> } >> > >> > >> > It seems you didn't inspect all users of vb2_fop_release(). There are 3 >> > drivers >> > that don't assign vb2_fop_release() to struct v4l2_file_operations directly >> > but >> > instead call it from within its own release() handler. Two of them do call >> > vb2_fop_release() with the video queue lock already held. >> > >> > $ git grep -n vb2_fop_rel -- drivers/media/ >> > >> > drivers/media/platform/exynos4-is/fimc-capture.c:552: ret = >> > vb2_fop_release(file); >> > drivers/media/platform/exynos4-is/fimc-lite.c:549: vb2_fop_release(file); >> > > > Very good catch, thanks! > >> > A rather ugly solution would be to open code the vb2_fop_release() function >> > in those driver, like in below patch (untested). Unless there are better >> > proposals I would queue the patch as below together with the $subject patch >> > upstream. > > IMHO this will lead to the same type of mistakes in the future. > > What about creating a function __vb2_fop_release that does exactly > the same as the original function but with an extra parameter bool > lock_held > > vb2_fop_release will be a wrapper for that funtion with lock_held== false Hmm, the parameter would be telling whether the lock is already held ? Perhaps we should inverse its meaning and it should indicate whether vb2_fop_release() should be taking the lock internally ? It seems to me more straightforward. > drivers that overload the fop_release and need to hold the lock will > call the __ function with lock_held= true > > What do you think? I was also considering this, it's probably better. I'm not sure about exporting functions prefixed with __ though. And the locking becomes less clear with such functions proliferation. Anyway, I'm in general personally OK with having an additional version like: __vb2_fop_release(struct file *filp, bool lock); Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Sylwester I have just posted a new version. Please take a look to it, it should fix your issue. I havent tried it in hw because I am out of the office. Regards! On Sat, Oct 19, 2013 at 12:51 PM, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote: > On 10/19/2013 12:22 PM, Ricardo Ribalda Delgado wrote: >> >> On Sat, Oct 19, 2013 at 11:55 AM, Sylwester Nawrocki >> <sylvester.nawrocki@gmail.com> wrote: >>> >>> > On 10/14/2013 09:41 AM, Ricardo Ribalda Delgado wrote: >>> >>>> >> >>>> >> vb2_fop_relase does not held the lock although it is modifying the >>>> >> queue->owner field. >>> >>> > [...] >>>> >>>> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >>>> >> b/drivers/media/v4l2-core/videobuf2-core.c >>>> >> index 9fc4bab..3a961ee 100644 >>>> >> --- a/drivers/media/v4l2-core/videobuf2-core.c >>>> >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>>> >> @@ -2588,8 +2588,15 @@ int vb2_fop_release(struct file *file) >>>> >> struct video_device *vdev = video_devdata(file); >>>> >> >>>> >> if (file->private_data == vdev->queue->owner) { >>>> >> + struct mutex *lock; >>>> >> + >>>> >> + lock = vdev->queue->lock ? vdev->queue->lock : >>>> >> vdev->lock; >>>> >> + if (lock) >>>> >> + mutex_lock(lock); >>>> >> vb2_queue_release(vdev->queue); >>>> >> vdev->queue->owner = NULL; >>>> >> + if (lock) >>>> >> + mutex_unlock(lock); >>>> >> } >>>> >> return v4l2_fh_release(file); >>>> >> } >>> >>> > >>> > >>> > It seems you didn't inspect all users of vb2_fop_release(). There are >>> > 3 >>> > drivers >>> > that don't assign vb2_fop_release() to struct v4l2_file_operations >>> > directly >>> > but >>> > instead call it from within its own release() handler. Two of them do >>> > call >>> > vb2_fop_release() with the video queue lock already held. >>> > >>> > $ git grep -n vb2_fop_rel -- drivers/media/ >>> > >>> > drivers/media/platform/exynos4-is/fimc-capture.c:552: ret = >>> > vb2_fop_release(file); >>> > drivers/media/platform/exynos4-is/fimc-lite.c:549: >>> > vb2_fop_release(file); >>> > >> >> >> Very good catch, thanks! >> >>> > A rather ugly solution would be to open code the vb2_fop_release() >>> > function >>> > in those driver, like in below patch (untested). Unless there are >>> > better >>> > proposals I would queue the patch as below together with the $subject >>> > patch >>> > upstream. >> >> >> IMHO this will lead to the same type of mistakes in the future. >> >> What about creating a function __vb2_fop_release that does exactly >> the same as the original function but with an extra parameter bool >> lock_held >> >> vb2_fop_release will be a wrapper for that funtion with lock_held== false > > > Hmm, the parameter would be telling whether the lock is already held ? > Perhaps > we should inverse its meaning and it should indicate whether > vb2_fop_release() > should be taking the lock internally ? It seems to me more straightforward. > > >> drivers that overload the fop_release and need to hold the lock will >> call the __ function with lock_held= true >> >> What do you think? > > > I was also considering this, it's probably better. I'm not sure about > exporting > functions prefixed with __ though. And the locking becomes less clear with > such > functions proliferation. > > Anyway, I'm in general personally OK with having an additional version like: > > __vb2_fop_release(struct file *filp, bool lock); > > > Regards, > Sylwester
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c index fb27ff7..e9a5c90 100644 --- a/drivers/media/platform/exynos4-is/fimc-capture.c +++ b/drivers/media/platform/exynos4-is/fimc-capture.c @@ -537,6 +537,7 @@ static int fimc_capture_release(struct file *file) { struct fimc_dev *fimc = video_drvdata(file); struct fimc_vid_cap *vc = &fimc->vid_cap; + struct video_device *vdev = &vc->ve.vdev; bool close = v4l2_fh_is_singular_file(file); int ret; @@ -545,11 +546,15 @@ static int fimc_capture_release(struct file *file) mutex_lock(&fimc->lock); if (close && vc->streaming) { - media_entity_pipeline_stop(&vc->ve.vdev.entity); + media_entity_pipeline_stop(&vdev->entity); vc->streaming = false; } - ret = vb2_fop_release(file); + if (file->private_data == vdev->queue->owner) { + vb2_queue_release(vdev->queue); + vdev->queue->owner = NULL; + } + ret = v4l2_fh_release(file); if (close) { clear_bit(ST_CAPT_BUSY, &fimc->state); @@ -557,7 +562,7 @@ static int fimc_capture_release(struct file *file) clear_bit(ST_CAPT_SUSPENDED, &fimc->state); fimc_md_graph_lock(&vc->ve); - vc->ve.vdev.entity.use_count--; + vdev->entity.use_count--; fimc_md_graph_unlock(&vc->ve); } diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c index e5798f7..182db3c 100644 --- a/drivers/media/platform/exynos4-is/fimc-lite.c +++ b/drivers/media/platform/exynos4-is/fimc-lite.c @@ -528,6 +528,7 @@ static int fimc_lite_release(struct file *file) { struct fimc_lite *fimc = video_drvdata(file); struct media_entity *entity = &fimc->ve.vdev.entity; + struct video_device *vdev = &fimc->ve.vdev; mutex_lock(&fimc->lock); @@ -546,7 +547,12 @@ static int fimc_lite_release(struct file *file) mutex_unlock(&entity->parent->graph_mutex); } - vb2_fop_release(file); + if (file->private_data == vdev->queue->owner) { + vb2_queue_release(vdev->queue); + vdev->queue->owner = NULL; + } + v4l2_fh_release(file); + pm_runtime_put(&fimc->pdev->dev); clear_bit(ST_FLITE_SUSPENDED, &fimc->state);