[PATCH 0/5] Remove redundant NULL checks.

[PATCH 0/5] Remove redundant NULL checks.

[Cyril Roelandt]

Remove redundant NULL checks before calls to functions that are equivalent to a
no-op when run on a NULL pointer.

These patches were generated by the following semantic patch, and manually
reviewed:

<smpl>
@r@
identifier noop_func;
identifier param;
type T;
@@
noop_func (T *param) {
	...
	if (!param) return;
	...
}

@@
identifier r.noop_func;
expression E;
statement S;
@@
(
- if (E) noop_func(E);
+ noop_func(E);
|
- if (E) { noop_func(E); E = NULL; }
+ noop_func(E);
+ E = NULL;
)
</smpl>

Regards,
Cyril Roelandt
---

Cyril Roelandt (5):
  radeon: Remove redundant NULL check before radeon_i2c_destroy().
  iommu: remove redundant NULL check before dma_ops_domain_free().
  staging: dgrp: remove redundant NULL check before
    unregister_dgrp_device().
  staging: tidspbridge: remove redundant NULL check before
    delete_msg_mgr().
  xen: remove redundant NULL check before unregister_and_remove_pcpu().

 drivers/gpu/drm/radeon/radeon_i2c.c       |    6 ++----
 drivers/iommu/amd_iommu.c                 |    3 +--
 drivers/staging/dgrp/dgrp_specproc.c      |    9 +++------
 drivers/staging/tidspbridge/core/msg_sm.c |    3 +--
 drivers/xen/pcpu.c                        |    3 +--
 5 files changed, 8 insertions(+), 16 deletions(-)

-- 
1.7.10.4

[PATCH 0/5] Remove redundant NULL checks.

[Cyril Roelandt]

Remove redundant NULL checks before calls to functions that are equivalent to a
no-op when run on a NULL pointer.

These patches were generated by the following semantic patch, and manually
reviewed:

<smpl>
@r@
identifier noop_func;
identifier param;
type T;
@@
noop_func (T *param) {
	...
	if (!param) return;
	...
}

@@
identifier r.noop_func;
expression E;
statement S;
@@
(
- if (E) noop_func(E);
+ noop_func(E);
|
- if (E) { noop_func(E); E = NULL; }
+ noop_func(E);
+ E = NULL;
)
</smpl>

Regards,
Cyril Roelandt
---

Cyril Roelandt (5):
  radeon: Remove redundant NULL check before radeon_i2c_destroy().
  iommu: remove redundant NULL check before dma_ops_domain_free().
  staging: dgrp: remove redundant NULL check before
    unregister_dgrp_device().
  staging: tidspbridge: remove redundant NULL check before
    delete_msg_mgr().
  xen: remove redundant NULL check before unregister_and_remove_pcpu().

 drivers/gpu/drm/radeon/radeon_i2c.c       |    6 ++----
 drivers/iommu/amd_iommu.c                 |    3 +--
 drivers/staging/dgrp/dgrp_specproc.c      |    9 +++------
 drivers/staging/tidspbridge/core/msg_sm.c |    3 +--
 drivers/xen/pcpu.c                        |    3 +--
 5 files changed, 8 insertions(+), 16 deletions(-)

-- 
1.7.10.4

[PATCH 1/5] radeon: Remove redundant NULL check before radeon_i2c_destroy().

[Cyril Roelandt]

radeon_i2c_destroy on a NULL pointer is a no-op.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/gpu/drm/radeon/radeon_i2c.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
index fc60b74..850df44 100644
--- a/drivers/gpu/drm/radeon/radeon_i2c.c
+++ b/drivers/gpu/drm/radeon/radeon_i2c.c
@@ -1032,10 +1032,8 @@ void radeon_i2c_fini(struct radeon_device *rdev)
 	int i;
 
 	for (i = 0; i < RADEON_MAX_I2C_BUS; i++) {
-		if (rdev->i2c_bus[i]) {
-			radeon_i2c_destroy(rdev->i2c_bus[i]);
-			rdev->i2c_bus[i] = NULL;
-		}
+		radeon_i2c_destroy(rdev->i2c_bus[i]);
+		rdev->i2c_bus[i] = NULL;
 	}
 }
 
-- 
1.7.10.4

[PATCH 1/5] radeon: Remove redundant NULL check before radeon_i2c_destroy().

[Cyril Roelandt]

radeon_i2c_destroy on a NULL pointer is a no-op.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/gpu/drm/radeon/radeon_i2c.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
index fc60b74..850df44 100644
--- a/drivers/gpu/drm/radeon/radeon_i2c.c
+++ b/drivers/gpu/drm/radeon/radeon_i2c.c
@@ -1032,10 +1032,8 @@ void radeon_i2c_fini(struct radeon_device *rdev)
 	int i;
 
 	for (i = 0; i < RADEON_MAX_I2C_BUS; i++) {
-		if (rdev->i2c_bus[i]) {
-			radeon_i2c_destroy(rdev->i2c_bus[i]);
-			rdev->i2c_bus[i] = NULL;
-		}
+		radeon_i2c_destroy(rdev->i2c_bus[i]);
+		rdev->i2c_bus[i] = NULL;
 	}
 }
 
-- 
1.7.10.4

[PATCH 2/5] iommu: remove redundant NULL check before dma_ops_domain_free().

[Cyril Roelandt]

dma_ops_domain_free on a NULL pointer is a no-op, so the NULL check in
amd_iommu_init_dma_ops() can be removed.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/iommu/amd_iommu.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index d33eaaf..98f555d 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3187,8 +3187,7 @@ int __init amd_iommu_init_dma_ops(void)
 free_domains:
 
 	for_each_iommu(iommu) {
-		if (iommu->default_dom)
-			dma_ops_domain_free(iommu->default_dom);
+		dma_ops_domain_free(iommu->default_dom);
 	}
 
 	return ret;
-- 
1.7.10.4

[PATCH 2/5] iommu: remove redundant NULL check before dma_ops_domain_free().

[Cyril Roelandt]

dma_ops_domain_free on a NULL pointer is a no-op, so the NULL check in
amd_iommu_init_dma_ops() can be removed.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/iommu/amd_iommu.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index d33eaaf..98f555d 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3187,8 +3187,7 @@ int __init amd_iommu_init_dma_ops(void)
 free_domains:
 
 	for_each_iommu(iommu) {
-		if (iommu->default_dom)
-			dma_ops_domain_free(iommu->default_dom);
+		dma_ops_domain_free(iommu->default_dom);
 	}
 
 	return ret;
-- 
1.7.10.4

[PATCH 3/5] staging: dgrp: remove redundant NULL check before unregister_dgrp_device().

[Cyril Roelandt]

unregister_dgrp_device on a NULL pointer is a no-op, so the NULL checks in
dgrp_remove_nd() can be removed.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/staging/dgrp/dgrp_specproc.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/dgrp/dgrp_specproc.c b/drivers/staging/dgrp/dgrp_specproc.c
index dfdaede..13c7ccf 100644
--- a/drivers/staging/dgrp/dgrp_specproc.c
+++ b/drivers/staging/dgrp/dgrp_specproc.c
@@ -777,14 +777,11 @@ static int dgrp_remove_nd(struct nd_struct *nd)
 		dgrp_remove_node_class_sysfs_files(nd);
 	}
 
-	if (nd->nd_mon_de)
-		unregister_dgrp_device(nd->nd_mon_de);
+	unregister_dgrp_device(nd->nd_mon_de);
 
-	if (nd->nd_ports_de)
-		unregister_dgrp_device(nd->nd_ports_de);
+	unregister_dgrp_device(nd->nd_ports_de);
 
-	if (nd->nd_dpa_de)
-		unregister_dgrp_device(nd->nd_dpa_de);
+	unregister_dgrp_device(nd->nd_dpa_de);
 
 	dgrp_tty_uninit(nd);
 
-- 
1.7.10.4

[PATCH 3/5] staging: dgrp: remove redundant NULL check before unregister_dgrp_device().

[Cyril Roelandt]

unregister_dgrp_device on a NULL pointer is a no-op, so the NULL checks in
dgrp_remove_nd() can be removed.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/staging/dgrp/dgrp_specproc.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/dgrp/dgrp_specproc.c b/drivers/staging/dgrp/dgrp_specproc.c
index dfdaede..13c7ccf 100644
--- a/drivers/staging/dgrp/dgrp_specproc.c
+++ b/drivers/staging/dgrp/dgrp_specproc.c
@@ -777,14 +777,11 @@ static int dgrp_remove_nd(struct nd_struct *nd)
 		dgrp_remove_node_class_sysfs_files(nd);
 	}
 
-	if (nd->nd_mon_de)
-		unregister_dgrp_device(nd->nd_mon_de);
+	unregister_dgrp_device(nd->nd_mon_de);
 
-	if (nd->nd_ports_de)
-		unregister_dgrp_device(nd->nd_ports_de);
+	unregister_dgrp_device(nd->nd_ports_de);
 
-	if (nd->nd_dpa_de)
-		unregister_dgrp_device(nd->nd_dpa_de);
+	unregister_dgrp_device(nd->nd_dpa_de);
 
 	dgrp_tty_uninit(nd);
 
-- 
1.7.10.4

[PATCH 4/5] staging: tidspbridge: remove redundant NULL check before delete_msg_mgr().

[Cyril Roelandt]

delete_msg_mgr on a NULL pointer is a no-op, so the NULL check in
bridge_msg_delete can be removed.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/staging/tidspbridge/core/msg_sm.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/msg_sm.c b/drivers/staging/tidspbridge/core/msg_sm.c
index ce9557e..7b517eb 100644
--- a/drivers/staging/tidspbridge/core/msg_sm.c
+++ b/drivers/staging/tidspbridge/core/msg_sm.c
@@ -198,8 +198,7 @@ out_err:
  */
 void bridge_msg_delete(struct msg_mgr *hmsg_mgr)
 {
-	if (hmsg_mgr)
-		delete_msg_mgr(hmsg_mgr);
+	delete_msg_mgr(hmsg_mgr);
 }
 
 /*
-- 
1.7.10.4

[PATCH 4/5] staging: tidspbridge: remove redundant NULL check before delete_msg_mgr().

[Cyril Roelandt]

delete_msg_mgr on a NULL pointer is a no-op, so the NULL check in
bridge_msg_delete can be removed.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/staging/tidspbridge/core/msg_sm.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/msg_sm.c b/drivers/staging/tidspbridge/core/msg_sm.c
index ce9557e..7b517eb 100644
--- a/drivers/staging/tidspbridge/core/msg_sm.c
+++ b/drivers/staging/tidspbridge/core/msg_sm.c
@@ -198,8 +198,7 @@ out_err:
  */
 void bridge_msg_delete(struct msg_mgr *hmsg_mgr)
 {
-	if (hmsg_mgr)
-		delete_msg_mgr(hmsg_mgr);
+	delete_msg_mgr(hmsg_mgr);
 }
 
 /*
-- 
1.7.10.4

[PATCH 5/5] xen: remove redundant NULL check before unregister_and_remove_pcpu().

[Cyril Roelandt]

unregister_and_remove_pcpu on a NULL pointer is a no-op, so the NULL check in
sync_pcpu can be removed.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/xen/pcpu.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/xen/pcpu.c b/drivers/xen/pcpu.c
index 067fcfa..5a27a45 100644
--- a/drivers/xen/pcpu.c
+++ b/drivers/xen/pcpu.c
@@ -278,8 +278,7 @@ static int sync_pcpu(uint32_t cpu, uint32_t *max_cpu)
 	 * Only those at cpu present map has its sys interface.
 	 */
 	if (info->flags & XEN_PCPU_FLAGS_INVALID) {
-		if (pcpu)
-			unregister_and_remove_pcpu(pcpu);
+		unregister_and_remove_pcpu(pcpu);
 		return 0;
 	}
 
-- 
1.7.10.4

[PATCH 5/5] xen: remove redundant NULL check before unregister_and_remove_pcpu().

[Cyril Roelandt]

unregister_and_remove_pcpu on a NULL pointer is a no-op, so the NULL check in
sync_pcpu can be removed.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/xen/pcpu.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/xen/pcpu.c b/drivers/xen/pcpu.c
index 067fcfa..5a27a45 100644
--- a/drivers/xen/pcpu.c
+++ b/drivers/xen/pcpu.c
@@ -278,8 +278,7 @@ static int sync_pcpu(uint32_t cpu, uint32_t *max_cpu)
 	 * Only those at cpu present map has its sys interface.
 	 */
 	if (info->flags & XEN_PCPU_FLAGS_INVALID) {
-		if (pcpu)
-			unregister_and_remove_pcpu(pcpu);
+		unregister_and_remove_pcpu(pcpu);
 		return 0;
 	}
 
-- 
1.7.10.4

[PATCH 5/5] xen: remove redundant NULL check before unregister_and_remove_pcpu().

[Cyril Roelandt]

unregister_and_remove_pcpu on a NULL pointer is a no-op, so the NULL check in
sync_pcpu can be removed.

Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
---
 drivers/xen/pcpu.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/xen/pcpu.c b/drivers/xen/pcpu.c
index 067fcfa..5a27a45 100644
--- a/drivers/xen/pcpu.c
+++ b/drivers/xen/pcpu.c
@@ -278,8 +278,7 @@ static int sync_pcpu(uint32_t cpu, uint32_t *max_cpu)
 	 * Only those at cpu present map has its sys interface.
 	 */
 	if (info->flags & XEN_PCPU_FLAGS_INVALID) {
-		if (pcpu)
-			unregister_and_remove_pcpu(pcpu);
+		unregister_and_remove_pcpu(pcpu);
 		return 0;
 	}
 
-- 
1.7.10.4

Re: [PATCH 2/5] iommu: remove redundant NULL check before dma_ops_domain_free().

[Joerg Roedel]

On Tue, Feb 12, 2013 at 05:01:50AM +0100, Cyril Roelandt wrote:
> dma_ops_domain_free on a NULL pointer is a no-op, so the NULL check in
> amd_iommu_init_dma_ops() can be removed.
> 
> Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
> ---
>  drivers/iommu/amd_iommu.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Applied to x86/amd, Thanks.

Re: [PATCH 2/5] iommu: remove redundant NULL check before dma_ops_domain_free().

[Joerg Roedel]

On Tue, Feb 12, 2013 at 05:01:50AM +0100, Cyril Roelandt wrote:
> dma_ops_domain_free on a NULL pointer is a no-op, so the NULL check in
> amd_iommu_init_dma_ops() can be removed.
> 
> Signed-off-by: Cyril Roelandt <tipecaml@gmail.com>
> ---
>  drivers/iommu/amd_iommu.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Applied to x86/amd, Thanks.

Re: [PATCH 2/5] iommu: remove redundant NULL check before dma_ops_domain_free().

[Joerg Roedel]

On Tue, Feb 12, 2013 at 05:01:50AM +0100, Cyril Roelandt wrote:
> dma_ops_domain_free on a NULL pointer is a no-op, so the NULL check in
> amd_iommu_init_dma_ops() can be removed.
> 
> Signed-off-by: Cyril Roelandt <tipecaml-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/iommu/amd_iommu.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Applied to x86/amd, Thanks.

[Cocci] [PATCH 0/5] Remove redundant NULL checks.

[Julia Lawall]

On Fri, 15 Feb 2013, Cyril Roelandt wrote:

> (Shouldn't we be discussing this on Coccinelle's mailing list ?)

Sure

> On 02/12/2013 11:15 PM, Julia Lawall wrote:
> > On Tue, 12 Feb 2013, Cyril Roelandt wrote:
> > 
> > > On 02/12/2013 07:32 AM, Julia Lawall wrote:
> > > > Why do you consider only void returning functions?
> > > > 
> > > 
> > > I was afraid to find cases like:
> > > 
> > > if (x)
> > >      ret = noop_func(x);
> > > /* Code where the value of "ret" matters */
> > > 
> > > In such cases, removing the NULL check might change the behaviour of the
> > > code.
> > > It is not a problem when the return value of noop_func is ignored, though
> > > (as
> > > in cpdma_ctlr_destroy() in
> > > drivers/net/ethernet/ti/davinci_cpdma.c).
> > 
> > OK, good point.  There are indeed two things happening in this case - the
> > call and the assignment.
> > 
> > > There are other issues with this semantic patch:
> > > 1) It only works if the noop_func is called in the file where it is
> > > defined,
> > > which means it cannot catch "if (x) kfree(x)", for instance. I think I
> > > could:
> > > - run Coccinelle on the whole kernel to find functions that are no-ops
> > > when
> > > given a NULL pointer as an argument;
> > > - generate a second semantic patch that looks for the "if (x)
> > > noop_func(x)"
> > > pattern with noop_func being one of the functions found by the first
> > > semantic
> > > patch.
> > 
> > You can use iteration for this.  There is an example in the demos
> > directory.  iteration.cocci, I believe.
> > 
> 
> I did that, and it seems to work well on small examples. I'm afraid it might
> take a long time on the Linux kernel, though. The patch is made of two parts:
> one that looks for functions that are no-ops if one of the parameters is
> NULL, and one that looks for redundant NULL checks. Will the second part of
> the semantic patch be run on every file in the kernel for every function
> found by the first part ? I found 340 functions that we might be interested
> in, which is a lot... I attached the semantic patch.

If you are using glimpse or idutils it should continue doing that for the 
iterations and complete in a reasonable amount of time.  

julia

> > > 2) It doesn't work with functions that take more than one argument.
> > 
> > That should be fixable.  You can use eg
> > 
> > expression list[n] es;
> > 
> 
> Indeed, works like a charm !
> 
> Cyril.
> 

[Cocci] [PATCH 0/5] Remove redundant NULL checks.

[Cyril Roelandt]

On 02/15/2013 07:53 AM, Julia Lawall wrote:
> If you are using glimpse or idutils it should continue doing that for the
> iterations and complete in a reasonable amount of time.

Indeed. There are __lots__ of matches, so I guess I'll mostly send 
patches to the staging tree where they'll probably be welcome.


Thanks for your help!

Cyril.