| Message ID | 20210119204508.9256-1-john.stultz@linaro.org |
|---|---|
| State | Accepted |
| Commit | cc84a8e65d87990ebb09f65a745b38e95b4d2721 |
| Headers | show |
| Series | [RESEND,1/3] dma-buf: system_heap: Make sure to return an error if we abort | expand |
Hi John, On Wed, 20 Jan 2021 at 02:15, John Stultz <john.stultz@linaro.org> 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 <sumit.semwal@linaro.org> > Cc: Liam Mark <lmark@codeaurora.org> > Cc: Laura Abbott <labbott@kernel.org> > Cc: Brian Starkey <Brian.Starkey@arm.com> > Cc: Hridya Valsaraju <hridya@google.com> > Cc: Suren Baghdasaryan <surenb@google.com> > Cc: Sandeep Patil <sspatil@google.com> > Cc: Daniel Mentz <danielmentz@google.com> > Cc: Chris Goldsworthy <cgoldswo@codeaurora.org> > Cc: Ørjan Eide <orjan.eide@arm.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Ezequiel Garcia <ezequiel@collabora.com> > Cc: Simon Ser <contact@emersion.fr> > Cc: James Jones <jajones@nvidia.com> > Cc: linux-media@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Suggested-by: Suren Baghdasaryan <surenb@google.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > 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 <john.stultz@linaro.org> 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 <sumit.semwal@linaro.org> > Cc: Liam Mark <lmark@codeaurora.org> > Cc: Laura Abbott <labbott@kernel.org> > Cc: Brian Starkey <Brian.Starkey@arm.com> > Cc: Hridya Valsaraju <hridya@google.com> > Cc: Suren Baghdasaryan <surenb@google.com> > Cc: Sandeep Patil <sspatil@google.com> > Cc: Daniel Mentz <danielmentz@google.com> > Cc: Chris Goldsworthy <cgoldswo@codeaurora.org> > Cc: Ørjan Eide <orjan.eide@arm.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Ezequiel Garcia <ezequiel@collabora.com> > Cc: Simon Ser <contact@emersion.fr> > Cc: James Jones <jajones@nvidia.com> > Cc: linux-media@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Suggested-by: Suren Baghdasaryan <surenb@google.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > 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 <sumit.semwal@linaro.org> wrote: > > Hi John, Suren, > > > On Wed, 20 Jan 2021 at 02:15, John Stultz <john.stultz@linaro.org> 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 <sumit.semwal@linaro.org> > > Cc: Liam Mark <lmark@codeaurora.org> > > Cc: Laura Abbott <labbott@kernel.org> > > Cc: Brian Starkey <Brian.Starkey@arm.com> > > Cc: Hridya Valsaraju <hridya@google.com> > > Cc: Suren Baghdasaryan <surenb@google.com> > > Cc: Sandeep Patil <sspatil@google.com> > > Cc: Daniel Mentz <danielmentz@google.com> > > Cc: Chris Goldsworthy <cgoldswo@codeaurora.org> > > Cc: Ørjan Eide <orjan.eide@arm.com> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Cc: Ezequiel Garcia <ezequiel@collabora.com> > > Cc: Simon Ser <contact@emersion.fr> > > Cc: James Jones <jajones@nvidia.com> > > Cc: linux-media@vger.kernel.org > > Cc: dri-devel@lists.freedesktop.org > > Suggested-by: Suren Baghdasaryan <surenb@google.com> > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > --- > > 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 <surenb@google.com> wrote: > On Thu, Jan 21, 2021 at 11:56 PM Sumit Semwal <sumit.semwal@linaro.org> wrote: > > On Wed, 20 Jan 2021 at 02:15, John Stultz <john.stultz@linaro.org> 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
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)
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 <sumit.semwal@linaro.org> Cc: Liam Mark <lmark@codeaurora.org> Cc: Laura Abbott <labbott@kernel.org> Cc: Brian Starkey <Brian.Starkey@arm.com> Cc: Hridya Valsaraju <hridya@google.com> Cc: Suren Baghdasaryan <surenb@google.com> Cc: Sandeep Patil <sspatil@google.com> Cc: Daniel Mentz <danielmentz@google.com> Cc: Chris Goldsworthy <cgoldswo@codeaurora.org> Cc: Ørjan Eide <orjan.eide@arm.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Ezequiel Garcia <ezequiel@collabora.com> Cc: Simon Ser <contact@emersion.fr> Cc: James Jones <jajones@nvidia.com> Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Suggested-by: Suren Baghdasaryan <surenb@google.com> Signed-off-by: John Stultz <john.stultz@linaro.org> --- drivers/dma-buf/heaps/system_heap.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)