[PATCH] bitops: Use volatile in generic atomic bitops.

[PATCH] bitops: Use volatile in generic atomic bitops.

[Will Newton]

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

The generic atomic bitops currently explicitly cast away the
volatile from the pointer passed to them. This will allow the
access to the bitfield to happen outside of the critical section
thus making the bitops no longer interrupt-safe. Remove this cast
and add a volatile keyword to make sure all accesses to the
bitfield happen inside the critical section.

Signed-off-by: Will Newton <will.newton@imgtec.com>
---
 include/asm-generic/bitops/atomic.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/asm-generic/bitops/atomic.h
b/include/asm-generic/bitops/atomic.h
index ecc44a8..57e4b1f 100644
--- a/include/asm-generic/bitops/atomic.h
+++ b/include/asm-generic/bitops/atomic.h
@@ -65,7 +65,7 @@ extern arch_spinlock_t
__atomic_hash[ATOMIC_HASH_SIZE] __lock_aligned;
 static inline void set_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long flags;

 	_atomic_spin_lock_irqsave(p, flags);
@@ -86,7 +86,7 @@ static inline void set_bit(int nr, volatile unsigned
long *addr)
 static inline void clear_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long flags;

 	_atomic_spin_lock_irqsave(p, flags);
@@ -107,7 +107,7 @@ static inline void clear_bit(int nr, volatile
unsigned long *addr)
 static inline void change_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long flags;

 	_atomic_spin_lock_irqsave(p, flags);
@@ -127,7 +127,7 @@ static inline void change_bit(int nr, volatile
unsigned long *addr)
 static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long old;
 	unsigned long flags;

@@ -151,7 +151,7 @@ static inline int test_and_set_bit(int nr,
volatile unsigned long *addr)
 static inline int test_and_clear_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long old;
 	unsigned long flags;

@@ -174,7 +174,7 @@ static inline int test_and_clear_bit(int nr,
volatile unsigned long *addr)
 static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long old;
 	unsigned long flags;

-- 
1.7.3.4

[-- Attachment #2: 0001-bitops-Use-volatile-in-generic-atomic-bitops.patch --]
[-- Type: text/x-patch, Size: 3162 bytes --]

From 62ff1302350d88fce1f7c30727046aea268fd989 Mon Sep 17 00:00:00 2001
From: Will Newton <will.newton@imgtec.com>
Date: Wed, 27 Jul 2011 11:40:29 +0100
Subject: [PATCH] bitops: Use volatile in generic atomic bitops.

The generic atomic bitops currently explicitly cast away the
volatile from the pointer passed to them. This will allow the
access to the bitfield to happen outside of the critical section
thus making the bitops no longer interrupt-safe. Remove this cast
and add a volatile keyword to make sure all accesses to the
bitfield happen inside the critical section.

Signed-off-by: Will Newton <will.newton@imgtec.com>
---
 include/asm-generic/bitops/atomic.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/asm-generic/bitops/atomic.h b/include/asm-generic/bitops/atomic.h
index ecc44a8..57e4b1f 100644
--- a/include/asm-generic/bitops/atomic.h
+++ b/include/asm-generic/bitops/atomic.h
@@ -65,7 +65,7 @@ extern arch_spinlock_t __atomic_hash[ATOMIC_HASH_SIZE] __lock_aligned;
 static inline void set_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long flags;
 
 	_atomic_spin_lock_irqsave(p, flags);
@@ -86,7 +86,7 @@ static inline void set_bit(int nr, volatile unsigned long *addr)
 static inline void clear_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long flags;
 
 	_atomic_spin_lock_irqsave(p, flags);
@@ -107,7 +107,7 @@ static inline void clear_bit(int nr, volatile unsigned long *addr)
 static inline void change_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long flags;
 
 	_atomic_spin_lock_irqsave(p, flags);
@@ -127,7 +127,7 @@ static inline void change_bit(int nr, volatile unsigned long *addr)
 static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long old;
 	unsigned long flags;
 
@@ -151,7 +151,7 @@ static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
 static inline int test_and_clear_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long old;
 	unsigned long flags;
 
@@ -174,7 +174,7 @@ static inline int test_and_clear_bit(int nr, volatile unsigned long *addr)
 static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	volatile unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long old;
 	unsigned long flags;
 
-- 
1.7.3.4

Re: [PATCH] bitops: Use volatile in generic atomic bitops.

[Arnd Bergmann]

On Wednesday 27 July 2011, Will Newton wrote:
> The generic atomic bitops currently explicitly cast away the
> volatile from the pointer passed to them. This will allow the
> access to the bitfield to happen outside of the critical section
> thus making the bitops no longer interrupt-safe. Remove this cast
> and add a volatile keyword to make sure all accesses to the
> bitfield happen inside the critical section.
> 
> Signed-off-by: Will Newton <will.newton@imgtec.com>

Have you observed this behavior? The interrupt disable/enable should
always come with a barrier that should prevent the bitops from
leaking out, so I don't see how this causes problems in practice.

	Arnd

Re: [PATCH] bitops: Use volatile in generic atomic bitops.

[Will Newton]

On Wed, Jul 27, 2011 at 1:55 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 27 July 2011, Will Newton wrote:
>> The generic atomic bitops currently explicitly cast away the
>> volatile from the pointer passed to them. This will allow the
>> access to the bitfield to happen outside of the critical section
>> thus making the bitops no longer interrupt-safe. Remove this cast
>> and add a volatile keyword to make sure all accesses to the
>> bitfield happen inside the critical section.
>>
>> Signed-off-by: Will Newton <will.newton@imgtec.com>
>
> Have you observed this behavior? The interrupt disable/enable should
> always come with a barrier that should prevent the bitops from
> leaking out, so I don't see how this causes problems in practice.

Yes, although my arch does not have these barriers. Now I see from
memory-barriers.txt that lock/unlock are required to implement a
compiler barrier, sorry for the noise!

Re: [PATCH] bitops: Use volatile in generic atomic bitops.

[Arnd Bergmann]

On Wednesday 27 July 2011, Will Newton wrote:
> >
> > Have you observed this behavior? The interrupt disable/enable should
> > always come with a barrier that should prevent the bitops from
> > leaking out, so I don't see how this causes problems in practice.
> 
> Yes, although my arch does not have these barriers. Now I see from
> memory-barriers.txt that lock/unlock are required to implement a
> compiler barrier, sorry for the noise!

Which architectures is this?

	Arnd

Re: [PATCH] bitops: Use volatile in generic atomic bitops.

[Will Newton]

On Wed, Jul 27, 2011 at 3:28 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 27 July 2011, Will Newton wrote:
>> >
>> > Have you observed this behavior? The interrupt disable/enable should
>> > always come with a barrier that should prevent the bitops from
>> > leaking out, so I don't see how this causes problems in practice.
>>
>> Yes, although my arch does not have these barriers. Now I see from
>> memory-barriers.txt that lock/unlock are required to implement a
>> compiler barrier, sorry for the noise!
>
> Which architectures is this?

Imagination Technologies META, which is a custom in-house
architecture. No immediate plans to upstream unfortunately. :-/

FWIW I checked all the in-tree architectures and it seems everyone
else gets this right (although I got a bit lost with tile).

Re: [PATCH] bitops: Use volatile in generic atomic bitops.

[Arnd Bergmann]

On Wednesday 27 July 2011, Will Newton wrote:
> Imagination Technologies META, which is a custom in-house
> architecture. No immediate plans to upstream unfortunately. :-/

Ah, interesting. We have a few other DSP architectures lined up
for upstream integration. Let me know when you get there as well.
Is there a git tree available somewhere?

> FWIW I checked all the in-tree architectures and it seems everyone
> else gets this right (although I got a bit lost with tile).

Yes, I think that you get into a lot of other problems if you
mess up the basic synchronization primitives, so they would have
noticed at some point.

	Arnd

Re: [PATCH] bitops: Use volatile in generic atomic bitops.

[Will Newton]

On Wed, Jul 27, 2011 at 4:22 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 27 July 2011, Will Newton wrote:
>> Imagination Technologies META, which is a custom in-house
>> architecture. No immediate plans to upstream unfortunately. :-/
>
> Ah, interesting. We have a few other DSP architectures lined up
> for upstream integration. Let me know when you get there as well.
> Is there a git tree available somewhere?

Not at the moment. It's something I would like to have though.

>> FWIW I checked all the in-tree architectures and it seems everyone
>> else gets this right (although I got a bit lost with tile).
>
> Yes, I think that you get into a lot of other problems if you
> mess up the basic synchronization primitives, so they would have
> noticed at some point.

On the contrary, the system was remarkably stable and the issue only
showed up in one particular driver under heavy load. ;-)

Re: [stable] [PATCH] bitops: Use volatile in generic atomic bitops.

[Greg KH]

On Wed, Jul 27, 2011 at 01:21:19PM +0100, Will Newton wrote:
> The generic atomic bitops currently explicitly cast away the
> volatile from the pointer passed to them. This will allow the
> access to the bitfield to happen outside of the critical section
> thus making the bitops no longer interrupt-safe. Remove this cast
> and add a volatile keyword to make sure all accesses to the
> bitfield happen inside the critical section.
> 
> Signed-off-by: Will Newton <will.newton@imgtec.com>
> ---
>  include/asm-generic/bitops/atomic.h |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>