Re: [PATCH] drm/vmwgfx: Fix scatterlist unmapping

From: Robin Murphy
Date: Fri Apr 27 2018 - 12:56:08 EST

Hi Thomas,

On 25/04/18 14:21, Thomas Hellstrom wrote:
Hi, Robin,

Thanks for the patch. It was some time since I put together that code, but I remember hitting something similar to

Even if it's clear from the documentation that orig_nents should be used.

Hmmm, it's odd that you would see issues - it's always been something that CONFIG_DMA_API_DEBUG would have screamed about, and as far as I'm aware for x86, nents and orig_nents should always end up equal anyway. I would definitely be interested to see the specific fault details if it can be reproduced. I suppose one possibility is that there's some path where you inadvertently unmap something which was never mapped, but passing nents=0 means you manage to get away with it without the DMA API backend trying to interpret any bogus DMA addresses/lengths.

FWIW, the rationale is that sync_sg/unmap_sg operate on sg->page (which can always be translated back to a meaningful CPU address for cache/write buffer maintenance), not sg->dma_address (which sometimes cannot), therefore passing a truncated list will have the effect of just not syncing the tail end of the buffer, which is clearly bad.



On 04/13/2018 05:14 PM, Robin Murphy wrote:
dma_unmap_sg() should be called with the same number of entries
originally passed to dma_map_sg(), not the number it returned, which may
be fewer. Admittedly this driver probably never runs on non-coherent
architectures where getting that wrong could lead to data loss, but it's
always good to be correct, and it's trivially easy to fix by just
restoring the SG table state before the call instead of afterwards.

Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>

Found by inspection while poking around TTM users.

 drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
index 21111fd091f9..971223d39469 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
@@ -369,9 +369,9 @@ static void vmw_ttm_unmap_from_dma(struct vmw_ttm_tt *vmw_tt)
ÂÂÂÂÂ struct device *dev = vmw_tt->dev_priv->dev->dev;
+ÂÂÂ vmw_tt->sgt.nents = vmw_tt->sgt.orig_nents;
ÂÂÂÂÂ dma_unmap_sg(dev, vmw_tt->sgt.sgl, vmw_tt->sgt.nents,
-ÂÂÂ vmw_tt->sgt.nents = vmw_tt->sgt.orig_nents;