diff mbox

[15/20] drm: simplify drm_*_set_unique()

Message ID 1409307166-12396-16-git-send-email-dh.herrmann@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Herrmann Aug. 29, 2014, 10:12 a.m. UTC 
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.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_pci.c      | 30 ++++++++----------------------
 drivers/gpu/drm/drm_platform.c | 31 ++++++++-----------------------
 2 files changed, 16 insertions(+), 45 deletions(-)

Comments

Thierry Reding Aug. 29, 2014, 12:39 p.m. UTC | #1 
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.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/gpu/drm/drm_pci.c      | 30 ++++++++----------------------
>  drivers/gpu/drm/drm_platform.c | 31 ++++++++-----------------------
>  2 files changed, 16 insertions(+), 45 deletions(-)
> 
> 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 *dev)
>  
>  static int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master)
>  {
> -	int len, ret;
> -	master->unique_len = 40;
> -	master->unique_size = master->unique_len;
> -	master->unique = kmalloc(master->unique_size, GFP_KERNEL);
> -	if (master->unique == NULL)
> +	master->unique = 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 = strlen(master->unique);
> +	master->unique_size = master->unique_len + 1;

[...]
> diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
[...]
>  static int drm_platform_set_busid(struct drm_device *dev, struct drm_master *master)
>  {
> -	int len, ret, id;
> -
> -	master->unique_len = 13 + strlen(dev->platformdev->name);
> -	master->unique_size = master->unique_len;
> -	master->unique = kmalloc(master->unique_len + 1, GFP_KERNEL);
> -
> -	if (master->unique == NULL)
> -		return -ENOMEM;
> +	int id;
>  
>  	id = 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 == -1)
> +	if (id < 0)
>  		id = 0;

Perhaps collapse all of the above into:

	int id = (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 = 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 = -EINVAL;
> -		goto err;
> -	}
> +	master->unique = kasprintf(GFP_KERNEL, "platform:%s:%02d",
> +						dev->platformdev->name, id);
> +	if (!master->unique)
> +		return -ENOMEM;
>  
> +	master->unique_len = strlen(master->unique);
> +	master->unique_size = 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 <treding@nvidia.com>
Daniel Vetter Aug. 29, 2014, 12:58 p.m. UTC | #2 
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.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_pci.c      | 30 ++++++++----------------------
>  drivers/gpu/drm/drm_platform.c | 31 ++++++++-----------------------
>  2 files changed, 16 insertions(+), 45 deletions(-)
> 
> 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 *dev)
>  
>  static int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master)
>  {
> -	int len, ret;
> -	master->unique_len = 40;
> -	master->unique_size = master->unique_len;
> -	master->unique = kmalloc(master->unique_size, GFP_KERNEL);
> -	if (master->unique == NULL)
> +	master->unique = 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));
> +	if (!master->unique)
>  		return -ENOMEM;
>  
> -
> -	len = snprintf(master->unique, master->unique_len,
> -		       "pci:%04x:%02x:%02x.%d",
> -		       drm_get_pci_domain(dev),
> -		       dev->pdev->bus->number,
> -		       PCI_SLOT(dev->pdev->devfn),
> -		       PCI_FUNC(dev->pdev->devfn));
> -
> -	if (len >= master->unique_len) {
> -		DRM_ERROR("buffer overflow");
> -		ret = -EINVAL;
> -		goto err;
> -	} else
> -		master->unique_len = len;
> -
> +	master->unique_len = strlen(master->unique);
> +	master->unique_size = master->unique_len + 1;
>  	return 0;
> -err:
> -	return ret;
>  }
>  
>  int drm_pci_set_unique(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
> index d5b76f1..0c09ddd 100644
> --- a/drivers/gpu/drm/drm_platform.c
> +++ b/drivers/gpu/drm/drm_platform.c
> @@ -70,35 +70,20 @@ err_free:
>  
>  static int drm_platform_set_busid(struct drm_device *dev, struct drm_master *master)
>  {
> -	int len, ret, id;
> -
> -	master->unique_len = 13 + strlen(dev->platformdev->name);
> -	master->unique_size = master->unique_len;
> -	master->unique = kmalloc(master->unique_len + 1, GFP_KERNEL);
> -
> -	if (master->unique == NULL)
> -		return -ENOMEM;
> +	int id;
>  
>  	id = 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 == -1)
> +	if (id < 0)
>  		id = 0;
>  
> -	len = 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 = -EINVAL;
> -		goto err;
> -	}
> +	master->unique = kasprintf(GFP_KERNEL, "platform:%s:%02d",
> +						dev->platformdev->name, id);
> +	if (!master->unique)
> +		return -ENOMEM;
>  
> +	master->unique_len = strlen(master->unique);
> +	master->unique_size = master->unique_len;
>  	return 0;
> -err:
> -	return ret;
>  }
>  
>  static struct drm_bus drm_platform_bus = {
> -- 
> 2.1.0
>
diff mbox

Patch

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 *dev)
 
 static int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master)
 {
-	int len, ret;
-	master->unique_len = 40;
-	master->unique_size = master->unique_len;
-	master->unique = kmalloc(master->unique_size, GFP_KERNEL);
-	if (master->unique == NULL)
+	master->unique = 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));
+	if (!master->unique)
 		return -ENOMEM;
 
-
-	len = snprintf(master->unique, master->unique_len,
-		       "pci:%04x:%02x:%02x.%d",
-		       drm_get_pci_domain(dev),
-		       dev->pdev->bus->number,
-		       PCI_SLOT(dev->pdev->devfn),
-		       PCI_FUNC(dev->pdev->devfn));
-
-	if (len >= master->unique_len) {
-		DRM_ERROR("buffer overflow");
-		ret = -EINVAL;
-		goto err;
-	} else
-		master->unique_len = len;
-
+	master->unique_len = strlen(master->unique);
+	master->unique_size = master->unique_len + 1;
 	return 0;
-err:
-	return ret;
 }
 
 int drm_pci_set_unique(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
index d5b76f1..0c09ddd 100644
--- a/drivers/gpu/drm/drm_platform.c
+++ b/drivers/gpu/drm/drm_platform.c
@@ -70,35 +70,20 @@  err_free:
 
 static int drm_platform_set_busid(struct drm_device *dev, struct drm_master *master)
 {
-	int len, ret, id;
-
-	master->unique_len = 13 + strlen(dev->platformdev->name);
-	master->unique_size = master->unique_len;
-	master->unique = kmalloc(master->unique_len + 1, GFP_KERNEL);
-
-	if (master->unique == NULL)
-		return -ENOMEM;
+	int id;
 
 	id = 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 == -1)
+	if (id < 0)
 		id = 0;
 
-	len = 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 = -EINVAL;
-		goto err;
-	}
+	master->unique = kasprintf(GFP_KERNEL, "platform:%s:%02d",
+						dev->platformdev->name, id);
+	if (!master->unique)
+		return -ENOMEM;
 
+	master->unique_len = strlen(master->unique);
+	master->unique_size = master->unique_len;
 	return 0;
-err:
-	return ret;
 }
 
 static struct drm_bus drm_platform_bus = {