| Message ID | 1349880405-26049-22-git-send-email-t.stanislaws@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomasz, Thanks for the patch. On Wednesday 10 October 2012 16:46:40 Tomasz Stanislawski wrote: > This patch adds taking reference to the device for MMAP buffers. > > Such buffers, may be exported using DMABUF mechanism. If the driver that > created a queue is unloaded then the queue is released, the device might be > released too. However, buffers cannot be released if they are referenced by > DMABUF descriptor(s). The device pointer kept in a buffer must be valid for > the whole buffer's lifetime. Therefore MMAP buffers should take a reference > to the device to avoid risk of dangling pointers. > > Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> But two small comments below. > --- > drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c > b/drivers/media/v4l2-core/videobuf2-dma-contig.c index b138b5c..2d661fd > 100644 > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c > @@ -148,6 +148,7 @@ static void vb2_dc_put(void *buf_priv) > kfree(buf->sgt_base); > } > dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr); > + put_device(buf->dev); > kfree(buf); > } > > @@ -168,6 +169,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long > size) return ERR_PTR(-ENOMEM); > } > > + /* prevent the device from release while the buffer is exported */ s/prevent/Prevent/ ? > + get_device(dev); > + > buf->dev = dev; What about just buf->dev = get_device(dev); > buf->size = size;
Hi Laurent, Thank your your review. On 10/11/2012 11:49 PM, Laurent Pinchart wrote: > Hi Tomasz, > > Thanks for the patch. > > On Wednesday 10 October 2012 16:46:40 Tomasz Stanislawski wrote: >> This patch adds taking reference to the device for MMAP buffers. >> >> Such buffers, may be exported using DMABUF mechanism. If the driver that >> created a queue is unloaded then the queue is released, the device might be >> released too. However, buffers cannot be released if they are referenced by >> DMABUF descriptor(s). The device pointer kept in a buffer must be valid for >> the whole buffer's lifetime. Therefore MMAP buffers should take a reference >> to the device to avoid risk of dangling pointers. >> >> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > But two small comments below. > >> --- >> drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c >> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index b138b5c..2d661fd >> 100644 >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c >> @@ -148,6 +148,7 @@ static void vb2_dc_put(void *buf_priv) >> kfree(buf->sgt_base); >> } >> dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr); >> + put_device(buf->dev); >> kfree(buf); >> } >> >> @@ -168,6 +169,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long >> size) return ERR_PTR(-ENOMEM); >> } >> >> + /* prevent the device from release while the buffer is exported */ > > s/prevent/Prevent/ ? > s/release/being released/ ? >> + get_device(dev); >> + >> buf->dev = dev; > > What about just > > buf->dev = get_device(dev); > Right, sorry I missed that from your previous review :). Regards, Tomasz Stanislawski >> buf->size = size;
Hi Tomasz, On Friday 12 October 2012 08:28:23 Tomasz Stanislawski wrote: > On 10/11/2012 11:49 PM, Laurent Pinchart wrote: > > On Wednesday 10 October 2012 16:46:40 Tomasz Stanislawski wrote: > >> This patch adds taking reference to the device for MMAP buffers. > >> > >> Such buffers, may be exported using DMABUF mechanism. If the driver that > >> created a queue is unloaded then the queue is released, the device might > >> be > >> released too. However, buffers cannot be released if they are referenced > >> by DMABUF descriptor(s). The device pointer kept in a buffer must be > >> valid for the whole buffer's lifetime. Therefore MMAP buffers should > >> take a reference to the device to avoid risk of dangling pointers. > >> > >> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> > >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > But two small comments below. > > > >> --- > >> > >> drivers/media/v4l2-core/videobuf2-dma-contig.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c > >> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index b138b5c..2d661fd > >> 100644 > >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c > >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c > >> @@ -148,6 +148,7 @@ static void vb2_dc_put(void *buf_priv) > >> kfree(buf->sgt_base); > >> } > >> dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr); > >> + put_device(buf->dev); > >> kfree(buf); > >> } > >> > >> @@ -168,6 +169,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned > >> long size) > >> return ERR_PTR(-ENOMEM); > >> } > >> > >> + /* prevent the device from release while the buffer is exported */ > > > > s/prevent/Prevent/ ? > > s/release/being released/ ? Oops. Of course :-) > >> + get_device(dev); > >> + > >> buf->dev = dev; > > > > What about just > > > > buf->dev = get_device(dev); > > Right, sorry I missed that from your previous review :).
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c index b138b5c..2d661fd 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c @@ -148,6 +148,7 @@ static void vb2_dc_put(void *buf_priv) kfree(buf->sgt_base); } dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr); + put_device(buf->dev); kfree(buf); } @@ -168,6 +169,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) return ERR_PTR(-ENOMEM); } + /* prevent the device from release while the buffer is exported */ + get_device(dev); + buf->dev = dev; buf->size = size;