Re: [PATCH 1/7] staging: fsl-mc: MC bus IRQ support

[Dan Carpenter <dan.carpenter@oracle.com>]

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