From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 22 Jul 2016 17:34:32 +0200
Further update suggestions were taken into account after a patch was applied from static source code analysis.
Markus Elfring (4): Delete unnecessary checks before two function calls Delete unnecessary if statement in __etnaviv_gem_new() Rename jump labels Optimize error handling in etnaviv_gem_new_userptr()
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 36 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 23 deletions(-)
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 22 Jul 2016 15:56:15 +0200
The functions drm_gem_object_unreference_unlocked() and vunmap() perform also input parameter validation. Thus the tests around their calls are not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 8c6f750..8eee742 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -535,8 +535,7 @@ void etnaviv_gem_describe_objects(struct etnaviv_drm_private *priv,
static void etnaviv_gem_shmem_release(struct etnaviv_gem_object *etnaviv_obj) { - if (etnaviv_obj->vaddr) - vunmap(etnaviv_obj->vaddr); + vunmap(etnaviv_obj->vaddr); put_pages(etnaviv_obj); }
@@ -670,9 +669,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, return obj;
fail: - if (obj) - drm_gem_object_unreference_unlocked(obj); - + drm_gem_object_unreference_unlocked(obj); return ERR_PTR(ret); }
On Fri, Jul 22, 2016 at 11:47 AM, SF Markus Elfring elfring@users.sourceforge.net wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 22 Jul 2016 15:56:15 +0200
The functions drm_gem_object_unreference_unlocked() and vunmap() perform also input parameter validation. Thus the tests around their calls are not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
Applied to drm-misc. In the future, could you please format your subjects starting with "drm/<driver>"? I've fixed this one and the next, but it'd be nice not to have to do that going forward.
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 8c6f750..8eee742 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -535,8 +535,7 @@ void etnaviv_gem_describe_objects(struct etnaviv_drm_private *priv,
static void etnaviv_gem_shmem_release(struct etnaviv_gem_object *etnaviv_obj) {
if (etnaviv_obj->vaddr)
vunmap(etnaviv_obj->vaddr);
vunmap(etnaviv_obj->vaddr); put_pages(etnaviv_obj);
}
@@ -670,9 +669,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, return obj;
fail:
if (obj)
drm_gem_object_unreference_unlocked(obj);
drm_gem_object_unreference_unlocked(obj); return ERR_PTR(ret);
}
-- 2.9.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 22 Jul 2016 16:45:22 +0200
Move a return statement into a block for successful function execution. Omit a duplicate check for the local variable "ret" then at the end.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 8eee742..851a8ba 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -661,13 +661,9 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, */ mapping = obj->filp->f_mapping; mapping_set_gfp_mask(mapping, GFP_HIGHUSER); + return obj; }
- if (ret) - goto fail; - - return obj; - fail: drm_gem_object_unreference_unlocked(obj); return ERR_PTR(ret);
Am 22.07.2016 17:48, schrieb SF Markus Elfring:
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 22 Jul 2016 16:45:22 +0200
Move a return statement into a block for successful function execution. Omit a duplicate check for the local variable "ret" then at the end.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 8eee742..851a8ba 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -661,13 +661,9 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, */ mapping = obj->filp->f_mapping; mapping_set_gfp_mask(mapping, GFP_HIGHUSER);
}return obj;
- if (ret)
goto fail;
- return obj;
fail: drm_gem_object_unreference_unlocked(obj); return ERR_PTR(ret);
From the program flow an readability it would be more nice the branch on error
ret = drm_gem_object_init(dev, obj, size); if (ret) goto fail;
just m 2 cents
re, wh
Move things around a little in __etnaviv_gem_new() to make it more readable.
Reported-by: Markus Elfring elfring@users.sourceforge.net Reported-by: walter harms wharms@bfs.de Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index df9bcba..7d13628 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -640,6 +640,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, u32 size, u32 flags) { struct drm_gem_object *obj = NULL; + struct address_space *mapping; int ret;
size = PAGE_ALIGN(size); @@ -650,23 +651,19 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, goto fail;
ret = drm_gem_object_init(dev, obj, size); - if (ret == 0) { - struct address_space *mapping; - - /* - * Our buffers are kept pinned, so allocating them - * from the MOVABLE zone is a really bad idea, and - * conflicts with CMA. See coments above new_inode() - * why this is required _and_ expected if you're - * going to pin these pages. - */ - mapping = file_inode(obj->filp)->i_mapping; - mapping_set_gfp_mask(mapping, GFP_HIGHUSER); - } - if (ret) goto fail;
+ /* + * Our buffers are kept pinned, so allocating them + * from the MOVABLE zone is a really bad idea, and + * conflicts with CMA. See coments above new_inode() + * why this is required _and_ expected if you're + * going to pin these pages. + */ + mapping = file_inode(obj->filp)->i_mapping; + mapping_set_gfp_mask(mapping, GFP_HIGHUSER); + return obj;
fail:
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 22 Jul 2016 16:51:00 +0200
Adjust jump targets according to the Linux coding style convention.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 851a8ba..0a5c00c 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -308,17 +308,17 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get( mapping = NULL; mutex_unlock(&gpu->mmu->lock); if (mapping) - goto out; + goto unlock; } else { mapping->use += 1; - goto out; + goto unlock; } }
pages = etnaviv_gem_get_pages(etnaviv_obj); if (IS_ERR(pages)) { ret = PTR_ERR(pages); - goto out; + goto unlock; }
/* @@ -330,7 +330,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get( mapping = kzalloc(sizeof(*mapping), GFP_KERNEL); if (!mapping) { ret = -ENOMEM; - goto out; + goto unlock; }
INIT_LIST_HEAD(&mapping->scan_node); @@ -349,7 +349,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get( else list_add_tail(&mapping->obj_node, &etnaviv_obj->vram_list);
-out: +unlock: mutex_unlock(&etnaviv_obj->lock);
if (ret) @@ -646,7 +646,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, ret = etnaviv_gem_new_impl(dev, size, flags, NULL, &etnaviv_gem_shmem_ops, &obj); if (ret) - goto fail; + goto unreference;
ret = drm_gem_object_init(dev, obj, size); if (ret == 0) { @@ -664,7 +664,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, return obj; }
-fail: +unreference: drm_gem_object_unreference_unlocked(obj); return ERR_PTR(ret); }
On Fri, Jul 22, 2016 at 11:50 AM, SF Markus Elfring elfring@users.sourceforge.net wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 22 Jul 2016 16:51:00 +0200
Adjust jump targets according to the Linux coding style convention.
I'm not convinced the old labels are really that bad, tbh.
Sean
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 851a8ba..0a5c00c 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -308,17 +308,17 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get( mapping = NULL; mutex_unlock(&gpu->mmu->lock); if (mapping)
goto out;
goto unlock; } else { mapping->use += 1;
goto out;
goto unlock; } } pages = etnaviv_gem_get_pages(etnaviv_obj); if (IS_ERR(pages)) { ret = PTR_ERR(pages);
goto out;
goto unlock; } /*
@@ -330,7 +330,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get( mapping = kzalloc(sizeof(*mapping), GFP_KERNEL); if (!mapping) { ret = -ENOMEM;
goto out;
goto unlock; } INIT_LIST_HEAD(&mapping->scan_node);
@@ -349,7 +349,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get( else list_add_tail(&mapping->obj_node, &etnaviv_obj->vram_list);
-out: +unlock: mutex_unlock(&etnaviv_obj->lock);
if (ret)
@@ -646,7 +646,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, ret = etnaviv_gem_new_impl(dev, size, flags, NULL, &etnaviv_gem_shmem_ops, &obj); if (ret)
goto fail;
goto unreference; ret = drm_gem_object_init(dev, obj, size); if (ret == 0) {
@@ -664,7 +664,7 @@ static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, return obj; }
-fail: +unreference: drm_gem_object_unreference_unlocked(obj); return ERR_PTR(ret); } -- 2.9.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 22 Jul 2016 17:17:48 +0200
Refactor this function implementation so that the drm_gem_object_unreference_unlocked() function will only be called once in case of a failure according to the Linux coding style recommendation for centralized exiting of functions.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 0a5c00c..007577c 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -909,15 +909,12 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file, get_task_struct(current);
ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base); - if (ret) { - drm_gem_object_unreference_unlocked(&etnaviv_obj->base); - return ret; - } + if (ret) + goto unreference;
ret = drm_gem_handle_create(file, &etnaviv_obj->base, handle); - +unreference: /* drop reference from allocate - handle holds it now */ drm_gem_object_unreference_unlocked(&etnaviv_obj->base); - return ret; }
On Fri, Jul 22, 2016 at 11:51 AM, SF Markus Elfring elfring@users.sourceforge.net wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Fri, 22 Jul 2016 17:17:48 +0200
Refactor this function implementation so that the drm_gem_object_unreference_unlocked() function will only be called once in case of a failure according to the Linux coding style recommendation for centralized exiting of functions.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
Applied to drm-misc. In the future, could you please format your subjects starting with "drm/<driver>"? I've fixed this one and the first, but it'd be nice not to have to do that going forward.
Sean
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 0a5c00c..007577c 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -909,15 +909,12 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file, get_task_struct(current);
ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
if (ret) {
drm_gem_object_unreference_unlocked(&etnaviv_obj->base);
return ret;
}
if (ret)
goto unreference; ret = drm_gem_handle_create(file, &etnaviv_obj->base, handle);
+unreference: /* drop reference from allocate - handle holds it now */ drm_gem_object_unreference_unlocked(&etnaviv_obj->base);
return ret;
}
2.9.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org