Re: [PATCH 1 of 3] Introduce __memcpy_toio32

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Matt Mackall]

On Tue, Dec 27, 2005 at 10:03:32PM -0800, David Wagner wrote:
> 
> In article <20051228035231.GA3356@waste.org> you write:
> >> +void fastcall __memcpy_toio32(volatile void __iomem *d, const void
> >*s, size_t count)
> >> +{
> >> +	volatile u32 __iomem *dst = d;
> >> +	const u32 *src = s;
> >> +
> >> +	while (--count >= 0) {
> >> +		__raw_writel(*src++, dst++);
> >> +}
> >
> >Suspicious use of volatile - writel is doing the actual write, this
> >function never does a dereference. 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++);
> 
> I don't think *s++ or d++ is going to be accepted by the compiler,
> given that s and d are both "void *"'s.  The posted code casts to
> "u32 *", which should make the dereference and autoincrement work
> correctly.
> 
> [Not posted to linux-kernel, in case I'm totally confused.]

No, you're absolutely right. It's idiomatic for things like strcpy,
where the type is known, but not for memcpy.

However, it just so happens that GCC will happily treat math on void
pointers as if they were char pointers. So this in fact will
compile without errors:

void cpy(void *a, void *b, int count)
{
        while (count--)
                copybyte(a++, b++);
}

Linus thinks this is a feature but I don't, so I'm not recommending
it. In fact, given the whole point of this function is to copy dwords,
not bytes, this GCCism fails us for type-safety.

At any rate, the dereference on s above _is_ a compile error - it's void
type.

[cc:ed back to lkml to keep me honest]

-- 
Mathematics is the supreme nostalgia of our time.

[PATCH 0 of 3] Add memcpy_toio32, a 32-bit MMIO copy routine

[Bryan O'Sullivan]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

Following some discussion with Matt, Andrew and Chris, here is a recast
of the 32-bit MMIO patch I posted the other day.  The routine is now
named memcpy_toio32, and is provided in generic and x86_64-optimised
forms.

I haven't added a memcpy_fromio32, or routines for other access sizes,
because our hardware doesn't need them.  If someone wants them for
reasons of symmetry, I can introduce them.

Signed-off-by: Bryan O'Sullivan <bos@pathscale.com>

[PATCH 1 of 3] Introduce __memcpy_toio32

[Bryan O'Sullivan]

This routine is an arch-independent building block for memcpy_toio32.
It copies data to a memory-mapped I/O region, using 32-bit accesses.
This style of access is required by some devices.

Signed-off-by: Bryan O'Sullivan <bos@pathscale.com>

diff -r 789a24638663 -r 7b7b442a4d63 include/asm-generic/iomap.h
--- a/include/asm-generic/iomap.h	Tue Dec 27 09:27:10 2005 +0800
+++ b/include/asm-generic/iomap.h	Tue Dec 27 15:41:48 2005 -0800
@@ -56,6 +56,12 @@
 extern void fastcall iowrite16_rep(void __iomem *port, const void *buf, unsigned long count);
 extern void fastcall iowrite32_rep(void __iomem *port, const void *buf, unsigned long count);
 
+/*
+ * MMIO copy routines.  These are guaranteed to operate in units denoted
+ * by their names.  This style of operation is required by some devices.
+ */
+extern void fastcall __memcpy_toio32(volatile void __iomem *to, const void *from, size_t count);
+
 /* Create a virtual mapping cookie for an IO port range */
 extern void __iomem *ioport_map(unsigned long port, unsigned int nr);
 extern void ioport_unmap(void __iomem *);
diff -r 789a24638663 -r 7b7b442a4d63 lib/iomap.c
--- a/lib/iomap.c	Tue Dec 27 09:27:10 2005 +0800
+++ b/lib/iomap.c	Tue Dec 27 15:41:48 2005 -0800
@@ -187,6 +187,22 @@
 EXPORT_SYMBOL(iowrite16_rep);
 EXPORT_SYMBOL(iowrite32_rep);
 
+/*
+ * Copy data to an MMIO region.  MMIO space accesses are performed
+ * in the sizes indicated in each function's name.
+ */
+void fastcall __memcpy_toio32(volatile void __iomem *d, const void *s, size_t count)
+{
+	volatile u32 __iomem *dst = d;
+	const u32 *src = s;
+
+	while (--count >= 0) {
+		__raw_writel(*src++, dst++);
+	}
+}
+
+EXPORT_SYMBOL_GPL(__memcpy_toio32);
+
 /* Create a virtual mapping cookie for an IO port range */
 void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Roland Dreier]

A couple of comments here:

 > +/*
 > + * Copy data to an MMIO region.  MMIO space accesses are performed
 > + * in the sizes indicated in each function's name.
 > + */
 > +void fastcall __memcpy_toio32(volatile void __iomem *d, const void *s, size_t count)
 > +{
 > +	volatile u32 __iomem *dst = d;
 > +	const u32 *src = s;
 > +
 > +	while (--count >= 0) {
 > +		__raw_writel(*src++, dst++);
 > +	}

I think the principle of least surprise calls for memcpy_toio32 to be
ordered the same way memcpy_toio is.  In other words there should be a
wmb() after the loop.

Also, no need for the { } for the while loop.

 > +}
 > +
 > +EXPORT_SYMBOL_GPL(__memcpy_toio32);

You're adding this symbol and exporting it even if the arch will
supply its own version.  So this is pure kernel .text bloat...

 - R.

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Bryan O'Sullivan]

On Tue, 2005-12-27 at 17:10 -0800, Roland Dreier wrote:

> I think the principle of least surprise calls for memcpy_toio32 to be
> ordered the same way memcpy_toio is.  In other words there should be a
> wmb() after the loop.

Will do.

> Also, no need for the { } for the while loop.

Fine.  There doesn't seem to be much consistency in whether to use
curlies for single-line blocks.

> You're adding this symbol and exporting it even if the arch will
> supply its own version.  So this is pure kernel .text bloat...

I don't know what you'd prefer, so let me enumerate a few alternatives,
and you can either tell me which you'd prefer, or point out something
I've missed that would be even better.  I'm entirely flexible on this.

      * Use the __HAVE_ARCH_* mechanism that include/asm-*/string.h
        uses.  Caveat: Linus has lately come out as hating this style.
        It makes for the smallest patch, though.
      * Define the generic code in lib/, and have each arch that really
        uses it export it.
      * Put generic code in include/asm-generic/algo-memcpy_toio32.h,
        and have each arch that needs it #include it somewhere and use
        it.

Have I missed anything?

	<b

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Matt Mackall]

On Wed, Dec 28, 2005 at 06:40:03AM -0800, Bryan O'Sullivan wrote:
> On Tue, 2005-12-27 at 17:10 -0800, Roland Dreier wrote:
> 
> > I think the principle of least surprise calls for memcpy_toio32 to be
> > ordered the same way memcpy_toio is.  In other words there should be a
> > wmb() after the loop.
> 
> Will do.
> 
> > Also, no need for the { } for the while loop.
> 
> Fine.  There doesn't seem to be much consistency in whether to use
> curlies for single-line blocks.

We've been very consistent in discouraging it in new code. Enforcement
of fine points of coding style is a post-2.5 phenomenon, so it hasn't
hit all the tree yet.

> > You're adding this symbol and exporting it even if the arch will
> > supply its own version.  So this is pure kernel .text bloat...
> 
> I don't know what you'd prefer, so let me enumerate a few alternatives,
> and you can either tell me which you'd prefer, or point out something
> I've missed that would be even better.  I'm entirely flexible on this.
> 
>       * Use the __HAVE_ARCH_* mechanism that include/asm-*/string.h
>         uses.  Caveat: Linus has lately come out as hating this style.
>         It makes for the smallest patch, though.
>       * Define the generic code in lib/, and have each arch that really
>         uses it export it.

I'd favor this, at least for this case. If it becomes more widely
used, we'll relocate the export.

>       * Put generic code in include/asm-generic/algo-memcpy_toio32.h,
>         and have each arch that needs it #include it somewhere and use
>         it.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Adrian Bunk]

On Wed, Dec 28, 2005 at 08:51:14AM -0600, Matt Mackall wrote:
> On Wed, Dec 28, 2005 at 06:40:03AM -0800, Bryan O'Sullivan wrote:
> > On Tue, 2005-12-27 at 17:10 -0800, Roland Dreier wrote:
> > 
> > > I think the principle of least surprise calls for memcpy_toio32 to be
> > > ordered the same way memcpy_toio is.  In other words there should be a
> > > wmb() after the loop.
> > 
> > Will do.
> > 
> > > Also, no need for the { } for the while loop.
> > 
> > Fine.  There doesn't seem to be much consistency in whether to use
> > curlies for single-line blocks.
> 
> We've been very consistent in discouraging it in new code. Enforcement
> of fine points of coding style is a post-2.5 phenomenon, so it hasn't
> hit all the tree yet.
> 
> > > You're adding this symbol and exporting it even if the arch will
> > > supply its own version.  So this is pure kernel .text bloat...
> > 
> > I don't know what you'd prefer, so let me enumerate a few alternatives,
> > and you can either tell me which you'd prefer, or point out something
> > I've missed that would be even better.  I'm entirely flexible on this.
> > 
> >       * Use the __HAVE_ARCH_* mechanism that include/asm-*/string.h
> >         uses.  Caveat: Linus has lately come out as hating this style.
> >         It makes for the smallest patch, though.
> >       * Define the generic code in lib/, and have each arch that really
> >         uses it export it.
> 
> I'd favor this, at least for this case. If it becomes more widely
> used, we'll relocate the export.
>...

I don't like this for two reasons:
- we are moving exports to the actual functions and steadily killing all
  *syms* files
- the lib-y approach has the disadvantage of completely omitting the
  function if it's used only in modules resulting in non-working
  modules

Where's the problem with the __HAVE_ARCH_* mechanism?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Matt Mackall]

On Sat, Dec 31, 2005 at 12:46:28AM +0100, Adrian Bunk wrote:
> On Wed, Dec 28, 2005 at 08:51:14AM -0600, Matt Mackall wrote:
> > On Wed, Dec 28, 2005 at 06:40:03AM -0800, Bryan O'Sullivan wrote:
> > > On Tue, 2005-12-27 at 17:10 -0800, Roland Dreier wrote:
> > > 
> > > > I think the principle of least surprise calls for memcpy_toio32 to be
> > > > ordered the same way memcpy_toio is.  In other words there should be a
> > > > wmb() after the loop.
> > > 
> > > Will do.
> > > 
> > > > Also, no need for the { } for the while loop.
> > > 
> > > Fine.  There doesn't seem to be much consistency in whether to use
> > > curlies for single-line blocks.
> > 
> > We've been very consistent in discouraging it in new code. Enforcement
> > of fine points of coding style is a post-2.5 phenomenon, so it hasn't
> > hit all the tree yet.
> > 
> > > > You're adding this symbol and exporting it even if the arch will
> > > > supply its own version.  So this is pure kernel .text bloat...
> > > 
> > > I don't know what you'd prefer, so let me enumerate a few alternatives,
> > > and you can either tell me which you'd prefer, or point out something
> > > I've missed that would be even better.  I'm entirely flexible on this.
> > > 
> > >       * Use the __HAVE_ARCH_* mechanism that include/asm-*/string.h
> > >         uses.  Caveat: Linus has lately come out as hating this style.
> > >         It makes for the smallest patch, though.
> > >       * Define the generic code in lib/, and have each arch that really
> > >         uses it export it.
> > 
> > I'd favor this, at least for this case. If it becomes more widely
> > used, we'll relocate the export.
> >...
> 
> I don't like this for two reasons:
> - we are moving exports to the actual functions and steadily killing all
>   *syms* files
> - the lib-y approach has the disadvantage of completely omitting the
>   function if it's used only in modules resulting in non-working
>   modules
> 
> Where's the problem with the __HAVE_ARCH_* mechanism?

The head penguin peed on it last week.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Linus Torvalds]

On Fri, 30 Dec 2005, Matt Mackall wrote:
> > 
> > Where's the problem with the __HAVE_ARCH_* mechanism?
> 
> The head penguin peed on it last week.

Actually "sprkinling with penguin pee" means that something is blessed 
(it's like a kernel baptism). Maybe that's not very civilized, but hey, 
penguins don't have thumbs, and are thus kind of limited in their actions. 
Don't be speciest.

So the head penguin didn't pee on it, it turned its back in disgust, and 
hoped that it would freeze to death in the arctic winter.

And no, I don't like the __HAVE_ARCH_xxx mechanisms at all. They are 
pointless, and hard to follow. If an architecture wants to use a generic 
mechanism, it should do one of the following (or a combination):

 - use the config file mechanism, and use

	obj-$(CONFIG_GENERIC_FOO) += generic-foo.c

   in a Makefile to link in the generic version.

   Examples: CONFIG_RWSEM_GENERIC_SPINLOCK.

 - just include the generic header from its own header, eg just do a

	#include <asm-generic/div64.h>

   or similar.

Now, the latter in particular is very easy to follow: if you look into the 
<asm/div64.h> file and see that it just includes <asm-generic/div64.h>, 
it's very obvious what is going on and where to find the real 
implementation. You never have to wonder what the indirection means. 

Similarly, anybody that fixes the generic header file can _trivially_ grep 
for its use. So the code stays clean, and there are absolutely zero 
compile-time conditionals, and the linkages both ways are obvious. And 
architectures that do _not_ use the generic routines are totally 
unaffected by them, and they don't need to specify any flags like "I have 
my own routines" to disable things.

Now, the CONFIG_GENERIC_FOO thing is a bit less obvious, and you may have 
to know about that config option in order to realize that a particular 
architecture is using a generic library routine, but at least with those 
Kconfig options, the language to describe them is clean these days, and 
it's _the_ standard way to express configuration information. So it may be 
a bit subtler and more indirect, but once you get used to it, it too is 
very clean.

In contrast, the __HAVE_ARCH_xxx thing has zero upsides. It just causes 
#ifdef mess in C source files, and unnecessary noise in standard header 
files. I know it's been there for a long time, but just grep for 
__HAVE_ARCH_MEMCPY and cry. Why the hell should all the architectures that 
have their own optimized memcpy() have to tell the rest of the world about 
it?

[ Yeah, I know why: bad implementation choice. It could easily have been 
  done with the asm-generic approach or CONFIG_GENERIC_MEMCPY, but it 
  wasn't. Note that the __HAVE_ARCH_xxx thing isn't even a standard form: 
  sometimes it's the negation: __ARCH_WANT_xxx, and sometimes it's called
  something else entirely, like USE_ELF_CORE_DUMP or HAVE_PCI_MMAP, or
  ARCH_HAS_PREFETCH or HAVE_CSUM_COPY_USER. My point being that it's 
  totally ad-hoc and random. ]

So we do have tons of ugly stuff, I just am trying to argue for not making 
more of it (I don't think it's a big enough deal that it would be worth it 
trying to clean up old uses).

If you look at the mutex patches, for example, I think everybody will 
agree that they look much _better_ after they moved to just using the 
trivial "#include <asm-generic/mutex-xyzzy.h>" format. At least I don't 
_think_ this is just a personal weird hang-up of mine. It's literally a 
cleanliness issue.

			Linus

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Adrian Bunk]

On Fri, Dec 30, 2005 at 04:23:46PM -0800, Linus Torvalds wrote:
>...
> > > Where's the problem with the __HAVE_ARCH_* mechanism?
>...
> And no, I don't like the __HAVE_ARCH_xxx mechanisms at all. They are 
> pointless, and hard to follow. If an architecture wants to use a generic 
> mechanism, it should do one of the following (or a combination):
> 
>  - use the config file mechanism, and use
> 
> 	obj-$(CONFIG_GENERIC_FOO) += generic-foo.c
> 
>    in a Makefile to link in the generic version.
> 
>    Examples: CONFIG_RWSEM_GENERIC_SPINLOCK.
> 
>  - just include the generic header from its own header, eg just do a
> 
> 	#include <asm-generic/div64.h>
> 
>    or similar.
> 
> Now, the latter in particular is very easy to follow: if you look into the 
> <asm/div64.h> file and see that it just includes <asm-generic/div64.h>, 
> it's very obvious what is going on and where to find the real 
> implementation. You never have to wonder what the indirection means. 
>...
> Now, the CONFIG_GENERIC_FOO thing is a bit less obvious, and you may have 
> to know about that config option in order to realize that a particular 
> architecture is using a generic library routine, but at least with those 
> Kconfig options, the language to describe them is clean these days, and 
> it's _the_ standard way to express configuration information. So it may be 
> a bit subtler and more indirect, but once you get used to it, it too is 
> very clean.
>...

OK, this I don't have any problem with.

I'm not yet fully convinced that __HAVE_ARCH_xxx is really that bad, but 
your proposed solution doesn't have the problems I had in mind.

What is OK:
  obj-$(CONFIG_GENERIC_FOO) += generic-foo.o

What is not OK:
  lib-y += generic-foo.o

The latter has the following disadvantages:
- it's non-obvious whether the object actually gets included in the 
  kernel
- if the contents of generic-foo.o is only used in modules, 
  generic-foo.o is _not_ included in the kernel resulting in an
  obvious breakage

> 			Linus

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Roland Dreier]

 > > You're adding this symbol and exporting it even if the arch will
 > > supply its own version.  So this is pure kernel .text bloat...

 > I don't know what you'd prefer, so let me enumerate a few alternatives,
 > and you can either tell me which you'd prefer, or point out something
 > I've missed that would be even better.  I'm entirely flexible on this.
 > 
 >       * Use the __HAVE_ARCH_* mechanism that include/asm-*/string.h
 >         uses.  Caveat: Linus has lately come out as hating this style.
 >         It makes for the smallest patch, though.
 >       * Define the generic code in lib/, and have each arch that really
 >         uses it export it.
 >       * Put generic code in include/asm-generic/algo-memcpy_toio32.h,
 >         and have each arch that needs it #include it somewhere and use
 >         it.

The middle alternative seems the cleanest, although I'm not sure where
the export really belongs.

I don't think I could really say the right way to do this without
thinking some more -- but I am positive that exporting a function that
will never ever be called is something we should work hard to avoid.

 - R.

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Roland Dreier]

Oh yeah:

 > +EXPORT_SYMBOL_GPL(__memcpy_toio32);

I think this is a sufficiently basic facility that it might as well be
plain EXPORT_SYMBOL(), although I don't mind making things harder on
non-GPL modules.

 - R.

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Matt Mackall]

On Tue, Dec 27, 2005 at 05:11:50PM -0800, Roland Dreier wrote:
> Oh yeah:
> 
>  > +EXPORT_SYMBOL_GPL(__memcpy_toio32);
> 
> I think this is a sufficiently basic facility that it might as well be
> plain EXPORT_SYMBOL(), although I don't mind making things harder on
> non-GPL modules.

This area is so murky, it verges on religion. Thus, I really think
this ought to be submitter's preference.

Any attempt to make a hard and fast rule of what's considered GPL or
not will potentially dilute any legal protection for exported symbols
predating the existence of the EXPORT_SYMBOL_GPL mechanism that are
nonetheless only reasonably used by GPL code (which by some
contributors' measures is all of them). 

In other words, we must be careful not to directly or indirectly
construe EXPORT_SYMBOL as a carte blanche for linking GPL-incompatible
code.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Matt Mackall]

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.

> +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'd prefer that we adopt a standard of not using extern for functions,
as it would make use of extern for variables (almost always
inappropriate, especially in C files) stick out more.

While people claim this has some documentation value ("I _meant_ for
this to be exported"), I think it actually has a net negative effect,
as quite a number of people actually think the "extern" keyword does
some unspecified magic here and ignore the namespace pollution of their
theoretically "un-externed" but not explicitly static functions.

There isn't any sort of consensus on this point as far as I know, so
this is just me venting.

>  /* Create a virtual mapping cookie for an IO port range */
>  extern void __iomem *ioport_map(unsigned long port, unsigned int nr);
>  extern void ioport_unmap(void __iomem *);
> diff -r 789a24638663 -r 7b7b442a4d63 lib/iomap.c
> --- a/lib/iomap.c	Tue Dec 27 09:27:10 2005 +0800
> +++ b/lib/iomap.c	Tue Dec 27 15:41:48 2005 -0800
> @@ -187,6 +187,22 @@
>  EXPORT_SYMBOL(iowrite16_rep);
>  EXPORT_SYMBOL(iowrite32_rep);
>  
> +/*
> + * Copy data to an MMIO region.  MMIO space accesses are performed
> + * in the sizes indicated in each function's name.
> + */
> +void fastcall __memcpy_toio32(volatile void __iomem *d, const void *s, size_t count)
> +{
> +	volatile u32 __iomem *dst = d;
> +	const u32 *src = s;
> +
> +	while (--count >= 0) {
> +		__raw_writel(*src++, dst++);
> +}

Suspicious use of volatile - writel is doing the actual write, this
function never does a dereference. 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++);

I'd personally write this as:

	while (count--)
		__raw_writel(*s++, d++);

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.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Bryan O'Sullivan]

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

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Matt Mackall]

On Wed, Dec 28, 2005 at 06:47:42AM -0800, Bryan O'Sullivan wrote:
> > > +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.

No, I'm actually just raising it for discussion. I personally don't
add them, even for consistency, as I'd like to eventually see them gone.
But if Christoph and Andrew won't care, my tiny crusade is probably lost.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Geert Uytterhoeven]

On Tue, 27 Dec 2005, Matt Mackall wrote:
> On Tue, Dec 27, 2005 at 03:41:55PM -0800, Bryan O'Sullivan wrote:
> >  /* Create a virtual mapping cookie for an IO port range */
> >  extern void __iomem *ioport_map(unsigned long port, unsigned int nr);
> >  extern void ioport_unmap(void __iomem *);
> > diff -r 789a24638663 -r 7b7b442a4d63 lib/iomap.c
> > --- a/lib/iomap.c	Tue Dec 27 09:27:10 2005 +0800
> > +++ b/lib/iomap.c	Tue Dec 27 15:41:48 2005 -0800
> > @@ -187,6 +187,22 @@
> >  EXPORT_SYMBOL(iowrite16_rep);
> >  EXPORT_SYMBOL(iowrite32_rep);
> >  
> > +/*
> > + * Copy data to an MMIO region.  MMIO space accesses are performed
> > + * in the sizes indicated in each function's name.
> > + */
> > +void fastcall __memcpy_toio32(volatile void __iomem *d, const void *s, size_t count)
> > +{
> > +	volatile u32 __iomem *dst = d;
> > +	const u32 *src = s;
> > +
> > +	while (--count >= 0) {
> > +		__raw_writel(*src++, dst++);
> > +}
> 
> Suspicious use of volatile - writel is doing the actual write, this
> function never does a dereference. 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++);
> 
> I'd personally write this as:
> 
> 	while (count--)
> 		__raw_writel(*s++, d++);

Indeed.

BTW, does the original loop really work? Size size_t is unsigned, >= 0 is
always true and we have a nice infinite loop?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH 1 of 3] Introduce __memcpy_toio32

[Bryan O'Sullivan]

On Wed, 2005-12-28 at 16:18 +0100, Geert Uytterhoeven wrote:

> BTW, does the original loop really work?

I had to turn it into a for loop shortly after I posted it, for exactly
the reason you pointed out.

	<b