| Message ID | 54BBE87C.9020705@users.sourceforge.net |
|---|---|
| State | Rejected |
| Headers | show |
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
> 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
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
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
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
>> 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
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
> Now, have *you* learned anything from this approach?
Yes, of course. ;-)
How would we like to tackle any corresponding software development challenges?
Regards,
Markus
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
> 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
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));