Re: [PATCH 1/3] Fix return check of dev_request_mem_region

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, Feb 18, 2016 at 04:58:49PM -0800, Andrey Smirnov wrote:
> On Thu, Feb 18, 2016 at 2:50 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > dev_request_mem_region returns an ERR_PTR, fix places which check for a
> > NULL pointer instead. This patch has been generated with this semantic
> > patch:
> >
> > // <smpl>
> > @@
> > expression e,e1,e2;
> > @@
> >
> > e = dev_request_mem_region(...)
> > ... when != e = e1
> > if (
> > -   e == NULL
> > +   IS_ERR(e)
> >    ) {
> >      ...
> >      return
> > -      e2
> > +      PTR_ERR(e)
> >      ;
> >      }
> > // </smpl>
> 
> This wouldn't handle correctly the cases where code bails out using
> goto (look for example at diff for phy-am335x.c in this patch). I
> played around with Cocinelle as well and here's what I came up with:
> 
> 
> // <smpl>
> @i@
> @@
> 
> #define CONFIG_TSE_USE_DEDICATED_DESC_MEM

This doesn't work for me. I assume you want to define
CONFIG_TSE_USE_DEDICATED_DESC_MEM to let coccinelle walk into the
correct path in drivers/net/altera_tse.c, but for me it still changes
the !CONFIG_TSE_USE_DEDICATED_DESC_MEM code path.

> 
> 
> // Handle immediate returns
> @@
> expression e;
> expression e1;
> @@
> 
> e = dev_request_mem_region(...);
> 
> ...
> 
> - if (e == NULL)
> -    return e1;
> + if (IS_ERR(e))
> +    return PTR_ERR(e);
> 
> @ rule1 @
> expression e;
> @@
> 
> e = dev_request_mem_region(...);
> 
> // Fix exit codepath first
> @@
> expression rule1.e;
> identifier ret, label;
> constant errno;
> @@
> 
> if (e == NULL)
> {
> 
>   ...
> 
> // Setting the ret code and jumping to error handling code
> (
> - ret = -errno;
> + ret = PTR_ERR(e);
> 
>   ...
> 
>   goto label;
> 
> // Return after doing some extra steps
> |
> 
> - return -errno;
> + return PTR_ERR(e);
> )
> }
> 
> // Fix the check itself. Having this as a standalone rule allows
> // to catch cases where error codepath doesn't bail out
> @depends on i@

I assume you mean "depends on rule1", because otherwise it never
actually changes the wrong error check.

Overall this looks better than my version. I recreated the patch with
your version and fixed up the altera-tse driver manually.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox