Reqbufs(0) need to release queued_list

Reqbufs(0) need to release queued_list

[Angela Wan]

Hi,
   As I have used videobuf2+soc_camera architecture on my camera
driver. I find a problem when I use Reqbuf(0), which only release
buffer, but not clear queued_list.
   Problem description:
   That is if upper layer uses qbuf then reqbuf0 directly, not having
stream on/dqbuf/off, then next time when streamon, videobuf2 could
still have the buffer from queued_list which having the buffer
released in privious reqbuf0, and the camera driver could access the
buffer already freed.
The steps that could cause problem for USERPTR:
Qbuf
Qbuf
Reqbuf 0
Reqbuf 20
Qbuf
Qbuf
Streamon   (queued_list still has the buffer already freed in the
previous reqbuf0)
.buf_queue (from camera driver, could access the buffer already freed)

   My question is if we could use __vb2_queue_release which calls
__vb2_queue_cancle(clear queue_list) and __vb2_queue_free(release
buffer) in Reqbuf(0), while not only use __vb2_queue_free.

Thank you

Angela Wan
Best Regards

Re: Reqbufs(0) need to release queued_list

[Guennadi Liakhovetski]

On Mon, 24 Oct 2011, Angela Wan wrote:

> Hi,
>    As I have used videobuf2+soc_camera architecture on my camera
> driver. I find a problem when I use Reqbuf(0), which only release
> buffer, but not clear queued_list.

Indeed, looks like vb2_reqbufs(p->count == 0) fails to clean up the queue?

Thanks
Guennadi

>    Problem description:
>    That is if upper layer uses qbuf then reqbuf0 directly, not having
> stream on/dqbuf/off, then next time when streamon, videobuf2 could
> still have the buffer from queued_list which having the buffer
> released in privious reqbuf0, and the camera driver could access the
> buffer already freed.
> The steps that could cause problem for USERPTR:
> Qbuf
> Qbuf
> Reqbuf 0
> Reqbuf 20
> Qbuf
> Qbuf
> Streamon   (queued_list still has the buffer already freed in the
> previous reqbuf0)
> .buf_queue (from camera driver, could access the buffer already freed)
> 
>    My question is if we could use __vb2_queue_release which calls
> __vb2_queue_cancle(clear queue_list) and __vb2_queue_free(release
> buffer) in Reqbuf(0), while not only use __vb2_queue_free.
> 
> Thank you
> 
> Angela Wan
> Best Regards
> --
> 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
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: Reqbufs(0) need to release queued_list

[Marek Szyprowski]

Hello,

On 2011-10-24 05:25, Angela Wan wrote:

>     As I have used videobuf2+soc_camera architecture on my camera
> driver. I find a problem when I use Reqbuf(0), which only release
> buffer, but not clear queued_list.

Thanks for pointing this bug. I will post a fix ASAP.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

[PATCH] media: vb2: reset queued list on REQBUFS(0) call

[Marek Szyprowski]

Queued list was not reset on REQBUFS(0) call. This caused enqueuing
a freed buffer to the driver.

Reported-by: Angela Wan <angela.j.wan@gmail.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
  drivers/media/video/videobuf2-core.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c 
b/drivers/media/video/videobuf2-core.c
index 3015e60..5722b81 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -254,6 +254,7 @@ static void __vb2_queue_free(struct vb2_queue *q)

  	q->num_buffers = 0;
  	q->memory = 0;
+	INIT_LIST_HEAD(&q->queued_list);
  }

  /**
-- 
1.7.1

Re: [PATCH] media: vb2: reset queued list on REQBUFS(0) call

[Angela Wan]

Hi, Marek
   Why not call vb2_queue_release directly in reqbufs(0) instead of
__vb2_queue_free, which could clear queued_count as well?

Angela
Best Regards

On Tue, Oct 25, 2011 at 3:59 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Queued list was not reset on REQBUFS(0) call. This caused enqueuing
> a freed buffer to the driver.
>
> Reported-by: Angela Wan <angela.j.wan@gmail.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/videobuf2-core.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c
> index 3015e60..5722b81 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -254,6 +254,7 @@ static void __vb2_queue_free(struct vb2_queue *q)
>
>        q->num_buffers = 0;
>        q->memory = 0;
> +       INIT_LIST_HEAD(&q->queued_list);
>  }
>
>  /**
> --
> 1.7.1
>
>
>

Re: [PATCH] media: vb2: reset queued list on REQBUFS(0) call

[Pawel Osciak]

Hi Angela,

On Tue, Oct 25, 2011 at 01:07, Angela Wan <angela.j.wan@gmail.com> wrote:
> Hi, Marek
>   Why not call vb2_queue_release directly in reqbufs(0) instead of
> __vb2_queue_free, which could clear queued_count as well?
>

vb2_queue_release is used to clean up streaming or fileio in progress,
neither of those can be active for reqbufs to proceed.

-- 
Best regards,
Pawel Osciak

Re: [PATCH] media: vb2: reset queued list on REQBUFS(0) call

[Pawel Osciak]

On Tue, Oct 25, 2011 at 00:59, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Queued list was not reset on REQBUFS(0) call. This caused enqueuing
> a freed buffer to the driver.
>
> Reported-by: Angela Wan <angela.j.wan@gmail.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/videobuf2-core.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c
> index 3015e60..5722b81 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -254,6 +254,7 @@ static void __vb2_queue_free(struct vb2_queue *q)
>
>        q->num_buffers = 0;
>        q->memory = 0;
> +       INIT_LIST_HEAD(&q->queued_list);
>  }
>
>  /**
> --
> 1.7.1
>
>
>

Acked-by: Pawel Osciak <pawel@osciak.com>

-- 
Best regards,
Pawel Osciak

Re: [PATCH] media: vb2: reset queued list on REQBUFS(0) call

[Mauro Carvalho Chehab]

Em 25-10-2011 06:11, Pawel Osciak escreveu:
> On Tue, Oct 25, 2011 at 00:59, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> Queued list was not reset on REQBUFS(0) call. This caused enqueuing
>> a freed buffer to the driver.
>>
>> Reported-by: Angela Wan <angela.j.wan@gmail.com>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/media/video/videobuf2-core.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/video/videobuf2-core.c
>> b/drivers/media/video/videobuf2-core.c
>> index 3015e60..5722b81 100644
>> --- a/drivers/media/video/videobuf2-core.c
>> +++ b/drivers/media/video/videobuf2-core.c
>> @@ -254,6 +254,7 @@ static void __vb2_queue_free(struct vb2_queue *q)
>>
>>        q->num_buffers = 0;
>>        q->memory = 0;
>> +       INIT_LIST_HEAD(&q->queued_list);
>>  }
>>
>>  /**
>> --
>> 1.7.1
>>
>>
>>
> 
> Acked-by: Pawel Osciak <pawel@osciak.com>
> 

This patch doesn't apply anymore. Is it still needed? If so, please rebase.

Thanks!
Mauro

RE: [PATCH] media: vb2: reset queued list on REQBUFS(0) call

[Marek Szyprowski]

Hello,

On Monday, November 07, 2011 2:46 PM Mauro Carvalho Chehab wrote:

> Em 25-10-2011 06:11, Pawel Osciak escreveu:
> > On Tue, Oct 25, 2011 at 00:59, Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> Queued list was not reset on REQBUFS(0) call. This caused enqueuing
> >> a freed buffer to the driver.
> >>
> >> Reported-by: Angela Wan <angela.j.wan@gmail.com>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> ---
> >>  drivers/media/video/videobuf2-core.c |    1 +
> >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/media/video/videobuf2-core.c
> >> b/drivers/media/video/videobuf2-core.c
> >> index 3015e60..5722b81 100644
> >> --- a/drivers/media/video/videobuf2-core.c
> >> +++ b/drivers/media/video/videobuf2-core.c
> >> @@ -254,6 +254,7 @@ static void __vb2_queue_free(struct vb2_queue *q)
> >>
> >>        q->num_buffers = 0;
> >>        q->memory = 0;
> >> +       INIT_LIST_HEAD(&q->queued_list);
> >>  }
> >>
> >>  /**
> >> --
> >> 1.7.1
> >>
> >>
> >>
> >
> > Acked-by: Pawel Osciak <pawel@osciak.com>
> >
> 
> This patch doesn't apply anymore. Is it still needed? If so, please rebase.

Yes, it is needed. This patch, together with other fixes (all rebased) is waiting in the
pull request I've sent on 4.11.2011 - "[GIT PULL] More Samsung patches for v3.2 (updated)"
thread. Do you want me to resend all of them?

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center