Subject: Re: [PATCH 1 of 3] Introduce __memcpy_toio32
From: "Bryan O'Sullivan" <bos@pathscale.com>
To: Matt Mackall <mpm@selenic.com>
Cc: linux-kernel@vger.kernel.org, akpm@osdl.org, hch@infradead.org
In-Reply-To: <20051228035231.GA3356@waste.org>
References: <patchbomb.1135726914@eng-12.pathscale.com>
	 <7b7b442a4d6338ae8ca7.1135726915@eng-12.pathscale.com>
	 <20051228035231.GA3356@waste.org>
Content-Type: text/plain
Organization: PathScale, Inc.
Date: Wed, 28 Dec 2005 06:47:42 -0800
Message-Id: <1135781263.1527.89.camel@serpentine.pathscale.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1661
Lines: 48

On Tue, 2005-12-27 at 21:52 -0600, Matt Mackall wrote:
> On Tue, Dec 27, 2005 at 03:41:55PM -0800, Bryan O'Sullivan wrote:
> > +/*
> > + * MMIO copy routines.  These are guaranteed to operate in units denoted
> > + * by their names.  This style of operation is required by some devices.
> > + */
> 
> Using kdoc style for new code is nice.

OK, will do.

> > +extern void fastcall __memcpy_toio32(volatile void __iomem *to, const void *from, size_t count);
> > +
> 
> Minor rant: extern is always redundant for function prototypes in C.

I know.  My intent was to keep the prototype consistent with the
prevailing style of other declarations in that same routine.  If you
think that cleanliness is more important, I'll be happy to change it.

> Suspicious use of volatile - writel is doing the actual write, this
> function never does a dereference.

Yeah.  I lost the plot there a bit.  I'll remove the volatiles.

>  As you've already got private
> copies of the pointers already in s and d, it's perfectily reasonable
> and idiomatic to do:
> 
> 	while (--count >= 0)
> 		__raw_writel(*s++, d++);

But pointer arithmetic is undefined on void pointers.  gcc lets you do
it, but it treats sizeof(void) as 1, which gives entirely the wrong
results in this case.

> And as you appear to be using the __raw.. version to avoid repeated
> mb()s, you probably ought to tack one on at the end.

Well spotted, thanks.

	<b

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