Re: [PATCH] grvga: Fix error handling issues
From: Julia Lawall
Date: Sun Jun 24 2012 - 01:15:32 EST
I see. You could still use a devm_ function for request_mem_region.
Also I noticed that the remove function uses iounmap directly, not
io_iounmap. The latter is just a wrapper for the former, but it could be
good to use the right name.
julia
On Sun, 24 Jun 2012, Emil Goode wrote:
The of_ioremap function is used in this code as well and I don't know of
a devm_ equivalent for it. For consistency I think it is better to leave
it as it is in this case. So I stick with v1 of this patch.
Thanks,
Emil
On Fri, 2012-06-22 at 18:21 +0200, Julia Lawall wrote:
On Fri, 22 Jun 2012, Emil Goode wrote:
Good idea, I will take another look at it tomorrow.
There is a devm_ function that combines request_mem_region and ioremap
that could be useful.
julia
Thanks,
Emil
On Fri, 2012-06-22 at 18:12 +0200, Julia Lawall wrote:
Maybe you could used the devm_ functions for request_mem_region and
ioremap so that the error handling can just be dropped?
julia
On Fri, 22 Jun 2012, Emil Goode wrote:
This patch fixes two problems with the error handling in the
grvga_probe function.
- If the call to grvga_parse_custom on line 370 fails we use
the wrong label so that release_mem_region will be called
without a call to request_mem_region being made.
- If the call to ioremap on line 436 fails we should not try
to call iounmap. I added an if statement to check whether or
not a call to iounmap should be made.
- I also changed the names of the labels to make the code
easier to read.
Signed-off-by: Emil Goode <emilgoode@xxxxxxxxx>
---
drivers/video/grvga.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
index da066c2..d9d688a 100644
--- a/drivers/video/grvga.c
+++ b/drivers/video/grvga.c
@@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
*/
if (fb_get_options("grvga", &options)) {
retval = -ENODEV;
- goto err;
+ goto free_fb;
}
if (!options || !*options)
@@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
if (grvga_parse_custom(this_opt, &info->var) < 0) {
dev_err(&dev->dev, "Failed to parse custom mode (%s).\n", this_opt);
retval = -EINVAL;
- goto err1;
+ goto free_fb;
}
} else if (!strncmp(this_opt, "addr", 4))
grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
@@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
if (!request_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
dev_err(&dev->dev, "registers already mapped\n");
retval = -EBUSY;
- goto err;
+ goto free_fb;
}
par->regs = of_ioremap(&dev->resource[0], 0,
@@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct platform_device *dev)
if (!par->regs) {
dev_err(&dev->dev, "failed to map registers\n");
retval = -ENOMEM;
- goto err1;
+ goto release_regs;
}
retval = fb_alloc_cmap(&info->cmap, 256, 0);
if (retval < 0) {
dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
retval = -ENOMEM;
- goto err2;
+ goto unmap_regs;
}
if (mode_opt) {
@@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
if (!retval || retval == 4) {
retval = -EINVAL;
- goto err3;
+ goto dealloc_cmap;
}
}
@@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
dev_err(&dev->dev, "failed to request memory region\n");
retval = -ENOMEM;
- goto err3;
+ goto dealloc_cmap;
}
virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
@@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
if (!virtual_start) {
dev_err(&dev->dev, "error mapping framebuffer memory\n");
retval = -ENOMEM;
- goto err4;
+ goto free_mem;
}
} else { /* Allocate frambuffer memory */
@@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
"unable to allocate framebuffer memory (%lu bytes)\n",
grvga_mem_size);
retval = -ENOMEM;
- goto err3;
+ goto dealloc_cmap;
}
physical_start = dma_map_single(&dev->dev, (void *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
@@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
retval = register_framebuffer(info);
if (retval < 0) {
dev_err(&dev->dev, "failed to register framebuffer\n");
- goto err4;
+ goto free_mem;
}
__raw_writel(physical_start, &par->regs->fb_pos);
@@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct platform_device *dev)
return 0;
-err4:
+free_mem:
dev_set_drvdata(&dev->dev, NULL);
if (grvga_fix_addr) {
release_mem_region(physical_start, grvga_mem_size);
- iounmap((void *)virtual_start);
+ if (virtual_start)
+ iounmap((void *)virtual_start);
} else
kfree((void *)virtual_start);
-err3:
+
+dealloc_cmap:
fb_dealloc_cmap(&info->cmap);
-err2:
+unmap_regs:
of_iounmap(&dev->resource[0], par->regs,
resource_size(&dev->resource[0]));
-err1:
+release_regs:
release_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]));
-err:
+free_fb:
framebuffer_release(info);
return retval;
--
1.7.10
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/