* [PATCH] Fixed list_del corruption in videobuf-core.c : videobuf_queue_cancel()
@ 2012-10-05 15:44 Andrei Mandychev
2012-10-08 14:50 ` Hiremath, Vaibhav
0 siblings, 1 reply; 5+ messages in thread
From: Andrei Mandychev @ 2012-10-05 15:44 UTC (permalink / raw)
To: hvaibhav
Cc: archit, tomi.valkeinen, sumit.semwal, linux-media, Andrei Mandychev
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH] Fixed list_del corruption in videobuf-core.c : videobuf_queue_cancel()
2012-10-05 15:44 [PATCH] Fixed list_del corruption in videobuf-core.c : videobuf_queue_cancel() Andrei Mandychev
@ 2012-10-08 14:50 ` Hiremath, Vaibhav
2012-10-08 15:39 ` Andrei Mandychev
[not found] ` <CAH9bG+Cp8gURyZ=cc3doCd_TR2CzLMrcKSGMKpe55jmCNYr+KQ@mail.gmail.com>
0 siblings, 2 replies; 5+ messages in thread
From: Hiremath, Vaibhav @ 2012-10-08 14:50 UTC (permalink / raw)
To: Andrei Mandychev
Cc: Taneja, Archit, Valkeinen, Tomi, Semwal, Sumit, linux-media,
Andrei Mandychev
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fixed list_del corruption in videobuf-core.c : videobuf_queue_cancel()
2012-10-08 14:50 ` Hiremath, Vaibhav
@ 2012-10-08 15:39 ` Andrei Mandychev
[not found] ` <CAH9bG+Cp8gURyZ=cc3doCd_TR2CzLMrcKSGMKpe55jmCNYr+KQ@mail.gmail.com>
1 sibling, 0 replies; 5+ messages in thread
From: Andrei Mandychev @ 2012-10-08 15:39 UTC (permalink / raw)
To: Hiremath, Vaibhav
Cc: Taneja, Archit, Valkeinen, Tomi, Semwal, Sumit, linux-media
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
>>
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] Fixed list_del corruption in videobuf-core.c : videobuf_queue_cancel()
[not found] ` <CAH9bG+Cp8gURyZ=cc3doCd_TR2CzLMrcKSGMKpe55jmCNYr+KQ@mail.gmail.com>
@ 2012-10-09 17:32 ` Hiremath, Vaibhav
2012-10-25 13:34 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 5+ messages in thread
From: Hiremath, Vaibhav @ 2012-10-09 17:32 UTC (permalink / raw)
To: Andrey Mandychev
Cc: Taneja, Archit, Valkeinen, Tomi, Semwal, Sumit, linux-media,
Andrei Mandychev
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
> >
> >
>
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fixed list_del corruption in videobuf-core.c : videobuf_queue_cancel()
2012-10-09 17:32 ` Hiremath, Vaibhav
@ 2012-10-25 13:34 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 5+ messages in thread
From: Mauro Carvalho Chehab @ 2012-10-25 13:34 UTC (permalink / raw)
To: Hiremath, Vaibhav
Cc: Andrey Mandychev, Taneja, Archit, Valkeinen, Tomi, Semwal, Sumit,
linux-media, Andrei Mandychev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-10-25 13:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-05 15:44 [PATCH] Fixed list_del corruption in videobuf-core.c : videobuf_queue_cancel() Andrei Mandychev
2012-10-08 14:50 ` Hiremath, Vaibhav
2012-10-08 15:39 ` Andrei Mandychev
[not found] ` <CAH9bG+Cp8gURyZ=cc3doCd_TR2CzLMrcKSGMKpe55jmCNYr+KQ@mail.gmail.com>
2012-10-09 17:32 ` Hiremath, Vaibhav
2012-10-25 13:34 ` Mauro Carvalho Chehab
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.