[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 5 May 2015 11:48:30 +0300
From: Dan Carpenter <dan.carpenter@...cle.com>
To: Jose Rivera <German.Rivera@...escale.com>
Cc: "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
"agraf@...e.de" <agraf@...e.de>, "arnd@...db.de" <arnd@...db.de>,
"bhupesh.sharma@...escale.com" <bhupesh.sharma@...escale.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Stuart Yoder <stuart.yoder@...escale.com>,
Scott Wood <scottwood@...escale.com>,
"nir.erez@...escale.com" <nir.erez@...escale.com>,
"itai.katz@...escale.com" <itai.katz@...escale.com>,
"bhamciu1@...escale.com" <bhamciu1@...escale.com>,
"R89243@...escale.com" <R89243@...escale.com>,
Richard Schmitt <richard.schmitt@...escale.com>
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);
> > > +
> > > + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists