diff mbox

Fixed list_del corruption in videobuf-core.c : videobuf_queue_cancel()

Message ID 1349451865-26678-1-git-send-email-andrei.mandychev@parrot.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrei Mandychev Oct. 5, 2012, 3:44 p.m. UTC 
If there is a buffer with VIDEOBUF_QUEUED state it won't be deleted properly
because the head of queue loses its elements by calling INIT_LIST_HEAD()
before videobuf_streamoff().
---
 drivers/media/video/omap/omap_vout.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Vaibhav Hiremath Oct. 8, 2012, 2:50 p.m. UTC | #1 
On Fri, Oct 05, 2012 at 21:14:25, Andrei Mandychev wrote:
> If there is a buffer with VIDEOBUF_QUEUED state it won't be deleted properly
> because the head of queue loses its elements by calling INIT_LIST_HEAD()
> before videobuf_streamoff().

"dma_queue" is driver internal queue and videobuf_streamoff() function 
will end up into buf_release() callback, which in our case doesn't do 
anything with dmaqueue. 


Did you face any runtime issues with this? I still did not understand 
about this corruption thing.

Thanks,
Vaibhav
> ---
>  drivers/media/video/omap/omap_vout.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
> index 409da0f..f02eb8e 100644
> --- a/drivers/media/video/omap/omap_vout.c
> +++ b/drivers/media/video/omap/omap_vout.c
> @@ -1738,8 +1738,8 @@ static int vidioc_streamoff(struct file *file, void *fh, enum v4l2_buf_type i)
>  		v4l2_err(&vout->vid_dev->v4l2_dev, "failed to change mode in"
>  				" streamoff\n");
>  
> -	INIT_LIST_HEAD(&vout->dma_queue);
>  	ret = videobuf_streamoff(&vout->vbq);
> +	INIT_LIST_HEAD(&vout->dma_queue);
>  
>  	return ret;
>  }
> -- 
> 1.7.9.5
> 
> 

--
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
Andrei Mandychev Oct. 8, 2012, 3:39 p.m. UTC | #2 
Hi,

 From time to time I get a backtrace listed below when I switch video 
from LCD to TV and vice versa. The video is switched without any problem 
but the kernel sometimes generates this trace. I decided to investigate 
it and discovered that it happens when you call VIDIOC_STREAMOFF ioctl 
and there are some buffers in the queue with VIDEOBUF_QUEUE state. I 
don't think it's a big issue but it generates annoying trace in the log.

<4>[  252.661529] list_del corruption. next->prev should be d1fe56ac, 
but was df49458c
<4>[  252.661529] Modules linked in: blackberry cdc_acm sierra option 
usb_wwan hso omap3_isp atmel_mxt_ts cp210x pl2303 usbserial 88w8688_wlan 
tun omap_hsmmc
<4>[  252.661590] [<c013e604>] (unwind_backtrace+0x0/0xf0) from 
[<c016f730>] (warn_slowpath_common+0x4c/0x64)
<4>[  252.661621] [<c016f730>] (warn_slowpath_common+0x4c/0x64) from 
[<c016f7c8>] (warn_slowpath_fmt+0x2c/0x3c)
<4>[  252.661621] [<c016f7c8>] (warn_slowpath_fmt+0x2c/0x3c) from 
[<c0295ec8>] (list_del+0x58/0x8c)
<4>[  252.661651] [<c0295ec8>] (list_del+0x58/0x8c) from [<c03ac538>] 
(videobuf_queue_cancel+0x7c/0x120)
<4>[  252.661682] [<c03ac538>] (videobuf_queue_cancel+0x7c/0x120) from 
[<c03ac630>] (videobuf_streamoff+0x54/0x64)
<4>[  252.661682] [<c03ac630>] (videobuf_streamoff+0x54/0x64) from 
[<c03b00c8>] (vidioc_streamoff+0x12c/0x148)
<4>[  252.661712] [<c03b00c8>] (vidioc_streamoff+0x12c/0x148) from 
[<c039f3f0>] (__video_do_ioctl+0x1a08/0x4698)
<4>[  252.661743] [<c039f3f0>] (__video_do_ioctl+0x1a08/0x4698) from 
[<c039d858>] (video_usercopy+0x35c/0x488)
<4>[  252.661743] [<c039d858>] (video_usercopy+0x35c/0x488) from 
[<c039c8e8>] (v4l2_ioctl+0x7c/0x12c)
<4>[  252.661773] [<c039c8e8>] (v4l2_ioctl+0x7c/0x12c) from [<c01f1954>] 
(vfs_ioctl+0x2c/0xac)
<4>[  252.661804] [<c01f1954>] (vfs_ioctl+0x2c/0xac) from [<c01f2008>] 
(do_vfs_ioctl+0x540/0x5a0)
<4>[  252.661804] [<c01f2008>] (do_vfs_ioctl+0x540/0x5a0) from 
[<c01f209c>] (sys_ioctl+0x34/0x54)
<4>[  252.661834] [<c01f209c>] (sys_ioctl+0x34/0x54) from [<c0138f40>] 
(ret_fast_syscall+0x0/0x30)


BR,
Andrei

On 10/08/2012 04:50 PM, Hiremath, Vaibhav wrote:
> On Fri, Oct 05, 2012 at 21:14:25, Andrei Mandychev wrote:
>> If there is a buffer with VIDEOBUF_QUEUED state it won't be deleted properly
>> because the head of queue loses its elements by calling INIT_LIST_HEAD()
>> before videobuf_streamoff().
> "dma_queue" is driver internal queue and videobuf_streamoff() function
> will end up into buf_release() callback, which in our case doesn't do
> anything with dmaqueue.
>
>
> Did you face any runtime issues with this? I still did not understand
> about this corruption thing.
>
> Thanks,
> Vaibhav
>> ---
>>   drivers/media/video/omap/omap_vout.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
>> index 409da0f..f02eb8e 100644
>> --- a/drivers/media/video/omap/omap_vout.c
>> +++ b/drivers/media/video/omap/omap_vout.c
>> @@ -1738,8 +1738,8 @@ static int vidioc_streamoff(struct file *file, void *fh, enum v4l2_buf_type i)
>>   		v4l2_err(&vout->vid_dev->v4l2_dev, "failed to change mode in"
>>   				" streamoff\n");
>>   
>> -	INIT_LIST_HEAD(&vout->dma_queue);
>>   	ret = videobuf_streamoff(&vout->vbq);
>> +	INIT_LIST_HEAD(&vout->dma_queue);
>>   
>>   	return ret;
>>   }
>> -- 
>> 1.7.9.5
>>
>>

--
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
Vaibhav Hiremath Oct. 9, 2012, 5:32 p.m. UTC | #3 
On Tue, Oct 09, 2012 at 14:08:18, Andrey Mandychev wrote:
> Hi,
> 
> Please find below some additional comments.
> 
> Actually it's not a real issue. It's more warning than issue. When you
> are trying to delete element from the queue the implementation of
> list_del (in list_debug.c) checks that previous element and next element
> of the element you are going to delete reference to this element
> properly. In other words the method checks the integrity of the queue
> before deleting the element and it generates warning if something is
> wrong. In my case the head looses a pointer to the next element because
> of INIT_LIST_HEAD() 
> and when we try to delete this element from the
> queue the list_del() generates warning because the previous element
> (i.e. head) doesn't reference to this element (element we want to
> delete).
> 
> void __list_del_entry(struct list_head *entry)
> {
>     struct list_head *prev, *next;
> 
>     prev = entry->prev;
>     next = entry->next;
> 
>     if (WARN(next == LIST_POISON1,
>         "list_del corruption, %p->next is LIST_POISON1 (%p)\n",
>         entry, LIST_POISON1) ||
>         WARN(prev == LIST_POISON2,
>         "list_del corruption, %p->prev is LIST_POISON2 (%p)\n",
>         entry, LIST_POISON2) ||
>         WARN(prev->next != entry,
>         "list_del corruption. prev->next should be %p, "
>         "but was %p\n", entry, prev->next) ||
>         WARN(next->prev != entry,
>         "list_del corruption. next->prev should be %p, "
>         "but was %p\n", entry, next->prev))
>         return;
> 
>     __list_del(prev, next);
> }
> 
> So my patch is a small improvement that avoids generating this kind of
> warning.
> 

Any mechanism or suggestion to reproduce this issue, which I can 
use to reproduce this issue. Just switching between LCD<=>TV, will be enough 
to hit this issue?

Thanks,
Vaibhav
> --
> BR,
> Andrei
> 
> 
> On Mon, Oct 8, 2012 at 4:50 PM, Hiremath, Vaibhav <hvaibhav@ti.com>
> wrote:
> 
> 
> 	On Fri, Oct 05, 2012 at 21:14:25, Andrei Mandychev wrote:
> 	> If there is a buffer with VIDEOBUF_QUEUED state it won't be
> deleted properly
> 	> because the head of queue loses its elements by calling
> INIT_LIST_HEAD()
> 	> before videobuf_streamoff().
> 	
> 	
> 	"dma_queue" is driver internal queue and videobuf_streamoff()
> function
> 	will end up into buf_release() callback, which in our case
> doesn't do
> 	anything with dmaqueue.
> 	
> 	
> 	Did you face any runtime issues with this? I still did not
> understand
> 	about this corruption thing.
> 	
> 	Thanks,
> 	Vaibhav
> 	
> 	> ---
> 	>  drivers/media/video/omap/omap_vout.c |    2 +-
> 	>  1 file changed, 1 insertion(+), 1 deletion(-)
> 	>
> 	> diff --git a/drivers/media/video/omap/omap_vout.c
> b/drivers/media/video/omap/omap_vout.c
> 	> index 409da0f..f02eb8e 100644
> 	> --- a/drivers/media/video/omap/omap_vout.c
> 	> +++ b/drivers/media/video/omap/omap_vout.c
> 	> @@ -1738,8 +1738,8 @@ static int vidioc_streamoff(struct file
> *file, void *fh, enum v4l2_buf_type i)
> 	>               v4l2_err(&vout->vid_dev->v4l2_dev, "failed to
> change mode in"
> 	>                               " streamoff\n");
> 	>
> 	> -     INIT_LIST_HEAD(&vout->dma_queue);
> 	>       ret = videobuf_streamoff(&vout->vbq);
> 	> +     INIT_LIST_HEAD(&vout->dma_queue);
> 	>
> 	>       return ret;
> 	>  }
> 	> --
> 	> 1.7.9.5
> 	>
> 	>
> 	
> 	
> 
> 
> 

--
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
Mauro Carvalho Chehab Oct. 25, 2012, 1:34 p.m. UTC | #4 
Hi Vaibhav/Andrei,

Em Tue, 9 Oct 2012 17:32:25 +0000
"Hiremath, Vaibhav" <hvaibhav@ti.com> escreveu:

> On Tue, Oct 09, 2012 at 14:08:18, Andrey Mandychev wrote:
> > Hi,
> > 
> > Please find below some additional comments.
> > 
> > Actually it's not a real issue. It's more warning than issue. When you
> > are trying to delete element from the queue the implementation of
> > list_del (in list_debug.c) checks that previous element and next element
> > of the element you are going to delete reference to this element
> > properly. In other words the method checks the integrity of the queue
> > before deleting the element and it generates warning if something is
> > wrong. In my case the head looses a pointer to the next element because
> > of INIT_LIST_HEAD() 
> > and when we try to delete this element from the
> > queue the list_del() generates warning because the previous element
> > (i.e. head) doesn't reference to this element (element we want to
> > delete).
> > 
> > void __list_del_entry(struct list_head *entry)
> > {
> >     struct list_head *prev, *next;
> > 
> >     prev = entry->prev;
> >     next = entry->next;
> > 
> >     if (WARN(next == LIST_POISON1,
> >         "list_del corruption, %p->next is LIST_POISON1 (%p)\n",
> >         entry, LIST_POISON1) ||
> >         WARN(prev == LIST_POISON2,
> >         "list_del corruption, %p->prev is LIST_POISON2 (%p)\n",
> >         entry, LIST_POISON2) ||
> >         WARN(prev->next != entry,
> >         "list_del corruption. prev->next should be %p, "
> >         "but was %p\n", entry, prev->next) ||
> >         WARN(next->prev != entry,
> >         "list_del corruption. next->prev should be %p, "
> >         "but was %p\n", entry, next->prev))
> >         return;
> > 
> >     __list_del(prev, next);
> > }
> > 
> > So my patch is a small improvement that avoids generating this kind of
> > warning.
> > 
> 
> Any mechanism or suggestion to reproduce this issue, which I can 
> use to reproduce this issue. Just switching between LCD<=>TV, will be enough 
> to hit this issue?
> 
> Thanks,
> Vaibhav
> > --
> > BR,
> > Andrei
> > 
> > 
> > On Mon, Oct 8, 2012 at 4:50 PM, Hiremath, Vaibhav <hvaibhav@ti.com>
> > wrote:
> > 
> > 
> > 	On Fri, Oct 05, 2012 at 21:14:25, Andrei Mandychev wrote:
> > 	> If there is a buffer with VIDEOBUF_QUEUED state it won't be
> > deleted properly
> > 	> because the head of queue loses its elements by calling
> > INIT_LIST_HEAD()
> > 	> before videobuf_streamoff().
> > 	
> > 	
> > 	"dma_queue" is driver internal queue and videobuf_streamoff()
> > function
> > 	will end up into buf_release() callback, which in our case
> > doesn't do
> > 	anything with dmaqueue.
> > 	
> > 	
> > 	Did you face any runtime issues with this? I still did not
> > understand
> > 	about this corruption thing.
> > 	
> > 	Thanks,
> > 	Vaibhav
> > 	
> > 	> ---
> > 	>  drivers/media/video/omap/omap_vout.c |    2 +-
> > 	>  1 file changed, 1 insertion(+), 1 deletion(-)
> > 	>
> > 	> diff --git a/drivers/media/video/omap/omap_vout.c
> > b/drivers/media/video/omap/omap_vout.c
> > 	> index 409da0f..f02eb8e 100644
> > 	> --- a/drivers/media/video/omap/omap_vout.c
> > 	> +++ b/drivers/media/video/omap/omap_vout.c
> > 	> @@ -1738,8 +1738,8 @@ static int vidioc_streamoff(struct file
> > *file, void *fh, enum v4l2_buf_type i)
> > 	>               v4l2_err(&vout->vid_dev->v4l2_dev, "failed to
> > change mode in"
> > 	>                               " streamoff\n");
> > 	>
> > 	> -     INIT_LIST_HEAD(&vout->dma_queue);
> > 	>       ret = videobuf_streamoff(&vout->vbq);
> > 	> +     INIT_LIST_HEAD(&vout->dma_queue);

Why do we ever need to call INIT_LIST_HEAD() here in the first place?

List initialization should happen only once, when vout is created.
After that, list add/del macros should be used.

Having a code like this here seems to indicate that there are something
wrong somewhere.

Regards,
Mauro
--
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
diff mbox

Patch

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index 409da0f..f02eb8e 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -1738,8 +1738,8 @@  static int vidioc_streamoff(struct file *file, void *fh, enum v4l2_buf_type i)
 		v4l2_err(&vout->vid_dev->v4l2_dev, "failed to change mode in"
 				" streamoff\n");
 
-	INIT_LIST_HEAD(&vout->dma_queue);
 	ret = videobuf_streamoff(&vout->vbq);
+	INIT_LIST_HEAD(&vout->dma_queue);
 
 	return ret;
 }