Skip to contentSkip to conversation

remove volatile from x86 cmos_lock

PATCH In review Steven Rostedt 2 participants 4 messages Jul 14, 2006 – Jul 15, 2006 1 message not in the archive

lore
Share & tools
lore

Steven Rostedt

author
#0 lore raw

PATCH 1/1 remove volatile from x86 cmos_lock

Here's another exercise to understand barriers. Once again, please review to see if this is indeed correct.

This patch removes the volatile keyword for cmos_lock and tries to make the code correct using barriers.

-- Steve

Signed-off-by Steven Rostedt <[email protected]>
include/asm-i386/mc146818rtc.h +7 -3
Index: linux-2.6.18-rc1/include/asm-i386/mc146818rtc.h
===================================================================
@@ -31,7 +31,7 @@
  * atomically claim the lock and set the owner.
  */
 #include <linux/smp.h>
-extern volatile unsigned long cmos_lock;
+extern unsigned long cmos_lock;
 
 /*
  * All of these below must be called with interrupts off, preempt

@@ -43,8 +43,10 @@ static inline void lock_cmos(unsigned ch
 	unsigned long new;
 	new = ((smp_processor_id()+1) << 8) | reg;
 	for (;;) {
-		if (cmos_lock)
+		if (cmos_lock) {
+			cpu_relax();
 			continue;
+		}
 		if (__cmpxchg(&cmos_lock, 0, new, sizeof(cmos_lock)) == 0)
 			return;
 	}

@@ -52,14 +54,16 @@ static inline void lock_cmos(unsigned ch
 
 static inline void unlock_cmos(void)
 {
-	cmos_lock = 0;
+	set_wmb(cmos_lock, 0);
 }
 static inline int do_i_have_lock_cmos(void)
 {
+	barrier();
 	return (cmos_lock >> 8) == (smp_processor_id()+1);
 }
 static inline unsigned char current_lock_cmos_reg(void)
 {
+	barrier();
 	return cmos_lock & 0xff;
 }
 #define lock_cmos_prefix(reg) \
arch/i386/kernel/time.c +1 -1
Index: linux-2.6.18-rc1/arch/i386/kernel/time.c
===================================================================
@@ -86,7 +86,7 @@ EXPORT_SYMBOL(rtc_lock);
  * register we are working with.  It is required for NMI access to the
  * CMOS/RTC registers.  See include/asm-i386/mc146818rtc.h for details.
  */
-volatile unsigned long cmos_lock = 0;
+unsigned long cmos_lock = 0;
 EXPORT_SYMBOL(cmos_lock);
 
 /* Routines for accessing the CMOS RAM/RTC. */

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#2 lore raw
Quoted 13 lines · Oliver Neukum

Am Freitag, 14. Juli 2006 16:48 schrieb Steven Rostedt:

include/asm-i386/mc146818rtc.h static inline void lock_cmos(unsigned ch · line 61

Quoted 10 lines
include/asm-i386/mc146818rtc.h +2 -1
@@ -52,14 +54,16 @@ static inline void lock_cmos(unsigned ch
 
 static inline void unlock_cmos(void)
 {
-       cmos_lock = 0;
+       set_wmb(cmos_lock, 0);
 }
 static inline int do_i_have_lock_cmos(void)
 {
+       barrier();

Shouldn't these be rmb() ?

I was thinking that too, but I'm still not sure when to use rmb or barrier. wmb seems pretty straight forward though. hmm, maybe this really should be a smb_rmb since I believe a barrier would be ok for UP.

-- Steve

#3 lore raw
Quoted 15 lines · Steven Rostedt
Quoted 13 lines · Oliver Neukum

Am Freitag, 14. Juli 2006 16:48 schrieb Steven Rostedt:

include/asm-i386/mc146818rtc.h static inline void lock_cmos(unsigned ch · line 61

Quoted 10 lines
include/asm-i386/mc146818rtc.h +2 -1
@@ -52,14 +54,16 @@ static inline void lock_cmos(unsigned ch
 
 static inline void unlock_cmos(void)
 {
-       cmos_lock = 0;
+       set_wmb(cmos_lock, 0);
 }
 static inline int do_i_have_lock_cmos(void)
 {
+       barrier();

Shouldn't these be rmb() ?

Quoted 3 lines

I was thinking that too, but I'm still not sure when to use rmb or barrier. wmb seems pretty straight forward though. hmm, maybe this really should be a smb_rmb since I believe a barrier would be ok for UP.

I'm more puzzled why it's inventing its own locking primitives instead of using one of the many the kernel provides. This stuff is prehistoric though. Hangover from the really early days ?

Dave

Signature · 1 line
http://www.codemonkey.org.uk
author
#4 lore raw
Quoted 20 lines · Dave Jones
Quoted 15 lines · Steven Rostedt
Quoted 13 lines · Oliver Neukum

Am Freitag, 14. Juli 2006 16:48 schrieb Steven Rostedt:

include/asm-i386/mc146818rtc.h static inline void lock_cmos(unsigned ch · line 61

Quoted 10 lines
include/asm-i386/mc146818rtc.h +2 -1
@@ -52,14 +54,16 @@ static inline void lock_cmos(unsigned ch
 
 static inline void unlock_cmos(void)
 {
-       cmos_lock = 0;
+       set_wmb(cmos_lock, 0);
 }
 static inline int do_i_have_lock_cmos(void)
 {
+       barrier();

Shouldn't these be rmb() ?

Quoted 3 lines

I was thinking that too, but I'm still not sure when to use rmb or barrier. wmb seems pretty straight forward though. hmm, maybe this really should be a smb_rmb since I believe a barrier would be ok for UP.

Quoted 6 lines

I'm more puzzled why it's inventing its own locking primitives instead of using one of the many the kernel provides. This stuff is prehistoric though. Hangover from the really early days ?

Dave

Here's your answer:

from rtc.hinclude/asm-i386/mc146818rtc.h:

/*
* This lock provides nmi access to the CMOS/RTC registers. It has some
* special properties. It is owned by a CPU and stores the index register
* currently being accessed (if owned). The idea here is that it works
* like a normal lock (normally). However, in an NMI, the NMI code will
* first check to see if its CPU owns the lock, meaning that the NMI
* interrupted during the read/write of the device. If it does, it goes ahead
* and performs the access and then restores the index register. If it does
* not, it locks normally.
*
* Note that since we are working with NMIs, we need this lock even in
* a non-SMP machine just to mark that the lock is owned.
*
* This only works with compare-and-swap. There is no other way to
* atomically claim the lock and set the owner.
*/

So it is dealing with NMIs. Since we are dealing with a lock that can be taken from NMI context, we can't just spin if it is locked. We need to check if the CPU that owns the lock is the same as the NMI CPU and if so just go on and run, otherwise you deadlock.

So really the only thing missing here are the memory barriers since it is not really protecting anything due to CPU reordering.

(I need to get rid of that set_wmb :P )

-- Steve