Removal of ____raw_readq() and ____raw_writeq() from asm-mips/io.h

Removal of ____raw_readq() and ____raw_writeq() from asm-mips/io.h

[Kevin Paul Herbert]

In edit 1.68, the non-interrupt locking versions of
raw_readq()/raw_writeq() were removed, in favor of locking ones. While
this makes sense in general, it breaks the compilation of the sb1250
which uses the non-locking versions (____raw_readq() and
____raw_writeq()) in interrupt handlers.

Personally, I think that it is very confusing to have so many similar
macros with similar names and increasing numbers of underscores, so I
don't really have a problem with this. I've modified
arch/mips/sibyte/sb1250/time.c and arch/mips/sibyte/sb1250/irq.c to use
the __ versions and have a few more instructions of overhead.

My question is whether this removal was intended or not, or whether
there are some other changes to the handlers in the sb1250-specific code
that got dropped somewhere.

If the consensus is that the ____ versions really should perish for the
sake of simplicity, I'll send my simple patches to the list to fix the
sb1250 build.

Thanks,
Kevin
-- 
Kevin Paul Herbert <kph@cisco.com>
cisco Systems, Inc.

Re: Removal of ____raw_readq() and ____raw_writeq() from asm-mips/io.h

[Ladislav Michl]

On Tue, Jan 27, 2004 at 05:58:31PM -0800, Kevin Paul Herbert wrote:
> In edit 1.68, the non-interrupt locking versions of
> raw_readq()/raw_writeq() were removed, in favor of locking ones. While
> this makes sense in general, it breaks the compilation of the sb1250
> which uses the non-locking versions (____raw_readq() and
> ____raw_writeq()) in interrupt handlers.
Why was someone using these function at all? if you don't need locking
simply do *reg_addr = val; 
> Personally, I think that it is very confusing to have so many similar
> macros with similar names and increasing numbers of underscores, so I
> don't really have a problem with this. I've modified
> arch/mips/sibyte/sb1250/time.c and arch/mips/sibyte/sb1250/irq.c to use
> the __ versions and have a few more instructions of overhead.
__ versions wasn't probably intended to use in C code. One should use
readq/writeq to get sane behaviour. These function was introduced to
hide architecture specific details. If you need something special, you
should introduce your own macros.
> My question is whether this removal was intended or not, or whether
> there are some other changes to the handlers in the sb1250-specific code
> that got dropped somewhere.
Yes it was. And I'm the one to blame for it ;)
> If the consensus is that the ____ versions really should perish for the
> sake of simplicity, I'll send my simple patches to the list to fix the
> sb1250 build.
Yes please.

	ladis

Re: Removal of ____raw_readq() and ____raw_writeq() from asm-mips/io.h

[Jes Sorensen]

>>>>> "Ladislav" == Ladislav Michl <ladis@linux-mips.org> writes:

Ladislav> On Tue, Jan 27, 2004 at 05:58:31PM -0800, Kevin Paul Herbert
Ladislav> wrote:
>> In edit 1.68, the non-interrupt locking versions of
>> raw_readq()/raw_writeq() were removed, in favor of locking
>> ones. While this makes sense in general, it breaks the compilation
>> of the sb1250 which uses the non-locking versions (____raw_readq()
>> and ____raw_writeq()) in interrupt handlers.

Ladislav> Why was someone using these function at all? if you don't
Ladislav> need locking simply do *reg_addr = val;

ARGHHHHHHHHHH!

If you are accessing memory mapped registers or memory on a PCI
device, ie. likely on a 1250, you *must* use the readX/__raw_readX
macros. Anybody just doing *reg = val on a PCI device should be
banned from writing code for life!

Jes

Re: Removal of ____raw_readq() and ____raw_writeq() from asm-mips/io.h

[Ladislav Michl]

On Wed, Jan 28, 2004 at 05:49:58AM -0500, Jes Sorensen wrote:
> >>>>> "Ladislav" == Ladislav Michl <ladis@linux-mips.org> writes:
> 
> Ladislav> On Tue, Jan 27, 2004 at 05:58:31PM -0800, Kevin Paul Herbert
> Ladislav> wrote:
> >> In edit 1.68, the non-interrupt locking versions of
> >> raw_readq()/raw_writeq() were removed, in favor of locking
> >> ones. While this makes sense in general, it breaks the compilation
> >> of the sb1250 which uses the non-locking versions (____raw_readq()
> >> and ____raw_writeq()) in interrupt handlers.
> 
> Ladislav> Why was someone using these function at all? if you don't
> Ladislav> need locking simply do *reg_addr = val;
> 
> ARGHHHHHHHHHH!
> 
> If you are accessing memory mapped registers or memory on a PCI
> device, ie. likely on a 1250, you *must* use the readX/__raw_readX
> macros. Anybody just doing *reg = val on a PCI device should be
> banned from writing code for life!

eh? I said nothing about PCI device. These ____raw_writeq are
used in board specific code. Anyway, defining struct sb_registers
and ioremaping it would be nice solution (I didn't read code too
carefully, so maybye not in this particular case where registers
are 64bit width, but I definitely prefer it in board specific code
over read[bwl]/write[bwl]). Also readq/writeq seems mips specific,
so rants about portability doesn't apply.

	ladis

Re: Removal of ____raw_readq() and ____raw_writeq() from asm-mips/io.h

[Ralf Baechle]

On Wed, Jan 28, 2004 at 03:08:28PM +0000, Ladislav Michl wrote:

> eh? I said nothing about PCI device. These ____raw_writeq are
> used in board specific code. Anyway, defining struct sb_registers
> and ioremaping it would be nice solution (I didn't read code too
> carefully, so maybye not in this particular case where registers
> are 64bit width, but I definitely prefer it in board specific code
> over read[bwl]/write[bwl]). Also readq/writeq seems mips specific,
> so rants about portability doesn't apply.

They're not MIPS specific; they're just not so common because some people
still believe 64-bit is something esotheric they don't need ;-)

Try grep -lw readq include/asm-*/io.h - 6 architectures implement it.

  Ralf

Re: Removal of ____raw_readq() and ____raw_writeq() from asm-mips/io.h

[Jes Sorensen]

>>>>> "Ladislav" == Ladislav Michl <ladis@linux-mips.org> writes:

Ladislav> On Wed, Jan 28, 2004 at 05:49:58AM -0500, Jes Sorensen
Ladislav> wrote:
>> If you are accessing memory mapped registers or memory on a PCI
>> device, ie. likely on a 1250, you *must* use the readX/__raw_readX
>> macros. Anybody just doing *reg = val on a PCI device should be
>> banned from writing code for life!

Ladislav> eh? I said nothing about PCI device. These ____raw_writeq
Ladislav> are used in board specific code. Anyway, defining struct
Ladislav> sb_registers and ioremaping it would be nice solution (I
Ladislav> didn't read code too carefully, so maybye not in this
Ladislav> particular case where registers are 64bit width, but I
Ladislav> definitely prefer it in board specific code over
Ladislav> read[bwl]/write[bwl]). Also readq/writeq seems mips
Ladislav> specific, so rants about portability doesn't apply.

Very wrong!

the readX/writeX macro names are for PCI and busses with similar
properties. One should never access anything through readX/writeX
without ioremaping it first.

readq/writeq are not mips specific, they are available on all/most 64
bit architectures, so portability rants do apply.

Jes

Re: Removal of ____raw_readq() and ____raw_writeq() from asm-mips/io.h

[Ralf Baechle]

On Tue, Jan 27, 2004 at 05:58:31PM -0800, Kevin Paul Herbert wrote:

> In edit 1.68, the non-interrupt locking versions of
> raw_readq()/raw_writeq() were removed, in favor of locking ones. While
> this makes sense in general, it breaks the compilation of the sb1250
> which uses the non-locking versions (____raw_readq() and
> ____raw_writeq()) in interrupt handlers.
> 
> Personally, I think that it is very confusing to have so many similar
> macros with similar names and increasing numbers of underscores, so I
> don't really have a problem with this. I've modified
> arch/mips/sibyte/sb1250/time.c and arch/mips/sibyte/sb1250/irq.c to use
> the __ versions and have a few more instructions of overhead.

You actually have no extra overhead - the old versions were broken such
that they were doing the locking thing anyway; this was the primary reason
for the fix.

As for the naming, in general Linux uses a double underscore name prefix
to indicate a more raw, basic version of a function.  This naming
principle applied twice leads to a quad underscore name prefix.  Which
is consequent but ugly.

> My question is whether this removal was intended or not, or whether
> there are some other changes to the handlers in the sb1250-specific code
> that got dropped somewhere.
> 
> If the consensus is that the ____ versions really should perish for the
> sake of simplicity, I'll send my simple patches to the list to fix the
> sb1250 build.

It was removed intensionally under the false assumption the quad-underscore
variants were unused.

  Ralf

Re: Removal of ____raw_readq() and ____raw_writeq() from asm-mips/io.h

[Ralf Baechle]

Below the patch which I've just checked in.

  Ralf

Index: include/asm-mips/io.h
===================================================================
RCS file: /home/cvs/linux/include/asm-mips/io.h,v
retrieving revision 1.68
diff -u -r1.68 io.h
--- include/asm-mips/io.h	19 Jan 2004 21:48:21 -0000	1.68
+++ include/asm-mips/io.h	28 Jan 2004 14:55:57 -0000
@@ -248,12 +248,10 @@
 #define __raw_readw(addr)	(*(volatile unsigned short *)(addr))
 #define __raw_readl(addr)	(*(volatile unsigned int *)(addr))
 #ifdef CONFIG_MIPS32
-#define __raw_readq(addr)						\
+#define ____raw_readq(addr)						\
 ({									\
-	unsigned long __flags;						\
 	u64 __res;							\
 									\
-	local_irq_save(__flags);					\
 	__asm__ __volatile__ (						\
 		"	.set	mips3		# ____raw_readq	\n"	\
 		"	ld	%L0, (%1)			\n"	\
@@ -262,12 +260,22 @@
 		"	.set	mips0				\n"	\
 		: "=r" (__res)						\
 		: "r" (addr));						\
+	__res;								\
+})
+#define __raw_readq(addr)						\
+({									\
+	unsigned long __flags;						\
+	u64 __res;							\
+									\
+	local_irq_save(__flags);					\
+	__res = ____raw_readq(addr);					\
 	local_irq_restore(__flags);					\
 	__res;								\
 })
 #endif
 #ifdef CONFIG_MIPS64
-#define __raw_readq(addr)	(*(volatile unsigned long *)(addr))
+#define ____raw_readq(addr)	(*(volatile unsigned long *)(addr))
+#define __raw_readq(addr)	____raw_readq(addr)
 #endif
 
 #define readb(addr)		__ioswab8(__raw_readb(addr))
@@ -279,12 +287,10 @@
 #define __raw_writew(w,addr)	((*(volatile unsigned short *)(addr)) = (w))
 #define __raw_writel(l,addr)	((*(volatile unsigned int *)(addr)) = (l))
 #ifdef CONFIG_MIPS32
-#define __raw_writeq(val,addr)						\
+#define ____raw_writeq(val,addr)						\
 ({									\
-	unsigned long __flags;						\
 	u64 __tmp;							\
 									\
-	local_irq_save(__flags);					\
 	__asm__ __volatile__ (						\
 		"	.set	mips3				\n"	\
 		"	dsll32	%L0, %L0, 0	# ____raw_writeq\n"	\
@@ -295,11 +301,19 @@
 		"	.set	mips0				\n"	\
 		: "=r" (__tmp)						\
 		: "0" ((unsigned long long)val), "r" (addr));		\
+})
+#define __raw_writeq(val,addr)						\
+({									\
+	unsigned long __flags;						\
+									\
+	local_irq_save(__flags);					\
+	____raw_writeq(val, addr);					\
 	local_irq_restore(__flags);					\
 })
 #endif
 #ifdef CONFIG_MIPS64
-#define __raw_writeq(l,addr)	((*(volatile unsigned long *)(addr)) = (l))
+#define ____raw_writeq(q,addr)	((*(volatile unsigned long *)(addr)) = (q))
+#define __raw_writeq(q,addr)	____raw_writeq(q, addr)
 #endif
 
 #define writeb(b,addr)		__raw_writeb(__ioswab8(b),(addr))