[PATCH] udmabuf: fix dma-buf cpu access

[PATCH] udmabuf: fix dma-buf cpu access

[Gurchetan Singh]

I'm just going to put Chia's review comment here since it sums
the issue rather nicely:

"(1) Semantically, a dma-buf is in DMA domain.  CPU access from the
importer must be surrounded by {begin,end}_cpu_access.  This gives the
exporter a chance to move the buffer to the CPU domain temporarily.

(2) When the exporter itself has other means to do CPU access, it is
only reasonable for the exporter to move the buffer to the CPU domain
before access, and to the DMA domain after access.  The exporter can
potentially reuse {begin,end}_cpu_access for that purpose.

Because of (1), udmabuf does need to implement the
{begin,end}_cpu_access hooks.  But "begin" should mean
dma_sync_sg_for_cpu and "end" should mean dma_sync_sg_for_device.

Because of (2), if userspace wants to continuing accessing through the
memfd mapping, it should call udmabuf's {begin,end}_cpu_access to
avoid cache issues."

Reported-by: Chia-I Wu <olvaffe@gmail.com>
Suggested-by: Chia-I Wu <olvaffe@gmail.com>
Fixes: 284562e1f34 ("udmabuf: implement begin_cpu_access/end_cpu_access hooks")
---
 drivers/dma-buf/udmabuf.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 61b0a2cff874..acb26c627d27 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -122,9 +122,8 @@ static int begin_cpu_udmabuf(struct dma_buf *buf,
 		if (IS_ERR(ubuf->sg))
 			return PTR_ERR(ubuf->sg);
 	} else {
-		dma_sync_sg_for_device(dev, ubuf->sg->sgl,
-				       ubuf->sg->nents,
-				       direction);
+		dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
+				    direction);
 	}
 
 	return 0;
@@ -139,7 +138,7 @@ static int end_cpu_udmabuf(struct dma_buf *buf,
 	if (!ubuf->sg)
 		return -EINVAL;
 
-	dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents, direction);
+	dma_sync_sg_for_device(dev, ubuf->sg->sgl, ubuf->sg->nents, direction);
 	return 0;
 }
 
-- 
2.24.1.735.g03f4e72817-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] udmabuf: fix dma-buf cpu access

[Gerd Hoffmann]

On Fri, Dec 13, 2019 at 11:33:59AM -0800, Gurchetan Singh wrote:
> I'm just going to put Chia's review comment here since it sums
> the issue rather nicely:
> 
> "(1) Semantically, a dma-buf is in DMA domain.  CPU access from the
> importer must be surrounded by {begin,end}_cpu_access.  This gives the
> exporter a chance to move the buffer to the CPU domain temporarily.
> 
> (2) When the exporter itself has other means to do CPU access, it is
> only reasonable for the exporter to move the buffer to the CPU domain
> before access, and to the DMA domain after access.  The exporter can
> potentially reuse {begin,end}_cpu_access for that purpose.
> 
> Because of (1), udmabuf does need to implement the
> {begin,end}_cpu_access hooks.  But "begin" should mean
> dma_sync_sg_for_cpu and "end" should mean dma_sync_sg_for_device.
> 
> Because of (2), if userspace wants to continuing accessing through the
> memfd mapping, it should call udmabuf's {begin,end}_cpu_access to
> avoid cache issues."
> 
> Reported-by: Chia-I Wu <olvaffe@gmail.com>
> Suggested-by: Chia-I Wu <olvaffe@gmail.com>
> Fixes: 284562e1f34 ("udmabuf: implement begin_cpu_access/end_cpu_access hooks")

Needs 12 hash digits.

Also your signed-off is missing ...

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] udmabuf: fix dma-buf cpu access

[Gurchetan Singh]

I'm just going to put Chia's review comment here since it sums
the issue rather nicely:

"(1) Semantically, a dma-buf is in DMA domain.  CPU access from the
importer must be surrounded by {begin,end}_cpu_access.  This gives the
exporter a chance to move the buffer to the CPU domain temporarily.

(2) When the exporter itself has other means to do CPU access, it is
only reasonable for the exporter to move the buffer to the CPU domain
before access, and to the DMA domain after access.  The exporter can
potentially reuse {begin,end}_cpu_access for that purpose.

Because of (1), udmabuf does need to implement the
{begin,end}_cpu_access hooks.  But "begin" should mean
dma_sync_sg_for_cpu and "end" should mean dma_sync_sg_for_device.

Because of (2), if userspace wants to continuing accessing through the
memfd mapping, it should call udmabuf's {begin,end}_cpu_access to
avoid cache issues."

Reported-by: Chia-I Wu <olvaffe@gmail.com>
Suggested-by: Chia-I Wu <olvaffe@gmail.com>
Fixes: 284562e1f348 ("udmabuf: implement begin_cpu_access/end_cpu_access hooks")
Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/dma-buf/udmabuf.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 61b0a2cff874..acb26c627d27 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -122,9 +122,8 @@ static int begin_cpu_udmabuf(struct dma_buf *buf,
 		if (IS_ERR(ubuf->sg))
 			return PTR_ERR(ubuf->sg);
 	} else {
-		dma_sync_sg_for_device(dev, ubuf->sg->sgl,
-				       ubuf->sg->nents,
-				       direction);
+		dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
+				    direction);
 	}
 
 	return 0;
@@ -139,7 +138,7 @@ static int end_cpu_udmabuf(struct dma_buf *buf,
 	if (!ubuf->sg)
 		return -EINVAL;
 
-	dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents, direction);
+	dma_sync_sg_for_device(dev, ubuf->sg->sgl, ubuf->sg->nents, direction);
 	return 0;
 }
 
-- 
2.24.1.735.g03f4e72817-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] udmabuf: fix dma-buf cpu access

[Gerd Hoffmann]

On Tue, Dec 17, 2019 at 03:02:28PM -0800, Gurchetan Singh wrote:
> I'm just going to put Chia's review comment here since it sums
> the issue rather nicely:
> 
> "(1) Semantically, a dma-buf is in DMA domain.  CPU access from the
> importer must be surrounded by {begin,end}_cpu_access.  This gives the
> exporter a chance to move the buffer to the CPU domain temporarily.
> 
> (2) When the exporter itself has other means to do CPU access, it is
> only reasonable for the exporter to move the buffer to the CPU domain
> before access, and to the DMA domain after access.  The exporter can
> potentially reuse {begin,end}_cpu_access for that purpose.
> 
> Because of (1), udmabuf does need to implement the
> {begin,end}_cpu_access hooks.  But "begin" should mean
> dma_sync_sg_for_cpu and "end" should mean dma_sync_sg_for_device.
> 
> Because of (2), if userspace wants to continuing accessing through the
> memfd mapping, it should call udmabuf's {begin,end}_cpu_access to
> avoid cache issues."
> 
> Reported-by: Chia-I Wu <olvaffe@gmail.com>
> Suggested-by: Chia-I Wu <olvaffe@gmail.com>
> Fixes: 284562e1f348 ("udmabuf: implement begin_cpu_access/end_cpu_access hooks")
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>

Pushed to drm-misc-next.

thanks,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel