x86: SGU UV Add volatile to macros that access chipset registers

PATCH Superseded Jack Steiner 4 participants 14 messages Sep 9, 2009 – Sep 18, 2009

Subsystems (3)

Memory management x86 x86 platform uv hpe superdome flex

lore

Share & tools

lore

Jack Steiner

author

2009-09-09 15:42 UTC #0 lore raw

PATCH 1/1 x86: SGU UV Add volatile to macros that access chipset registers

Add "volatile" to the SGI UV read/write macros that are used to access chipset memory mapped registers.

Signed-off-by Jack Steiner <[email protected]>

---
 arch/x86/include/asm/uv/uv_hub.h |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

arch/x86/include/asm/uv/uv_hub.h +5 -5

Index: linux/arch/x86/include/asm/uv/uv_hub.h
===================================================================
@@ -248,7 +248,7 @@ static inline int uv_apicid_to_pnode(int
  * Access global MMRs using the low memory MMR32 space. This region supports
  * faster MMR access but not all MMRs are accessible in this space.
  */
-static inline unsigned long *uv_global_mmr32_address(int pnode,
+static inline volatile unsigned long *uv_global_mmr32_address(int pnode,
 				unsigned long offset)
 {
 	return __va(UV_GLOBAL_MMR32_BASE |

@@ -271,7 +271,7 @@ static inline unsigned long uv_read_glob
  * Access Global MMR space using the MMR space located at the top of physical
  * memory.
  */
-static inline unsigned long *uv_global_mmr64_address(int pnode,
+static inline volatile unsigned long *uv_global_mmr64_address(int pnode,
 				unsigned long offset)
 {
 	return __va(UV_GLOBAL_MMR64_BASE |

@@ -294,7 +294,7 @@ static inline unsigned long uv_read_glob
  * Access hub local MMRs. Faster than using global space but only local MMRs
  * are accessible.
  */
-static inline unsigned long *uv_local_mmr_address(unsigned long offset)
+static inline volatile unsigned long *uv_local_mmr_address(unsigned long offset)
 {
 	return __va(UV_LOCAL_MMR_BASE | offset);
 }

@@ -311,12 +311,12 @@ static inline void uv_write_local_mmr(un
 
 static inline unsigned char uv_read_local_mmr8(unsigned long offset)
 {
-	return *((unsigned char *)uv_local_mmr_address(offset));
+	return *((volatile unsigned char *)uv_local_mmr_address(offset));
 }
 
 static inline void uv_write_local_mmr8(unsigned long offset, unsigned char val)
 {
-	*((unsigned char *)uv_local_mmr_address(offset)) = val;
+	*((volatile unsigned char *)uv_local_mmr_address(offset)) = val;
 }
 
 /*

Daniel Walker

2009-09-09 16:09 UTC #1 lore raw

Quoted 2 lines · Jack Steiner

Add "volatile" to the SGI UV read/write macros that are used to access chipset memory mapped registers.

There is a considerable document regarding the usage of volatile in the kernel (Documentation/volatile-considered-harmful.txt). Considering that document, can you give a more descriptive reason why your using "volatile" here ?

Daniel

Jack Steiner

author

2009-09-09 18:01 UTC #2 lore raw

Quoted 9 lines · Daniel Walker

Quoted 2 lines · Jack Steiner
Add "volatile" to the SGI UV read/write macros that are used to access chipset memory mapped registers.
There is a considerable document regarding the usage of volatile in the kernel (Documentation/volatile-considered-harmful.txt). Considering that document, can you give a more descriptive reason why your using "volatile" here ?

I knew that "volatile" would catch someone's attention :-)

Volatile is being added to the accessor functions that are used to read/write memory-mapped I/O registers located within the UV chipset. The use of volatile is hidden within the functions and is not exposed to the users of the functions.

Note that the use is limited to the accessor functions in the header file. No .c files are changed or need to know about volatile.

This seems to be consistent with other uses of volatile within the kernel.

--- jack

Daniel Walker

2009-09-09 18:11 UTC #3 lore raw

Quoted 19 lines · Jack Steiner

Quoted 9 lines · Daniel Walker
Quoted 2 lines · Jack Steiner
Add "volatile" to the SGI UV read/write macros that are used to access chipset memory mapped registers.
There is a considerable document regarding the usage of volatile in the kernel (Documentation/volatile-considered-harmful.txt). Considering that document, can you give a more descriptive reason why your using "volatile" here ?
I knew that "volatile" would catch someone's attention :-)
Volatile is being added to the accessor functions that are used to read/write memory-mapped I/O registers located within the UV chipset. The use of volatile is hidden within the functions and is not exposed to the users of the functions.

Quoted 5 lines

Note that the use is limited to the accessor functions in the header file. No .c files are changed or need to know about volatile.
This seems to be consistent with other uses of volatile within the kernel.

The document that I cited specifically addresses memory accessors as not needing the volatile keyword .. So your still not addressing exactly why your code needs it .. Are your accessors special in some way? Is there some defect your seeing without the volatile keyword?

Daniel

Chris Friesen

2009-09-09 18:55 UTC #4 lore raw

Quoted 12 lines · Daniel Walker

Quoted 10 lines · Jack Steiner
Volatile is being added to the accessor functions that are used to read/write memory-mapped I/O registers located within the UV chipset. The use of volatile is hidden within the functions and is not exposed to the users of the functions.
Note that the use is limited to the accessor functions in the header file. No .c files are changed or need to know about volatile.
This seems to be consistent with other uses of volatile within the kernel.

Quoted 7 lines

The document that I cited specifically addresses memory accessors as not needing the volatile keyword .. So your still not addressing exactly why your code needs it .. Are your accessors special in some way? Is there some defect your seeing without the volatile keyword?
From that document:

"There are still a few rare situations where volatile makes sense in the kernel:

- The above-mentioned accessor functions might use volatile on architectures where direct I/O memory access does work. Essentially, each accessor call becomes a little critical section on its own and ensures that the access happens as expected by the programmer."

However, the fact that these functions are returning volatile pointers is a bit sketchy. Normally accesses to such memory would be wrapped entirely in a read/write function which would handle the volatility internally to the function.

In this case there's no point in the function returning a "volatile unsigned long *" if it's going to be cast to a volatile pointer before being dereferenced.

Chris

Jack Steiner

author

2009-09-09 19:38 UTC #5 lore raw

Quoted 31 lines · Chris Friesen

Quoted 12 lines · Daniel Walker
Quoted 10 lines · Jack Steiner
Volatile is being added to the accessor functions that are used to read/write memory-mapped I/O registers located within the UV chipset. The use of volatile is hidden within the functions and is not exposed to the users of the functions.
Note that the use is limited to the accessor functions in the header file. No .c files are changed or need to know about volatile.
This seems to be consistent with other uses of volatile within the kernel.
Quoted 7 lines
The document that I cited specifically addresses memory accessors as not needing the volatile keyword .. So your still not addressing exactly why your code needs it .. Are your accessors special in some way? Is there some defect your seeing without the volatile keyword?
From that document:
"There are still a few rare situations where volatile makes sense in the kernel:
- The above-mentioned accessor functions might use volatile on architectures where direct I/O memory access does work. Essentially, each accessor call becomes a little critical section on its own and ensures that the access happens as expected by the programmer."

Quoted 8 lines

However, the fact that these functions are returning volatile pointers is a bit sketchy. Normally accesses to such memory would be wrapped entirely in a read/write function which would handle the volatility internally to the function.
In this case there's no point in the function returning a "volatile unsigned long *" if it's going to be cast to a volatile pointer before being dereferenced.

make sense.

How is this..... Avoids adding volatile to any return types.
--------------------------------------------------------

Add "volatile" to the SGI UV read/write macros that are used to access chipset memory mapped registers.

Signed-off-by Jack Steiner <[email protected]>

---
 arch/x86/include/asm/uv/uv_hub.h |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

arch/x86/include/asm/uv/uv_hub.h +8 -8

Index: linux/arch/x86/include/asm/uv/uv_hub.h
===================================================================
@@ -258,13 +258,13 @@ static inline unsigned long *uv_global_m
 static inline void uv_write_global_mmr32(int pnode, unsigned long offset,
 				 unsigned long val)
 {
-	*uv_global_mmr32_address(pnode, offset) = val;
+	*(volatile unsigned long *)uv_global_mmr32_address(pnode, offset) = val;
 }
 
 static inline unsigned long uv_read_global_mmr32(int pnode,
 						 unsigned long offset)
 {
-	return *uv_global_mmr32_address(pnode, offset);
+	return *(volatile unsigned long *)uv_global_mmr32_address(pnode, offset);
 }
 
 /*

@@ -281,13 +281,13 @@ static inline unsigned long *uv_global_m
 static inline void uv_write_global_mmr64(int pnode, unsigned long offset,
 				unsigned long val)
 {
-	*uv_global_mmr64_address(pnode, offset) = val;
+	*(volatile unsigned long *)uv_global_mmr64_address(pnode, offset) = val;
 }
 
 static inline unsigned long uv_read_global_mmr64(int pnode,
 						 unsigned long offset)
 {
-	return *uv_global_mmr64_address(pnode, offset);
+	return *(volatile unsigned long *)uv_global_mmr64_address(pnode, offset);
 }
 
 /*

@@ -301,22 +301,22 @@ static inline unsigned long *uv_local_mm
 
 static inline unsigned long uv_read_local_mmr(unsigned long offset)
 {
-	return *uv_local_mmr_address(offset);
+	return *(volatile unsigned char *)uv_local_mmr_address(offset);
 }
 
 static inline void uv_write_local_mmr(unsigned long offset, unsigned long val)
 {
-	*uv_local_mmr_address(offset) = val;
+	*(volatile unsigned char *)uv_local_mmr_address(offset) = val;
 }
 
 static inline unsigned char uv_read_local_mmr8(unsigned long offset)
 {
-	return *((unsigned char *)uv_local_mmr_address(offset));
+	return *((volatile unsigned char *)uv_local_mmr_address(offset));
 }
 
 static inline void uv_write_local_mmr8(unsigned long offset, unsigned char val)
 {
-	*((unsigned char *)uv_local_mmr_address(offset)) = val;
+	*((volatile unsigned char *)uv_local_mmr_address(offset)) = val;
 }
 
 /*

H. Peter Anvin

reviewer

2009-09-10 00:48 UTC #6 lore raw

arch/x86/include/asm/uv/uv_hub.h static inline unsigned long *uv_local_mm · line 320

Quoted 7 lines · Jack Steiner

static inline void uv_write_local_mmr8(unsigned long offset, unsigned char val) {
arch/x86/include/asm/uv/uv_hub.h +1 -1
-	*((unsigned char *)uv_local_mmr_address(offset)) = val;
+	*((volatile unsigned char *)uv_local_mmr_address(offset)) = val;
 }

Why aren't you simply using __writeb() here, and the other memory accessors we already have in the other places?

-hpa

Jack Steiner

author

2009-09-10 02:21 UTC #7 lore raw

Quoted 11 lines · H. Peter Anvin

arch/x86/include/asm/uv/uv_hub.h static inline unsigned long *uv_local_mm · line 320
Quoted 7 lines · Jack Steiner
static inline void uv_write_local_mmr8(unsigned long offset, unsigned char val) {
arch/x86/include/asm/uv/uv_hub.h +1 -1
-	*((unsigned char *)uv_local_mmr_address(offset)) = val;
+	*((volatile unsigned char *)uv_local_mmr_address(offset)) = val;
 }
Why aren't you simply using __writeb() here, and the other memory accessors we already have in the other places?

Excellent idea. New patch to follow...

--- jack

Jack Steiner

author

2009-09-10 02:22 UTC #8 lore raw

PATCH v2 1/1 x86: SGU UV Add volatile semantics to macros that access chipset registers

In reply to H. Peter Anvin

Add volatile-semantics to the SGI UV read/write macros that are used to access chipset memory mapped registers. No direct references to volatile are made. Instead the readq/writeq macros are used.

Signed-off-by Jack Steiner <[email protected]>

---
 arch/x86/include/asm/uv/uv_hub.h |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

arch/x86/include/asm/uv/uv_hub.h +9 -8

Index: linux/arch/x86/include/asm/uv/uv_hub.h
===================================================================
@@ -15,6 +15,7 @@
 #include <linux/numa.h>
 #include <linux/percpu.h>
 #include <linux/timer.h>
+#include <linux/io.h>
 #include <asm/types.h>
 #include <asm/percpu.h>
 #include <asm/uv/uv_mmrs.h>

@@ -258,13 +259,13 @@ static inline unsigned long *uv_global_m
 static inline void uv_write_global_mmr32(int pnode, unsigned long offset,
 				 unsigned long val)
 {
-	*uv_global_mmr32_address(pnode, offset) = val;
+	writeq(val, uv_global_mmr32_address(pnode, offset));
 }
 
 static inline unsigned long uv_read_global_mmr32(int pnode,
 						 unsigned long offset)
 {
-	return *uv_global_mmr32_address(pnode, offset);
+	return readq(uv_global_mmr32_address(pnode, offset));
 }
 
 /*

@@ -281,13 +282,13 @@ static inline unsigned long *uv_global_m
 static inline void uv_write_global_mmr64(int pnode, unsigned long offset,
 				unsigned long val)
 {
-	*uv_global_mmr64_address(pnode, offset) = val;
+	writeq(val, uv_global_mmr64_address(pnode, offset));
 }
 
 static inline unsigned long uv_read_global_mmr64(int pnode,
 						 unsigned long offset)
 {
-	return *uv_global_mmr64_address(pnode, offset);
+	return readq(uv_global_mmr64_address(pnode, offset));
 }
 
 /*

@@ -301,22 +302,22 @@ static inline unsigned long *uv_local_mm
 
 static inline unsigned long uv_read_local_mmr(unsigned long offset)
 {
-	return *uv_local_mmr_address(offset);
+	return readq(uv_local_mmr_address(offset));
 }
 
 static inline void uv_write_local_mmr(unsigned long offset, unsigned long val)
 {
-	*uv_local_mmr_address(offset) = val;
+	writeq(val, uv_local_mmr_address(offset));
 }
 
 static inline unsigned char uv_read_local_mmr8(unsigned long offset)
 {
-	return *((unsigned char *)uv_local_mmr_address(offset));
+	return readq(uv_local_mmr_address(offset));
 }
 
 static inline void uv_write_local_mmr8(unsigned long offset, unsigned char val)
 {
-	*((unsigned char *)uv_local_mmr_address(offset)) = val;
+	writeq(val, uv_local_mmr_address(offset));
 }
 
 /*

H. Peter Anvin

reviewer

2009-09-10 03:06 UTC #9 lore raw

Re: [PATCH V2] x86: SGU UV Add volatile semantics to macros that access chipset registers

Quoted 6 lines · Jack Steiner

Add volatile-semantics to the SGI UV read/write macros that are used to access chipset memory mapped registers. No direct references to volatile are made. Instead the readq/writeq macros are used.
Signed-off-by Jack Steiner <[email protected]>

The -q part of readq/writeq is qword, 64 bits. It looks like you're replacing references of other sizes with qword references; was that intended?

-hpa

Signature · 2 lines

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Jack Steiner

author

2009-09-10 03:23 UTC #10 lore raw

Re: [PATCH V2] x86: SGU UV Add volatile semantics to macros that access chipset registers

Quoted 11 lines · H. Peter Anvin

Quoted 6 lines · Jack Steiner
Add volatile-semantics to the SGI UV read/write macros that are used to access chipset memory mapped registers. No direct references to volatile are made. Instead the readq/writeq macros are used.
Signed-off-by Jack Steiner <[email protected]>
The -q part of readq/writeq is qword, 64 bits. It looks like you're replacing references of other sizes with qword references; was that intended?

No, it was not. Most macros are quad-word but I see one that should have been "char".

Thanks for catching that. New patch in the morning.

--- jack

Jack Steiner

author

2009-09-10 14:31 UTC #11 lore raw

PATCH v3 1/1 x86: SGU UV Add volatile semantics to macros that access chipset registers

In reply to H. Peter Anvin

Add volatile-semantics to the SGI UV read/write macros that are used to access chipset memory mapped registers. No direct references to volatile are made. Instead the readq/writeq macros are used.

Signed-off-by Jack Steiner <[email protected]>

---
 arch/x86/include/asm/uv/uv_hub.h |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

arch/x86/include/asm/uv/uv_hub.h +9 -8

Index: linux/arch/x86/include/asm/uv/uv_hub.h
===================================================================
@@ -15,6 +15,7 @@
 #include <linux/numa.h>
 #include <linux/percpu.h>
 #include <linux/timer.h>
+#include <linux/io.h>
 #include <asm/types.h>
 #include <asm/percpu.h>
 #include <asm/uv/uv_mmrs.h>

@@ -258,13 +259,13 @@ static inline unsigned long *uv_global_m
 static inline void uv_write_global_mmr32(int pnode, unsigned long offset,
 				 unsigned long val)
 {
-	*uv_global_mmr32_address(pnode, offset) = val;
+	writeq(val, uv_global_mmr32_address(pnode, offset));
 }
 
 static inline unsigned long uv_read_global_mmr32(int pnode,
 						 unsigned long offset)
 {
-	return *uv_global_mmr32_address(pnode, offset);
+	return readq(uv_global_mmr32_address(pnode, offset));
 }
 
 /*

@@ -281,13 +282,13 @@ static inline unsigned long *uv_global_m
 static inline void uv_write_global_mmr64(int pnode, unsigned long offset,
 				unsigned long val)
 {
-	*uv_global_mmr64_address(pnode, offset) = val;
+	writeq(val, uv_global_mmr64_address(pnode, offset));
 }
 
 static inline unsigned long uv_read_global_mmr64(int pnode,
 						 unsigned long offset)
 {
-	return *uv_global_mmr64_address(pnode, offset);
+	return readq(uv_global_mmr64_address(pnode, offset));
 }
 
 /*

@@ -301,22 +302,22 @@ static inline unsigned long *uv_local_mm
 
 static inline unsigned long uv_read_local_mmr(unsigned long offset)
 {
-	return *uv_local_mmr_address(offset);
+	return readq(uv_local_mmr_address(offset));
 }
 
 static inline void uv_write_local_mmr(unsigned long offset, unsigned long val)
 {
-	*uv_local_mmr_address(offset) = val;
+	writeq(val, uv_local_mmr_address(offset));
 }
 
 static inline unsigned char uv_read_local_mmr8(unsigned long offset)
 {
-	return *((unsigned char *)uv_local_mmr_address(offset));
+	return readb(uv_local_mmr_address(offset));
 }
 
 static inline void uv_write_local_mmr8(unsigned long offset, unsigned char val)
 {
-	*((unsigned char *)uv_local_mmr_address(offset)) = val;
+	writeb(val, uv_local_mmr_address(offset));
 }
 
 /*

tip-bot for Jack Steiner

author

2009-09-18 12:07 UTC #12 lore raw

[tip:x86/urgent] x86: SGI UV: Add volatile semantics to macros that access chipset registers

Commit-ID:  8dc579e868addafd24c0a015c12f0e536b1084b1
Gitweb:     http://git.kernel.org/tip/8dc579e868addafd24c0a015c12f0e536b1084b1
Author:     Jack Steiner <[email protected]>
AuthorDate: Thu, 10 Sep 2009 09:31:49 -0500
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 18 Sep 2009 14:05:32 +0200

x86: SGI UV: Add volatile semantics to macros that access chipset registers

Add volatile-semantics to the SGI UV read/write macros that are used to access chipset memory mapped registers. No direct references to volatile are made. Instead the readq/writeq macros are used.

Signed-off-by Jack Steiner <[email protected]>

Cc [email protected]

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/include/asm/uv/uv_hub.h |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

arch/x86/include/asm/uv/uv_hub.h +9 -8

index 03a0cbd..04eb6c9 100644
@@ -15,6 +15,7 @@
 #include <linux/numa.h>
 #include <linux/percpu.h>
 #include <linux/timer.h>
+#include <linux/io.h>
 #include <asm/types.h>
 #include <asm/percpu.h>
 #include <asm/uv/uv_mmrs.h>

@@ -258,13 +259,13 @@ static inline unsigned long *uv_global_mmr32_address(int pnode,
 static inline void uv_write_global_mmr32(int pnode, unsigned long offset,
 				 unsigned long val)
 {
-	*uv_global_mmr32_address(pnode, offset) = val;
+	writeq(val, uv_global_mmr32_address(pnode, offset));
 }
 
 static inline unsigned long uv_read_global_mmr32(int pnode,
 						 unsigned long offset)
 {
-	return *uv_global_mmr32_address(pnode, offset);
+	return readq(uv_global_mmr32_address(pnode, offset));
 }
 
 /*

@@ -281,13 +282,13 @@ static inline unsigned long *uv_global_mmr64_address(int pnode,
 static inline void uv_write_global_mmr64(int pnode, unsigned long offset,
 				unsigned long val)
 {
-	*uv_global_mmr64_address(pnode, offset) = val;
+	writeq(val, uv_global_mmr64_address(pnode, offset));
 }
 
 static inline unsigned long uv_read_global_mmr64(int pnode,
 						 unsigned long offset)
 {
-	return *uv_global_mmr64_address(pnode, offset);
+	return readq(uv_global_mmr64_address(pnode, offset));
 }
 
 /*

@@ -301,22 +302,22 @@ static inline unsigned long *uv_local_mmr_address(unsigned long offset)
 
 static inline unsigned long uv_read_local_mmr(unsigned long offset)
 {
-	return *uv_local_mmr_address(offset);
+	return readq(uv_local_mmr_address(offset));
 }
 
 static inline void uv_write_local_mmr(unsigned long offset, unsigned long val)
 {
-	*uv_local_mmr_address(offset) = val;
+	writeq(val, uv_local_mmr_address(offset));
 }
 
 static inline unsigned char uv_read_local_mmr8(unsigned long offset)
 {
-	return *((unsigned char *)uv_local_mmr_address(offset));
+	return readb(uv_local_mmr_address(offset));
 }
 
 static inline void uv_write_local_mmr8(unsigned long offset, unsigned char val)
 {
-	*((unsigned char *)uv_local_mmr_address(offset)) = val;
+	writeb(val, uv_local_mmr_address(offset));
 }
 
 /*

Jack Steiner

author

2009-09-09 19:20 UTC #13 lore raw

In reply to Daniel Walker

Quoted 24 lines · Daniel Walker

Quoted 23 lines · Jack Steiner
Quoted 9 lines · Daniel Walker
Quoted 2 lines · Jack Steiner
Add "volatile" to the SGI UV read/write macros that are used to access chipset memory mapped registers.
There is a considerable document regarding the usage of volatile in the kernel (Documentation/volatile-considered-harmful.txt). Considering that document, can you give a more descriptive reason why your using "volatile" here ?
I knew that "volatile" would catch someone's attention :-)
Volatile is being added to the accessor functions that are used to read/write memory-mapped I/O registers located within the UV chipset. The use of volatile is hidden within the functions and is not exposed to the users of the functions.
Note that the use is limited to the accessor functions in the header file. No .c files are changed or need to know about volatile.

Quoted 6 lines

Quoted 1 line
This seems to be consistent with other uses of volatile within the kernel.
The document that I cited specifically addresses memory accessors as not needing the volatile keyword .. So your still not addressing exactly why your code needs it .. Are your accessors special in some way? Is there some defect your seeing without the volatile keyword?

The code is not yet in the kernel but the function that exposed the bug was one that had multiple access to the chipset RTC clock. This is a free running clock that counts at nsec rate. The compiler optimized the function so that a single read of the clock was done. This broken the timing that was trying to measure elapsed time.

I scanned the standard header files and see numerous instances where inline functions use volatile. The usage of volatile in the UV macros (at least to me) seems consistent:

	mmio_config_xxx()
	native_apic_mem_xxx()
	readb(), etc.
	bit_ops functions
	etc...

What am I missing?