* [PATCH] grvga: Fix error handling issues @ 2012-06-22 16:11 Emil Goode 2012-06-22 16:12 ` Julia Lawall 2012-06-23 22:33 ` Dan Carpenter 0 siblings, 2 replies; 12+ messages in thread From: Emil Goode @ 2012-06-22 16:11 UTC (permalink / raw) To: FlorianSchandinat; +Cc: linux-fbdev, linux-kernel, kernel-janitors, Emil Goode 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@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-22 16:11 [PATCH] grvga: Fix error handling issues Emil Goode @ 2012-06-22 16:12 ` Julia Lawall 2012-06-22 16:20 ` Emil Goode 2012-06-23 22:33 ` Dan Carpenter 1 sibling, 1 reply; 12+ messages in thread From: Julia Lawall @ 2012-06-22 16:12 UTC (permalink / raw) To: Emil Goode; +Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors 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@gmail.com> > --- > 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-22 16:12 ` Julia Lawall @ 2012-06-22 16:20 ` Emil Goode 2012-06-22 16:21 ` Julia Lawall 0 siblings, 1 reply; 12+ messages in thread From: Emil Goode @ 2012-06-22 16:20 UTC (permalink / raw) To: Julia Lawall Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors Good idea, I will take another look at it tomorrow. 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@gmail.com> > > --- > > 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-22 16:20 ` Emil Goode @ 2012-06-22 16:21 ` Julia Lawall 2012-06-23 22:01 ` Emil Goode 0 siblings, 1 reply; 12+ messages in thread From: Julia Lawall @ 2012-06-22 16:21 UTC (permalink / raw) To: Emil Goode Cc: Julia Lawall, FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors 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@gmail.com> > > > --- > > > 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-22 16:21 ` Julia Lawall @ 2012-06-23 22:01 ` Emil Goode 2012-06-24 5:15 ` Julia Lawall 0 siblings, 1 reply; 12+ messages in thread From: Emil Goode @ 2012-06-23 22:01 UTC (permalink / raw) To: Julia Lawall Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors 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@gmail.com> > > > > --- > > > > 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-23 22:01 ` Emil Goode @ 2012-06-24 5:15 ` Julia Lawall 2012-06-24 5:18 ` Julia Lawall 2012-06-24 10:36 ` Emil Goode 0 siblings, 2 replies; 12+ messages in thread From: Julia Lawall @ 2012-06-24 5:15 UTC (permalink / raw) To: Emil Goode Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors, w.sang 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@gmail.com> >>>>> --- >>>>> 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-24 5:15 ` Julia Lawall @ 2012-06-24 5:18 ` Julia Lawall 2012-06-24 10:36 ` Emil Goode 1 sibling, 0 replies; 12+ messages in thread From: Julia Lawall @ 2012-06-24 5:18 UTC (permalink / raw) To: Julia Lawall Cc: Emil Goode, FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors, w.sang 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@gmail.com> >>>>>> --- >>>>>> 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 >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-24 5:15 ` Julia Lawall 2012-06-24 5:18 ` Julia Lawall @ 2012-06-24 10:36 ` Emil Goode 2012-06-25 5:20 ` Julia Lawall 1 sibling, 1 reply; 12+ messages in thread From: Emil Goode @ 2012-06-24 10:36 UTC (permalink / raw) To: Julia Lawall Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors, w.sang 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 :) Thanks, Emil On Sun, 2012-06-24 at 07:15 +0200, 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. > > 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@gmail.com> > >>>>> --- > >>>>> 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-24 10:36 ` Emil Goode @ 2012-06-25 5:20 ` Julia Lawall 2012-06-25 9:58 ` Emil Goode 0 siblings, 1 reply; 12+ messages in thread From: Julia Lawall @ 2012-06-25 5:20 UTC (permalink / raw) To: Emil Goode Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors, w.sang 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: > > 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@gmail.com> > > >>>>> --- > > >>>>> 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 > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-25 5:20 ` Julia Lawall @ 2012-06-25 9:58 ` Emil Goode 0 siblings, 0 replies; 12+ messages in thread From: Emil Goode @ 2012-06-25 9:58 UTC (permalink / raw) To: Julia Lawall Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors, w.sang It would be nice to get rid of the indention levels in the error handling code and the devm is to prefer from the older api. I'm traveling but will probably send another version of this patch this evening. Thanks, Emil On Mon, 2012-06-25 at 07:20 +0200, Julia Lawall wrote: > 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: > > > 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@gmail.com> > > > >>>>> --- > > > >>>>> 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 > > > > > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-22 16:11 [PATCH] grvga: Fix error handling issues Emil Goode 2012-06-22 16:12 ` Julia Lawall @ 2012-06-23 22:33 ` Dan Carpenter 2012-06-24 9:42 ` Emil Goode 1 sibling, 1 reply; 12+ messages in thread From: Dan Carpenter @ 2012-06-23 22:33 UTC (permalink / raw) To: Emil Goode; +Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors On Fri, Jun 22, 2012 at 06:11:19PM +0200, 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. > Doesn't iounmap() have a check for NULL? On x86 it does. if ((void __force *)addr <= high_memory) return; regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] grvga: Fix error handling issues 2012-06-23 22:33 ` Dan Carpenter @ 2012-06-24 9:42 ` Emil Goode 0 siblings, 0 replies; 12+ messages in thread From: Emil Goode @ 2012-06-24 9:42 UTC (permalink / raw) To: Dan Carpenter Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors This code is for the sparc architecture and the iounmap doesn't check for NULL directly as in the x86 version. I think it is safer not to pass NULL to the sparc version. Thanks, Emil On Sun, 2012-06-24 at 01:33 +0300, Dan Carpenter wrote: > On Fri, Jun 22, 2012 at 06:11:19PM +0200, 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. > > > > Doesn't iounmap() have a check for NULL? On x86 it does. > > if ((void __force *)addr <= high_memory) > return; > > regards, > dan carpenter > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-06-25 9:58 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-06-22 16:11 [PATCH] grvga: Fix error handling issues Emil Goode 2012-06-22 16:12 ` Julia Lawall 2012-06-22 16:20 ` Emil Goode 2012-06-22 16:21 ` Julia Lawall 2012-06-23 22:01 ` Emil Goode 2012-06-24 5:15 ` Julia Lawall 2012-06-24 5:18 ` Julia Lawall 2012-06-24 10:36 ` Emil Goode 2012-06-25 5:20 ` Julia Lawall 2012-06-25 9:58 ` Emil Goode 2012-06-23 22:33 ` Dan Carpenter 2012-06-24 9:42 ` Emil Goode
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).