[PATCH] fix flags length in net 9p

[PATCH] fix flags length in net 9p

[Steven Rostedt]

Some files in the net/9p directory uses "int" for flags. This can
cause hard to find bugs on some architectures. This patch converts the
flags to use "long" instead.

This bug was discovered by doing an allyesconfig make on the -rt kernel
where checks are done to ensure all flags are of size sizeof(long).

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 net/9p/trans_virtio.c |    2 +-
 net/9p/util.c         |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Index: linux-compile.git/net/9p/trans_virtio.c
===================================================================
--- linux-compile.git.orig/net/9p/trans_virtio.c	2008-04-09 18:45:15.000000000 -0400
+++ linux-compile.git/net/9p/trans_virtio.c	2008-05-01 17:02:25.000000000 -0400
@@ -134,7 +134,7 @@ static void p9_virtio_close(struct p9_tr
 {
 	struct virtio_chan *chan = trans->priv;
 	int count;
-	unsigned int flags;
+	unsigned long flags;

 	spin_lock_irqsave(&chan->lock, flags);
 	p9_idpool_destroy(chan->tagpool);
Index: linux-compile.git/net/9p/util.c
===================================================================
--- linux-compile.git.orig/net/9p/util.c	2008-02-14 23:09:39.000000000 -0500
+++ linux-compile.git/net/9p/util.c	2008-05-01 17:02:35.000000000 -0400
@@ -71,7 +71,7 @@ int p9_idpool_get(struct p9_idpool *p)
 {
 	int i = 0;
 	int error;
-	unsigned int flags;
+	unsigned long flags;

 retry:
 	if (idr_pre_get(&p->pool, GFP_KERNEL) == 0)
@@ -102,7 +102,7 @@ EXPORT_SYMBOL(p9_idpool_get);

 void p9_idpool_put(int id, struct p9_idpool *p)
 {
-	unsigned int flags;
+	unsigned long flags;
 	spin_lock_irqsave(&p->lock, flags);
 	idr_remove(&p->pool, id);
 	spin_unlock_irqrestore(&p->lock, flags);

Re: [PATCH] fix flags length in net 9p

[Andrew Morton]

On Thu, 1 May 2008 17:08:05 -0400 (EDT)
Steven Rostedt <rostedt@goodmis.org> wrote:

> Some files in the net/9p directory uses "int" for flags. This can
> cause hard to find bugs on some architectures. This patch converts the
> flags to use "long" instead.

gargh.

> This bug was discovered by doing an allyesconfig make on the -rt kernel
> where checks are done to ensure all flags are of size sizeof(long).

I was about to suggest that we do something like that...

I wonder how messy it is.  I long ago lost the ability to follow the
convolutions in include/linux/spinlock*.h :(  gotta patch?

Re: [PATCH] fix flags length in net 9p

[Steven Rostedt]

[ I added Ingo since I think he created the check ]

On Thu, 1 May 2008, Andrew Morton wrote:

> On Thu, 1 May 2008 17:08:05 -0400 (EDT)
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > Some files in the net/9p directory uses "int" for flags. This can
> > cause hard to find bugs on some architectures. This patch converts the
> > flags to use "long" instead.
>
> gargh.
>
> > This bug was discovered by doing an allyesconfig make on the -rt kernel
> > where checks are done to ensure all flags are of size sizeof(long).
>
> I was about to suggest that we do something like that...
>
> I wonder how messy it is.  I long ago lost the ability to follow the
> convolutions in include/linux/spinlock*.h :(  gotta patch?

It's not messy at all, it's actually quite elegant. I can pull it out of
the -rt tree, or perhaps Ingo can quickly whip one up as well.

Ingo, you want me to pull it out?

-- Steve

Re: [PATCH] fix flags length in net 9p

[Daniel Walker]

On Thu, 2008-05-01 at 14:19 -0700, Andrew Morton wrote:
> On Thu, 1 May 2008 17:08:05 -0400 (EDT)
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Some files in the net/9p directory uses "int" for flags. This can
> > cause hard to find bugs on some architectures. This patch converts the
> > flags to use "long" instead.
> 
> gargh.
> 
> > This bug was discovered by doing an allyesconfig make on the -rt kernel
> > where checks are done to ensure all flags are of size sizeof(long).
> 
> I was about to suggest that we do something like that...
> 
> I wonder how messy it is.  I long ago lost the ability to follow the
> convolutions in include/linux/spinlock*.h :(  gotta patch?

The check that's in -rt for this uses BUILD_BUG_ON(), and it's extremely
cryptic when there is a failure .. Most people will be baffled why the
build stopped. If a check went into mainline it should at least give you
some sort of idea what's happening ..

Daniel

Re: [PATCH] fix flags length in net 9p

[Steven Rostedt]

On Thu, 1 May 2008, Daniel Walker wrote:
>
> The check that's in -rt for this uses BUILD_BUG_ON(), and it's extremely
> cryptic when there is a failure .. Most people will be baffled why the
> build stopped. If a check went into mainline it should at least give you
> some sort of idea what's happening ..

I can probably change it to be a bit more helpful. I'll work on that too.

-- Steve

Re: [PATCH] fix flags length in net 9p

[Adrian Bunk]

On Thu, May 01, 2008 at 02:26:00PM -0700, Daniel Walker wrote:
> 
> On Thu, 2008-05-01 at 14:19 -0700, Andrew Morton wrote:
> > On Thu, 1 May 2008 17:08:05 -0400 (EDT)
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > Some files in the net/9p directory uses "int" for flags. This can
> > > cause hard to find bugs on some architectures. This patch converts the
> > > flags to use "long" instead.
> > 
> > gargh.
> > 
> > > This bug was discovered by doing an allyesconfig make on the -rt kernel
> > > where checks are done to ensure all flags are of size sizeof(long).
> > 
> > I was about to suggest that we do something like that...
> > 
> > I wonder how messy it is.  I long ago lost the ability to follow the
> > convolutions in include/linux/spinlock*.h :(  gotta patch?
> 
> The check that's in -rt for this uses BUILD_BUG_ON(), and it's extremely
> cryptic when there is a failure .. Most people will be baffled why the
> build stopped. If a check went into mainline it should at least give you
> some sort of idea what's happening ..

A cryptic compile error is _much_ better than a hard to find runtime 
error.

> Daniel

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] fix flags length in net 9p

[Daniel Walker]

On Fri, 2008-05-02 at 00:41 +0300, Adrian Bunk wrote:

> > The check that's in -rt for this uses BUILD_BUG_ON(), and it's extremely
> > cryptic when there is a failure .. Most people will be baffled why the
> > build stopped. If a check went into mainline it should at least give you
> > some sort of idea what's happening ..
> 
> A cryptic compile error is _much_ better than a hard to find runtime 
> error.

I think we ultimately want something informative if it's possible.
That's what I want.

Daniel

Re: [PATCH] fix flags length in net 9p

[Andrew Morton]

On Thu, 01 May 2008 14:26:00 -0700
Daniel Walker <dwalker@mvista.com> wrote:

> 
> On Thu, 2008-05-01 at 14:19 -0700, Andrew Morton wrote:
> > On Thu, 1 May 2008 17:08:05 -0400 (EDT)
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > Some files in the net/9p directory uses "int" for flags. This can
> > > cause hard to find bugs on some architectures. This patch converts the
> > > flags to use "long" instead.
> > 
> > gargh.
> > 
> > > This bug was discovered by doing an allyesconfig make on the -rt kernel
> > > where checks are done to ensure all flags are of size sizeof(long).
> > 
> > I was about to suggest that we do something like that...
> > 
> > I wonder how messy it is.  I long ago lost the ability to follow the
> > convolutions in include/linux/spinlock*.h :(  gotta patch?
> 
> The check that's in -rt for this uses BUILD_BUG_ON(), and it's extremely
> cryptic when there is a failure .. Most people will be baffled why the
> build stopped. If a check went into mainline it should at least give you
> some sort of idea what's happening ..
> 

I think a warning is good enough here.  There are all sorts of warnings if,
which ignored, will crash your box.

And there is only one type which we use to hold processor flags and that is
unsigned long.


So a plain old

#define must_be_ulong(p)
	do {
		if (&p == (unsigned long *)0)
			;
	} while (0)

(or whatever)

should suffice.

Re: [PATCH] fix flags length in net 9p

[Alexey Dobriyan]

On Thu, May 01, 2008 at 02:26:00PM -0700, Daniel Walker wrote:
> 
> On Thu, 2008-05-01 at 14:19 -0700, Andrew Morton wrote:
> > On Thu, 1 May 2008 17:08:05 -0400 (EDT)
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > Some files in the net/9p directory uses "int" for flags. This can
> > > cause hard to find bugs on some architectures. This patch converts the
> > > flags to use "long" instead.
> > 
> > gargh.
> > 
> > > This bug was discovered by doing an allyesconfig make on the -rt kernel
> > > where checks are done to ensure all flags are of size sizeof(long).
> > 
> > I was about to suggest that we do something like that...
> > 
> > I wonder how messy it is.  I long ago lost the ability to follow the
> > convolutions in include/linux/spinlock*.h :(  gotta patch?
> 
> The check that's in -rt for this uses BUILD_BUG_ON(), and it's extremely
> cryptic when there is a failure .. Most people will be baffled why the
> build stopped. If a check went into mainline it should at least give you
> some sort of idea what's happening ..

One comment near BUILD_BUG_ON is enough (and everything we can do).

Re: [PATCH] fix flags length in net 9p

[Steven Rostedt]

On Fri, 2 May 2008, Alexey Dobriyan wrote:
> >
> > The check that's in -rt for this uses BUILD_BUG_ON(), and it's extremely
> > cryptic when there is a failure .. Most people will be baffled why the
> > build stopped. If a check went into mainline it should at least give you
> > some sort of idea what's happening ..
>
> One comment near BUILD_BUG_ON is enough (and everything we can do).

You might want to put a few comments by spinlock functions too, since I
hit this bug in the -rt patch and it confused my for about 10 minutes. Of
course I first blamed my own work on the spinlocks before I noticed that
the caller was using "int".

But actually, I like the idea of having a irqflags_t type that must be
used.

struct irqflags_s {
	unsigned long flags;
};
typedef struct irqflags_s irqflags_t;

This would force everyone to use this structure, and it would be an easy
script to create to do a the cross the board conversion. The only problems
would be where someone didn't use "flags" as the name, but that can be
fixed as well.

-- Steve

Re: [PATCH] fix flags length in net 9p

[Alexey Dobriyan]

On Thu, May 01, 2008 at 02:19:19PM -0700, Andrew Morton wrote:
> On Thu, 1 May 2008 17:08:05 -0400 (EDT)
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Some files in the net/9p directory uses "int" for flags. This can
> > cause hard to find bugs on some architectures. This patch converts the
> > flags to use "long" instead.
> 
> gargh.
> 
> > This bug was discovered by doing an allyesconfig make on the -rt kernel
> > where checks are done to ensure all flags are of size sizeof(long).
> 
> I was about to suggest that we do something like that...
> 
> I wonder how messy it is.  I long ago lost the ability to follow the
> convolutions in include/linux/spinlock*.h :(  gotta patch?

Me, me, take me back!


commit ee3ce191e8eaa4cc15c51a28b34143b36404c4f5
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date:   Sat Nov 25 11:09:36 2006 -0800

    [PATCH] Enforce "unsigned long flags;" when spinlocking
    
    Make it break or warn if you pass to spin_lock_irqsave() and friends
    something different from "unsigned long flags;".  Suprisingly large amount
    of these was caught by recent commit
    c53421b18f205c5f97c604ae55c6a921f034b0f6 and others.
    
    Idea is largely from FRV typechecking. Suggestions from Andrew Morton.
    All stupid typos in first version fixed.
    
    Passes allmodconfig on i386, x86_64, alpha, arm as well as my usual config.
    
    Note #1: checking with sparse is still needed, because a driver can save
    	 and pass around flags or something. So far patch is very intrusive.
    Note #2: techically, we should break only if
    		sizeof(flags) < sizeof(unsigned long),
    	 however, the more pain for getting suspicious code into kernel,
    	 the better.
    
    Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
    Cc: Ingo Molnar <mingo@elte.hu>
    Signed-off-by: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 412e025..4fe740b 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -11,6 +11,12 @@
 #ifndef _LINUX_TRACE_IRQFLAGS_H
 #define _LINUX_TRACE_IRQFLAGS_H
 
+#define BUILD_CHECK_IRQ_FLAGS(flags)					\
+	do {								\
+		BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long));	\
+		typecheck(unsigned long, flags);			\
+	} while (0)
+
 #ifdef CONFIG_TRACE_IRQFLAGS
   extern void trace_hardirqs_on(void);
   extern void trace_hardirqs_off(void);
@@ -50,10 +56,15 @@
 #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 {					\
+		BUILD_CHECK_IRQ_FLAGS(flags);	\
+		raw_local_irq_save(flags);	\
+		trace_hardirqs_off();		\
+	} while (0)
 
 #define local_irq_restore(flags)				\
 	do {							\
+		BUILD_CHECK_IRQ_FLAGS(flags);			\
 		if (raw_irqs_disabled_flags(flags)) {		\
 			raw_local_irq_restore(flags);		\
 			trace_hardirqs_off();			\
@@ -69,8 +80,16 @@
  */
 # 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)		\
+	do {					\
+		BUILD_CHECK_IRQ_FLAGS(flags);	\
+		local_irq_save(flags);		\
+	} while (0)
+# define raw_local_irq_restore(flags)		\
+	do {					\
+		BUILD_CHECK_IRQ_FLAGS(flags);	\
+		local_irq_restore(flags);	\
+	} while (0)
 #endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
 
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
@@ -80,7 +99,11 @@
 		raw_safe_halt();				\
 	} while (0)
 
-#define local_save_flags(flags)		raw_local_save_flags(flags)
+#define local_save_flags(flags)			\
+	do {					\
+		BUILD_CHECK_IRQ_FLAGS(flags);	\
+		raw_local_save_flags(flags);	\
+	} while (0)
 
 #define irqs_disabled()						\
 ({								\
@@ -90,7 +113,11 @@
 	raw_irqs_disabled_flags(flags);				\
 })
 
-#define irqs_disabled_flags(flags)	raw_irqs_disabled_flags(flags)
+#define irqs_disabled_flags(flags)	\
+({					\
+	BUILD_CHECK_IRQ_FLAGS(flags);	\
+	raw_irqs_disabled_flags(flags);	\
+})
 #endif		/* CONFIG_X86 */
 
 #endif
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index b800d2d..54ad370 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -52,6 +52,7 @@
 #include <linux/thread_info.h>
 #include <linux/kernel.h>
 #include <linux/stringify.h>
+#include <linux/irqflags.h>
 
 #include <asm/system.h>
 
@@ -183,13 +184,37 @@ do {								\
 #define read_lock(lock)			_read_lock(lock)
 
 #if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
-#define spin_lock_irqsave(lock, flags)	flags = _spin_lock_irqsave(lock)
-#define read_lock_irqsave(lock, flags)	flags = _read_lock_irqsave(lock)
-#define write_lock_irqsave(lock, flags)	flags = _write_lock_irqsave(lock)
+#define spin_lock_irqsave(lock, flags)			\
+	do {						\
+		BUILD_CHECK_IRQ_FLAGS(flags);		\
+		flags = _spin_lock_irqsave(lock);	\
+	} while (0)
+#define read_lock_irqsave(lock, flags)			\
+	do {						\
+		BUILD_CHECK_IRQ_FLAGS(flags);		\
+		flags = _read_lock_irqsave(lock);	\
+	} while (0)
+#define write_lock_irqsave(lock, flags)			\
+	do {						\
+		BUILD_CHECK_IRQ_FLAGS(flags);		\
+		flags = _write_lock_irqsave(lock);	\
+	} while (0)
 #else
-#define spin_lock_irqsave(lock, flags)	_spin_lock_irqsave(lock, flags)
-#define read_lock_irqsave(lock, flags)	_read_lock_irqsave(lock, flags)
-#define write_lock_irqsave(lock, flags)	_write_lock_irqsave(lock, flags)
+#define spin_lock_irqsave(lock, flags)			\
+	do {						\
+		BUILD_CHECK_IRQ_FLAGS(flags);		\
+		_spin_lock_irqsave(lock, flags);	\
+	} while (0)
+#define read_lock_irqsave(lock, flags)			\
+	do {						\
+		BUILD_CHECK_IRQ_FLAGS(flags);		\
+		_read_lock_irqsave(lock, flags);	\
+	} while (0)
+#define write_lock_irqsave(lock, flags)			\
+	do {						\
+		BUILD_CHECK_IRQ_FLAGS(flags);		\
+		_write_lock_irqsave(lock, flags);	\
+	} while (0)
 #endif
 
 #define spin_lock_irq(lock)		_spin_lock_irq(lock)
@@ -225,15 +250,24 @@ do {								\
 #endif
 
 #define spin_unlock_irqrestore(lock, flags) \
-					_spin_unlock_irqrestore(lock, flags)
+	do {						\
+		BUILD_CHECK_IRQ_FLAGS(flags);		\
+		_spin_unlock_irqrestore(lock, flags);	\
+	} while (0)
 #define spin_unlock_bh(lock)		_spin_unlock_bh(lock)
 
 #define read_unlock_irqrestore(lock, flags) \
-					_read_unlock_irqrestore(lock, flags)
+	do {						\
+		BUILD_CHECK_IRQ_FLAGS(flags);		\
+		_read_unlock_irqrestore(lock, flags);	\
+	} while (0)
 #define read_unlock_bh(lock)		_read_unlock_bh(lock)
 
 #define write_unlock_irqrestore(lock, flags) \
-					_write_unlock_irqrestore(lock, flags)
+	do {						\
+		BUILD_CHECK_IRQ_FLAGS(flags);		\
+		_write_unlock_irqrestore(lock, flags);	\
+	} while (0)
 #define write_unlock_bh(lock)		_write_unlock_bh(lock)
 
 #define spin_trylock_bh(lock)	__cond_lock(lock, _spin_trylock_bh(lock))
@@ -247,6 +281,7 @@ do {								\
 
 #define spin_trylock_irqsave(lock, flags) \
 ({ \
+	BUILD_CHECK_IRQ_FLAGS(flags); \
 	local_irq_save(flags); \
 	spin_trylock(lock) ? \
 	1 : ({ local_irq_restore(flags); 0; }); \



Seriously, if people can suggest _good_ *** for the following idiom

	flags = spin_lock_irq***(&lock);
		...
	spin_unlock_irqrestore(&lock, flags);

I can do tree-wide conversion with irq_flags_t and new locking
primitive.

If people can't, I can do just irq_flags_t conversion and enforce build
breakage if one use something other than irq_flags_t .

Re: [PATCH] fix flags length in net 9p

[Steven Rostedt]

On Fri, 2 May 2008, Alexey Dobriyan wrote:

> On Thu, May 01, 2008 at 02:19:19PM -0700, Andrew Morton wrote:
> > On Thu, 1 May 2008 17:08:05 -0400 (EDT)
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > > Some files in the net/9p directory uses "int" for flags. This can
> > > cause hard to find bugs on some architectures. This patch converts the
> > > flags to use "long" instead.
> >
> > gargh.
> >
> > > This bug was discovered by doing an allyesconfig make on the -rt kernel
> > > where checks are done to ensure all flags are of size sizeof(long).
> >
> > I was about to suggest that we do something like that...
> >
> > I wonder how messy it is.  I long ago lost the ability to follow the
> > convolutions in include/linux/spinlock*.h :(  gotta patch?
>
> Me, me, take me back!
>
>
> commit ee3ce191e8eaa4cc15c51a28b34143b36404c4f5
> Author: Alexey Dobriyan <adobriyan@gmail.com>
> Date:   Sat Nov 25 11:09:36 2006 -0800
>
>     [PATCH] Enforce "unsigned long flags;" when spinlocking
>
>     Make it break or warn if you pass to spin_lock_irqsave() and friends
>     something different from "unsigned long flags;".  Suprisingly large amount
>     of these was caught by recent commit
>     c53421b18f205c5f97c604ae55c6a921f034b0f6 and others.
>
>     Idea is largely from FRV typechecking. Suggestions from Andrew Morton.
>     All stupid typos in first version fixed.
>
>     Passes allmodconfig on i386, x86_64, alpha, arm as well as my usual config.
>
>     Note #1: checking with sparse is still needed, because a driver can save
>     	 and pass around flags or something. So far patch is very intrusive.
>     Note #2: techically, we should break only if
>     		sizeof(flags) < sizeof(unsigned long),
>     	 however, the more pain for getting suspicious code into kernel,
>     	 the better.
>
>     Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>     Cc: Ingo Molnar <mingo@elte.hu>
>     Signed-off-by: Andrew Morton <akpm@osdl.org>
>     Signed-off-by: Linus Torvalds <torvalds@osdl.org>
>
> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> index 412e025..4fe740b 100644
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -11,6 +11,12 @@
>  #ifndef _LINUX_TRACE_IRQFLAGS_H
>  #define _LINUX_TRACE_IRQFLAGS_H
>
> +#define BUILD_CHECK_IRQ_FLAGS(flags)					\
> +	do {								\
> +		BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long));	\
> +		typecheck(unsigned long, flags);			\
> +	} while (0)
> +
>  #ifdef CONFIG_TRACE_IRQFLAGS
>    extern void trace_hardirqs_on(void);
>    extern void trace_hardirqs_off(void);
> @@ -50,10 +56,15 @@
>  #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 {					\
> +		BUILD_CHECK_IRQ_FLAGS(flags);	\
> +		raw_local_irq_save(flags);	\
> +		trace_hardirqs_off();		\
> +	} while (0)
>
>  #define local_irq_restore(flags)				\
>  	do {							\
> +		BUILD_CHECK_IRQ_FLAGS(flags);			\
>  		if (raw_irqs_disabled_flags(flags)) {		\
>  			raw_local_irq_restore(flags);		\
>  			trace_hardirqs_off();			\
> @@ -69,8 +80,16 @@
>   */
>  # 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)		\
> +	do {					\
> +		BUILD_CHECK_IRQ_FLAGS(flags);	\
> +		local_irq_save(flags);		\
> +	} while (0)
> +# define raw_local_irq_restore(flags)		\
> +	do {					\
> +		BUILD_CHECK_IRQ_FLAGS(flags);	\
> +		local_irq_restore(flags);	\
> +	} while (0)
>  #endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
>
>  #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
> @@ -80,7 +99,11 @@
>  		raw_safe_halt();				\
>  	} while (0)
>
> -#define local_save_flags(flags)		raw_local_save_flags(flags)
> +#define local_save_flags(flags)			\
> +	do {					\
> +		BUILD_CHECK_IRQ_FLAGS(flags);	\
> +		raw_local_save_flags(flags);	\
> +	} while (0)
>
>  #define irqs_disabled()						\
>  ({								\
> @@ -90,7 +113,11 @@
>  	raw_irqs_disabled_flags(flags);				\
>  })
>
> -#define irqs_disabled_flags(flags)	raw_irqs_disabled_flags(flags)
> +#define irqs_disabled_flags(flags)	\
> +({					\
> +	BUILD_CHECK_IRQ_FLAGS(flags);	\
> +	raw_irqs_disabled_flags(flags);	\
> +})
>  #endif		/* CONFIG_X86 */
>
>  #endif
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index b800d2d..54ad370 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -52,6 +52,7 @@
>  #include <linux/thread_info.h>
>  #include <linux/kernel.h>
>  #include <linux/stringify.h>
> +#include <linux/irqflags.h>
>
>  #include <asm/system.h>
>
> @@ -183,13 +184,37 @@ do {								\
>  #define read_lock(lock)			_read_lock(lock)
>
>  #if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
> -#define spin_lock_irqsave(lock, flags)	flags = _spin_lock_irqsave(lock)
> -#define read_lock_irqsave(lock, flags)	flags = _read_lock_irqsave(lock)
> -#define write_lock_irqsave(lock, flags)	flags = _write_lock_irqsave(lock)
> +#define spin_lock_irqsave(lock, flags)			\
> +	do {						\
> +		BUILD_CHECK_IRQ_FLAGS(flags);		\
> +		flags = _spin_lock_irqsave(lock);	\
> +	} while (0)
> +#define read_lock_irqsave(lock, flags)			\
> +	do {						\
> +		BUILD_CHECK_IRQ_FLAGS(flags);		\
> +		flags = _read_lock_irqsave(lock);	\
> +	} while (0)
> +#define write_lock_irqsave(lock, flags)			\
> +	do {						\
> +		BUILD_CHECK_IRQ_FLAGS(flags);		\
> +		flags = _write_lock_irqsave(lock);	\
> +	} while (0)
>  #else
> -#define spin_lock_irqsave(lock, flags)	_spin_lock_irqsave(lock, flags)
> -#define read_lock_irqsave(lock, flags)	_read_lock_irqsave(lock, flags)
> -#define write_lock_irqsave(lock, flags)	_write_lock_irqsave(lock, flags)
> +#define spin_lock_irqsave(lock, flags)			\
> +	do {						\
> +		BUILD_CHECK_IRQ_FLAGS(flags);		\
> +		_spin_lock_irqsave(lock, flags);	\
> +	} while (0)
> +#define read_lock_irqsave(lock, flags)			\
> +	do {						\
> +		BUILD_CHECK_IRQ_FLAGS(flags);		\
> +		_read_lock_irqsave(lock, flags);	\
> +	} while (0)
> +#define write_lock_irqsave(lock, flags)			\
> +	do {						\
> +		BUILD_CHECK_IRQ_FLAGS(flags);		\
> +		_write_lock_irqsave(lock, flags);	\
> +	} while (0)
>  #endif
>
>  #define spin_lock_irq(lock)		_spin_lock_irq(lock)
> @@ -225,15 +250,24 @@ do {								\
>  #endif
>
>  #define spin_unlock_irqrestore(lock, flags) \
> -					_spin_unlock_irqrestore(lock, flags)
> +	do {						\
> +		BUILD_CHECK_IRQ_FLAGS(flags);		\
> +		_spin_unlock_irqrestore(lock, flags);	\
> +	} while (0)
>  #define spin_unlock_bh(lock)		_spin_unlock_bh(lock)
>
>  #define read_unlock_irqrestore(lock, flags) \
> -					_read_unlock_irqrestore(lock, flags)
> +	do {						\
> +		BUILD_CHECK_IRQ_FLAGS(flags);		\
> +		_read_unlock_irqrestore(lock, flags);	\
> +	} while (0)
>  #define read_unlock_bh(lock)		_read_unlock_bh(lock)
>
>  #define write_unlock_irqrestore(lock, flags) \
> -					_write_unlock_irqrestore(lock, flags)
> +	do {						\
> +		BUILD_CHECK_IRQ_FLAGS(flags);		\
> +		_write_unlock_irqrestore(lock, flags);	\
> +	} while (0)
>  #define write_unlock_bh(lock)		_write_unlock_bh(lock)
>
>  #define spin_trylock_bh(lock)	__cond_lock(lock, _spin_trylock_bh(lock))
> @@ -247,6 +281,7 @@ do {								\
>
>  #define spin_trylock_irqsave(lock, flags) \
>  ({ \
> +	BUILD_CHECK_IRQ_FLAGS(flags); \
>  	local_irq_save(flags); \
>  	spin_trylock(lock) ? \
>  	1 : ({ local_irq_restore(flags); 0; }); \
>
>
>
> Seriously, if people can suggest _good_ *** for the following idiom
>
> 	flags = spin_lock_irq***(&lock);
> 		...
> 	spin_unlock_irqrestore(&lock, flags);
>
> I can do tree-wide conversion with irq_flags_t and new locking
> primitive.

I don't think we need to have a new primitive.

>
> If people can't, I can do just irq_flags_t conversion and enforce build
> breakage if one use something other than irq_flags_t .

We can simply have all users of flags use the new irqflags_t type, and
just do a one time big patch (grant you, that would break all pending
patches before that). But we can also write up a script that can convert
patches as well.

-- Steve

Re: [PATCH] fix flags length in net 9p

[Andrew Morton]

On Thu, 1 May 2008 17:50:38 -0400 (EDT)
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> 
> 
> > +		typecheck(unsigned long, flags);

ooh, I'd forgotten about typecheck.

Just use typecheck.  The BUILD_BUG_ON is overkill.

> We can simply have all users of flags use the new irqflags_t type, and
> just do a one time big patch (grant you, that would break all pending
> patches before that). But we can also write up a script that can convert
> patches as well.

y:/usr/src/linux-2.6.25> grep -rl 'unsigned long flags' . | wc -l
2311

eargh.  I agree with me.

Re: [PATCH] fix flags length in net 9p

[Christoph Hellwig]

On Thu, May 01, 2008 at 05:08:05PM -0400, Steven Rostedt wrote:
> 
> Some files in the net/9p directory uses "int" for flags. This can
> cause hard to find bugs on some architectures. This patch converts the
> flags to use "long" instead.
> 
> This bug was discovered by doing an allyesconfig make on the -rt kernel
> where checks are done to ensure all flags are of size sizeof(long).

We had checks like that in mainline aswell that caught a lot of errors
like this one.  Probably got lost in one of the gazillions of spinlock
rewrites..

Re: [PATCH] fix flags length in net 9p

[Enrico Weigelt]

* Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> Some files in the net/9p directory uses "int" for flags. This can
> cause hard to find bugs on some architectures. This patch converts the
> flags to use "long" instead.

IMHO it would be more consequent and clean to introduce an separate 
type for this and only use specific macros for manipulation. Of course
this introduced some typing overhead, but enforces the coder to think
more carefully about his code. For critical things like kernel stuff,
I really prefer this way.


BTW: I just wondered a bit how the 9p driver finds out the numeric
user ID's for 9P user names.


cu
-- 
---------------------------------------------------------------------
 Enrico Weigelt    ==   metux IT service - http://www.metux.de/
---------------------------------------------------------------------
 Please visit the OpenSource QM Taskforce:
 	http://wiki.metux.de/public/OpenSource_QM_Taskforce
 Patches / Fixes for a lot dozens of packages in dozens of versions:
	http://patches.metux.de/
---------------------------------------------------------------------