Re: [bug report] net: ipa: reduce arguments to ipa_table_init_add()

[Dan Carpenter <error27@gmail.com>]

On Sat, Nov 19, 2022 at 03:21:48AM -0600, Alex Elder wrote:
> On 11/18/22 03:50, Dan Carpenter wrote:
> > On Thu, Nov 17, 2022 at 07:47:27AM +0300, Dan Carpenter wrote:
> > > Heh.  It really feels like this line should have generated a checker
> > > warning as well.  I've created two versions.  The first complains when
> > > ever there is a divide used as a condition:
> > > 
> > > 	if (a / b) {
> > > 
> > > The other complains when it's part of a logical && or ||.
> > > 
> > > 	if (a && a / b) {
> > > 
> > > drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition: 'hash_mem->size / 8'
> > > drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition (logical): 'hash_mem->size / 8'
> > > 
> > > I'll test them out tonight and see if either gives useful results.
> > 
> > I modified the test to ignore macros.  Otherwise we get warnings where
> > macros are trying to avoid divide by zero bugs.  There was no advantage
> > in treating logicals as special so I dropped that.
> > 
> > The results are kind of mind bending.  I think maybe people sometimes
> > accidentally write "if (a / b) {" meaning does it divide cleanly?  When
> > that should be written as: "if ((a % b) == 0) {".
> 
> Interesting.  I looked at the first few.  I think the nvdimm ones
> might be using "if (cleared / 512)" to mean "if (cleared >= 512",
> and in that case, the >= might actually be more efficient than the
> divide.  On the real-time clock one it looked like a similar usage.
> Regardless, it's not a typical idiom so I don't think it's
> straightforward to understand.

Yeah.  I'm going to publish the check and the new warning message will
be:

drivers/nvdimm/claim.c:287 nsio_rw_bytes() warn: replace divide condition 'cleared / 512' with 'cleared >= 512'

regards,
dan carpenter