| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 21 Aug 2017 14:21:40 +0200
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Stanimir Varbanov <stanimir.varbanov@...aro.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Hans Verkuil <hverkuil@...all.nl>
Cc: Pawel Osciak <pawel@...iak.com>,
Kyungmin Park <kyungmin.park@...sung.com>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 1/7 v3] media: vb2: add bidirectional flag in vb2_queue
Hi Stanimir,
On 2017-08-21 13:34, Stanimir Varbanov wrote:
> This change is intended to give to the v4l2 drivers a choice to
> change the default behavior of the v4l2-core DMA mapping direction
> from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
> OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
>
> Initially the issue with DMA mapping direction has been found in
> Venus encoder driver where the hardware (firmware side) adds few
> lines padding on bottom of the image buffer, and the consequence
> is triggering of IOMMU protection faults.
>
> This will help supporting venus encoder (and probably other drivers
> in the future) which wants to map output type of buffers as
> read/write.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@...aro.org>
Thanks for the patch.
Acked-by: Marek Szyprowski <m.szyprowski@...sung.com>
While touching this, I would love to unify set_page_dirty_lock()
related code in videobuf2-dc, videobuf2-sg and videobuf2-vmalloc.
IMHO the pattern used in videobuf2-vmalloc should be copied to
videobuf2-sg (currently checks for dma_dir for every single page)
and videobuf2-dc (currently it lacks any checks and calls
set_page_dirty_lock() unconditionally). If you have a little bit
of spare time, please prepare a separate patch for the above
mentioned fix.
> ---
> v3: update V4L2 dma-sg/contig and vmalloc memory type ops with a
> check for DMA_BIDIRECTIONAL.
> v2: move dma_dir into private section.
>
> drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++---------
> drivers/media/v4l2-core/videobuf2-dma-contig.c | 3 ++-
> drivers/media/v4l2-core/videobuf2-dma-sg.c | 6 ++++--
> drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 ++++--
> include/media/videobuf2-core.h | 13 +++++++++++++
> 5 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 0924594989b4..cb115ba6a1d2 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -194,8 +194,6 @@ static void __enqueue_in_driver(struct vb2_buffer *vb);
> static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> {
> struct vb2_queue *q = vb->vb2_queue;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> void *mem_priv;
> int plane;
> int ret = -ENOMEM;
> @@ -209,7 +207,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>
> mem_priv = call_ptr_memop(vb, alloc,
> q->alloc_devs[plane] ? : q->dev,
> - q->dma_attrs, size, dma_dir, q->gfp_flags);
> + q->dma_attrs, size, q->dma_dir, q->gfp_flags);
> if (IS_ERR_OR_NULL(mem_priv)) {
> if (mem_priv)
> ret = PTR_ERR(mem_priv);
> @@ -978,8 +976,6 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
> void *mem_priv;
> unsigned int plane;
> int ret = 0;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> bool reacquired = vb->planes[0].mem_priv == NULL;
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> @@ -1030,7 +1026,7 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
> mem_priv = call_ptr_memop(vb, get_userptr,
> q->alloc_devs[plane] ? : q->dev,
> planes[plane].m.userptr,
> - planes[plane].length, dma_dir);
> + planes[plane].length, q->dma_dir);
> if (IS_ERR(mem_priv)) {
> dprintk(1, "failed acquiring userspace memory for plane %d\n",
> plane);
> @@ -1096,8 +1092,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> void *mem_priv;
> unsigned int plane;
> int ret = 0;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> bool reacquired = vb->planes[0].mem_priv == NULL;
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> @@ -1156,7 +1150,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> /* Acquire each plane's memory */
> mem_priv = call_ptr_memop(vb, attach_dmabuf,
> q->alloc_devs[plane] ? : q->dev,
> - dbuf, planes[plane].length, dma_dir);
> + dbuf, planes[plane].length, q->dma_dir);
> if (IS_ERR(mem_priv)) {
> dprintk(1, "failed to attach dmabuf\n");
> ret = PTR_ERR(mem_priv);
> @@ -2003,6 +1997,11 @@ int vb2_core_queue_init(struct vb2_queue *q)
> if (q->buf_struct_size == 0)
> q->buf_struct_size = sizeof(struct vb2_buffer);
>
> + if (q->bidirectional)
> + q->dma_dir = DMA_BIDIRECTIONAL;
> + else
> + q->dma_dir = q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(vb2_core_queue_init);
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 5b90a66b9e78..9f389f36566d 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -508,7 +508,8 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
> buf->dma_dir = dma_dir;
>
> offset = vaddr & ~PAGE_MASK;
> - vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
> + vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
> + dma_dir == DMA_BIDIRECTIONAL);
> if (IS_ERR(vec)) {
> ret = PTR_ERR(vec);
> goto fail_buf;
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> index 54f33938d45b..6808231a6bdc 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> @@ -239,7 +239,8 @@ static void *vb2_dma_sg_get_userptr(struct device *dev, unsigned long vaddr,
> buf->offset = vaddr & ~PAGE_MASK;
> buf->size = size;
> buf->dma_sgt = &buf->sg_table;
> - vec = vb2_create_framevec(vaddr, size, buf->dma_dir == DMA_FROM_DEVICE);
> + vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
> + dma_dir == DMA_BIDIRECTIONAL);
> if (IS_ERR(vec))
> goto userptr_fail_pfnvec;
> buf->vec = vec;
> @@ -292,7 +293,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
> vm_unmap_ram(buf->vaddr, buf->num_pages);
> sg_free_table(buf->dma_sgt);
> while (--i >= 0) {
> - if (buf->dma_dir == DMA_FROM_DEVICE)
> + if (buf->dma_dir == DMA_FROM_DEVICE ||
> + buf->dma_dir == DMA_BIDIRECTIONAL)
> set_page_dirty_lock(buf->pages[i]);
> }
> vb2_destroy_framevec(buf->vec);
> diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> index 6bc130fe84f6..3a7c80cd1a17 100644
> --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
> +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> @@ -87,7 +87,8 @@ static void *vb2_vmalloc_get_userptr(struct device *dev, unsigned long vaddr,
> buf->dma_dir = dma_dir;
> offset = vaddr & ~PAGE_MASK;
> buf->size = size;
> - vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
> + vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
> + dma_dir == DMA_BIDIRECTIONAL);
> if (IS_ERR(vec)) {
> ret = PTR_ERR(vec);
> goto fail_pfnvec_create;
> @@ -137,7 +138,8 @@ static void vb2_vmalloc_put_userptr(void *buf_priv)
> pages = frame_vector_pages(buf->vec);
> if (vaddr)
> vm_unmap_ram((void *)vaddr, n_pages);
> - if (buf->dma_dir == DMA_FROM_DEVICE)
> + if (buf->dma_dir == DMA_FROM_DEVICE ||
> + buf->dma_dir == DMA_BIDIRECTIONAL)
> for (i = 0; i < n_pages; i++)
> set_page_dirty_lock(pages[i]);
> } else {
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cb97c224be73..ef9b64398c8c 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -427,6 +427,16 @@ struct vb2_buf_ops {
> * @dev: device to use for the default allocation context if the driver
> * doesn't fill in the @alloc_devs array.
> * @dma_attrs: DMA attributes to use for the DMA.
> + * @bidirectional: when this flag is set the DMA direction for the buffers of
> + * this queue will be overridden with DMA_BIDIRECTIONAL direction.
> + * This is useful in cases where the hardware (firmware) writes to
> + * a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
> + * buffer which is mapped for write (DMA_FROM_DEVICE) in order
> + * to satisfy some internal hardware restrictions or adds a padding
> + * needed by the processing algorithm. In case the DMA mapping is
> + * not bidirectional but the hardware (firmware) trying to access
> + * the buffer (in the opposite direction) this could lead to an
> + * IOMMU protection faults.
> * @fileio_read_once: report EOF after reading the first buffer
> * @fileio_write_immediately: queue buffer after each write() call
> * @allow_zero_bytesused: allow bytesused == 0 to be passed to the driver
> @@ -465,6 +475,7 @@ struct vb2_buf_ops {
> * Private elements (won't appear at the uAPI book):
> * @mmap_lock: private mutex used when buffers are allocated/freed/mmapped
> * @memory: current memory type used
> + * @dma_dir: DMA mapping direction.
> * @bufs: videobuf buffer structures
> * @num_buffers: number of allocated/used buffers
> * @queued_list: list of buffers currently queued from userspace
> @@ -495,6 +506,7 @@ struct vb2_queue {
> unsigned int io_modes;
> struct device *dev;
> unsigned long dma_attrs;
> + unsigned bidirectional:1;
> unsigned fileio_read_once:1;
> unsigned fileio_write_immediately:1;
> unsigned allow_zero_bytesused:1;
> @@ -516,6 +528,7 @@ struct vb2_queue {
> /* private: internal use only */
> struct mutex mmap_lock;
> unsigned int memory;
> + enum dma_data_direction dma_dir;
> struct vb2_buffer *bufs[VB2_MAX_FRAME];
> unsigned int num_buffers;
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Powered by blists - more mailing lists