From: Julia Lawall <julia.lawall@lip6.fr>
To: Julia Lawall <julia.lawall@lip6.fr>
Cc: Emil Goode <emilgoode@gmail.com>,
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
Date: Sun, 24 Jun 2012 05:18:29 +0000 [thread overview]
Message-ID: <alpine.DEB.2.02.1206240717120.2178@localhost6.localdomain6> (raw)
In-Reply-To: <alpine.DEB.2.02.1206240711580.2178@localhost6.localdomain6>
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
>>
>
next prev parent reply other threads:[~2012-06-24 5:18 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.02.1206240717120.2178@localhost6.localdomain6 \
--to=julia.lawall@lip6.fr \
--cc=FlorianSchandinat@gmx.de \
--cc=emilgoode@gmail.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=w.sang@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox