[PATCH 1/1] videobuf: Initialize lists in videobuf_buffer.

[PATCH 1/1] videobuf: Initialize lists in videobuf_buffer.

[achew]

From: Andrew Chew <achew@nvidia.com>

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 <achew@nvidia.com>
---
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

Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer.

[Figo.zhang]

> 
> 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.

Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer.

[Figo.zhang]

于 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.

Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer.

[Hans Verkuil]

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

Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer.

[Hans Verkuil]

On Tuesday, November 16, 2010 21:24:43 achew@nvidia.com wrote:
> From: Andrew Chew <achew@nvidia.com>
> 
> 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 <achew@nvidia.com>
> ---
> 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

Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer.

[Figo.zhang]

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.

Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer.

[Hans Verkuil]

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

Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer.

[Laurent Pinchart]

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

Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer.

[Hans Verkuil]

> 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