If we abort from the allocation due to a fatal_signal_pending(),
be sure we report an error so any return code paths don't trip
over the fact that the allocation didn't succeed.
Cc: Sumit Semwal <[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Hridya Valsaraju <[email protected]>
Cc: Suren Baghdasaryan <[email protected]>
Cc: Sandeep Patil <[email protected]>
Cc: Daniel Mentz <[email protected]>
Cc: Chris Goldsworthy <[email protected]>
Cc: Ørjan Eide <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: Simon Ser <[email protected]>
Cc: James Jones <[email protected]>
Cc: [email protected]
Cc: [email protected]
Suggested-by: Suren Baghdasaryan <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
drivers/dma-buf/heaps/system_heap.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 17e0e9a68baf..405351aad2a8 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -363,8 +363,10 @@ static int system_heap_allocate(struct dma_heap *heap,
* Avoid trying to allocate memory if the process
* has been killed by SIGKILL
*/
- if (fatal_signal_pending(current))
+ if (fatal_signal_pending(current)) {
+ ret = -EINTR;
goto free_buffer;
+ }
page = alloc_largest_available(size_remaining, max_order);
if (!page)
--
2.17.1
Every heap needs to create a dmabuf and then export it to a fd
via dma_buf_fd(), so to consolidate things a bit, have the heaps
just return a struct dmabuf * and let the top level
dma_heap_buffer_alloc() call handle creating the fd via
dma_buf_fd().
Cc: Sumit Semwal <[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Hridya Valsaraju <[email protected]>
Cc: Suren Baghdasaryan <[email protected]>
Cc: Sandeep Patil <[email protected]>
Cc: Daniel Mentz <[email protected]>
Cc: Chris Goldsworthy <[email protected]>
Cc: Ørjan Eide <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: Simon Ser <[email protected]>
Cc: James Jones <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
drivers/dma-buf/dma-heap.c | 14 +++++++++++++-
drivers/dma-buf/heaps/cma_heap.c | 22 +++++++---------------
drivers/dma-buf/heaps/system_heap.c | 21 +++++++--------------
include/linux/dma-heap.h | 12 ++++++------
4 files changed, 33 insertions(+), 36 deletions(-)
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index afd22c9dbdcf..6b5db954569f 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -52,6 +52,9 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
unsigned int fd_flags,
unsigned int heap_flags)
{
+ struct dma_buf *dmabuf;
+ int fd;
+
/*
* Allocations from all heaps have to begin
* and end on page boundaries.
@@ -60,7 +63,16 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
if (!len)
return -EINVAL;
- return heap->ops->allocate(heap, len, fd_flags, heap_flags);
+ dmabuf = heap->ops->allocate(heap, len, fd_flags, heap_flags);
+ if (IS_ERR(dmabuf))
+ return PTR_ERR(dmabuf);
+
+ fd = dma_buf_fd(dmabuf, fd_flags);
+ if (fd < 0) {
+ dma_buf_put(dmabuf);
+ /* just return, as put will call release and that will free */
+ }
+ return fd;
}
static int dma_heap_open(struct inode *inode, struct file *file)
diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 0c76cbc3fb11..985c41ffd85b 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -272,10 +272,10 @@ static const struct dma_buf_ops cma_heap_buf_ops = {
.release = cma_heap_dma_buf_release,
};
-static int cma_heap_allocate(struct dma_heap *heap,
- unsigned long len,
- unsigned long fd_flags,
- unsigned long heap_flags)
+static struct dma_buf *cma_heap_allocate(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags)
{
struct cma_heap *cma_heap = dma_heap_get_drvdata(heap);
struct cma_heap_buffer *buffer;
@@ -290,7 +290,7 @@ static int cma_heap_allocate(struct dma_heap *heap,
buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
if (!buffer)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&buffer->attachments);
mutex_init(&buffer->lock);
@@ -349,15 +349,7 @@ static int cma_heap_allocate(struct dma_heap *heap,
ret = PTR_ERR(dmabuf);
goto free_pages;
}
-
- ret = dma_buf_fd(dmabuf, fd_flags);
- if (ret < 0) {
- dma_buf_put(dmabuf);
- /* just return, as put will call release and that will free */
- return ret;
- }
-
- return ret;
+ return dmabuf;
free_pages:
kfree(buffer->pages);
@@ -366,7 +358,7 @@ static int cma_heap_allocate(struct dma_heap *heap,
free_buffer:
kfree(buffer);
- return ret;
+ return ERR_PTR(ret);
}
static const struct dma_heap_ops cma_heap_ops = {
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 2321c91891f6..7b154424aeb3 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -332,10 +332,10 @@ static struct page *alloc_largest_available(unsigned long size,
return NULL;
}
-static int system_heap_allocate(struct dma_heap *heap,
- unsigned long len,
- unsigned long fd_flags,
- unsigned long heap_flags)
+static struct dma_buf *system_heap_allocate(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags)
{
struct system_heap_buffer *buffer;
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
@@ -350,7 +350,7 @@ static int system_heap_allocate(struct dma_heap *heap,
buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
if (!buffer)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&buffer->attachments);
mutex_init(&buffer->lock);
@@ -400,14 +400,7 @@ static int system_heap_allocate(struct dma_heap *heap,
ret = PTR_ERR(dmabuf);
goto free_pages;
}
-
- ret = dma_buf_fd(dmabuf, fd_flags);
- if (ret < 0) {
- dma_buf_put(dmabuf);
- /* just return, as put will call release and that will free */
- return ret;
- }
- return ret;
+ return dmabuf;
free_pages:
for_each_sgtable_sg(table, sg, i) {
@@ -421,7 +414,7 @@ static int system_heap_allocate(struct dma_heap *heap,
__free_pages(page, compound_order(page));
kfree(buffer);
- return ret;
+ return ERR_PTR(ret);
}
static const struct dma_heap_ops system_heap_ops = {
diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
index 454e354d1ffb..5bc5c946af58 100644
--- a/include/linux/dma-heap.h
+++ b/include/linux/dma-heap.h
@@ -16,15 +16,15 @@ struct dma_heap;
/**
* struct dma_heap_ops - ops to operate on a given heap
- * @allocate: allocate dmabuf and return fd
+ * @allocate: allocate dmabuf and return struct dma_buf ptr
*
- * allocate returns dmabuf fd on success, -errno on error.
+ * allocate returns dmabuf on success, ERR_PTR(-errno) on error.
*/
struct dma_heap_ops {
- int (*allocate)(struct dma_heap *heap,
- unsigned long len,
- unsigned long fd_flags,
- unsigned long heap_flags);
+ struct dma_buf *(*allocate)(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags);
};
/**
--
2.17.1
We shouldn't vunmap more then we vmap, but if we do, make
sure we complain loudly.
Cc: Sumit Semwal <[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Hridya Valsaraju <[email protected]>
Cc: Suren Baghdasaryan <[email protected]>
Cc: Sandeep Patil <[email protected]>
Cc: Daniel Mentz <[email protected]>
Cc: Chris Goldsworthy <[email protected]>
Cc: Ørjan Eide <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: Simon Ser <[email protected]>
Cc: James Jones <[email protected]>
Cc: [email protected]
Cc: [email protected]
Suggested-by: Suren Baghdasaryan <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
drivers/dma-buf/heaps/cma_heap.c | 1 +
drivers/dma-buf/heaps/system_heap.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 364fc2f3e499..0c76cbc3fb11 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -232,6 +232,7 @@ static void cma_heap_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map)
struct cma_heap_buffer *buffer = dmabuf->priv;
mutex_lock(&buffer->lock);
+ WARN_ON(buffer->vmap_cnt == 0);
if (!--buffer->vmap_cnt) {
vunmap(buffer->vaddr);
buffer->vaddr = NULL;
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 405351aad2a8..2321c91891f6 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -273,6 +273,7 @@ static void system_heap_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map)
struct system_heap_buffer *buffer = dmabuf->priv;
mutex_lock(&buffer->lock);
+ WARN_ON(buffer->vmap_cnt == 0);
if (!--buffer->vmap_cnt) {
vunmap(buffer->vaddr);
buffer->vaddr = NULL;
--
2.17.1
Hi John,
On Wed, 20 Jan 2021 at 02:15, John Stultz <[email protected]> wrote:
>
> Every heap needs to create a dmabuf and then export it to a fd
> via dma_buf_fd(), so to consolidate things a bit, have the heaps
> just return a struct dmabuf * and let the top level
> dma_heap_buffer_alloc() call handle creating the fd via
> dma_buf_fd().
Thanks for the patch! LGTM, feels a lot neater now. I'll merge into
drm-misc-next.
>
> Cc: Sumit Semwal <[email protected]>
> Cc: Liam Mark <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: Brian Starkey <[email protected]>
> Cc: Hridya Valsaraju <[email protected]>
> Cc: Suren Baghdasaryan <[email protected]>
> Cc: Sandeep Patil <[email protected]>
> Cc: Daniel Mentz <[email protected]>
> Cc: Chris Goldsworthy <[email protected]>
> Cc: Ørjan Eide <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Ezequiel Garcia <[email protected]>
> Cc: Simon Ser <[email protected]>
> Cc: James Jones <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: John Stultz <[email protected]>
> ---
> drivers/dma-buf/dma-heap.c | 14 +++++++++++++-
> drivers/dma-buf/heaps/cma_heap.c | 22 +++++++---------------
> drivers/dma-buf/heaps/system_heap.c | 21 +++++++--------------
> include/linux/dma-heap.h | 12 ++++++------
> 4 files changed, 33 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index afd22c9dbdcf..6b5db954569f 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -52,6 +52,9 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> unsigned int fd_flags,
> unsigned int heap_flags)
> {
> + struct dma_buf *dmabuf;
> + int fd;
> +
> /*
> * Allocations from all heaps have to begin
> * and end on page boundaries.
> @@ -60,7 +63,16 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> if (!len)
> return -EINVAL;
>
> - return heap->ops->allocate(heap, len, fd_flags, heap_flags);
> + dmabuf = heap->ops->allocate(heap, len, fd_flags, heap_flags);
> + if (IS_ERR(dmabuf))
> + return PTR_ERR(dmabuf);
> +
> + fd = dma_buf_fd(dmabuf, fd_flags);
> + if (fd < 0) {
> + dma_buf_put(dmabuf);
> + /* just return, as put will call release and that will free */
> + }
> + return fd;
> }
>
> static int dma_heap_open(struct inode *inode, struct file *file)
> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> index 0c76cbc3fb11..985c41ffd85b 100644
> --- a/drivers/dma-buf/heaps/cma_heap.c
> +++ b/drivers/dma-buf/heaps/cma_heap.c
> @@ -272,10 +272,10 @@ static const struct dma_buf_ops cma_heap_buf_ops = {
> .release = cma_heap_dma_buf_release,
> };
>
> -static int cma_heap_allocate(struct dma_heap *heap,
> - unsigned long len,
> - unsigned long fd_flags,
> - unsigned long heap_flags)
> +static struct dma_buf *cma_heap_allocate(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags)
> {
> struct cma_heap *cma_heap = dma_heap_get_drvdata(heap);
> struct cma_heap_buffer *buffer;
> @@ -290,7 +290,7 @@ static int cma_heap_allocate(struct dma_heap *heap,
>
> buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> if (!buffer)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> INIT_LIST_HEAD(&buffer->attachments);
> mutex_init(&buffer->lock);
> @@ -349,15 +349,7 @@ static int cma_heap_allocate(struct dma_heap *heap,
> ret = PTR_ERR(dmabuf);
> goto free_pages;
> }
> -
> - ret = dma_buf_fd(dmabuf, fd_flags);
> - if (ret < 0) {
> - dma_buf_put(dmabuf);
> - /* just return, as put will call release and that will free */
> - return ret;
> - }
> -
> - return ret;
> + return dmabuf;
>
> free_pages:
> kfree(buffer->pages);
> @@ -366,7 +358,7 @@ static int cma_heap_allocate(struct dma_heap *heap,
> free_buffer:
> kfree(buffer);
>
> - return ret;
> + return ERR_PTR(ret);
> }
>
> static const struct dma_heap_ops cma_heap_ops = {
> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index 2321c91891f6..7b154424aeb3 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -332,10 +332,10 @@ static struct page *alloc_largest_available(unsigned long size,
> return NULL;
> }
>
> -static int system_heap_allocate(struct dma_heap *heap,
> - unsigned long len,
> - unsigned long fd_flags,
> - unsigned long heap_flags)
> +static struct dma_buf *system_heap_allocate(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags)
> {
> struct system_heap_buffer *buffer;
> DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> @@ -350,7 +350,7 @@ static int system_heap_allocate(struct dma_heap *heap,
>
> buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> if (!buffer)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> INIT_LIST_HEAD(&buffer->attachments);
> mutex_init(&buffer->lock);
> @@ -400,14 +400,7 @@ static int system_heap_allocate(struct dma_heap *heap,
> ret = PTR_ERR(dmabuf);
> goto free_pages;
> }
> -
> - ret = dma_buf_fd(dmabuf, fd_flags);
> - if (ret < 0) {
> - dma_buf_put(dmabuf);
> - /* just return, as put will call release and that will free */
> - return ret;
> - }
> - return ret;
> + return dmabuf;
>
> free_pages:
> for_each_sgtable_sg(table, sg, i) {
> @@ -421,7 +414,7 @@ static int system_heap_allocate(struct dma_heap *heap,
> __free_pages(page, compound_order(page));
> kfree(buffer);
>
> - return ret;
> + return ERR_PTR(ret);
> }
>
> static const struct dma_heap_ops system_heap_ops = {
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> index 454e354d1ffb..5bc5c946af58 100644
> --- a/include/linux/dma-heap.h
> +++ b/include/linux/dma-heap.h
> @@ -16,15 +16,15 @@ struct dma_heap;
>
> /**
> * struct dma_heap_ops - ops to operate on a given heap
> - * @allocate: allocate dmabuf and return fd
> + * @allocate: allocate dmabuf and return struct dma_buf ptr
> *
> - * allocate returns dmabuf fd on success, -errno on error.
> + * allocate returns dmabuf on success, ERR_PTR(-errno) on error.
> */
> struct dma_heap_ops {
> - int (*allocate)(struct dma_heap *heap,
> - unsigned long len,
> - unsigned long fd_flags,
> - unsigned long heap_flags);
> + struct dma_buf *(*allocate)(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags);
> };
>
> /**
> --
> 2.17.1
>
Best,
Sumit.
Hi John,
On Wed, 20 Jan 2021 at 02:15, John Stultz <[email protected]> wrote:
>
> If we abort from the allocation due to a fatal_signal_pending(),
> be sure we report an error so any return code paths don't trip
> over the fact that the allocation didn't succeed.
Thanks for the patch; LGTM, will push into drm-misc-next.
>
> Cc: Sumit Semwal <[email protected]>
> Cc: Liam Mark <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: Brian Starkey <[email protected]>
> Cc: Hridya Valsaraju <[email protected]>
> Cc: Suren Baghdasaryan <[email protected]>
> Cc: Sandeep Patil <[email protected]>
> Cc: Daniel Mentz <[email protected]>
> Cc: Chris Goldsworthy <[email protected]>
> Cc: Ørjan Eide <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Ezequiel Garcia <[email protected]>
> Cc: Simon Ser <[email protected]>
> Cc: James Jones <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Suggested-by: Suren Baghdasaryan <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> drivers/dma-buf/heaps/system_heap.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index 17e0e9a68baf..405351aad2a8 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -363,8 +363,10 @@ static int system_heap_allocate(struct dma_heap *heap,
> * Avoid trying to allocate memory if the process
> * has been killed by SIGKILL
> */
> - if (fatal_signal_pending(current))
> + if (fatal_signal_pending(current)) {
> + ret = -EINTR;
> goto free_buffer;
> + }
>
> page = alloc_largest_available(size_remaining, max_order);
> if (!page)
> --
> 2.17.1
>
Best,
Sumit.
Hi John, Suren,
On Wed, 20 Jan 2021 at 02:15, John Stultz <[email protected]> wrote:
>
> We shouldn't vunmap more then we vmap, but if we do, make
> sure we complain loudly.
I was checking the general usage of vunmap in the kernel, and I
couldn't find many instances where we need to WARN_ON for the vunmap
count more than vmap count. Is there a specific need for this in the heaps?
Best,
Sumit.
>
> Cc: Sumit Semwal <[email protected]>
> Cc: Liam Mark <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: Brian Starkey <[email protected]>
> Cc: Hridya Valsaraju <[email protected]>
> Cc: Suren Baghdasaryan <[email protected]>
> Cc: Sandeep Patil <[email protected]>
> Cc: Daniel Mentz <[email protected]>
> Cc: Chris Goldsworthy <[email protected]>
> Cc: Ørjan Eide <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Ezequiel Garcia <[email protected]>
> Cc: Simon Ser <[email protected]>
> Cc: James Jones <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Suggested-by: Suren Baghdasaryan <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> drivers/dma-buf/heaps/cma_heap.c | 1 +
> drivers/dma-buf/heaps/system_heap.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> index 364fc2f3e499..0c76cbc3fb11 100644
> --- a/drivers/dma-buf/heaps/cma_heap.c
> +++ b/drivers/dma-buf/heaps/cma_heap.c
> @@ -232,6 +232,7 @@ static void cma_heap_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map)
> struct cma_heap_buffer *buffer = dmabuf->priv;
>
> mutex_lock(&buffer->lock);
> + WARN_ON(buffer->vmap_cnt == 0);
> if (!--buffer->vmap_cnt) {
> vunmap(buffer->vaddr);
> buffer->vaddr = NULL;
> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index 405351aad2a8..2321c91891f6 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -273,6 +273,7 @@ static void system_heap_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map)
> struct system_heap_buffer *buffer = dmabuf->priv;
>
> mutex_lock(&buffer->lock);
> + WARN_ON(buffer->vmap_cnt == 0);
> if (!--buffer->vmap_cnt) {
> vunmap(buffer->vaddr);
> buffer->vaddr = NULL;
> --
> 2.17.1
>
On Thu, Jan 21, 2021 at 11:56 PM Sumit Semwal <[email protected]> wrote:
>
> Hi John, Suren,
>
>
> On Wed, 20 Jan 2021 at 02:15, John Stultz <[email protected]> wrote:
> >
> > We shouldn't vunmap more then we vmap, but if we do, make
> > sure we complain loudly.
>
> I was checking the general usage of vunmap in the kernel, and I
> couldn't find many instances where we need to WARN_ON for the vunmap
> count more than vmap count. Is there a specific need for this in the heaps?
Hi Sumit,
My worry was that buffer->vmap_cnt could silently go negative. But if
this warning is not consistent with other places we do refcounted
vmap/vunmap then feel free to ignore my suggestion.
Thanks!
>
> Best,
> Sumit.
> >
> > Cc: Sumit Semwal <[email protected]>
> > Cc: Liam Mark <[email protected]>
> > Cc: Laura Abbott <[email protected]>
> > Cc: Brian Starkey <[email protected]>
> > Cc: Hridya Valsaraju <[email protected]>
> > Cc: Suren Baghdasaryan <[email protected]>
> > Cc: Sandeep Patil <[email protected]>
> > Cc: Daniel Mentz <[email protected]>
> > Cc: Chris Goldsworthy <[email protected]>
> > Cc: Ørjan Eide <[email protected]>
> > Cc: Robin Murphy <[email protected]>
> > Cc: Ezequiel Garcia <[email protected]>
> > Cc: Simon Ser <[email protected]>
> > Cc: James Jones <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Suggested-by: Suren Baghdasaryan <[email protected]>
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > drivers/dma-buf/heaps/cma_heap.c | 1 +
> > drivers/dma-buf/heaps/system_heap.c | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> > index 364fc2f3e499..0c76cbc3fb11 100644
> > --- a/drivers/dma-buf/heaps/cma_heap.c
> > +++ b/drivers/dma-buf/heaps/cma_heap.c
> > @@ -232,6 +232,7 @@ static void cma_heap_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map)
> > struct cma_heap_buffer *buffer = dmabuf->priv;
> >
> > mutex_lock(&buffer->lock);
> > + WARN_ON(buffer->vmap_cnt == 0);
> > if (!--buffer->vmap_cnt) {
> > vunmap(buffer->vaddr);
> > buffer->vaddr = NULL;
> > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> > index 405351aad2a8..2321c91891f6 100644
> > --- a/drivers/dma-buf/heaps/system_heap.c
> > +++ b/drivers/dma-buf/heaps/system_heap.c
> > @@ -273,6 +273,7 @@ static void system_heap_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map)
> > struct system_heap_buffer *buffer = dmabuf->priv;
> >
> > mutex_lock(&buffer->lock);
> > + WARN_ON(buffer->vmap_cnt == 0);
> > if (!--buffer->vmap_cnt) {
> > vunmap(buffer->vaddr);
> > buffer->vaddr = NULL;
> > --
> > 2.17.1
> >
On Fri, Jan 22, 2021 at 2:21 PM Suren Baghdasaryan <[email protected]> wrote:
> On Thu, Jan 21, 2021 at 11:56 PM Sumit Semwal <[email protected]> wrote:
> > On Wed, 20 Jan 2021 at 02:15, John Stultz <[email protected]> wrote:
> > >
> > > We shouldn't vunmap more then we vmap, but if we do, make
> > > sure we complain loudly.
> >
> > I was checking the general usage of vunmap in the kernel, and I
> > couldn't find many instances where we need to WARN_ON for the vunmap
> > count more than vmap count. Is there a specific need for this in the heaps?
>
> Hi Sumit,
> My worry was that buffer->vmap_cnt could silently go negative. But if
> this warning is not consistent with other places we do refcounted
> vmap/vunmap then feel free to ignore my suggestion.
>
Yea,
My sense is that it didn't seem like it would hurt, and if the
warning happened to be tripped, it would be good to catch.
However, if you are skeptical, feel free to drop that patch from this
series for now (it shouldn't impact the following patches).
thanks
-john