[PATCH] Prevent IDE boot ops on NUMA system in mainline

[PATCH] Prevent IDE boot ops on NUMA system in mainline

[Andi Kleen]

Without this patch a Opteron test system here oopses at boot with currentg git. 

Calling to_pci_dev() on a NULL pointer gives a negative value so the following NULL 
pointer check never triggers and then an illegal address is referenced. Check the 
unadjusted original device pointer for NULL instead.

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/include/linux/ide.h
===================================================================
--- linux.orig/include/linux/ide.h
+++ linux/include/linux/ide.h
@@ -1294,7 +1294,7 @@ static inline void ide_dump_identify(u8 
 static inline int hwif_to_node(ide_hwif_t *hwif)
 {
 	struct pci_dev *dev = to_pci_dev(hwif->dev);
-	return dev ? pcibus_to_node(dev->bus) : -1;
+	return hwif->dev ? pcibus_to_node(dev->bus) : -1;
 }
 
 static inline ide_drive_t *ide_get_paired_drive(ide_drive_t *drive)

Re: [PATCH] Prevent IDE boot ops on NUMA system in mainline

[Linus Torvalds]

On Mon, 11 Feb 2008, Andi Kleen wrote:
> 
> Without this patch a Opteron test system here oopses at boot with currentg git. 
> 
> Calling to_pci_dev() on a NULL pointer gives a negative value so the following NULL 
> pointer check never triggers and then an illegal address is referenced. Check the 
> unadjusted original device pointer for NULL instead.
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> 
> Index: linux/include/linux/ide.h
> ===================================================================
> --- linux.orig/include/linux/ide.h
> +++ linux/include/linux/ide.h
> @@ -1294,7 +1294,7 @@ static inline void ide_dump_identify(u8 
>  static inline int hwif_to_node(ide_hwif_t *hwif)
>  {
>  	struct pci_dev *dev = to_pci_dev(hwif->dev);
> -	return dev ? pcibus_to_node(dev->bus) : -1;
> +	return hwif->dev ? pcibus_to_node(dev->bus) : -1;
>  }

Ok, I applied this, but it causes a *lot* of noise about "unused variable 
'dev'" because pcibus_to_node() is defined to be -1 when you don't do any 
strange NUMA thing (so that "dev->bus" usage thing is never even seen by 
the compiler.

So we should probably make pcibus_to_node() be an inline function for that 
case, or just make that thing be

	return hwif->dev ?
		pcibus_to_node(to_pci_dev(hwif->dev)->bus)
	       :
		-1;

or something. Because now things are really noisy.

Preferences, anybody? Bartlomiej?

		Linus

Re: [PATCH] Prevent IDE boot ops on NUMA system in mainline

[Linus Torvalds]

On Mon, 11 Feb 2008, Linus Torvalds wrote:
> 
> So we should probably make pcibus_to_node() be an inline function for that 
> case

Or, we could just do the ugliest patch ever, namely

	-#define pcibus_to_node(node)   (-1)
	+#define pcibus_to_node(node)   ((int)(long)(node),-1)

Wow. It's so ugly it's almost wraps around and comes out the other sides 
and looks pretty.

		Linus

Re: [PATCH] Prevent IDE boot ops on NUMA system in mainline

[Andi Kleen]

On Mon, Feb 11, 2008 at 09:37:18AM -0800, Linus Torvalds wrote:
> 
> 
> On Mon, 11 Feb 2008, Linus Torvalds wrote:
> > 
> > So we should probably make pcibus_to_node() be an inline function for that 
> > case
> 
> Or, we could just do the ugliest patch ever, namely
> 
> 	-#define pcibus_to_node(node)   (-1)
> 	+#define pcibus_to_node(node)   ((int)(long)(node),-1)
> 
> Wow. It's so ugly it's almost wraps around and comes out the other sides 
> and looks pretty.

(void)arg, ... is better. Trick originally from Jan Beulich I think
(his code is always a good source for useful new C tricks) 

-Andi

Re: [PATCH] Prevent IDE boot ops on NUMA system in mainline

[Andi Kleen]

On Mon, Feb 11, 2008 at 09:33:14AM -0800, Linus Torvalds wrote:
> 
> 
> On Mon, 11 Feb 2008, Andi Kleen wrote:
> > 
> > Without this patch a Opteron test system here oopses at boot with currentg git. 
> > 
> > Calling to_pci_dev() on a NULL pointer gives a negative value so the following NULL 
> > pointer check never triggers and then an illegal address is referenced. Check the 
> > unadjusted original device pointer for NULL instead.
> > 
> > Signed-off-by: Andi Kleen <ak@suse.de>
> > 
> > Index: linux/include/linux/ide.h
> > ===================================================================
> > --- linux.orig/include/linux/ide.h
> > +++ linux/include/linux/ide.h
> > @@ -1294,7 +1294,7 @@ static inline void ide_dump_identify(u8 
> >  static inline int hwif_to_node(ide_hwif_t *hwif)
> >  {
> >  	struct pci_dev *dev = to_pci_dev(hwif->dev);
> > -	return dev ? pcibus_to_node(dev->bus) : -1;
> > +	return hwif->dev ? pcibus_to_node(dev->bus) : -1;
> >  }
> 
> Ok, I applied this, but it causes a *lot* of noise about "unused variable 
> 'dev'" because pcibus_to_node() is defined to be -1 when you don't do any 
> strange NUMA thing (so that "dev->bus" usage thing is never even seen by 
> the compiler.
> 
> So we should probably make pcibus_to_node() be an inline function for that 
> case, or just make that thing be
> 
> 	return hwif->dev ?
> 		pcibus_to_node(to_pci_dev(hwif->dev)->bus)
> 	       :
> 		-1;
> 
> or something. Because now things are really noisy.

Yes sorry; I only checked NUMA builds before sending it off. 
The easiest way is probably just the traditional (void) cast
I fixed most of the topology macros while I was at it.

-Andi

---

Make topology fallback macros reference their arguments.

This avoids warnings with unreferenced variables in the !NUMA case.

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/include/asm-generic/topology.h
===================================================================
--- linux.orig/include/asm-generic/topology.h
+++ linux/include/asm-generic/topology.h
@@ -30,19 +30,19 @@
 /* Other architectures wishing to use this simple topology API should fill
    in the below functions as appropriate in their own <asm/topology.h> file. */
 #ifndef cpu_to_node
-#define cpu_to_node(cpu)	(0)
+#define cpu_to_node(cpu)	((void)(cpu),0)
 #endif
 #ifndef parent_node
-#define parent_node(node)	(0)
+#define parent_node(node)	((void)(node),0)
 #endif
 #ifndef node_to_cpumask
-#define node_to_cpumask(node)	(cpu_online_map)
+#define node_to_cpumask(node)	((void)node, cpu_online_map)
 #endif
 #ifndef node_to_first_cpu
-#define node_to_first_cpu(node)	(0)
+#define node_to_first_cpu(node)	((void)(node),0)
 #endif
 #ifndef pcibus_to_node
-#define pcibus_to_node(node)	(-1)
+#define pcibus_to_node(bus)	((void)(bus), -1)
 #endif
 
 #ifndef pcibus_to_cpumask

Re: [PATCH] Prevent IDE boot ops on NUMA system in mainline

[Bartlomiej Zolnierkiewicz]

On Monday 11 February 2008, Andi Kleen wrote:
> 
> Without this patch a Opteron test system here oopses at boot with currentg git. 
> 
> Calling to_pci_dev() on a NULL pointer gives a negative value so the following NULL 
> pointer check never triggers and then an illegal address is referenced. Check the 
> unadjusted original device pointer for NULL instead.
> 
> Signed-off-by: Andi Kleen <ak@suse.de>

Thanks Andi!

This may explain+resolve ide_generic OOPS-es reported by Pavel/Kamalesh/Nish.

[ I also see that Linus has already merged the patch and you've followed
  up on pcibus_to_node() issue so there is nothing left for me to do...

  I really love when things turn out like this... ;) ]

> Index: linux/include/linux/ide.h
> ===================================================================
> --- linux.orig/include/linux/ide.h
> +++ linux/include/linux/ide.h
> @@ -1294,7 +1294,7 @@ static inline void ide_dump_identify(u8 
>  static inline int hwif_to_node(ide_hwif_t *hwif)
>  {
>  	struct pci_dev *dev = to_pci_dev(hwif->dev);
> -	return dev ? pcibus_to_node(dev->bus) : -1;
> +	return hwif->dev ? pcibus_to_node(dev->bus) : -1;
>  }
>  
>  static inline ide_drive_t *ide_get_paired_drive(ide_drive_t *drive)