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);
> > > +
> > > + if ((int16_t)irq_count >
> > > + mc_bus->resource_pools[FSL_MC_POOL_IRQ].max_count) {
> >
> > Why are we casting this? Also can you align it like:
> >
> This casting is done for safety, to prevent the comparison to be done
> in "unsigned int" due to integer promotion rules.
We are truncating away the top bytes but then we use them later.
Fortunately we use them only to print out a warning, but if we used them
for anything else it would be a serious bug.
Are you expecting .max_count to be negative?
If not then both sides are positive and type promotion is fine. We can
delete the first (buggy) warning, like I said and just leave the second
warning. It will now complain if any of bits 16 to 31 are set where
before it wouldn't.
> > to read what "goto error;" does. The error handling here calls
> > devm_kfree() which is not needed... devm_ functions automatically clean
> > up after themselves. This seems a pattern throughout. Do a search for
> > devm_free() and see which ones are really needed or not.
> >
> I know that memory allocated with devm_kzalloc() is freed at the end of the
> lifetime of the device it is attached to. However, in error paths, why wait
> until the device is destroyed? Why not free the memory earlier so that it
> can be used for other purposes?
My understanding is that devm_ functions are supposed to be used in the
probe() functions to simplify the error handling. So hopefully the
device lifetime ends as soon as this function returns a failure.
devm_ function are not a use them everywhere because now the kernel has
garbage collection type thing.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
