lkml.org 
[lkml]   [2012]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] grvga: Fix error handling issues
    From
    Date
    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
    > >




    \
     
     \ /
      Last update: 2012-06-24 13:21    [W:0.049 / U:4.196 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site