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(-)
Index: linux/arch/x86/include/asm/uv/uv_hub.h
===================================================================
--- linux.orig/arch/x86/include/asm/uv/uv_hub.h 2009-08-10 01:45:42.000000000 -0500
+++ linux/arch/x86/include/asm/uv/uv_hub.h 2009-08-10 11:43:39.000000000 -0500
@@ -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;
}
/*
On Wed, 2009-09-09 at 10:42 -0500, Jack Steiner wrote:
> 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
On Wed, Sep 09, 2009 at 09:10:00AM -0700, Daniel Walker wrote:
> On Wed, 2009-09-09 at 10:42 -0500, Jack Steiner wrote:
> > 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
On Wed, 2009-09-09 at 13:01 -0500, Jack Steiner wrote:
> On Wed, Sep 09, 2009 at 09:10:00AM -0700, Daniel Walker wrote:
> > On Wed, 2009-09-09 at 10:42 -0500, Jack Steiner wrote:
> > > 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.
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
On 09/09/2009 12:11 PM, Daniel Walker wrote:
> On Wed, 2009-09-09 at 13:01 -0500, Jack Steiner wrote:
>> 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.
>
> 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
On Wed, Sep 09, 2009 at 11:11:25AM -0700, Daniel Walker wrote:
> On Wed, 2009-09-09 at 13:01 -0500, Jack Steiner wrote:
> > On Wed, Sep 09, 2009 at 09:10:00AM -0700, Daniel Walker wrote:
> > > On Wed, 2009-09-09 at 10:42 -0500, Jack Steiner wrote:
> > > > 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.
>
> 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?
On Wed, Sep 09, 2009 at 12:54:29PM -0600, Chris Friesen wrote:
> On 09/09/2009 12:11 PM, Daniel Walker wrote:
> > On Wed, 2009-09-09 at 13:01 -0500, Jack Steiner wrote:
> >> 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.
> >
> > 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.
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(-)
Index: linux/arch/x86/include/asm/uv/uv_hub.h
===================================================================
--- linux.orig/arch/x86/include/asm/uv/uv_hub.h 2009-09-09 01:34:02.000000000 -0500
+++ linux/arch/x86/include/asm/uv/uv_hub.h 2009-09-09 14:36:23.000000000 -0500
@@ -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;
}
/*
On 09/09/2009 12:38 PM, Jack Steiner wrote:
>
> 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;
> }
>
Why aren't you simply using __writeb() here, and the other memory
accessors we already have in the other places?
-hpa
On Wed, Sep 09, 2009 at 05:44:23PM -0700, H. Peter Anvin wrote:
> On 09/09/2009 12:38 PM, Jack Steiner wrote:
> >
> > 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;
> > }
> >
>
> 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
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(-)
Index: linux/arch/x86/include/asm/uv/uv_hub.h
===================================================================
--- linux.orig/arch/x86/include/asm/uv/uv_hub.h 2009-09-09 01:34:02.000000000 -0500
+++ linux/arch/x86/include/asm/uv/uv_hub.h 2009-09-09 20:51:53.000000000 -0500
@@ -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));
}
/*
On 09/09/2009 07:22 PM, Jack Steiner wrote:
> 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
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On Wed, Sep 09, 2009 at 08:05:30PM -0700, H. Peter Anvin wrote:
> On 09/09/2009 07:22 PM, Jack Steiner wrote:
> > 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
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(-)
Index: linux/arch/x86/include/asm/uv/uv_hub.h
===================================================================
--- linux.orig/arch/x86/include/asm/uv/uv_hub.h 2009-09-09 01:34:02.000000000 -0500
+++ linux/arch/x86/include/asm/uv/uv_hub.h 2009-09-09 20:51:53.000000000 -0500
@@ -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));
}
/*
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]
Cc: [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(-)
diff --git a/arch/x86/include/asm/uv/uv_hub.h b/arch/x86/include/asm/uv/uv_hub.h
index 03a0cbd..04eb6c9 100644
--- a/arch/x86/include/asm/uv/uv_hub.h
+++ b/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_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));
}
/*