On Tue, 2015-05-05 at 19:40 +0300, Dan Carpenter wrote: > On Tue, May 05, 2015 at 04:08:49PM +0000, Jose Rivera wrote: > > > Subject: Re: [PATCH 1/7] staging: fsl-mc: MC bus IRQ support > > > > > > On Mon, May 04, 2015 at 10:09:08PM +0000, Jose Rivera wrote: > > > > > > + WARN_ON((int16_t)irq_count < 0); > > > > > > > > > > This code is doing "WARN_ON(test_bit(15, (unsigned long > > > *)&irq_count));". > > > > > That seems like nonsense. Anyway, just delete the WARN_ON(). > > > > > > > > > I disagree. This WARN_ON is checking that irq_count is in the expected > > > > range (it fits in int16_t as a positive number). The > > > > dprc_scan_objects() function expects irq_count to be of type "unsigned > > > > int" (which is 32-bit unsigned) > > > > > > > > > > You're not allowed to disagree because it's a testable thing and not an > > > opinion about style or something. :P What you want is: > > > > > > WARN_ON(irq_count > SHRT_MAX); > > > > > I see your point now. The check "(int16_t)irq_count < 0)" will not be able > > to catch 0x10000 > 0x7fff, but "irq_count > SHRT_MAX) will. So I'll > > make the suggested change, but I would prefer to use S16_MAX rather than > > SHRT_MAX. > > > > Huh? I didn't even know about the S16_MAX definition. There are > literally no users of it in the kernel. It's not very fair because > there are few users of SHRT_MAX. But there are literally no users of > S32_MAX in the kernel and 358 users of INT_MAX. > > Don't insist that you must be special and different from everyone else. There are some users of U16_MAX, U32_MAX, and U64_MAX. Why use a limit for a different type than is being used? Why have s16/s32 at all if you're going to conflate it with short/int elsewhere? That said, I don't see where this code is actually using s16 (or int16_t) for irq_count except in these weird error checks. German, why do you need to check against 0x7fff (whatever you call it) at all? Won't comparing to a promoted-to-unsigned-int .max_count (as you do immediately after the WARN_ON) suffice? -Scott _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
