| Message ID | 1455875288-4370-13-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomi, Thank you for the patch. On Friday 19 February 2016 11:47:47 Tomi Valkeinen wrote: > omap_gem_dma_sync() calls dma_map_page() but does not check the possible > error with dma_mapping_error(). If DMA-API debugging is enabled, the > debug layer will give a warning if dma_mapping_error() has not been > used. > > This patch adds proper error handling to omap_gem_dma_sync(). > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_gem.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c > b/drivers/gpu/drm/omapdrm/omap_gem.c index 7098190815f1..a60c30a59f7e > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_gem.c > +++ b/drivers/gpu/drm/omapdrm/omap_gem.c > @@ -775,9 +775,18 @@ void omap_gem_dma_sync(struct drm_gem_object *obj, > > for (i = 0; i < npages; i++) { > if (!omap_obj->addrs[i]) { > - omap_obj->addrs[i] = dma_map_page(dev->dev, pages[i], 0, > + dma_addr_t addr; > + > + addr = dma_map_page(dev->dev, pages[i], 0, > PAGE_SIZE, DMA_BIDIRECTIONAL); > + > + if (dma_mapping_error(dev->dev, addr)) { > + dev_warn(dev->dev, "omap_gem_dma_sync: dma_map_page failed\n"); Same comment as for the previous patch. No need to unmap ? > + break; > + } > + > dirty = true; > + omap_obj->addrs[i] = addr; > } > }
On 24/02/16 00:14, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Friday 19 February 2016 11:47:47 Tomi Valkeinen wrote: >> omap_gem_dma_sync() calls dma_map_page() but does not check the possible >> error with dma_mapping_error(). If DMA-API debugging is enabled, the >> debug layer will give a warning if dma_mapping_error() has not been >> used. >> >> This patch adds proper error handling to omap_gem_dma_sync(). >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> --- >> drivers/gpu/drm/omapdrm/omap_gem.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 7098190815f1..a60c30a59f7e >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c >> @@ -775,9 +775,18 @@ void omap_gem_dma_sync(struct drm_gem_object *obj, >> >> for (i = 0; i < npages; i++) { >> if (!omap_obj->addrs[i]) { >> - omap_obj->addrs[i] = dma_map_page(dev->dev, pages[i], 0, >> + dma_addr_t addr; >> + >> + addr = dma_map_page(dev->dev, pages[i], 0, >> PAGE_SIZE, DMA_BIDIRECTIONAL); >> + >> + if (dma_mapping_error(dev->dev, addr)) { >> + dev_warn(dev->dev, "omap_gem_dma_sync: dma_map_page > failed\n"); > > Same comment as for the previous patch. > > No need to unmap ? I don't know... Maybe, maybe not. The function doesn't return any error, and the mapped pages are stored in omap_obj->addrs[]. So, if the failure was temporary, next time we have already mapped the pages that did succeed. If the failure was not temporary, well, we don't pass any error anyway, so the pages have to be unmapped somewhere in any case. So possibly we could unmap, but I don't see us leaking anything even if the dma_map_page fails. Tomi
Hi Tomi, On Thursday 25 February 2016 17:45:48 Tomi Valkeinen wrote: > On 24/02/16 00:14, Laurent Pinchart wrote: > > On Friday 19 February 2016 11:47:47 Tomi Valkeinen wrote: > >> omap_gem_dma_sync() calls dma_map_page() but does not check the possible > >> error with dma_mapping_error(). If DMA-API debugging is enabled, the > >> debug layer will give a warning if dma_mapping_error() has not been > >> used. > >> > >> This patch adds proper error handling to omap_gem_dma_sync(). > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > >> --- > >> > >> drivers/gpu/drm/omapdrm/omap_gem.c | 11 ++++++++++- > >> 1 file changed, 10 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c > >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 7098190815f1..a60c30a59f7e > >> 100644 > >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c > >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c > >> @@ -775,9 +775,18 @@ void omap_gem_dma_sync(struct drm_gem_object *obj, > >> > >> for (i = 0; i < npages; i++) { > >> if (!omap_obj->addrs[i]) { > >> - omap_obj->addrs[i] = dma_map_page(dev->dev, pages[i], 0, > >> + dma_addr_t addr; > >> + > >> + addr = dma_map_page(dev->dev, pages[i], 0, > >> PAGE_SIZE, DMA_BIDIRECTIONAL); > >> + > >> + if (dma_mapping_error(dev->dev, addr)) { > >> + dev_warn(dev->dev, "omap_gem_dma_sync: dma_map_page > > failed\n"); > > > > Same comment as for the previous patch. > > > > No need to unmap ? > > I don't know... Maybe, maybe not. > > The function doesn't return any error, and the mapped pages are stored > in omap_obj->addrs[]. So, if the failure was temporary, next time we > have already mapped the pages that did succeed. If the failure was not > temporary, well, we don't pass any error anyway, so the pages have to be > unmapped somewhere in any case. > > So possibly we could unmap, but I don't see us leaking anything even if > the dma_map_page fails. OK. It's not a new issue anyway, so I'm fine with the patch (after fixing the dev_warn()).
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 7098190815f1..a60c30a59f7e 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -775,9 +775,18 @@ void omap_gem_dma_sync(struct drm_gem_object *obj, for (i = 0; i < npages; i++) { if (!omap_obj->addrs[i]) { - omap_obj->addrs[i] = dma_map_page(dev->dev, pages[i], 0, + dma_addr_t addr; + + addr = dma_map_page(dev->dev, pages[i], 0, PAGE_SIZE, DMA_BIDIRECTIONAL); + + if (dma_mapping_error(dev->dev, addr)) { + dev_warn(dev->dev, "omap_gem_dma_sync: dma_map_page failed\n"); + break; + } + dirty = true; + omap_obj->addrs[i] = addr; } }
omap_gem_dma_sync() calls dma_map_page() but does not check the possible error with dma_mapping_error(). If DMA-API debugging is enabled, the debug layer will give a warning if dma_mapping_error() has not been used. This patch adds proper error handling to omap_gem_dma_sync(). Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/omap_gem.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)