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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150430114957.GW14154@mwanda>
Date:	Thu, 30 Apr 2015 14:49:57 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	"J. German Rivera" <German.Rivera@...escale.com>
Cc:	gregkh@...uxfoundation.org, arnd@...db.de,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
	stuart.yoder@...escale.com, bhupesh.sharma@...escale.com,
	agraf@...e.de, bhamciu1@...escale.com, nir.erez@...escale.com,
	itai.katz@...escale.com, scottwood@...escale.com,
	R89243@...escale.com, richard.schmitt@...escale.com
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@...escale.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/