Date: Thu, 30 Apr 2015 14:49:57 +0300
From: Dan Carpenter <dan.carpenter@oracle.com>
To: "J. German Rivera" <German.Rivera@freescale.com>
Cc: gregkh@linuxfoundation.org, arnd@arndb.de, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org, stuart.yoder@freescale.com,
        bhupesh.sharma@freescale.com, agraf@suse.de, bhamciu1@freescale.com,
        nir.erez@freescale.com, itai.katz@freescale.com,
        scottwood@freescale.com, R89243@freescale.com,
        richard.schmitt@freescale.com
Subject: Re: [PATCH 1/7] staging: fsl-mc: MC bus IRQ support
Message-ID: <20150430114957.GW14154@mwanda>
References: <1430242750-17745-1-git-send-email-German.Rivera@freescale.com>
 <1430242750-17745-2-git-send-email-German.Rivera@freescale.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1430242750-17745-2-git-send-email-German.Rivera@freescale.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3035
Lines: 91

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@freescale.com>

These things are totally useless to the rest of us.  Don't add them.


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

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

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

		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.


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

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.

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/