by Julia Lawall

[permalink] [raw]

Subject: Re: [PATCH] grvga: Fix error handling issues

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 <[email protected]>
> > >>>>> ---
> > >>>>> 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 [email protected]
> > >>>>> 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 [email protected]
> > >>> 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 [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
>
>
>

2012-06-25 09:55:34

by Emil Goode

[permalink] [raw]

Subject: Re: [PATCH] grvga: Fix error handling issues

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 <[email protected]>
> > > >>>>> ---
> > > >>>>> 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 [email protected]
> > > >>>>> 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 [email protected]
> > > >>> 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 [email protected]
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > >
> >
> >
> >