Re: [PATCH] grvga: Fix error handling issues

On Sun, 24 Jun 2012, Emil Goode wrote:

Yes I considered converting to devm_ and keeping the of_iounmap in.
But I just feel like I'm mixing two api's. If converting to devm_ really
is the right thing to do, tell me and I will send another patch :)

devm is safer and simpler, because you can just get rid of error handling
code rather than ensuring over time that it continues to be done in all of
the place where it is needed.  So I would think that it is always better
to use it.  But others may think differently.  Another option would be to
only use it for request_mem_region, which you can do consistently in this
case, and leave both of_ioremap and ioremap as they are.

julia

Thanks,

Emil

On Sun, 2012-06-24 at 07:15 +0200, Julia Lawall wrote:

quoted

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:

quoted

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:

quoted

On Fri, 22 Jun 2012, Emil Goode wrote:

quoted

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

quoted

Thanks,

Emil

On Fri, 2012-06-22 at 18:12 +0200, Julia Lawall wrote:

quoted

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:

quoted

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 <redacted>
---
 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@vger.kernel.org
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@vger.kernel.org
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html