Re: [PATCH v2] videobuf2: Add missing lock held on vb2_fop_relase

[Ricardo Ribalda Delgado]

Hello

Anybody has a comment here? If not I will post a patch with the
modifications propossed by Sylwester.


Thanks!

On Sat, Oct 19, 2013 at 10:08 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@xxxxxxxxx> wrote:
> Hello Sylwester
>
>
> On Sat, Oct 19, 2013 at 8:27 PM, Sylwester Nawrocki
> <sylvester.nawrocki@xxxxxxxxx> wrote:
>> On 10/19/2013 06:07 PM, Ricardo Ribalda wrote:
>> [...]
>>>
>>> ---
>>>   drivers/media/platform/exynos4-is/fimc-capture.c |  2 +-
>>>   drivers/media/platform/exynos4-is/fimc-lite.c    |  2 +-
>>>   drivers/media/usb/em28xx/em28xx-video.c          |  2 +-
>>>   drivers/media/v4l2-core/videobuf2-core.c         | 18 +++++++++++++++++-
>>>   include/media/videobuf2-core.h                   |  2 ++
>>>   5 files changed, 22 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..c38d247c 100644
>>> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
>>> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
>>> @@ -549,7 +549,7 @@ static int fimc_capture_release(struct file *file)
>>>                 vc->streaming = false;
>>>         }
>>>
>>> -       ret = vb2_fop_release(file);
>>> +       ret = __vb2_fop_release(file, true);
>>>
>>>         if (close) {
>>>                 clear_bit(ST_CAPT_BUSY,&fimc->state);
>>>
>>> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c
>>> b/drivers/media/platform/exynos4-is/fimc-lite.c
>>> index e5798f7..021d804 100644
>>> --- a/drivers/media/platform/exynos4-is/fimc-lite.c
>>> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
>>> @@ -546,7 +546,7 @@ static int fimc_lite_release(struct file *file)
>>>                 mutex_unlock(&entity->parent->graph_mutex);
>>>         }
>>>
>>> -       vb2_fop_release(file);
>>> +       __vb2_fop_release(file, true);
>>>         pm_runtime_put(&fimc->pdev->dev);
>>>         clear_bit(ST_FLITE_SUSPENDED,&fimc->state);
>>>
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c
>>> b/drivers/media/usb/em28xx/em28xx-video.c
>>> index 9d10334..6a5c147 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>>> @@ -1664,7 +1664,7 @@ static int em28xx_v4l2_close(struct file *filp)
>>>         em28xx_videodbg("users=%d\n", dev->users);
>>>
>>>         mutex_lock(&dev->lock);
>>> -       vb2_fop_release(filp);
>>> +       __vb2_fop_release(filp, false);
>>
>>
>> I believe no modifications are needed for this driver.
>>
>>
>>>         if (dev->users == 1) {
>>>                 /* the device is already disconnect,
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>>> b/drivers/media/v4l2-core/videobuf2-core.c
>>> index 594c75e..ce309a8 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -2619,16 +2619,32 @@ int vb2_fop_mmap(struct file *file, struct
>>> vm_area_struct *vma)
>>>   }
>>>   EXPORT_SYMBOL_GPL(vb2_fop_mmap);
>>>
>>> -int vb2_fop_release(struct file *file)
>>> +int __vb2_fop_release(struct file *file, bool lock_is_held)
>>>   {
>>>         struct video_device *vdev = video_devdata(file);
>>> +       struct mutex *lock;
>>>
>>>         if (file->private_data == vdev->queue->owner) {
>>> +               if (lock_is_held)
>>> +                       lock = NULL;
>>> +               else
>>> +                       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);
>>>   }
>>> +EXPORT_SYMBOL_GPL(__vb2_fop_release);
>>> +
>>> +int vb2_fop_release(struct file *file)
>>> +{
>>> +       return __vb2_fop_release(file, false);
>>> +}
>>>   EXPORT_SYMBOL_GPL(vb2_fop_release);
>>
>>
>> It might be better to make it something like:
>>
>
> The rationale behind my patch (and probably not properly commented) is
> that the vb2_fop_release must be used ONLY as a file operantion
> handler.
>
> If the user makes its own function for relase the __vb2_fop_release
> function must be used and the infrastructure must be notified about
> the status of he lock (he is on his own).
>
> I believe my approach is simpler because It has only two functions
> (instead of 3) and the user understand the difference of the two
> functions just by looking at the arguments. In the future we could
> even check statically that  vb2_fop_release is not called inside a
> driver.
>
> Anyway, this is just a detail :), the most important part is that the
> oops is fixed, and that all the drivers that worked keep working.
>
> Lets wait for more comments and then lets post a new patch (with two
> functions and better documentation, or three functions).
>
> Thank you very much for you comments!!!
>
>> static int _vb2_fop_release(struct file *file, bool locked)
>>
>> {
>>         struct video_device *vdev = video_devdata(file);
>>         struct mutex *lock;
>>
>>         if (file->private_data == vdev->queue->owner) {
>>                 lock = vdev->queue->lock ?
>>                         vdev->queue->lock : vdev->lock;
>>
>>                 if (lock && !locked)
>>
>>                         mutex_lock(lock);
>>
>>                 vb2_queue_release(vdev->queue);
>>                 vdev->queue->owner = NULL;
>>                 if (lock && !locked)
>>                         mutex_unlock(lock);
>>         }
>>         return v4l2_fh_release(file);
>> }
>>
>> int vb2_fop_release(struct file *file)
>> {
>>         return _vb2_fop_release(file, false);
>> }
>> EXPORT_SYMBOL_GPL(vb2_fop_release);
>>
>> /*
>>  * This function should be used instead of vb2_fop_release()
>>  * if the caller already holds the video queue mutex.
>>  */
>> int __vb2_fop_release(struct file *file)
>> {
>>         return _vb2_fop_release(file, true);
>> }
>> EXPORT_SYMBOL_GPL(__vb2_fop_release);
>>
>> since __vb2_fop_release(file, false); is basically useless, it is same
>> as vb2_fop_release(file);
>>
>>
>>>   ssize_t vb2_fop_write(struct file *file, char __user *buf,
>>> diff --git a/include/media/videobuf2-core.h
>>> b/include/media/videobuf2-core.h
>>> index 6781258..cd1e4d5 100644
>>> --- a/include/media/videobuf2-core.h
>>> +++ b/include/media/videobuf2-core.h
>>> @@ -491,6 +491,8 @@ int vb2_ioctl_expbuf(struct file *file, void *priv,
>>>
>>>   int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma);
>>>   int vb2_fop_release(struct file *file);
>>> +/* must be used if the lock is held. */
>>
>>
>> Let's put any comments at the function body, not here.
>>
>>> +int __vb2_fop_release(struct file *file, bool lock_is_held);
>>
>>
>> int __vb2_fop_release(struct file *file);
>>
>>
>>>   ssize_t vb2_fop_write(struct file *file, char __user *buf,
>>>                 size_t count, loff_t *ppos);
>>>   ssize_t vb2_fop_read(struct file *file, char __user *buf,
>>
>>
>> Thanks,
>> Sylwester
>
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/