Re: [PATCH] grvga: Fix error handling issues

[Emil Goode]

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@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/