[PATCH 1/1] MTD: Deletion of unnecessary checks before two function calls

[PATCH 1/1] MTD: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 20 Nov 2014 13:50:43 +0100

The functions kfree() and pci_dev_put() test whether their argument is NULL
and then return immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mtd/chips/cfi_cmdset_0001.c | 3 +--
 drivers/mtd/devices/pmc551.c        | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index 3096f3d..286b97a 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -2654,8 +2654,7 @@ static void cfi_intelext_destroy(struct mtd_info *mtd)
 	kfree(cfi);
 	for (i = 0; i < mtd->numeraseregions; i++) {
 		region = &mtd->eraseregions[i];
-		if (region->lockmap)
-			kfree(region->lockmap);
+		kfree(region->lockmap);
 	}
 	kfree(mtd->eraseregions);
 }
diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c
index f02603e..708b7e8 100644
--- a/drivers/mtd/devices/pmc551.c
+++ b/drivers/mtd/devices/pmc551.c
@@ -812,8 +812,7 @@ static int __init init_pmc551(void)
 	}
 
 	/* Exited early, reference left over */
-	if (PCI_Device)
-		pci_dev_put(PCI_Device);
+	pci_dev_put(PCI_Device);
 
 	if (!pmc551list) {
 		printk(KERN_NOTICE "pmc551: not detected\n");
-- 
2.1.3

Re: [PATCH 1/1] MTD: Deletion of unnecessary checks before two function calls

[Brian Norris]

On Thu, Nov 20, 2014 at 01:55:23PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 20 Nov 2014 13:50:43 +0100
> 
> The functions kfree() and pci_dev_put() test whether their argument is NULL
> and then return immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied to l2-mtd.git.

Brian

[PATCH] MTD: Deletion of checks before the function call "iounmap"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 18 Jan 2015 17:30:23 +0100

The iounmap() function performs also input parameter validation.
Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mtd/maps/latch-addr-flash.c |  3 +--
 drivers/mtd/maps/pci.c              |  6 ++----
 drivers/mtd/maps/physmap_of.c       |  3 +--
 drivers/mtd/maps/plat-ram.c         |  3 +--
 drivers/mtd/maps/pxa2xx-flash.c     |  6 ++----
 drivers/mtd/maps/sa1100-flash.c     |  3 +--
 drivers/mtd/nand/fsl_elbc_nand.c    |  3 +--
 drivers/mtd/nand/fsl_ifc_nand.c     |  3 +--
 drivers/mtd/nand/mpc5121_nfc.c      |  3 +--
 drivers/mtd/onenand/samsung.c       | 12 ++++--------
 10 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/drivers/mtd/maps/latch-addr-flash.c b/drivers/mtd/maps/latch-addr-flash.c
index cadfbe0..60496cd 100644
--- a/drivers/mtd/maps/latch-addr-flash.c
+++ b/drivers/mtd/maps/latch-addr-flash.c
@@ -109,8 +109,7 @@ static int latch_addr_flash_remove(struct platform_device *dev)
 		map_destroy(info->mtd);
 	}
 
-	if (info->map.virt != NULL)
-		iounmap(info->map.virt);
+	iounmap(info->map.virt);
 
 	if (info->res != NULL)
 		release_mem_region(info->res->start, resource_size(info->res));
diff --git a/drivers/mtd/maps/pci.c b/drivers/mtd/maps/pci.c
index eb0242e..0006794 100644
--- a/drivers/mtd/maps/pci.c
+++ b/drivers/mtd/maps/pci.c
@@ -118,8 +118,7 @@ intel_iq80310_init(struct pci_dev *dev, struct map_pci_info *map)
 static void
 intel_iq80310_exit(struct pci_dev *dev, struct map_pci_info *map)
 {
-	if (map->base)
-		iounmap(map->base);
+	iounmap(map->base);
 	pci_write_config_dword(dev, 0x44, map->map.map_priv_2);
 }
 
@@ -202,8 +201,7 @@ intel_dc21285_init(struct pci_dev *dev, struct map_pci_info *map)
 static void
 intel_dc21285_exit(struct pci_dev *dev, struct map_pci_info *map)
 {
-	if (map->base)
-		iounmap(map->base);
+	iounmap(map->base);
 
 	/*
 	 * We need to undo the PCI BAR2/PCI ROM BAR address alteration.
diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index f35cd20..89f5d42 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -57,8 +57,7 @@ static int of_flash_remove(struct platform_device *dev)
 		if (info->list[i].mtd)
 			map_destroy(info->list[i].mtd);
 
-		if (info->list[i].map.virt)
-			iounmap(info->list[i].map.virt);
+		iounmap(info->list[i].map.virt);
 
 		if (info->list[i].res) {
 			release_resource(info->list[i].res);
diff --git a/drivers/mtd/maps/plat-ram.c b/drivers/mtd/maps/plat-ram.c
index 4b65c08..da09274 100644
--- a/drivers/mtd/maps/plat-ram.c
+++ b/drivers/mtd/maps/plat-ram.c
@@ -104,8 +104,7 @@ static int platram_remove(struct platform_device *pdev)
 		kfree(info->area);
 	}
 
-	if (info->map.virt != NULL)
-		iounmap(info->map.virt);
+	iounmap(info->map.virt);
 
 	kfree(info);
 
diff --git a/drivers/mtd/maps/pxa2xx-flash.c b/drivers/mtd/maps/pxa2xx-flash.c
index 12fa75d..a5f27b9 100644
--- a/drivers/mtd/maps/pxa2xx-flash.c
+++ b/drivers/mtd/maps/pxa2xx-flash.c
@@ -89,8 +89,7 @@ static int pxa2xx_flash_probe(struct platform_device *pdev)
 
 	if (!info->mtd) {
 		iounmap((void *)info->map.virt);
-		if (info->map.cached)
-			iounmap(info->map.cached);
+		iounmap(info->map.cached);
 		return -EIO;
 	}
 	info->mtd->owner = THIS_MODULE;
@@ -110,8 +109,7 @@ static int pxa2xx_flash_remove(struct platform_device *dev)
 
 	map_destroy(info->mtd);
 	iounmap(info->map.virt);
-	if (info->map.cached)
-		iounmap(info->map.cached);
+	iounmap(info->map.cached);
 	kfree(info);
 	return 0;
 }
diff --git a/drivers/mtd/maps/sa1100-flash.c b/drivers/mtd/maps/sa1100-flash.c
index ea69720..22e5d7d 100644
--- a/drivers/mtd/maps/sa1100-flash.c
+++ b/drivers/mtd/maps/sa1100-flash.c
@@ -58,8 +58,7 @@ static void sa1100_destroy_subdev(struct sa_subdev_info *subdev)
 {
 	if (subdev->mtd)
 		map_destroy(subdev->mtd);
-	if (subdev->map.virt)
-		iounmap(subdev->map.virt);
+	iounmap(subdev->map.virt);
 	release_mem_region(subdev->map.phys, subdev->map.size);
 }
 
diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
index 04b22fd..a0f2a91 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c
@@ -801,8 +801,7 @@ static int fsl_elbc_chip_remove(struct fsl_elbc_mtd *priv)
 
 	kfree(priv->mtd.name);
 
-	if (priv->vbase)
-		iounmap(priv->vbase);
+	iounmap(priv->vbase);
 
 	elbc_fcm_ctrl->chips[priv->bank] = NULL;
 	kfree(priv);
diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 4c05f4f..360d9a3 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -995,8 +995,7 @@ static int fsl_ifc_chip_remove(struct fsl_ifc_mtd *priv)
 
 	kfree(priv->mtd.name);
 
-	if (priv->vbase)
-		iounmap(priv->vbase);
+	iounmap(priv->vbase);
 
 	ifc_nand_ctrl->chips[priv->bank] = NULL;
 
diff --git a/drivers/mtd/nand/mpc5121_nfc.c b/drivers/mtd/nand/mpc5121_nfc.c
index 1f12e5b..34fa6db 100644
--- a/drivers/mtd/nand/mpc5121_nfc.c
+++ b/drivers/mtd/nand/mpc5121_nfc.c
@@ -621,8 +621,7 @@ static void mpc5121_nfc_free(struct device *dev, struct mtd_info *mtd)
 	if (prv->clk)
 		clk_disable_unprepare(prv->clk);
 
-	if (prv->csreg)
-		iounmap(prv->csreg);
+	iounmap(prv->csreg);
 }
 
 static int mpc5121_nfc_probe(struct platform_device *op)
diff --git a/drivers/mtd/onenand/samsung.c b/drivers/mtd/onenand/samsung.c
index 19cfb97..24ac4cb 100644
--- a/drivers/mtd/onenand/samsung.c
+++ b/drivers/mtd/onenand/samsung.c
@@ -1001,8 +1001,7 @@ static int s3c_onenand_probe(struct platform_device *pdev)
 	return 0;
 
 scan_failed:
-	if (onenand->dma_addr)
-		iounmap(onenand->dma_addr);
+	iounmap(onenand->dma_addr);
 dma_ioremap_failed:
 	if (onenand->dma_res)
 		release_mem_region(onenand->dma_res->start,
@@ -1011,8 +1010,7 @@ dma_ioremap_failed:
 oob_buf_fail:
 	kfree(onenand->page_buf);
 page_buf_fail:
-	if (onenand->ahb_addr)
-		iounmap(onenand->ahb_addr);
+	iounmap(onenand->ahb_addr);
 ahb_ioremap_failed:
 	if (onenand->ahb_res)
 		release_mem_region(onenand->ahb_res->start,
@@ -1036,13 +1034,11 @@ static int s3c_onenand_remove(struct platform_device *pdev)
 	struct mtd_info *mtd = platform_get_drvdata(pdev);
 
 	onenand_release(mtd);
-	if (onenand->ahb_addr)
-		iounmap(onenand->ahb_addr);
+	iounmap(onenand->ahb_addr);
 	if (onenand->ahb_res)
 		release_mem_region(onenand->ahb_res->start,
 				   resource_size(onenand->ahb_res));
-	if (onenand->dma_addr)
-		iounmap(onenand->dma_addr);
+	iounmap(onenand->dma_addr);
 	if (onenand->dma_res)
 		release_mem_region(onenand->dma_res->start,
 				   resource_size(onenand->dma_res));
-- 
2.2.2

Re: [PATCH] MTD: Deletion of checks before the function call "iounmap"

[Brian Norris]

On Sun, Jan 18, 2015 at 06:08:12PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 18 Jan 2015 17:30:23 +0100
> 
> The iounmap() function performs also input parameter validation.
> Thus the test around the call is not needed.

Is this guaranteed for all arch'es? I expect that it would be, but I see
that, for instance, ARM allows replaceable iounmap() for
subarchitectures. Also, I see checks for various sorts of static
mappings in ARM and x86; these likely (always?) cover the NULL case, but
they're not always straightforward.

Anyway, I'm essentially saying that I'd like to be 100% sure we have a
guarantee before dropping all these.

> This issue was detected by using the Coccinelle software.

What script? Hand-rolled I guess?

Brian

Re: [PATCH] MTD: Deletion of checks before the function call "iounmap"

[SF Markus Elfring]

> Anyway, I'm essentially saying that I'd like to be 100% sure we have a
> guarantee before dropping all these.

You can not be absolutely sure. There are various implementation details
which will eventually need further considerations.

I hope that a reasonable confidence can be achieved here.


>> This issue was detected by using the Coccinelle software.
> 
> What script?

I published scripts for static source code analysis in March 2004.


> Hand-rolled I guess?

An extended version found some update candidates in source files for Linux
according to the software development status of "next-20141226".

My approach is still incomplete at the moment.

Regards,
Markus

Re: [PATCH] MTD: Deletion of checks before the function call "iounmap"

[Dan Carpenter]

The sparc iounmap() implementation in arch/sparc/kernel/ioport.c looks
it prints an error message if you pass a NULL pointer.

regards,
dan carpenter

Re: [PATCH] MTD: Deletion of checks before the function call "iounmap"

[Brian Norris]

On Mon, Jan 19, 2015 at 07:19:34PM +0100, SF Markus Elfring wrote:
> > Anyway, I'm essentially saying that I'd like to be 100% sure we have a
> > guarantee before dropping all these.
> 
> You can not be absolutely sure. There are various implementation details
> which will eventually need further considerations.

Let's consider them first. We don't ship code that is missing the
implementation details. (At least, we try not to!)

> I hope that a reasonable confidence can be achieved here.
> 
> 
> >> This issue was detected by using the Coccinelle software.
> > 
> > What script?
> 
> I published scripts for static source code analysis in March 2004.

I didn't ask "when?"; where?

> > Hand-rolled I guess?
> 
> An extended version found some update candidates in source files for Linux
> according to the software development status of "next-20141226".
> 
> My approach is still incomplete at the moment.

Then please complete it.

Thanks,
Brian

Re: [PATCH] MTD: Deletion of checks before the function call "iounmap"

[Brian Norris]

On Mon, Jan 19, 2015 at 09:20:47PM +0300, Dan Carpenter wrote:
> The sparc iounmap() implementation in arch/sparc/kernel/ioport.c looks
> it prints an error message if you pass a NULL pointer.

Seems that way. Thanks.

Nak to the patch then.

Brian

Re: [PATCH] MTD: Deletion of checks before the function call "iounmap"

[SF Markus Elfring]

>> I published scripts for static source code analysis in March 2004.
> 
> I didn't ask "when?"; where?

https://lkml.org/lkml/2014/3/5/356
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/
https://systeme.lip6.fr/pipermail/cocci/2014-March/000676.html

Do you get further ideas from this software analysis approach?

Regards,
Markus

Re: [PATCH] MTD: Deletion of checks before the function call "iounmap"

[Brian Norris]

On Mon, Jan 19, 2015 at 08:07:34PM +0100, SF Markus Elfring wrote:
> >> I published scripts for static source code analysis in March 2004.

^^ That would be March 2014, not March 2004.

> > I didn't ask "when?"; where?
> 
> https://lkml.org/lkml/2014/3/5/356
> http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/
> https://systeme.lip6.fr/pipermail/cocci/2014-March/000676.html
> 
> Do you get further ideas from this software analysis approach?

I suppose. I've gotten the idea that I can generate a ton of patches
using an automated tool, not test or research them, possibly add bugs to
the kernel, and still get a good percentage of my patches merged.

Now, have *you* learned anything from this approach?

Brian

Re: [PATCH] MTD: Deletion of checks before the function call "iounmap"

[SF Markus Elfring]

> Now, have *you* learned anything from this approach?

Yes, of course.   ;-)

How would we like to tackle any corresponding software development challenges?

Regards,
Markus

Re: [PATCH] MTD: Deletion of checks before the function call "iounmap"

[Brian Norris]

Replying, against my better judgment...

On Mon, Jan 19, 2015 at 2:00 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> Now, have *you* learned anything from this approach?
>
> Yes, of course.   ;-)

Do you have any evidence of this? Details? The only evidence I see is
a long thread of similar patches from the last several months, of
varying (though not increasing) quality. Prior objections to your
approach were met with a distinct lack of self-awareness on your part.

> How would we like to tackle any corresponding software development challenges?

I don't really see many challenges here to tackle, except for the
social issue of dealing with patch bots like you. Your automated
patches are generally not solving real problems, but they are at times
introducing bugs. So the most efficient "solution" to this "challenge"
may simply be to ignore your patches.

Brian

Re: [PATCH] MTD: Deletion of checks before the function call "iounmap"

[SF Markus Elfring]

> The only evidence I see is a long thread of similar patches from
> the last several months, of varying (though not increasing) quality.

I find it acceptable that some of my update suggestions do not fit
to your quality expectations at the moment.


> Prior objections to your approach were met with a distinct lack
> of self-awareness on your part.

I guess that a bit more clarification of this view will help
to resolve some open issues, won't it?


> I don't really see many challenges here to tackle, except for the
> social issue of dealing with patch bots like you.

I hope that you will like other automatic source code analysis results.


> Your automated patches are generally not solving real problems,

I try to clean-up Linux source files a bit. Problem solving
with a higher value might follow ...


> but they are at times introducing bugs.

There are the usual different opinions and disagreements
which might be still waiting for a constructive solution.

Regards,
Markus

[PATCH] UBIFS: Delete unnecessary checks before the function call "iput"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 4 Nov 2015 20:00:42 +0100

The iput() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/ubifs/recovery.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ubifs/recovery.c b/fs/ubifs/recovery.c
index 2a1f0bc..586d593 100644
--- a/fs/ubifs/recovery.c
+++ b/fs/ubifs/recovery.c
@@ -1331,8 +1331,7 @@ void ubifs_destroy_size_tree(struct ubifs_info *c)
 	struct size_entry *e, *n;
 
 	rbtree_postorder_for_each_entry_safe(e, n, &c->size_tree, rb) {
-		if (e->inode)
-			iput(e->inode);
+		iput(e->inode);
 		kfree(e);
 	}
 
@@ -1533,8 +1532,7 @@ int ubifs_recover_size(struct ubifs_info *c)
 				err = fix_size_in_place(c, e);
 				if (err)
 					return err;
-				if (e->inode)
-					iput(e->inode);
+				iput(e->inode);
 			}
 		}
 
-- 
2.6.2

Re: [PATCH] UBIFS: Delete unnecessary checks before the function call "iput"

[Richard Weinberger]

On Wed, Nov 4, 2015 at 8:10 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 4 Nov 2015 20:00:42 +0100
>
> The iput() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  fs/ubifs/recovery.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ubifs/recovery.c b/fs/ubifs/recovery.c
> index 2a1f0bc..586d593 100644
> --- a/fs/ubifs/recovery.c
> +++ b/fs/ubifs/recovery.c
> @@ -1331,8 +1331,7 @@ void ubifs_destroy_size_tree(struct ubifs_info *c)
>         struct size_entry *e, *n;
>
>         rbtree_postorder_for_each_entry_safe(e, n, &c->size_tree, rb) {
> -               if (e->inode)
> -                       iput(e->inode);
> +               iput(e->inode);
>                 kfree(e);
>         }
>
> @@ -1533,8 +1532,7 @@ int ubifs_recover_size(struct ubifs_info *c)
>                                 err = fix_size_in_place(c, e);
>                                 if (err)
>                                         return err;
> -                               if (e->inode)
> -                                       iput(e->inode);
> +                               iput(e->inode);
>                         }
>                 }

Applied!

-- 
Thanks,
//richard