From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 15/20] drm: simplify drm_*_set_unique() Date: Fri, 29 Aug 2014 14:39:20 +0200 Message-ID: <20140829123917.GO17519@ulmo> References: <1409307166-12396-1-git-send-email-dh.herrmann@gmail.com> <1409307166-12396-16-git-send-email-dh.herrmann@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1765722783==" Return-path: Received: from mail-wg0-f47.google.com (mail-wg0-f47.google.com [74.125.82.47]) by gabe.freedesktop.org (Postfix) with ESMTP id AD8CD6E6D0 for ; Fri, 29 Aug 2014 05:39:22 -0700 (PDT) Received: by mail-wg0-f47.google.com with SMTP id z12so2052767wgg.18 for ; Fri, 29 Aug 2014 05:39:21 -0700 (PDT) In-Reply-To: <1409307166-12396-16-git-send-email-dh.herrmann@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: David Herrmann Cc: Daniel Vetter , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1765722783== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="G+DT6X5ssgZ56VG3" Content-Disposition: inline --G+DT6X5ssgZ56VG3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Aug 29, 2014 at 12:12:41PM +0200, David Herrmann wrote: > Lets use kasprintf() to avoid pre-allocating the buffer. This is really > nothing to optimize for speed and the input is trusted, so kasprintf() is > just fine. >=20 > Signed-off-by: David Herrmann > --- > drivers/gpu/drm/drm_pci.c | 30 ++++++++---------------------- > drivers/gpu/drm/drm_platform.c | 31 ++++++++----------------------- > 2 files changed, 16 insertions(+), 45 deletions(-) >=20 > diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c > index 020cfd9..8efea6b 100644 > --- a/drivers/gpu/drm/drm_pci.c > +++ b/drivers/gpu/drm/drm_pci.c > @@ -129,31 +129,17 @@ static int drm_get_pci_domain(struct drm_device *de= v) > =20 > static int drm_pci_set_busid(struct drm_device *dev, struct drm_master *= master) > { > - int len, ret; > - master->unique_len =3D 40; > - master->unique_size =3D master->unique_len; > - master->unique =3D kmalloc(master->unique_size, GFP_KERNEL); > - if (master->unique =3D=3D NULL) > + master->unique =3D kasprintf(GFP_KERNEL, "pci:%04x:%02x:%02x.%d", > + drm_get_pci_domain(dev), > + dev->pdev->bus->number, > + PCI_SLOT(dev->pdev->devfn), > + PCI_FUNC(dev->pdev->devfn)); I think we've been trying to standardize on aligning parameters on subsequent lines with the first parameter on the first line. [...] > + master->unique_len =3D strlen(master->unique); > + master->unique_size =3D master->unique_len + 1; [...] > diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platfor= m.c [...] > static int drm_platform_set_busid(struct drm_device *dev, struct drm_mas= ter *master) > { > - int len, ret, id; > - > - master->unique_len =3D 13 + strlen(dev->platformdev->name); > - master->unique_size =3D master->unique_len; > - master->unique =3D kmalloc(master->unique_len + 1, GFP_KERNEL); > - > - if (master->unique =3D=3D NULL) > - return -ENOMEM; > + int id; > =20 > id =3D dev->platformdev->id; > - > - /* if only a single instance of the platform device, id will be > - * set to -1.. use 0 instead to avoid a funny looking bus-id: > - */ > - if (id =3D=3D -1) > + if (id < 0) > id =3D 0; Perhaps collapse all of the above into: int id =3D (dev->platformdev->id < 0) ? 0 : dev->platformdev->id; ? Not that it matters much. I suspect we could easily remove all traces of this particular function in a next step. > - len =3D snprintf(master->unique, master->unique_len, > - "platform:%s:%02d", dev->platformdev->name, id); > - > - if (len > master->unique_len) { > - DRM_ERROR("Unique buffer overflowed\n"); > - ret =3D -EINVAL; > - goto err; > - } > + master->unique =3D kasprintf(GFP_KERNEL, "platform:%s:%02d", > + dev->platformdev->name, id); > + if (!master->unique) > + return -ENOMEM; > =20 > + master->unique_len =3D strlen(master->unique); > + master->unique_size =3D master->unique_len; unique_size is weird. It seems to me like it should always be unique_len + 1. Why drm_platform_bus should be special escapes me. Also, after this patch it seems to be completely unused, so perhaps we should just drop it. All of those comments can either be addressed in a separate patch (or ignored), though, so: Reviewed-by: Thierry Reding --G+DT6X5ssgZ56VG3 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUAHR1AAoJEN0jrNd/PrOhQnkP/3zYR0rOdKiwSnT+KvO30nys sv4DpA0GNUiHDNK8m4/d0kwuzG3rlpIIAHD3HU4BAVs42u8aJju9oTjUgqx/lsFM TG8ncsAoysD+0+gyvYByxUVC70oUFkYg2nF9LQkeae98T+JlxJMFIK5GoLBPgeur zob6KWQfK2KSVcQEu5/lnkcMcxy2SnFryau95b4d9QrGBUVe+y1sm3LmcE9m4Yy4 HqRW5YyK05Tk+i874fWanQC4te4ITl79eN8DJjjFXGazT+IcudYj946tBHYDx3z/ 0ZDX0w+aTHBK3HJh29EQydyc1dMT0WsOVxUGU+U//EWoB3bprbmovhv70kzhtQyh LEs5L0HyJQz14Vu4f8dG07PXfXPyxT1NK1T2PvFQP0fadJFWzup5o/YvvAYMWRaA b8C6yiuoogFShkuak17UthZ1HXxJ+EiUtOt4MzcCEyfgep9pr22+06h0XYFKQFyH JtZMX4+Al6Ub29Ay4qOgQ1fC3mJT5Gos5pkIH+Brdi5V+Mu5ZnjcqtlOxBJYPIxu bRxFe3ydBJOBuGhCNUxL1M+LQTYgr0d7Aes/VFA5ghJ9VcouYSEI7BNe2YP6tZ7J uCjK9ceU/Qlo7rTPQ1EeuSDxai7IcOofHsZWXfXmi6rZHqnAabMtV5ItKqiyDNX2 ct+vOioDhIE38rqXe44a =MJcn -----END PGP SIGNATURE----- --G+DT6X5ssgZ56VG3-- --===============1765722783== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1765722783==--