Time for a code audit?

[Kershner, David A]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter at oracle.com]
> Sent: Friday, February 12, 2016 5:02 PM
> To: Romer, Benjamin M <Benjamin.Romer at unisys.com>
> Cc: gregkh at linuxfoundation.org; driverdev-devel at linuxdriverproject.org;
> Sell, Timothy C <Timothy.Sell at unisys.com>; *S-Par-Maintainer
> <SParMaintainer at unisys.com>
> Subject: Re: Time for a code audit?
> 
> I looked at the Smatch warnings, plus some bonus stuff I'm still working
> on.
> 
> drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries()
> warn: inconsistent indenting
> drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries()
> warn: inconsistent indenting
> drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries()
> warn: XXX should 'inp_pfn + i' be a 64 bit type?
> 
> This is because the left side is u64 but we wrap at u32.
> 

Dan, thanks for the find. Though I'm curious what options are you using when you run smatch to produce the "warn: XXX should 'inp_pfn +I' be a 64 bit type" message? When I run smatch locally I don't see that message but I see the other two and it is definitely a problem in the codebase that I'm looking at. 

David 

> drivers/staging/unisys/visorbus/visorbus_main.c:787
> visordriver_probe_device() warn: 'sem:&dev->visordriver_callback_lock' is
> sometimes locked here and sometimes unlocked.
> drivers/staging/unisys/visorbus/visorchipset.c:542 toolaction_show() warn:
> XXX passing uninitialized 'tool_action'
> drivers/staging/unisys/visorbus/visorchipset.c:609 error_show() warn: XXX
> passing uninitialized 'error'
> drivers/staging/unisys/visorbus/visorchipset.c:639 textid_show() warn: XXX
> passing uninitialized 'text_id'
> drivers/staging/unisys/visorbus/visorchipset.c:669 remaining_steps_show()
> warn: XXX passing uninitialized 'remaining_steps'
> 
> These with XXX are if channel->nbytes is too small.
> 
> drivers/staging/unisys/visorbus/visorchipset.c:2217 visorchipset_ioctl() warn:
> user controlled 'adjustment' cast to postive rl = 's64min-s64max'
> 
> This is because we read a s64 and pass it to a function as u64.
> 
> Do an:  grep "rc = -1" drivers/staging/unisys/ -R
> 
> Also "if (rc != 0)" is a double negative.  != zero is good style when
> you are talking about the number zero or for strcmp() functions.
> 
> Ho ho ho.
> int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev-
> >devnodes[0]);
> 
> You guys know that when I read
> drivers/staging/unisys/visorbus/visorbus_main.c there is still a lot
> that makes me itch.  All the unnecessary initialization to -1 and the
> goto aways.
> 
> char s[99];  Why 99?  Why s?  I'm not a fan of a lot of the naming.
> What is s?  What is x?
> 
> Blank lines after declaration sections.
> 
>    781          fix_vbus_dev_info(dev);
>    782          up(&dev->visordriver_callback_lock);
>    783          rc = 0;
>    784  away:
>    785          if (rc != 0)
>    786                  put_device(&dev->device);
>    787          return rc;
>    788  }
> 
> This is like in my kitchen because when I'm making spaghetti I throw
> some against the wall to see if it is done and eventual the whole wall
> has tiny spots of pasta stuck to it.
> 
> I'm half way through the first file and this code is *almost* so close
> to being ready, but could you just go through it once more and do a
> final tidy up.
> 
> regards,
> dan carpenter