linux-kernel - RE: [PATCH 2/3] (Resend part #1) Add the RapidIO support to powerpcarchitecture 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] [day] [month] [year] [list]

Message-ID: <ABF87B0B6A38C0458E319AC973ED68AEACF8A0@zch01exm26.fsl.freescale.net>
Date:	Fri, 4 Jan 2008 15:02:42 +0800
From:	"Zhang Wei" <Wei.Zhang@...escale.com>
To:	"Stephen Rothwell" <sfr@...b.auug.org.au>
Cc:	<linuxppc-dev@...abs.org>, <paulus@...ba.org>,
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/3] (Resend part #1) Add the RapidIO support to powerpcarchitecture with memory mapping support.

Hi,  

Thanks!

Maybe I should make a clean and split them into small patches.

Cheers!
Wei.

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