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

[Jose Rivera]

Dan,

Thanks for your comments. My replies inline.

Thanks,

German

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx]
> Sent: Thursday, April 30, 2015 6:50 AM
> To: Rivera Jose-B46482
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; arnd@xxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Yoder Stuart-
> B08248; Sharma Bhupesh-B45370; agraf@xxxxxxx; Hamciuc Bogdan-BHAMCIU1;
> Erez Nir-RM30794; katz Itai-RM05202; Wood Scott-B07421; Marginean
> Alexandru-R89243; Schmitt Richard-B43082
> Subject: Re: [PATCH 1/7] staging: fsl-mc: MC bus IRQ support
> 
> On Tue, Apr 28, 2015 at 12:39:04PM -0500, J. German Rivera wrote:
> > Change-Id: I2a986c465989c3811de19cfe9ed0b77168250cb1
> > Reviewed-on: http://git.am.freescale.net:8181/33626
> > Tested-by: Review Code-CDREVIEW <CDREVIEW@xxxxxxxxxxxxx>
> 
> These things are totally useless to the rest of us.  Don't add them.
> 
Agreed. I'll remove them in the next respin.

> 
> > diff --git a/drivers/staging/fsl-mc/TODO b/drivers/staging/fsl-mc/TODO
> > index d78288b..1b8868d 100644
> > --- a/drivers/staging/fsl-mc/TODO
> > +++ b/drivers/staging/fsl-mc/TODO
> > @@ -8,6 +8,9 @@
> >  * Add at least one device driver for a DPAA2 object (child device of
> the
> >    fsl-mc bus).
> >
> > +* Enable code blocks guarded by #ifdef GIC_ITS_MC_SUPPORT, when
> > +GIC-ITS
> > +  support for the MC MSIs gets merged.
> > +
> 
> When will that be?  I'd really prefer to not add these ifdeffed bits at
> all.
> 
The owner of the GIC-ITS support patches would need to answer the "when"
question. These #ifdefs are needed to be able to make the code compile
without the GIC-ITS support in place. Of course, the code will not be moved
out of staging with these #ifdefs. There is already an item for this
in the drivers/staging/fsl-mc/TODO file.

> > +	if (status & (DPRC_IRQ_EVENT_OBJ_ADDED |
> > +		      DPRC_IRQ_EVENT_OBJ_REMOVED |
> > +		      DPRC_IRQ_EVENT_CONTAINER_DESTROYED |
> > +		      DPRC_IRQ_EVENT_OBJ_DESTROYED |
> > +		      DPRC_IRQ_EVENT_OBJ_CREATED)) {
> > +		unsigned int irq_count;
> > +
> > +		error = dprc_scan_objects(mc_dev, &irq_count);
> > +		if (error < 0) {
> > +			dev_err(dev, "dprc_scan_objects() failed: %d\n",
> error);
> > +			goto out;
> > +		}
> > +
> > +		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)

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

> 		if (irq_count >
> 		    mc_bus->resource_pools[FSL_MC_POOL_IRQ].max_count) {
> 
> [tab][tab][space][space][space][space]mc_bus->resource_pools[
> 
> That way you can tell the if condition from the indented block.  Plus
> that is standard kernel indenting style these days.
> 
Agreed. I'll change this in the next respin.

> 
> [ snip ]
> 
> > +	irqs = devm_kzalloc(&mc_dev->dev, irq_count * sizeof(irqs[0]),
> > +			    GFP_KERNEL);
> > +	if (!irqs) {
> > +		error = -ENOMEM;
> > +		dev_err(&mc_dev->dev, "No memory to allocate irqs[]\n");
> > +		goto error;
> 
> I kind of hate One Err Style error handling, because the error labels are
> so general...  You can never guess the point of it until you scroll down
>
Agreed. I will replace the generic error cleanup exit point with
error-specific cleanup exit points, and return directly when no cleanup
is needed.

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

> Also the error message isn't needed here.  kzalloc() has its own better
> error messages built-in.  Adding these error messages which will never be
> printed is just a waste of RAM.
> 
Agreed. Error message removed.

> In other words this should look like:
> 
> 	irqs = devm_kcalloc(&mc_dev->dev, sizeof(*irqs), irq_count,
> 			    GFP_KERNEL);
> 	if (!irqs)
> 		return -ENOMEM;
> 
> regards,
> dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/