Re: [PATCH] grvga: Fix error handling issues

Julia Lawall <julia.lawall@xxxxxxx> · Sun, 24 Jun 2012 07:18:29 +0200 (CEST)

On Sun, 24 Jun 2012, Julia Lawall wrote:


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.


Oops, drop the last comment; the function uses both.

julia



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/




Re: [PATCH] grvga: Fix error handling issues
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]