Thread (26 messages) 26 messages, 16 authors, 2007-10-27

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

From: Alexey Dobriyan <hidden>
Date: 2007-10-27 20:14:49
Also in: lkml

On Sat, Oct 27, 2007 at 09:20:43PM +0200, Roman Zippel wrote:
On Sun, 21 Oct 2007, Alexey Dobriyan wrote:
quoted
So far remedies were:
a) grep(1) -- obviously fragile. I tried at some point grepping for
   spin_lock_irqsave(), found quite a few, but it became booooring quickly.
b) BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)) -- was tried,
   brutally broke some arches, survived one commit before revert :^)
   Doesn't work on i386 where sizeof(unsigned int) == sizeof(unsigned long).

So it would be nice to have something more robust.
If it's just about the type checking, something like below should pretty 
much do the same.
It won't catch, the following if both variables are unsigned long:

	spin_lock_irqsave(&lock, flags);
		[stuff]
	spin_unlock_irqrestore(&lock, foo->flags);

It won't catch "static unsigned long flags;". With sparse, we can
eventually mark type as "on-stack only".
quoted
* irq_flags_t allows arch maintainers to eventually switch to something
  smaller than "unsigned long" if they want to.
Considering how painful this conversion could be, the question is whether 
this is really needed.
In the long run, I believe, this is needed. We want more bugs to be
found automatically, so we can have more time for non-trivial bugs.
Comments so far suggest some archs don't need all 
of it, but does someone need more?
I don't know.
quoted hunk ↗ jump to hunk
--- linux-2.6.orig/include/linux/irqflags.h
+++ linux-2.6/include/linux/irqflags.h
@@ -41,6 +41,10 @@
 # define INIT_TRACE_IRQFLAGS
 #endif
 
+static __always_inline void __irq_flags_check(unsigned long *flags)
+{
+}
+
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
 
 #include <asm/irqflags.h>
@@ -50,10 +54,11 @@
 #define local_irq_disable() \
 	do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0)
 #define local_irq_save(flags) \
-	do { raw_local_irq_save(flags); trace_hardirqs_off(); } while (0)
+	do { __irq_flags_check(&flags); raw_local_irq_save(flags); trace_hardirqs_off(); } while (0)
 
 #define local_irq_restore(flags)				\
 	do {							\
+		__irq_flags_check(&flags);			\
 		if (raw_irqs_disabled_flags(flags)) {		\
 			raw_local_irq_restore(flags);		\
 			trace_hardirqs_off();			\
@@ -69,8 +74,8 @@
  */
 # define raw_local_irq_disable()	local_irq_disable()
 # define raw_local_irq_enable()		local_irq_enable()
-# define raw_local_irq_save(flags)	local_irq_save(flags)
-# define raw_local_irq_restore(flags)	local_irq_restore(flags)
+# define raw_local_irq_save(flags)	({ __irq_flags_check(&flags); local_irq_save(flags); })
+# define raw_local_irq_restore(flags)	({ __irq_flags_check(&flags); local_irq_restore(flags); })
 #endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
 
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help