Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753220Ab2FXFSe (ORCPT ); Sun, 24 Jun 2012 01:18:34 -0400 Received: from mail4-relais-sop.national.inria.fr ([192.134.164.105]:23474 "EHLO mail4-relais-sop.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751932Ab2FXFSd (ORCPT ); Sun, 24 Jun 2012 01:18:33 -0400 X-IronPort-AV: E=Sophos;i="4.77,466,1336341600"; d="scan'208";a="148563951" Date: Sun, 24 Jun 2012 07:18:29 +0200 (CEST) From: Julia Lawall X-X-Sender: jll@localhost6.localdomain6 To: Julia Lawall cc: Emil Goode , FlorianSchandinat@gmx.de, linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, w.sang@pengutronix.de Subject: Re: [PATCH] grvga: Fix error handling issues In-Reply-To: Message-ID: References: <1340381479-6371-1-git-send-email-emilgoode@gmail.com> <1340382044.3963.2.camel@localhost> <1340488911.5321.10.camel@localhost> User-Agent: Alpine 2.02 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7799 Lines: 242 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 >>>>>> --- >>>>>> 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 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/