From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCH 11/33] drm/omap: use dma_mapping_error in omap_gem_attach_pages Date: Thu, 25 Feb 2016 17:39:35 +0200 Message-ID: <56CF2037.1090008@ti.com> References: <1455875288-4370-1-git-send-email-tomi.valkeinen@ti.com> <1455875288-4370-12-git-send-email-tomi.valkeinen@ti.com> <7707410.Pkpu1GDU6i@avalon> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1803562227==" Return-path: Received: from comal.ext.ti.com (comal.ext.ti.com [198.47.26.152]) by gabe.freedesktop.org (Postfix) with ESMTPS id 91E146E8D9 for ; Thu, 25 Feb 2016 15:39:41 +0000 (UTC) In-Reply-To: <7707410.Pkpu1GDU6i@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1803562227== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="L25Nli9EwL4aiQnIdGkEiKdGknM5BVNi2" --L25Nli9EwL4aiQnIdGkEiKdGknM5BVNi2 Content-Type: multipart/mixed; boundary="mV8eLA6NqjOdOTmkS0Rd0tu1S2JUl1OFX" From: Tomi Valkeinen To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org, Rob Clark Message-ID: <56CF2037.1090008@ti.com> Subject: Re: [PATCH 11/33] drm/omap: use dma_mapping_error in omap_gem_attach_pages References: <1455875288-4370-1-git-send-email-tomi.valkeinen@ti.com> <1455875288-4370-12-git-send-email-tomi.valkeinen@ti.com> <7707410.Pkpu1GDU6i@avalon> In-Reply-To: <7707410.Pkpu1GDU6i@avalon> --mV8eLA6NqjOdOTmkS0Rd0tu1S2JUl1OFX Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 24/02/16 00:10, Laurent Pinchart wrote: > Hi Tomi, >=20 > Thank you for the patch. >=20 > On Friday 19 February 2016 11:47:46 Tomi Valkeinen wrote: >> omap_gem_attach_pages() 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() ha= s >> not been used. >> >> This patch adds proper error handling to omap_gem_attach_pages(). >> >> Signed-off-by: Tomi Valkeinen >> --- >> drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 8495a1a4b617..7098190815f1 >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c >> @@ -264,6 +264,18 @@ static int omap_gem_attach_pages(struct drm_gem_o= bject >> *obj) for (i =3D 0; i < npages; i++) { >> addrs[i] =3D dma_map_page(dev->dev, pages[i], >> 0, PAGE_SIZE, DMA_BIDIRECTIONAL); >> + >> + if (dma_mapping_error(dev->dev, addrs[i])) { >> + dev_warn(dev->dev, "omap_gem_attach_pages: dma_map_page=20 > failed\n"); >=20 > Using dev_warn(dev->dev, "%s: failed to map page\n", __func__); and pro= per=20 > line breaks you'll have no trouble complying with the 80 characters per= line=20 > limit :-) Ok. >> + >> + for (i =3D i - 1; i >=3D 0; --i) { >=20 > Maybe i-- instead of i =3D i - 1 ? Hmm I don't know... I do like assignment in the initializer more than i--. And why i--? Why not --i? =3D) >> + dma_unmap_page(dev->dev, addrs[i], >> + PAGE_SIZE, DMA_BIDIRECTIONAL); >> + } >> + >> + ret =3D -ENOMEM; >> + goto free_addrs; >> + } >> } >> } else { >> addrs =3D kzalloc(npages * sizeof(*addrs), GFP_KERNEL); >> @@ -277,6 +289,8 @@ static int omap_gem_attach_pages(struct drm_gem_ob= ject >> *obj) omap_obj->pages =3D pages; >> >> return 0; >> +free_addrs: >> + kfree(addrs); >> >=20 > I'd move this blank line before free_addrs. Yes, that makes it cleaner. Tomi --mV8eLA6NqjOdOTmkS0Rd0tu1S2JUl1OFX-- --L25Nli9EwL4aiQnIdGkEiKdGknM5BVNi2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWzyA3AAoJEPo9qoy8lh71Pf4P+wflJi9eyNaOXlVlGHzuqI1K ij1HcAUHZQ/wiSrPeZ0pOjqQTFWDlyY63z2lHXbQiiPm6GEYsOXpK8bU+QR1xR0a 1tGsIMipxLBr45yMtx8fdXOcq7soGn4HHkE8n2gzkAa4Ff5V5RlMUngCMSHdUVd8 MVcav5ASjj21tafwxqXLEuyOBXMgPWBWfCGsr4By5WnyzaBcNKhf8wlHTWXQLDiJ bIUHMwh0TwCUe4/qzPh/dXo3WLPcWN8uNpm6DVqbGgvU/TDpeHgxzwQ96vi1RpiT rPQKR+AKL/hA22pBCNS7T2eNVyTB5WeKasjlQoFuSjpMQZVJFFC3ShHS/3V0HYro r/J0QCREG/ufXXvQ6gm2MNoAFE+HmMWhVcxXMsoErDUaQePdXDfSkkQTLJ9trvHa nAxu3Jg7pZwKELigNBER+s2MSSeEL98VbBSlmQed41kA9yBbPT6khdjewUsA71RW TZAYxy40SydtSQb4FlPep3Eb/tvXmW61ZjoVmMdKqmQhsZ6RFC4qgiBWAg5tBzev HnoI2yuf/eIfrlNXIWoIQ4XWpDg4MPVTfoVq4kqbCcQMS5ifOe68H/vXBFCF2uBz ltHnY9uDoJXU2bGyrnU9Lztcl8Ffp185HtbenG7NH7flkPScBucQ8Hu+HoM5wws+ MwaLfWQveaHtlVQW6kT5 =5icL -----END PGP SIGNATURE----- --L25Nli9EwL4aiQnIdGkEiKdGknM5BVNi2-- --===============1803562227== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1803562227==--