diff mbox series

[v7,18/21] dma-buf: Move dma_buf_mmap() to dynamic locking specification

Message ID 20221017172229.42269-19-dmitry.osipenko@collabora.com
State New
Headers
Series Move all drivers to a common dma-buf locking convention |

Commit Message

Dmitry Osipenko Oct. 17, 2022, 5:22 p.m. UTC 
  Move dma_buf_mmap() function to the dynamic locking specification by
taking the reservation lock. Neither of the today's drivers take the
reservation lock within the mmap() callback, hence it's safe to enforce
the locking.

Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
Acked-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/dma-buf/dma-buf.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Daniel Vetter Nov. 7, 2022, 5:25 p.m. UTC | #1 
On Mon, 17 Oct 2022 at 19:25, Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> Move dma_buf_mmap() function to the dynamic locking specification by
> taking the reservation lock. Neither of the today's drivers take the
> reservation lock within the mmap() callback, hence it's safe to enforce
> the locking.
>
> Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
> Acked-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Just noticed this while reading code ... this patch seems to have
missed dma_buf_mmap_internal()?

Might be good if at least some drivers gain a dma_resv_assert_held in
that path to make sure we're not quite this bad, together with fixing
this issue.
-Daniel

> ---
>  drivers/dma-buf/dma-buf.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index f54c649f922a..f149b384f4dd 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -1390,6 +1390,8 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF);
>  int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
>                  unsigned long pgoff)
>  {
> +       int ret;
> +
>         if (WARN_ON(!dmabuf || !vma))
>                 return -EINVAL;
>
> @@ -1410,7 +1412,11 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
>         vma_set_file(vma, dmabuf->file);
>         vma->vm_pgoff = pgoff;
>
> -       return dmabuf->ops->mmap(dmabuf, vma);
> +       dma_resv_lock(dmabuf->resv, NULL);
> +       ret = dmabuf->ops->mmap(dmabuf, vma);
> +       dma_resv_unlock(dmabuf->resv);
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);
>
> --
> 2.37.3
>
Dmitry Osipenko Nov. 7, 2022, 5:37 p.m. UTC | #2 
On 11/7/22 20:25, Daniel Vetter wrote:
>> Move dma_buf_mmap() function to the dynamic locking specification by
>> taking the reservation lock. Neither of the today's drivers take the
>> reservation lock within the mmap() callback, hence it's safe to enforce
>> the locking.
>>
>> Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
>> Acked-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Just noticed this while reading code ... this patch seems to have
> missed dma_buf_mmap_internal()?
> 
> Might be good if at least some drivers gain a dma_resv_assert_held in
> that path to make sure we're not quite this bad, together with fixing
> this issue.

Good catch! I'll prepare the patches.
diff mbox series

Patch

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index f54c649f922a..f149b384f4dd 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1390,6 +1390,8 @@  EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF);
 int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
 		 unsigned long pgoff)
 {
+	int ret;
+
 	if (WARN_ON(!dmabuf || !vma))
 		return -EINVAL;
 
@@ -1410,7 +1412,11 @@  int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
 	vma_set_file(vma, dmabuf->file);
 	vma->vm_pgoff = pgoff;
 
-	return dmabuf->ops->mmap(dmabuf, vma);
+	dma_resv_lock(dmabuf->resv, NULL);
+	ret = dmabuf->ops->mmap(dmabuf, vma);
+	dma_resv_unlock(dmabuf->resv);
+
+	return ret;
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);