[PATCH 1/3] Fix return check of dev_request_mem_region

[Andrey Smirnov]

On Fri, Feb 19, 2016 at 12:29 AM, Sascha Hauer <s.hauer at pengutronix.de> wrote:
> 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 at 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.

Hmmm, that is very strange. You are correct, I did include that bit
for that exact purpose. Perhaps we are using different versions of the
tool? Mine is "spatch version 1.0.1 with Python support and with PCRE
support". The way I invoked it was "spatch --sp-file check.cocci
--very-quiet --dir barebox"

>
>>
>>
>> // 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.
>

Darn! I missed that regression. The problem with making it depending
on "rule1" is that it misses the cases where only the incorrect check
is performed and no bailing out is done (atmel_nand.c).


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

In case you missed it there's also "dove.c" that needs to be corrected manually.