videobuf: Initialize lists in videobuf_buffer.

PATCH In review [email protected] 4 participants 9 messages Nov 16, 2010 – Nov 17, 2010 1 message not in the archive Media (V4L/DVB)

lore

Share & tools

lore

[email protected]

author

2010-11-16 20:27 UTC #0 lore raw

PATCH 1/1 videobuf: Initialize lists in videobuf_buffer.

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(-)

drivers/media/video/videobuf-dma-contig.c +2 -0

index c969111..f7e0f86 100644
@@ -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;

drivers/media/video/videobuf-dma-sg.c +2 -0

index 20f227e..5af3217 100644
@@ -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);

drivers/media/video/videobuf-vmalloc.c +2 -0

index df14258..8babedd 100644
@@ -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),

Signature · 1 line

1.7.0.4

Figo.zhang

2010-11-17 01:11 UTC #1 lore raw

drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer *__videobuf_alloc_vb(size_t size) · line 197

Quoted 12 lines

drivers/media/video/videobuf-dma-contig.c +2 -0

index c969111..f7e0f86 100644
@@ -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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Figo.zhang

2010-11-17 01:40 UTC #3 lore raw

Quoted 15 lines · Andrew Chew

Quoted 7 lines
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/media/video/videobuf-dma-contig.c
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer
Quoted 4 lines
index c969111..f7e0f86 100644
drivers/media/video/videobuf-dma-contig.c +0 -0
@@ -193,6 +193,8 @@ static struct videobuf_buffer
*__videobuf_alloc_vb(size_t size)
Quoted 7 lines
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer *__videobuf_alloc_vb(size_t size) · line 197
Quoted 5 lines
drivers/media/video/videobuf-dma-contig.c +2 -0
   	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.

Quoted 3 lines

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.

Hans Verkuil

2010-11-17 07:11 UTC #4 lore raw

In reply to a message not in the archive

Quoted 15 lines · On Wednesday, November 17, 2010 02:38:09 Andrew Chew wrote:

Quoted 7 lines
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/media/video/videobuf-dma-contig.c
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer
Quoted 4 lines
index c969111..f7e0f86 100644
drivers/media/video/videobuf-dma-contig.c +0 -0
@@ -193,6 +193,8 @@ static struct videobuf_buffer 
*__videobuf_alloc_vb(size_t size)
Quoted 7 lines
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer *__videobuf_alloc_vb(size_t size) · line 197
Quoted 5 lines
drivers/media/video/videobuf-dma-contig.c +2 -0
  	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.

Quoted 4 lines

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

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by Cisco

Figo.zhang

2010-11-17 07:17 UTC #5 lore raw

Quoted 16 lines · Hans Verkuil

Quoted 15 lines · On Wednesday, November 17, 2010 02:38:09 Andrew Chew wrote:
Quoted 14 lines
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/media/video/videobuf-dma-contig.c
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer
Quoted 4 lines
index c969111..f7e0f86 100644
drivers/media/video/videobuf-dma-contig.c +0 -0
@@ -193,6 +193,8 @@ static struct videobuf_buffer
*__videobuf_alloc_vb(size_t size)
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer *__videobuf_alloc_vb(size_t size) · line 197
Quoted 5 lines
drivers/media/video/videobuf-dma-contig.c +2 -0
   	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.

Quoted 7 lines

Quoted 4 lines
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.

Hans Verkuil

2010-11-17 07:53 UTC #6 lore raw

Quoted 25 lines · On Wednesday, November 17, 2010 08:16:27 Figo.zhang wrote:

Quoted 16 lines · Hans Verkuil
Quoted 15 lines · On Wednesday, November 17, 2010 02:38:09 Andrew Chew wrote:
Quoted 14 lines
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/media/video/videobuf-dma-contig.c
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer
Quoted 4 lines
index c969111..f7e0f86 100644
drivers/media/video/videobuf-dma-contig.c +0 -0
@@ -193,6 +193,8 @@ static struct videobuf_buffer
*__videobuf_alloc_vb(size_t size)
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer *__videobuf_alloc_vb(size_t size) · line 197
Quoted 5 lines
drivers/media/video/videobuf-dma-contig.c +2 -0
   	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.
Quoted 7 lines
Quoted 4 lines
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.

Quoted 7 lines

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

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by Cisco

Laurent Pinchart

2010-11-17 12:37 UTC #7 lore raw

In reply to Hans Verkuil

Hi Hans,

Quoted 28 lines · On Wednesday 17 November 2010 08:11:06 Hans Verkuil wrote:

Quoted 27 lines · On Wednesday, November 17, 2010 02:38:09 Andrew Chew wrote:
Quoted 13 lines
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/media/video/videobuf-dma-contig.c
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer
Quoted 4 lines
index c969111..f7e0f86 100644
drivers/media/video/videobuf-dma-contig.c +0 -0
@@ -193,6 +193,8 @@ static struct videobuf_buffer
*__videobuf_alloc_vb(size_t size)
Quoted 2 lines
drivers/media/video/videobuf-dma-contig.c +0 -0
  	if (vb) {
 
Quoted 7 lines
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer *__videobuf_alloc_vb(size_t size) · line 197
Quoted 5 lines
drivers/media/video/videobuf-dma-contig.c +2 -0
  		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.

Quoted 5 lines

Quoted 1 line
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.

Signature · 3 lines

Regards,

Laurent Pinchart

Hans Verkuil

2010-11-17 12:45 UTC #8 lore raw

Quoted 35 lines

Hi Hans,
Quoted 31 lines · On Wednesday 17 November 2010 08:11:06 Hans Verkuil wrote:
Quoted 16 lines · On Wednesday, November 17, 2010 02:38:09 Andrew Chew wrote:
Quoted 16 lines
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/media/video/videobuf-dma-contig.c
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer
Quoted 4 lines
index c969111..f7e0f86 100644
drivers/media/video/videobuf-dma-contig.c +0 -0
@@ -193,6 +193,8 @@ static struct videobuf_buffer
*__videobuf_alloc_vb(size_t size)
Quoted 5 lines
drivers/media/video/videobuf-dma-contig.c +0 -0
  	if (vb) {
 
  		mem = vb->priv = ((char *)vb) + size;
  		mem->magic = MAGIC_DC_MEM;
 
Quoted 7 lines
Quoted 4 lines
drivers/media/video/videobuf-dma-contig.c static struct videobuf_buffer · line 199
Quoted 2 lines
drivers/media/video/videobuf-dma-contig.c +2 -0
+		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
Quoted 5 lines
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?

Quoted 8 lines

Quoted 4 lines
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

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by Cisco

Hans Verkuil

2010-11-17 07:13 UTC #9 lore raw

In reply to [email protected]

Quoted 9 lines · 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

Quoted 52 lines

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(-)

drivers/media/video/videobuf-dma-contig.c +2 -0

index c969111..f7e0f86 100644
@@ -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;

drivers/media/video/videobuf-dma-sg.c +2 -0

index 20f227e..5af3217 100644
@@ -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);

drivers/media/video/videobuf-vmalloc.c +2 -0

index df14258..8babedd 100644
@@ -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),

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by Cisco