[PATCH Resend] - SN: validate smp_affinity mask on intr redirect

[PATCH Resend] - SN: validate smp_affinity mask on intr redirect

[John Keller]

On SN, only allow one bit to be set in the smp_affinty mask
when redirecting an interrupt. Currently setting multiple
bits is allowed, but only the first bit is used in
determining the CPU to redirect to. This has caused confusion
among some customers.

Signed-off-by: John Keller <jpk@sgi.com>
---

Patch reworked as per Andrew's option "b".


 arch/ia64/kernel/irq.c |   11 +++++++++++
 include/asm-ia64/irq.h |    2 ++
 kernel/irq/proc.c      |    7 +++++++
 3 files changed, 20 insertions(+)


Index: linux-2.6/kernel/irq/proc.c
=================================--- linux-2.6.orig/kernel/irq/proc.c	2007-05-02 09:03:14.440820500 -0500
+++ linux-2.6/kernel/irq/proc.c	2007-05-03 16:00:46.746068614 -0500
@@ -27,6 +27,10 @@ static int irq_affinity_read_proc(char *
 	return len;
 }
 
+#ifndef is_affinity_mask_valid
+#define is_affinity_mask_valid() 1
+#endif
+
 int no_irq_affinity;
 static int irq_affinity_write_proc(struct file *file, const char __user *buffer,
 				   unsigned long count, void *data)
@@ -42,6 +46,9 @@ static int irq_affinity_write_proc(struc
 	if (err)
 		return err;
 
+	if (!is_affinity_mask_valid(new_value))
+		return -EINVAL;
+
 	/*
 	 * Do not allow disabling IRQs completely - it's a too easy
 	 * way to make the system unusable accidentally :-) At least
Index: linux-2.6/arch/ia64/kernel/irq.c
=================================--- linux-2.6.orig/arch/ia64/kernel/irq.c	2007-05-02 09:03:14.504828433 -0500
+++ linux-2.6/arch/ia64/kernel/irq.c	2007-05-03 16:15:47.604740039 -0500
@@ -104,6 +104,17 @@ void set_irq_affinity_info (unsigned int
 		irq_redir[irq] = (char) (redir & 0xff);
 	}
 }
+
+bool is_affinity_mask_valid(cpumask_t cpumask)
+{
+	if (ia64_platform_is("sn2")) {
+		/* Only allow one CPU to be specified in the smp_affinity mask */
+		if (cpus_weight(cpumask) != 1)
+			return false;
+	}
+	return true;
+}
+
 #endif /* CONFIG_SMP */
 
 #ifdef CONFIG_HOTPLUG_CPU
Index: linux-2.6/include/asm-ia64/irq.h
=================================--- linux-2.6.orig/include/asm-ia64/irq.h	2007-05-02 09:03:14.640845292 -0500
+++ linux-2.6/include/asm-ia64/irq.h	2007-05-03 16:02:31.934991908 -0500
@@ -30,4 +30,6 @@ extern void disable_irq_nosync (unsigned
 extern void enable_irq (unsigned int);
 extern void set_irq_affinity_info (unsigned int irq, int dest, int redir);
 
+#define is_affinity_mask_valid is_affinity_mask_valid
+
 #endif /* _ASM_IA64_IRQ_H */

RE: [PATCH Resend] - SN: validate smp_affinity mask on intr redirect

[Luck, Tony]

 
+#ifndef is_affinity_mask_valid
+#define is_affinity_mask_valid() 1
+#endif
+
 int no_irq_affinity;
 static int irq_affinity_write_proc(struct file *file, const char __user *buffer,
 				   unsigned long count, void *data)
@@ -42,6 +46,9 @@ static int irq_affinity_write_proc(struc
 	if (err)
 		return err;
 
+	if (!is_affinity_mask_valid(new_value))
+		return -EINVAL;

This results in a warning:
kernel/irq/proc.c:49: warning: implicit declaration of function `is_affinity_mask_valid'

I can't put a declaration for is_affinity_mask_valid into
include/asm-ia64/irq.h because that results in errors in
files that include this, but don't have a definition for
cpumask_t.

I could add the declaration as a #else clause to that
#ifndef in kernel/irq/proc.c, but that would no doubt
result is complaints from the style police that function
prototypes belong in header files, not in ".c" files.

Last option is to move the #if to linux/irq.h:

#ifndef is_affinity_mask_valid
#define is_affinity_mask_valid 1
#else
extern bool is_affinity_mask_valid(cpumask_t cpumask);
#endif

But that seems to spoil the whole tricksiness factor and
doesn't really look any better that using ARCH_HAS_IRQ_AFFINITY_MASK_CHECK

-Tony

Re: [PATCH Resend] - SN: validate smp_affinity mask on intr

[Andrew Morton]

On Tue, 8 May 2007 11:03:20 -0700
"Luck, Tony" <tony.luck@intel.com> wrote:

> +#ifndef is_affinity_mask_valid
> +#define is_affinity_mask_valid() 1
> +#endif
> +
>  int no_irq_affinity;
>  static int irq_affinity_write_proc(struct file *file, const char __user *buffer,
>  				   unsigned long count, void *data)
> @@ -42,6 +46,9 @@ static int irq_affinity_write_proc(struc
>  	if (err)
>  		return err;
>  
> +	if (!is_affinity_mask_valid(new_value))
> +		return -EINVAL;
> 
> This results in a warning:
> kernel/irq/proc.c:49: warning: implicit declaration of function `is_affinity_mask_valid'
> 

It had a dopey little bug:


diff -puN kernel/irq/proc.c~sn-validate-smp_affinity-mask-on-intr-redirect-fix kernel/irq/proc.c
--- a/kernel/irq/proc.c~sn-validate-smp_affinity-mask-on-intr-redirect-fix
+++ a/kernel/irq/proc.c
@@ -28,7 +28,7 @@ static int irq_affinity_read_proc(char *
 }
 
 #ifndef is_affinity_mask_valid
-#define is_affinity_mask_valid() 1
+#define is_affinity_mask_valid(val) 1
 #endif
 
 int no_irq_affinity;
_

RE: [PATCH Resend] - SN: validate smp_affinity mask on intr redirect

[Luck, Tony]

> It had a dopey little bug:
>
> -#define is_affinity_mask_valid() 1
> +#define is_affinity_mask_valid(val) 1

That would fix warnings on non-ia64 systems (which is
a step in the right direction).  But on ia64 I have
the

#define is_affinity_mask_valid is_affinity_mask_valid

in play at that point, so I have a real function call
which doesn't have an in-scope declaration, so I get:

kernel/irq/proc.c:49: warning: implicit declaration of function `is_affinity_mask_valid'

-Tony

Re: [PATCH Resend] - SN: validate smp_affinity mask on intr

[Andrew Morton]

On Tue, 8 May 2007 13:14:26 -0700
"Luck, Tony" <tony.luck@intel.com> wrote:

> > It had a dopey little bug:
> >
> > -#define is_affinity_mask_valid() 1
> > +#define is_affinity_mask_valid(val) 1
> 
> That would fix warnings on non-ia64 systems (which is
> a step in the right direction).  But on ia64 I have
> the
> 
> #define is_affinity_mask_valid is_affinity_mask_valid
> 
> in play at that point, so I have a real function call
> which doesn't have an in-scope declaration, so I get:
> 
> kernel/irq/proc.c:49: warning: implicit declaration of function `is_affinity_mask_valid'
> 

hm, I wonder if John sent the right patch.

The obvious fix fixes it up for me:

--- a/include/asm-ia64/irq.h~sn-validate-smp_affinity-mask-on-intr-redirect-fix-2
+++ a/include/asm-ia64/irq.h
@@ -11,6 +11,9 @@
  * 02/29/00     D.Mosberger	moved most things into hw_irq.h
  */
 
+#include <linux/types.h>
+#include <linux/cpumask.h>
+
 #define NR_IRQS		256
 #define NR_IRQ_VECTORS	NR_IRQS
 
@@ -29,6 +32,7 @@ extern void disable_irq (unsigned int);
 extern void disable_irq_nosync (unsigned int);
 extern void enable_irq (unsigned int);
 extern void set_irq_affinity_info (unsigned int irq, int dest, int redir);
+bool is_affinity_mask_valid(cpumask_t cpumask);
 
 #define is_affinity_mask_valid is_affinity_mask_valid
 
_

but the new includes are always a worry.

We need the cpumask.h include because we went and created a stupid typedef.
 I think at the time we did this because cpumask_t could optionally be a
plain old "long".  However I don't think we ended up doing that so we could
now do:

-typedef struct { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
+typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;

and then we can forward-declare `struct cpumask' in include/asm-ia64/irq.h
rather than adding the nested include.

Re: [PATCH Resend] - SN: validate smp_affinity mask on intr

[John Keller]

> 
> On Tue, 8 May 2007 13:14:26 -0700
> "Luck, Tony" <tony.luck@intel.com> wrote:
> 
> > > It had a dopey little bug:
> > >
> > > -#define is_affinity_mask_valid() 1
> > > +#define is_affinity_mask_valid(val) 1
> > 
> > That would fix warnings on non-ia64 systems (which is
> > a step in the right direction).  But on ia64 I have
> > the
> > 
> > #define is_affinity_mask_valid is_affinity_mask_valid
> > 
> > in play at that point, so I have a real function call
> > which doesn't have an in-scope declaration, so I get:
> > 
> > kernel/irq/proc.c:49: warning: implicit declaration of function `is_affinity_mask_valid'
> > 
> 
> hm, I wonder if John sent the right patch.


I sure thought I had sent the right patch, but obviously not.
Thanks for setting things straight.

John


> 
> The obvious fix fixes it up for me:
> 
> --- a/include/asm-ia64/irq.h~sn-validate-smp_affinity-mask-on-intr-redirect-fix-2
> +++ a/include/asm-ia64/irq.h
> @@ -11,6 +11,9 @@
>   * 02/29/00     D.Mosberger	moved most things into hw_irq.h
>   */
>  
> +#include <linux/types.h>
> +#include <linux/cpumask.h>
> +
>  #define NR_IRQS		256
>  #define NR_IRQ_VECTORS	NR_IRQS
>  
> @@ -29,6 +32,7 @@ extern void disable_irq (unsigned int);
>  extern void disable_irq_nosync (unsigned int);
>  extern void enable_irq (unsigned int);
>  extern void set_irq_affinity_info (unsigned int irq, int dest, int redir);
> +bool is_affinity_mask_valid(cpumask_t cpumask);
>  
>  #define is_affinity_mask_valid is_affinity_mask_valid
>  
> _
> 
> but the new includes are always a worry.
> 
> We need the cpumask.h include because we went and created a stupid typedef.
>  I think at the time we did this because cpumask_t could optionally be a
> plain old "long".  However I don't think we ended up doing that so we could
> now do:
> 
> -typedef struct { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
> +typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
> 
> and then we can forward-declare `struct cpumask' in include/asm-ia64/irq.h
> rather than adding the nested include.
> 
> 
> 

Re: [PATCH Resend] - SN: validate smp_affinity mask on intr redirect

[Peter Chubb]

Jack>  }
Jack> +
Jack> +bool is_affinity_mask_valid(cpumask_t cpumask)
Jack> +{
Jack> +	if (ia64_platform_is("sn2")) {
Jack> +		/* Only allow one CPU to be specified in the smp_affinity mask */
Jack> +		if (cpus_weight(cpumask) != 1)
Jack> +			return false;

Why not just:
		return cpus_weight(cpumask) = 1;


It's a Boolean; treat it as one.
(If you thought the average kernel programmer (who's s/he?) understood
the logical implication rule it could be:
	return !ia64_platform_is("sn2") || cpus_weight(cpumask) = 1;
)
--
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
http://www.ertos.nicta.com.au           ERTOS within National ICT Australia