lkml.org 
[lkml]   [2015]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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);

> > > +
> > > + 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



\
 
 \ /
  Last update: 2015-05-05 11:21    [W:0.151 / U:0.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site