linux-kernel - Re: [PATCH 2/3] (Resend part #1) Add the RapidIO support to powerpc architecture with memory mapping 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]

Date:	Sun, 23 Dec 2007 09:39:11 +1100
From:	Stephen Rothwell <sfr@...b.auug.org.au>
To:	Zhang Wei <wei.zhang@...ESCALE.COM>
Cc:	mporter@...nel.crashing.org, paulus@...ba.org,
	galak@...nel.crashing.org, linuxppc-dev@...abs.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] (Resend part #1) Add the RapidIO support to powerpc
 architecture with memory mapping support.

Hi,

This is a very large patch.  It may be easier to review if it could be
split on some logical way, that is at all possible (I don't know either
way).  This is just a quick note about some more trivial things.

On Fri, 21 Dec 2007 17:58:43 +0800 Zhang Wei <wei.zhang@...ESCALE.COM> wrote:
>
> +struct rio_priv {
> +	volatile void __iomem *regs_win;
> +	volatile struct rio_atmu_regs __iomem *atmu_regs;
> +	volatile struct rio_atmu_regs __iomem *maint_atmu_regs;
> +	volatile struct rio_atmu_regs __iomem *dbell_atmu_regs;
> +	volatile void __iomem *dbell_win;
> +	volatile void __iomem *maint_win;
> +	volatile struct rio_msg_regs __iomem *msg_regs;

Paulus has said that any pointer marked __iomem does not need to be
volatile ...

> +static int of_cells_get(struct device_node *np, const char *str)
> +{
> +	struct device_node *tmp = NULL;
> +	const int *var = NULL;

These initializations are unnecessary.

> +	var = of_get_property(np, str, NULL);
> +	tmp = of_get_parent(np);
> +
> +	while (!var && tmp) {
> +		var = (int *)of_get_property(tmp, str, NULL);

While I applaud the number of casts remove by this patch, this one is an
unnecessary addition.

> +		of_node_put(tmp);
> +		tmp = of_get_parent(np);

You should do the above two line in the opposite order. Also do you
really want to keep getting the parent of the same node over and over
(i.e. you never change np)?

> +	}

You probably want a final of_node_put(tmp).


> +	INFO("Phy type: ");
> +	switch (phy_type) {
> +	case RIO_PHY_SERIAL:
> +		printk("serial\n");
> +		break;
> +	case RIO_PHY_PARALLEL:
> +		printk("parallel");

Missing \n

> +	port = kzalloc(sizeof(struct rio_mport), GFP_KERNEL);
> +	if (!port) {
> +		ERR("Can't alloc memory for 'port'\n");
> +		rc = -ENOMEM;
> +		goto err;
> +	}
>  	port->id = 0;
>  	port->index = 0;

These two could go as you just allocated zeroed memory.

-- 
Cheers,
Stephen Rothwell                    sfr@...b.auug.org.au
http://www.canb.auug.org.au/~sfr/

Content of type "application/pgp-signature" skipped