* [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer. @ 2010-11-16 20:24 achew 2010-11-17 1:10 ` Figo.zhang 2010-11-17 7:13 ` Hans Verkuil 0 siblings, 2 replies; 9+ messages in thread From: achew @ 2010-11-16 20:24 UTC (permalink / raw) To: zhangtianfei, hverkuil, pawel; +Cc: linux-media, linux-kernel, Andrew Chew 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer. 2010-11-16 20:24 [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer achew @ 2010-11-17 1:10 ` Figo.zhang [not found] ` <643E69AA4436674C8F39DCC2C05F763816BB828A40@HQMAIL03.nvidia.com> 2010-11-17 7:13 ` Hans Verkuil 1 sibling, 1 reply; 9+ messages in thread From: Figo.zhang @ 2010-11-17 1:10 UTC (permalink / raw) To: achew; +Cc: hverkuil, pawel, linux-media, linux-kernel > > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <643E69AA4436674C8F39DCC2C05F763816BB828A40@HQMAIL03.nvidia.com>]
* Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer. [not found] ` <643E69AA4436674C8F39DCC2C05F763816BB828A40@HQMAIL03.nvidia.com> @ 2010-11-17 1:39 ` Figo.zhang 2010-11-17 7:11 ` Hans Verkuil 1 sibling, 0 replies; 9+ messages in thread From: Figo.zhang @ 2010-11-17 1:39 UTC (permalink / raw) To: Andrew Chew Cc: hverkuil@xs4all.nl, pawel@osciak.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org 于 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer. [not found] ` <643E69AA4436674C8F39DCC2C05F763816BB828A40@HQMAIL03.nvidia.com> 2010-11-17 1:39 ` Figo.zhang @ 2010-11-17 7:11 ` Hans Verkuil 2010-11-17 7:16 ` Figo.zhang 2010-11-17 12:37 ` Laurent Pinchart 1 sibling, 2 replies; 9+ messages in thread From: Hans Verkuil @ 2010-11-17 7:11 UTC (permalink / raw) To: Andrew Chew Cc: 'Figo.zhang', pawel@osciak.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer. 2010-11-17 7:11 ` Hans Verkuil @ 2010-11-17 7:16 ` Figo.zhang 2010-11-17 7:52 ` Hans Verkuil 2010-11-17 12:37 ` Laurent Pinchart 1 sibling, 1 reply; 9+ messages in thread From: Figo.zhang @ 2010-11-17 7:16 UTC (permalink / raw) To: Hans Verkuil Cc: Andrew Chew, pawel@osciak.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer. 2010-11-17 7:16 ` Figo.zhang @ 2010-11-17 7:52 ` Hans Verkuil 0 siblings, 0 replies; 9+ messages in thread From: Hans Verkuil @ 2010-11-17 7:52 UTC (permalink / raw) To: Figo.zhang Cc: Andrew Chew, pawel@osciak.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer. 2010-11-17 7:11 ` Hans Verkuil 2010-11-17 7:16 ` Figo.zhang @ 2010-11-17 12:37 ` Laurent Pinchart 2010-11-17 12:44 ` Hans Verkuil 1 sibling, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2010-11-17 12:37 UTC (permalink / raw) To: Hans Verkuil Cc: Andrew Chew, 'Figo.zhang', pawel@osciak.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer. 2010-11-17 12:37 ` Laurent Pinchart @ 2010-11-17 12:44 ` Hans Verkuil 0 siblings, 0 replies; 9+ messages in thread From: Hans Verkuil @ 2010-11-17 12:44 UTC (permalink / raw) To: Laurent Pinchart Cc: Andrew Chew, 'Figo.zhang', pawel@osciak.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer. 2010-11-16 20:24 [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer achew 2010-11-17 1:10 ` Figo.zhang @ 2010-11-17 7:13 ` Hans Verkuil 1 sibling, 0 replies; 9+ messages in thread From: Hans Verkuil @ 2010-11-17 7:13 UTC (permalink / raw) To: achew; +Cc: zhangtianfei, pawel, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-11-17 12:47 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-16 20:24 [PATCH 1/1] videobuf: Initialize lists in videobuf_buffer achew
2010-11-17 1:10 ` Figo.zhang
[not found] ` <643E69AA4436674C8F39DCC2C05F763816BB828A40@HQMAIL03.nvidia.com>
2010-11-17 1:39 ` Figo.zhang
2010-11-17 7:11 ` Hans Verkuil
2010-11-17 7:16 ` Figo.zhang
2010-11-17 7:52 ` Hans Verkuil
2010-11-17 12:37 ` Laurent Pinchart
2010-11-17 12:44 ` Hans Verkuil
2010-11-17 7:13 ` Hans Verkuil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).