> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: Friday, February 12, 2016 5:02 PM > To: Romer, Benjamin M <Benjamin.Romer@xxxxxxxxxx> > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; > Sell, Timothy C <Timothy.Sell@xxxxxxxxxx>; *S-Par-Maintainer > <SParMaintainer@xxxxxxxxxx> > 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
