From: Andrew Chew <[email protected]>
There are two struct list_head's in struct videobuf_buffer.
Prior to this fix, all we did for initialization of struct videobuf_buffer
was to zero out its memory. This does not properly initialize this struct's
two list_head members.
This patch immediately calls INIT_LIST_HEAD on both lists after the kzalloc,
so that the two lists are initialized properly.
Signed-off-by: Andrew Chew <[email protected]>
---
I thought I'd submit a patch for this anyway. Without this, the existing
camera host drivers will spew an ugly warning on every videobuf allocation,
which gets annoying really fast.
drivers/media/video/videobuf-dma-contig.c | 2 ++
drivers/media/video/videobuf-dma-sg.c | 2 ++
drivers/media/video/videobuf-vmalloc.c | 2 ++
3 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/videobuf-dma-contig.c b/drivers/media/video/videobuf-dma-contig.c
index c969111..f7e0f86 100644
--- a/drivers/media/video/videobuf-dma-contig.c
+++ b/drivers/media/video/videobuf-dma-contig.c
@@ -193,6 +193,8 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size)
if (vb) {
mem = vb->priv = ((char *)vb) + size;
mem->magic = MAGIC_DC_MEM;
+ INIT_LIST_HEAD(&vb->stream);
+ INIT_LIST_HEAD(&vb->queue);
}
return vb;
diff --git a/drivers/media/video/videobuf-dma-sg.c b/drivers/media/video/videobuf-dma-sg.c
index 20f227e..5af3217 100644
--- a/drivers/media/video/videobuf-dma-sg.c
+++ b/drivers/media/video/videobuf-dma-sg.c
@@ -430,6 +430,8 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size)
mem = vb->priv = ((char *)vb) + size;
mem->magic = MAGIC_SG_MEM;
+ INIT_LIST_HEAD(&vb->stream);
+ INIT_LIST_HEAD(&vb->queue);
videobuf_dma_init(&mem->dma);
diff --git a/drivers/media/video/videobuf-vmalloc.c b/drivers/media/video/videobuf-vmalloc.c
index df14258..8babedd 100644
--- a/drivers/media/video/videobuf-vmalloc.c
+++ b/drivers/media/video/videobuf-vmalloc.c
@@ -146,6 +146,8 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size)
mem = vb->priv = ((char *)vb) + size;
mem->magic = MAGIC_VMAL_MEM;
+ INIT_LIST_HEAD(&vb->stream);
+ INIT_LIST_HEAD(&vb->queue);
dprintk(1, "%s: allocated at %p(%ld+%ld) & %p(%ld)\n",
__func__, vb, (long)sizeof(*vb), (long)size - sizeof(*vb),
--
1.7.0.4
>
> diff --git a/drivers/media/video/videobuf-dma-contig.c b/drivers/media/video/videobuf-dma-contig.c
> index c969111..f7e0f86 100644
> --- a/drivers/media/video/videobuf-dma-contig.c
> +++ b/drivers/media/video/videobuf-dma-contig.c
> @@ -193,6 +193,8 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size)
> if (vb) {
> mem = vb->priv = ((char *)vb) + size;
> mem->magic = MAGIC_DC_MEM;
> + INIT_LIST_HEAD(&vb->stream);
> + INIT_LIST_HEAD(&vb->queue);
i think it no need to be init, it just a list-entry.
δΊ 11/17/2010 09:38 AM, Andrew Chew ει:
>>> diff --git a/drivers/media/video/videobuf-dma-contig.c
>> b/drivers/media/video/videobuf-dma-contig.c
>>> index c969111..f7e0f86 100644
>>> --- a/drivers/media/video/videobuf-dma-contig.c
>>> +++ b/drivers/media/video/videobuf-dma-contig.c
>>> @@ -193,6 +193,8 @@ static struct videobuf_buffer
>> *__videobuf_alloc_vb(size_t size)
>>> if (vb) {
>>> mem = vb->priv = ((char *)vb) + size;
>>> mem->magic = MAGIC_DC_MEM;
>>> + INIT_LIST_HEAD(&vb->stream);
>>> + INIT_LIST_HEAD(&vb->queue);
>>
>> i think it no need to be init, it just a list-entry.
>
> Okay, if that's really the case, then sh_mobile_ceu_camera.c, pxa_camera.c, mx1_camera.c, mx2_camera.c, and omap1_camera.c needs to be fixed to remove that WARN_ON(!list_empty(&vb->queue)); in their videobuf_prepare() methods, because those WARN_ON's are assuming that vb->queue is properly initialized as a list head.
>
> Which will it be?
yes, i think those WARN_ONs are no need.
On Wednesday, November 17, 2010 02:38:09 Andrew Chew wrote:
> > > diff --git a/drivers/media/video/videobuf-dma-contig.c
> > b/drivers/media/video/videobuf-dma-contig.c
> > > index c969111..f7e0f86 100644
> > > --- a/drivers/media/video/videobuf-dma-contig.c
> > > +++ b/drivers/media/video/videobuf-dma-contig.c
> > > @@ -193,6 +193,8 @@ static struct videobuf_buffer
> > *__videobuf_alloc_vb(size_t size)
> > > if (vb) {
> > > mem = vb->priv = ((char *)vb) + size;
> > > mem->magic = MAGIC_DC_MEM;
> > > + INIT_LIST_HEAD(&vb->stream);
> > > + INIT_LIST_HEAD(&vb->queue);
> >
> > i think it no need to be init, it just a list-entry.
>
> Okay, if that's really the case, then sh_mobile_ceu_camera.c, pxa_camera.c, mx1_camera.c, mx2_camera.c, and omap1_camera.c needs to be fixed to remove that WARN_ON(!list_empty(&vb->queue)); in their videobuf_prepare() methods, because those WARN_ON's are assuming that vb->queue is properly initialized as a list head.
>
> Which will it be?
>
These list entries need to be inited. It is bad form to have uninitialized
list entries. It is not as if this is a big deal to initialize them properly.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
On Tuesday, November 16, 2010 21:24:43 [email protected] wrote:
> From: Andrew Chew <[email protected]>
>
> There are two struct list_head's in struct videobuf_buffer.
> Prior to this fix, all we did for initialization of struct videobuf_buffer
> was to zero out its memory. This does not properly initialize this struct's
> two list_head members.
>
> This patch immediately calls INIT_LIST_HEAD on both lists after the kzalloc,
> so that the two lists are initialized properly.
Rather than doing this for all videobuf variants I would suggest that you
do this in videobuf-core.c, videobuf_alloc_vb().
Regards,
Hans
>
> Signed-off-by: Andrew Chew <[email protected]>
> ---
> I thought I'd submit a patch for this anyway. Without this, the existing
> camera host drivers will spew an ugly warning on every videobuf allocation,
> which gets annoying really fast.
>
> drivers/media/video/videobuf-dma-contig.c | 2 ++
> drivers/media/video/videobuf-dma-sg.c | 2 ++
> drivers/media/video/videobuf-vmalloc.c | 2 ++
> 3 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/videobuf-dma-contig.c b/drivers/media/video/videobuf-dma-contig.c
> index c969111..f7e0f86 100644
> --- a/drivers/media/video/videobuf-dma-contig.c
> +++ b/drivers/media/video/videobuf-dma-contig.c
> @@ -193,6 +193,8 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size)
> if (vb) {
> mem = vb->priv = ((char *)vb) + size;
> mem->magic = MAGIC_DC_MEM;
> + INIT_LIST_HEAD(&vb->stream);
> + INIT_LIST_HEAD(&vb->queue);
> }
>
> return vb;
> diff --git a/drivers/media/video/videobuf-dma-sg.c b/drivers/media/video/videobuf-dma-sg.c
> index 20f227e..5af3217 100644
> --- a/drivers/media/video/videobuf-dma-sg.c
> +++ b/drivers/media/video/videobuf-dma-sg.c
> @@ -430,6 +430,8 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size)
>
> mem = vb->priv = ((char *)vb) + size;
> mem->magic = MAGIC_SG_MEM;
> + INIT_LIST_HEAD(&vb->stream);
> + INIT_LIST_HEAD(&vb->queue);
>
> videobuf_dma_init(&mem->dma);
>
> diff --git a/drivers/media/video/videobuf-vmalloc.c b/drivers/media/video/videobuf-vmalloc.c
> index df14258..8babedd 100644
> --- a/drivers/media/video/videobuf-vmalloc.c
> +++ b/drivers/media/video/videobuf-vmalloc.c
> @@ -146,6 +146,8 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size)
>
> mem = vb->priv = ((char *)vb) + size;
> mem->magic = MAGIC_VMAL_MEM;
> + INIT_LIST_HEAD(&vb->stream);
> + INIT_LIST_HEAD(&vb->queue);
>
> dprintk(1, "%s: allocated at %p(%ld+%ld) & %p(%ld)\n",
> __func__, vb, (long)sizeof(*vb), (long)size - sizeof(*vb),
>
--
Hans Verkuil - video4linux developer - sponsored by Cisco
On 11/17/2010 03:11 PM, Hans Verkuil wrote:
> On Wednesday, November 17, 2010 02:38:09 Andrew Chew wrote:
>>>> diff --git a/drivers/media/video/videobuf-dma-contig.c
>>> b/drivers/media/video/videobuf-dma-contig.c
>>>> index c969111..f7e0f86 100644
>>>> --- a/drivers/media/video/videobuf-dma-contig.c
>>>> +++ b/drivers/media/video/videobuf-dma-contig.c
>>>> @@ -193,6 +193,8 @@ static struct videobuf_buffer
>>> *__videobuf_alloc_vb(size_t size)
>>>> if (vb) {
>>>> mem = vb->priv = ((char *)vb) + size;
>>>> mem->magic = MAGIC_DC_MEM;
>>>> + INIT_LIST_HEAD(&vb->stream);
>>>> + INIT_LIST_HEAD(&vb->queue);
>>>
>>> i think it no need to be init, it just a list-entry.
>>
>> Okay, if that's really the case, then sh_mobile_ceu_camera.c, pxa_camera.c, mx1_camera.c, mx2_camera.c, and omap1_camera.c needs to be fixed to remove that WARN_ON(!list_empty(&vb->queue)); in their videobuf_prepare() methods, because those WARN_ON's are assuming that vb->queue is properly initialized as a list head.
>>
>> Which will it be?
>>
>
> These list entries need to be inited. It is bad form to have uninitialized
> list entries. It is not as if this is a big deal to initialize them properly.
in kernel source code, list entry are not often to be inited.
for example, see mm/vmscan.c register_shrinker(), no one init the
shrinker->list.
another example: see mm/swapfile.c add_swap_extent(), no one init the
new_se->list.
On Wednesday, November 17, 2010 08:16:27 Figo.zhang wrote:
> On 11/17/2010 03:11 PM, Hans Verkuil wrote:
> > On Wednesday, November 17, 2010 02:38:09 Andrew Chew wrote:
> >>>> diff --git a/drivers/media/video/videobuf-dma-contig.c
> >>> b/drivers/media/video/videobuf-dma-contig.c
> >>>> index c969111..f7e0f86 100644
> >>>> --- a/drivers/media/video/videobuf-dma-contig.c
> >>>> +++ b/drivers/media/video/videobuf-dma-contig.c
> >>>> @@ -193,6 +193,8 @@ static struct videobuf_buffer
> >>> *__videobuf_alloc_vb(size_t size)
> >>>> if (vb) {
> >>>> mem = vb->priv = ((char *)vb) + size;
> >>>> mem->magic = MAGIC_DC_MEM;
> >>>> + INIT_LIST_HEAD(&vb->stream);
> >>>> + INIT_LIST_HEAD(&vb->queue);
> >>>
> >>> i think it no need to be init, it just a list-entry.
> >>
> >> Okay, if that's really the case, then sh_mobile_ceu_camera.c, pxa_camera.c, mx1_camera.c, mx2_camera.c, and omap1_camera.c needs to be fixed to remove that WARN_ON(!list_empty(&vb->queue)); in their videobuf_prepare() methods, because those WARN_ON's are assuming that vb->queue is properly initialized as a list head.
> >>
> >> Which will it be?
> >>
> >
> > These list entries need to be inited. It is bad form to have uninitialized
> > list entries. It is not as if this is a big deal to initialize them properly.
>
> in kernel source code, list entry are not often to be inited.
>
> for example, see mm/vmscan.c register_shrinker(), no one init the
> shrinker->list.
>
> another example: see mm/swapfile.c add_swap_extent(), no one init the
> new_se->list.
I have to think some more about this. I'll get back to this today. BTW, I do
agree that the WARN_ON's are bogus and should be removed.
And BTW, this isn't going to work either (mx1_camera.c):
static int mx1_camera_setup_dma(struct mx1_camera_dev *pcdev)
{
struct videobuf_buffer *vbuf = &pcdev->active->vb;
struct device *dev = pcdev->icd->dev.parent;
int ret;
if (unlikely(!pcdev->active)) {
dev_err(dev, "DMA End IRQ with no active buffer\n");
return -EFAULT;
}
The vbuf assignment should be moved after the 'if'.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
> Hi Hans,
>
> On Wednesday 17 November 2010 08:11:06 Hans Verkuil wrote:
>> On Wednesday, November 17, 2010 02:38:09 Andrew Chew wrote:
>> > > > diff --git a/drivers/media/video/videobuf-dma-contig.c
>> > >
>> > > b/drivers/media/video/videobuf-dma-contig.c
>> > >
>> > > > index c969111..f7e0f86 100644
>> > > > --- a/drivers/media/video/videobuf-dma-contig.c
>> > > > +++ b/drivers/media/video/videobuf-dma-contig.c
>> > > > @@ -193,6 +193,8 @@ static struct videobuf_buffer
>> > >
>> > > *__videobuf_alloc_vb(size_t size)
>> > >
>> > > > if (vb) {
>> > > >
>> > > > mem = vb->priv = ((char *)vb) + size;
>> > > > mem->magic = MAGIC_DC_MEM;
>> > > >
>> > > > + INIT_LIST_HEAD(&vb->stream);
>> > > > + INIT_LIST_HEAD(&vb->queue);
>> > >
>> > > i think it no need to be init, it just a list-entry.
>> >
>> > Okay, if that's really the case, then sh_mobile_ceu_camera.c,
>> > pxa_camera.c, mx1_camera.c, mx2_camera.c, and omap1_camera.c needs to
>> be
>> > fixed to remove that WARN_ON(!list_empty(&vb->queue)); in their
>> > videobuf_prepare() methods, because those WARN_ON's are assuming that
>> > vb->queue is properly initialized as a list head.
>> >
>> > Which will it be?
>>
>> These list entries need to be inited. It is bad form to have
>> uninitialized
>> list entries. It is not as if this is a big deal to initialize them
>> properly.
>
> I disagree with that. List heads must be initialized, but there's no point
> in
> initializing list entries.
You are right. I got confused due to some problems I had in the past in
another driver, but it turned out to be a list header that caused the
problems, not a list entry. So removing the bogus WARN_ONs is sufficient.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
Hi Hans,
On Wednesday 17 November 2010 08:11:06 Hans Verkuil wrote:
> On Wednesday, November 17, 2010 02:38:09 Andrew Chew wrote:
> > > > diff --git a/drivers/media/video/videobuf-dma-contig.c
> > >
> > > b/drivers/media/video/videobuf-dma-contig.c
> > >
> > > > index c969111..f7e0f86 100644
> > > > --- a/drivers/media/video/videobuf-dma-contig.c
> > > > +++ b/drivers/media/video/videobuf-dma-contig.c
> > > > @@ -193,6 +193,8 @@ static struct videobuf_buffer
> > >
> > > *__videobuf_alloc_vb(size_t size)
> > >
> > > > if (vb) {
> > > >
> > > > mem = vb->priv = ((char *)vb) + size;
> > > > mem->magic = MAGIC_DC_MEM;
> > > >
> > > > + INIT_LIST_HEAD(&vb->stream);
> > > > + INIT_LIST_HEAD(&vb->queue);
> > >
> > > i think it no need to be init, it just a list-entry.
> >
> > Okay, if that's really the case, then sh_mobile_ceu_camera.c,
> > pxa_camera.c, mx1_camera.c, mx2_camera.c, and omap1_camera.c needs to be
> > fixed to remove that WARN_ON(!list_empty(&vb->queue)); in their
> > videobuf_prepare() methods, because those WARN_ON's are assuming that
> > vb->queue is properly initialized as a list head.
> >
> > Which will it be?
>
> These list entries need to be inited. It is bad form to have uninitialized
> list entries. It is not as if this is a big deal to initialize them
> properly.
I disagree with that. List heads must be initialized, but there's no point in
initializing list entries.
--
Regards,
Laurent Pinchart