Add memcpy_toio32, a 32-bit MMIO copy routine

+EXPORT_SYMBOL_GPL(__memcpy_toio32);

+/*
+ * MMIO copy routines. These are guaranteed to operate in units denoted
+ * by their names. This style of operation is required by some devices.
+ */

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

lib/iomap.c +2 -0

@@ -187,6 +187,22 @@
 EXPORT_SYMBOL(iowrite16_rep);
 EXPORT_SYMBOL(iowrite32_rep);
 
+/*
+ * Copy data to an MMIO region.  MMIO space accesses are performed

Oh yeah:

Quoted 1 line

+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.

diff -r 7b7b442a4d63 -r 042b7d9004ac arch/x86_64/lib/memcpy32.S

arch/x86_64/lib/memcpy32.S new file +15 -0

@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2003, 2004, 2005 PathScale, Inc.
+ */
+
+/*
+ * memcpy32 - Copy a memory block, 32 bits at a time.
+ *
+ * Count is number of dwords; it need not be a qword multiple.
+ * Input:
+ * rdi destination
+ * rsi source
+ * rdx count
+ */
+
+ 	.globl memcpy32

arch/x86_64/lib/memcpy32.S line 21

Quoted 8 lines

arch/x86_64/lib/memcpy32.S new file +8 -0

+
+ 	.globl memcpy32
+memcpy32:
+	movl %edx,%ecx
+	shrl $1,%ecx
+	andl $1,%edx
+	rep
+	movsq

arch/x86_64/lib/memcpy32.S line 25

Quoted 4 lines

arch/x86_64/lib/memcpy32.S new file +4 -0

+	movl %edx,%ecx
+	rep
+	movsd
+	ret

Any reason this needs its own .S file? One wonders if the

        .p2align 4

in memcpy.S is appropriate here too. Splitting rep movsq across two lines is a little weird to me too, but I see Andi did it too.

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.

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

Quoted 4 lines · Bryan O'Sullivan

+/*
+ * 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.

include/asm-generic/iomap.h line 64

Quoted 2 lines

include/asm-generic/iomap.h +2 -0

+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.

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++);

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.

It's better to do an include here. Duplicating prototypes in .c files is frowned upon (despite the fact that it's already done here).

We've been steadily moving towards grouping EXPORTs with function definitions. Do *_ksyms.c exist solely to provide exports for functions defined in assembly at this point? If so, perhaps we ought to come up with a suitable export macro for asm files.

Any reason this needs its own .S file?

One wonders if the

        .p2align 4

in memcpy.S is appropriate here too.

Quoted 3 lines · Roland Dreier

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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 4 lines

include/asm-generic/iomap.h line 64

Quoted 2 lines

include/asm-generic/iomap.h +2 -0

+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.

Quoted 12 lines · Bryan O'Sullivan

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

lib/iomap.c +2 -0

@@ -187,6 +187,22 @@
 EXPORT_SYMBOL(iowrite16_rep);
 EXPORT_SYMBOL(iowrite32_rep);
 
+/*
+ * Copy data to an MMIO region.  MMIO space accesses are performed

lib/iomap.c line 201

Quoted 10 lines

lib/iomap.c +10 -0

+ * 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++);

BTW, does the original loop really work?

Quoted 2 lines

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.

Quoted 6 lines · Bryan O'Sullivan

Quoted 3 lines · Roland Dreier

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.

Quoted 6 lines

Will do.

Quoted 1 line

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

* 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.
...

Quoted 31 lines · Matt Mackall

Quoted 6 lines · Bryan O'Sullivan

Quoted 3 lines · Roland Dreier

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.

Quoted 6 lines

Will do.

Quoted 1 line

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

* 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.

Quoted 3 lines

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?

Quoted 2 lines

Where's the problem with the __HAVE_ARCH_* mechanism?

The head penguin peed on it last week.

Quoted 2 lines

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.

Quoted 7 lines

Quoted 2 lines

The head penguin peed on it last week.

Actually "sprinkling 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.

At least they could have used water instead of pee.

...

Quoted 1 line

Quoted 1 line

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

Linus

Did it really take 3 years to develop this?

Anyway this contains
copyright but not GPL, not allowing us to distribute it.