linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Fix crash on boot in kmalloc_node IDE changes
@ 2005-07-06 13:30 Andi Kleen
  2005-07-06 14:05 ` Bartlomiej Zolnierkiewicz
                   ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Andi Kleen @ 2005-07-06 13:30 UTC (permalink / raw)
  To: linux-ide, torvalds, linux-kernel, christoph


Without this patch a dual Xeon EM64T machine would oops on boot
because the hwif pointer here was NULL. I also added a check for
pci_dev because it's doubtful that all IDE devices have pci_devs.

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


Index: linux/drivers/ide/ide-probe.c
===================================================================
--- linux.orig/drivers/ide/ide-probe.c
+++ linux/drivers/ide/ide-probe.c
@@ -978,8 +978,10 @@ static int ide_init_queue(ide_drive_t *d
 	 *	do not.
 	 */
 
-	q = blk_init_queue_node(do_ide_request, &ide_lock,
-				pcibus_to_node(drive->hwif->pci_dev->bus));
+	int node = 0; /* Should be -1 */
+	if (drive->hwif && drive->hwif->pci_dev)
+		node = pcibus_to_node(drive->hwif->pci_dev->bus); 
+	q = blk_init_queue_node(do_ide_request, &ide_lock, node);
 	if (!q)
 		return 1;
 
@@ -1096,8 +1098,13 @@ static int init_irq (ide_hwif_t *hwif)
 		hwgroup->hwif->next = hwif;
 		spin_unlock_irq(&ide_lock);
 	} else {
-		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
-			pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus));
+		int node = 0; 
+		if (hwif->drives[0].hwif) { 
+			struct pci_dev *pdev = hwif->drives[0].hwif->pci_dev;  
+			if (pdev)
+				node = pcibus_to_node(pdev->bus);
+		}
+		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,node);
 		if (!hwgroup)
 	       		goto out_up;
 



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-06 13:30 [PATCH] Fix crash on boot in kmalloc_node IDE changes Andi Kleen
@ 2005-07-06 14:05 ` Bartlomiej Zolnierkiewicz
  2005-07-06 14:09   ` Andi Kleen
  2005-07-06 16:34 ` Christoph Lameter
  2005-07-07 16:21 ` [another PATCH] " Christoph Lameter
  2 siblings, 1 reply; 24+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2005-07-06 14:05 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-ide, torvalds, linux-kernel, christoph

On 7/6/05, Andi Kleen <ak@suse.de> wrote:
> 
> Without this patch a dual Xeon EM64T machine would oops on boot
> because the hwif pointer here was NULL. I also added a check for
> pci_dev because it's doubtful that all IDE devices have pci_devs.
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> 
> 
> Index: linux/drivers/ide/ide-probe.c
> ===================================================================
> --- linux.orig/drivers/ide/ide-probe.c
> +++ linux/drivers/ide/ide-probe.c
> @@ -978,8 +978,10 @@ static int ide_init_queue(ide_drive_t *d
>          *      do not.
>          */
> 
> -       q = blk_init_queue_node(do_ide_request, &ide_lock,
> -                               pcibus_to_node(drive->hwif->pci_dev->bus));
> +       int node = 0; /* Should be -1 */
> +       if (drive->hwif && drive->hwif->pci_dev)
> +               node = pcibus_to_node(drive->hwif->pci_dev->bus);
> +       q = blk_init_queue_node(do_ide_request, &ide_lock, node);
>         if (!q)
>                 return 1;

drive->hwif check is redundant, please remove it

> @@ -1096,8 +1098,13 @@ static int init_irq (ide_hwif_t *hwif)
>                 hwgroup->hwif->next = hwif;
>                 spin_unlock_irq(&ide_lock);
>         } else {
> -               hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
> -                       pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus));
> +               int node = 0;
> +               if (hwif->drives[0].hwif) {
> +                       struct pci_dev *pdev = hwif->drives[0].hwif->pci_dev;
> +                       if (pdev)
> +                               node = pcibus_to_node(pdev->bus);
> +               }

ditto, moreover hwif->drives[0].hwif == hwif

> +               hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,node);
>                 if (!hwgroup)
>                         goto out_up;

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-06 14:05 ` Bartlomiej Zolnierkiewicz
@ 2005-07-06 14:09   ` Andi Kleen
  2005-07-06 14:35     ` Bartlomiej Zolnierkiewicz
  0 siblings, 1 reply; 24+ messages in thread
From: Andi Kleen @ 2005-07-06 14:09 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: Andi Kleen, linux-ide, torvalds, linux-kernel, christoph

> drive->hwif check is redundant, please remove it

It's not. My first version didn't have it but it still crashed.
It's what actually prevents the crash.
I also don't know why, but it's true.

The machine had four IDE controllers BTW (on board an an external Promise
card)

-Andi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-06 14:09   ` Andi Kleen
@ 2005-07-06 14:35     ` Bartlomiej Zolnierkiewicz
  2005-07-06 18:21       ` Andi Kleen
  0 siblings, 1 reply; 24+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2005-07-06 14:35 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-ide, torvalds, linux-kernel, christoph

On 7/6/05, Andi Kleen <ak@suse.de> wrote:
> > drive->hwif check is redundant, please remove it
> 
> It's not. My first version didn't have it but it still crashed.
> It's what actually prevents the crash.
> I also don't know why, but it's true.

very weird as HWIF(drive) == drive->hwif:

	ide_hwif_t *hwif = HWIF(drive);

...

	q = blk_init_queue_node(do_ide_request, &ide_lock,
				pcibus_to_node(drive->hwif->pci_dev->bus));
	if (!q)
		return 1;

...

	if (!hwif->rqsize) {

you should OOPS here

Bartlomiej

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-06 13:30 [PATCH] Fix crash on boot in kmalloc_node IDE changes Andi Kleen
  2005-07-06 14:05 ` Bartlomiej Zolnierkiewicz
@ 2005-07-06 16:34 ` Christoph Lameter
  2005-07-06 16:45   ` Andi Kleen
  2005-07-07 16:21 ` [another PATCH] " Christoph Lameter
  2 siblings, 1 reply; 24+ messages in thread
From: Christoph Lameter @ 2005-07-06 16:34 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-ide, torvalds, linux-kernel

On Wed, 6 Jul 2005, Andi Kleen wrote:

> -	q = blk_init_queue_node(do_ide_request, &ide_lock,
> -				pcibus_to_node(drive->hwif->pci_dev->bus));
> +	int node = 0; /* Should be -1 */

Why is this not -1?

> +		int node = 0; 
> +		if (hwif->drives[0].hwif) { 

Also needs to be -1.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-06 16:34 ` Christoph Lameter
@ 2005-07-06 16:45   ` Andi Kleen
  2005-07-06 17:07     ` Christoph Lameter
  0 siblings, 1 reply; 24+ messages in thread
From: Andi Kleen @ 2005-07-06 16:45 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Andi Kleen, linux-ide, torvalds, linux-kernel

On Wed, Jul 06, 2005 at 09:34:28AM -0700, Christoph Lameter wrote:
> On Wed, 6 Jul 2005, Andi Kleen wrote:
> 
> > -	q = blk_init_queue_node(do_ide_request, &ide_lock,
> > -				pcibus_to_node(drive->hwif->pci_dev->bus));
> > +	int node = 0; /* Should be -1 */
> 
> Why is this not -1?

Because there is no code in rc3 that handles -1 in kmalloc_node.

If you add a patch that handles it then feel free to change.
But fixing the bootup has the highest priority.

> 
> > +		int node = 0; 
> > +		if (hwif->drives[0].hwif) { 
> 
> Also needs to be -1.

Then it would crash again.


-Andi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-06 16:45   ` Andi Kleen
@ 2005-07-06 17:07     ` Christoph Lameter
  0 siblings, 0 replies; 24+ messages in thread
From: Christoph Lameter @ 2005-07-06 17:07 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-ide, torvalds, linux-kernel

On Wed, 6 Jul 2005, Andi Kleen wrote:

> On Wed, Jul 06, 2005 at 09:34:28AM -0700, Christoph Lameter wrote:
> > On Wed, 6 Jul 2005, Andi Kleen wrote:
> > 
> > > -	q = blk_init_queue_node(do_ide_request, &ide_lock,
> > > -				pcibus_to_node(drive->hwif->pci_dev->bus));
> > > +	int node = 0; /* Should be -1 */
> > 
> > Why is this not -1?
> 
> Because there is no code in rc3 that handles -1 in kmalloc_node.
> 
> If you add a patch that handles it then feel free to change.
> But fixing the bootup has the highest priority.
> 
> > 
> > > +		int node = 0; 
> > > +		if (hwif->drives[0].hwif) { 
> > 
> > Also needs to be -1.
> 
> Then it would crash again.

The patch follows. Maybe add that and include a signoff by me?:

Index: linux-2.6.git/mm/slab.c
===================================================================
--- linux-2.6.git.orig/mm/slab.c	2005-07-05 17:03:30.000000000 -0700
+++ linux-2.6.git/mm/slab.c	2005-07-05 18:25:20.000000000 -0700
@@ -2372,6 +2372,9 @@
 	struct slab *slabp;
 	kmem_bufctl_t next;
 
+	if (nodeid == -1)
+		return kmem_cache_alloc_node(cachep, flags);
+
 	for (loop = 0;;loop++) {
 		struct list_head *q;
 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-06 14:35     ` Bartlomiej Zolnierkiewicz
@ 2005-07-06 18:21       ` Andi Kleen
  0 siblings, 0 replies; 24+ messages in thread
From: Andi Kleen @ 2005-07-06 18:21 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz; +Cc: linux-ide, torvalds, linux-kernel, christoph

On Wed, 6 Jul 2005 16:35:11 +0200
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> On 7/6/05, Andi Kleen <ak@suse.de> wrote:
> > > drive->hwif check is redundant, please remove it
> > 
> > It's not. My first version didn't have it but it still crashed.
> > It's what actually prevents the crash.
> > I also don't know why, but it's true.
> 
> very weird as HWIF(drive) == drive->hwif:
> 
> 	ide_hwif_t *hwif = HWIF(drive);
> 
> ...
> 
> 	q = blk_init_queue_node(do_ide_request, &ide_lock,
> 				pcibus_to_node(drive->hwif->pci_dev->bus));
                                               ^^^^^^^^^^^^^^

I oops on that one. Maybe it takes the early return. 

> 	if (!q)
> 		return 1;
> 
> ...
> 
> 	if (!hwif->rqsize) {
> 
> you should OOPS here


I would appreciate if some form of this patch is merged asap because it breaks
booting on my test machines.

-Andi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-06 13:30 [PATCH] Fix crash on boot in kmalloc_node IDE changes Andi Kleen
  2005-07-06 14:05 ` Bartlomiej Zolnierkiewicz
  2005-07-06 16:34 ` Christoph Lameter
@ 2005-07-07 16:21 ` Christoph Lameter
  2005-07-07 16:24   ` Andi Kleen
  2005-07-07 16:38   ` Linus Torvalds
  2 siblings, 2 replies; 24+ messages in thread
From: Christoph Lameter @ 2005-07-07 16:21 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-ide, torvalds, linux-kernel

On Wed, 6 Jul 2005, Andi Kleen wrote:

> Without this patch a dual Xeon EM64T machine would oops on boot
> because the hwif pointer here was NULL. I also added a check for
> pci_dev because it's doubtful that all IDE devices have pci_devs.

Here is IMHO the right way to fix this. Test for the hwif != NULL and
test for pci_dev != NULL before determining the node number of the pci 
bus that the device is connected to. Maybe we need a hwif_to_node for ide 
drivers that is also able to determine the locality of other hardware?

Signed-off-by: Christoph Lameter <christoph@lameter.com>

Index: linux-2.6.git/drivers/ide/ide-probe.c
===================================================================
--- linux-2.6.git.orig/drivers/ide/ide-probe.c	2005-06-23 11:38:02.000000000 -0700
+++ linux-2.6.git/drivers/ide/ide-probe.c	2005-07-07 09:15:04.000000000 -0700
@@ -979,7 +979,8 @@
 	 */
 
 	q = blk_init_queue_node(do_ide_request, &ide_lock,
-				pcibus_to_node(drive->hwif->pci_dev->bus));
+				(drive->hwif && drive->hwif->pci_dev) ?
+				pcibus_to_node(drive->hwif->pci_dev->bus): -1);
 	if (!q)
 		return 1;
 
@@ -1097,7 +1098,8 @@
 		spin_unlock_irq(&ide_lock);
 	} else {
 		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
-			pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus));
+			(hwif->drives[0].hwif && hwif->drives[0].hwif->pci_dev) ?
+			pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus) : -1);
 		if (!hwgroup)
 	       		goto out_up;
 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 16:21 ` [another PATCH] " Christoph Lameter
@ 2005-07-07 16:24   ` Andi Kleen
  2005-07-07 16:32     ` Christoph Lameter
  2005-07-07 16:38   ` Linus Torvalds
  1 sibling, 1 reply; 24+ messages in thread
From: Andi Kleen @ 2005-07-07 16:24 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Andi Kleen, linux-ide, torvalds, linux-kernel

On Thu, Jul 07, 2005 at 09:21:55AM -0700, Christoph Lameter wrote:
> On Wed, 6 Jul 2005, Andi Kleen wrote:
> 
> > Without this patch a dual Xeon EM64T machine would oops on boot
> > because the hwif pointer here was NULL. I also added a check for
> > pci_dev because it's doubtful that all IDE devices have pci_devs.
> 
> Here is IMHO the right way to fix this. Test for the hwif != NULL and
> test for pci_dev != NULL before determining the node number of the pci 
> bus that the device is connected to. Maybe we need a hwif_to_node for ide 
> drivers that is also able to determine the locality of other hardware?

Hmm? Where is the difference? 

This is 100% equivalent to my code except that you compressed
it all into a single expression.

The former one was more readable.

-Andi


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 16:24   ` Andi Kleen
@ 2005-07-07 16:32     ` Christoph Lameter
  2005-07-07 16:36       ` Andi Kleen
  2005-07-07 16:46       ` Bartlomiej Zolnierkiewicz
  0 siblings, 2 replies; 24+ messages in thread
From: Christoph Lameter @ 2005-07-07 16:32 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-ide, torvalds, linux-kernel

On Thu, 7 Jul 2005, Andi Kleen wrote:

> On Thu, Jul 07, 2005 at 09:21:55AM -0700, Christoph Lameter wrote:
> > On Wed, 6 Jul 2005, Andi Kleen wrote:
> > 
> > > Without this patch a dual Xeon EM64T machine would oops on boot
> > > because the hwif pointer here was NULL. I also added a check for
> > > pci_dev because it's doubtful that all IDE devices have pci_devs.
> > 
> > Here is IMHO the right way to fix this. Test for the hwif != NULL and
> > test for pci_dev != NULL before determining the node number of the pci 
> > bus that the device is connected to. Maybe we need a hwif_to_node for ide 
> > drivers that is also able to determine the locality of other hardware?
> 
> Hmm? Where is the difference? 

node = -1 if the node cannot be determined.

> This is 100% equivalent to my code except that you compressed
> it all into a single expression.

My patch consistently checks for hwif != NULL and pci_dev != NULL. 
There was someother stuff in your patch. This patch does not add any 
additional variables and is more readable.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 16:32     ` Christoph Lameter
@ 2005-07-07 16:36       ` Andi Kleen
  2005-07-07 17:15         ` Christoph Lameter
  2005-07-07 16:46       ` Bartlomiej Zolnierkiewicz
  1 sibling, 1 reply; 24+ messages in thread
From: Andi Kleen @ 2005-07-07 16:36 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Andi Kleen, linux-ide, torvalds, linux-kernel

On Thu, Jul 07, 2005 at 09:32:51AM -0700, Christoph Lameter wrote:
> On Thu, 7 Jul 2005, Andi Kleen wrote:
> 
> > On Thu, Jul 07, 2005 at 09:21:55AM -0700, Christoph Lameter wrote:
> > > On Wed, 6 Jul 2005, Andi Kleen wrote:
> > > 
> > > > Without this patch a dual Xeon EM64T machine would oops on boot
> > > > because the hwif pointer here was NULL. I also added a check for
> > > > pci_dev because it's doubtful that all IDE devices have pci_devs.
> > > 
> > > Here is IMHO the right way to fix this. Test for the hwif != NULL and
> > > test for pci_dev != NULL before determining the node number of the pci 
> > > bus that the device is connected to. Maybe we need a hwif_to_node for ide 
> > > drivers that is also able to determine the locality of other hardware?
> > 
> > Hmm? Where is the difference? 
> 
> node = -1 if the node cannot be determined.

But that will crash right now.

Trading one crash for another doesn't seem to be particularly useful.


> > This is 100% equivalent to my code except that you compressed
> > it all into a single expression.
> 
> My patch consistently checks for hwif != NULL and pci_dev != NULL. 
> There was someother stuff in your patch. This patch does not add any 
> additional variables and is more readable.

No it was exactly the same except for a working node number.

-Andi


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 16:21 ` [another PATCH] " Christoph Lameter
  2005-07-07 16:24   ` Andi Kleen
@ 2005-07-07 16:38   ` Linus Torvalds
  2005-07-07 16:40     ` Andi Kleen
  2005-07-07 17:23     ` Christoph Lameter
  1 sibling, 2 replies; 24+ messages in thread
From: Linus Torvalds @ 2005-07-07 16:38 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Andi Kleen, linux-ide, linux-kernel



On Thu, 7 Jul 2005, Christoph Lameter wrote:
> 
> Here is IMHO the right way to fix this. Test for the hwif != NULL and
> test for pci_dev != NULL before determining the node number of the pci 
> bus that the device is connected to.

I think this is pretty unreadable.

If you make it use a trivial inline function for the thing, I think that 
would be ok, though.

Complex expressions are bad.

		Linus

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 16:38   ` Linus Torvalds
@ 2005-07-07 16:40     ` Andi Kleen
  2005-07-07 17:23     ` Christoph Lameter
  1 sibling, 0 replies; 24+ messages in thread
From: Andi Kleen @ 2005-07-07 16:40 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Christoph Lameter, Andi Kleen, linux-ide, linux-kernel

On Thu, Jul 07, 2005 at 09:38:17AM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 7 Jul 2005, Christoph Lameter wrote:
> > 
> > Here is IMHO the right way to fix this. Test for the hwif != NULL and
> > test for pci_dev != NULL before determining the node number of the pci 
> > bus that the device is connected to.
> 
> I think this is pretty unreadable.
> 
> If you make it use a trivial inline function for the thing, I think that 
> would be ok, though.
> 
> Complex expressions are bad.

You could just use the previous patch I posted (hint hint...;) 

-Andi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 16:32     ` Christoph Lameter
  2005-07-07 16:36       ` Andi Kleen
@ 2005-07-07 16:46       ` Bartlomiej Zolnierkiewicz
  1 sibling, 0 replies; 24+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2005-07-07 16:46 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Andi Kleen, linux-ide, torvalds, linux-kernel

On 7/7/05, Christoph Lameter <christoph@lameter.com> wrote:
> On Thu, 7 Jul 2005, Andi Kleen wrote:
> 
> > On Thu, Jul 07, 2005 at 09:21:55AM -0700, Christoph Lameter wrote:
> > > On Wed, 6 Jul 2005, Andi Kleen wrote:
> > >
> > > > Without this patch a dual Xeon EM64T machine would oops on boot
> > > > because the hwif pointer here was NULL. I also added a check for
> > > > pci_dev because it's doubtful that all IDE devices have pci_devs.
> > >
> > > Here is IMHO the right way to fix this. Test for the hwif != NULL and
> > > test for pci_dev != NULL before determining the node number of the pci
> > > bus that the device is connected to. Maybe we need a hwif_to_node for ide
> > > drivers that is also able to determine the locality of other hardware?
> >
> > Hmm? Where is the difference?
> 
> node = -1 if the node cannot be determined.
> 
> > This is 100% equivalent to my code except that you compressed
> > it all into a single expression.
> 
> My patch consistently checks for hwif != NULL and pci_dev != NULL.
> There was someother stuff in your patch. This patch does not add any
> additional variables and is more readable.

seconded but hwif != NULL still just hides some other issue

* hwifs and drives are not allocated dynamically
* drive->hwif is initialized to hwif for every hwif very early in the
driver initialization

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 16:36       ` Andi Kleen
@ 2005-07-07 17:15         ` Christoph Lameter
  0 siblings, 0 replies; 24+ messages in thread
From: Christoph Lameter @ 2005-07-07 17:15 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-ide, torvalds, linux-kernel

On Thu, 7 Jul 2005, Andi Kleen wrote:

> > node = -1 if the node cannot be determined.
> 
> But that will crash right now.

That was fixed. Have a look at the git logs.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 16:38   ` Linus Torvalds
  2005-07-07 16:40     ` Andi Kleen
@ 2005-07-07 17:23     ` Christoph Lameter
  2005-07-07 17:31       ` Jens Axboe
  2005-07-07 18:57       ` Linus Torvalds
  1 sibling, 2 replies; 24+ messages in thread
From: Christoph Lameter @ 2005-07-07 17:23 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Andi Kleen, linux-ide, linux-kernel

On Thu, 7 Jul 2005, Linus Torvalds wrote:

> If you make it use a trivial inline function for the thing, I think that 
> would be ok, though.

Like this?

Index: linux-2.6.git/drivers/ide/ide-probe.c
===================================================================
--- linux-2.6.git.orig/drivers/ide/ide-probe.c	2005-06-23 11:38:02.000000000 -0700
+++ linux-2.6.git/drivers/ide/ide-probe.c	2005-07-07 10:22:02.000000000 -0700
@@ -960,6 +960,15 @@
 }
 #endif /* MAX_HWIFS > 1 */
 
+static inline int hwif_to_node(ide_hwif_t *hwif)
+{
+	if (hwif && hwif->pci_dev)
+		return pcibus_to_node(hwif->pci_dev->bus);
+	else
+		/* Add ways to determine the node of other busses here */
+		return -1;
+}
+
 /*
  * init request queue
  */
@@ -978,8 +987,7 @@
 	 *	do not.
 	 */
 
-	q = blk_init_queue_node(do_ide_request, &ide_lock,
-				pcibus_to_node(drive->hwif->pci_dev->bus));
+	q = blk_init_queue_node(do_ide_request, &ide_lock, hwif_to_node(hwif));
 	if (!q)
 		return 1;
 
@@ -1097,7 +1105,7 @@
 		spin_unlock_irq(&ide_lock);
 	} else {
 		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
-			pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus));
+					hwif_to_node(hwif->drives[0].hwif));
 		if (!hwgroup)
 	       		goto out_up;
 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 17:23     ` Christoph Lameter
@ 2005-07-07 17:31       ` Jens Axboe
  2005-07-07 18:57       ` Linus Torvalds
  1 sibling, 0 replies; 24+ messages in thread
From: Jens Axboe @ 2005-07-07 17:31 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Linus Torvalds, Andi Kleen, linux-ide, linux-kernel

On Thu, Jul 07 2005, Christoph Lameter wrote:
> On Thu, 7 Jul 2005, Linus Torvalds wrote:
> 
> > If you make it use a trivial inline function for the thing, I think that 
> > would be ok, though.
> 
> Like this?
> 
> Index: linux-2.6.git/drivers/ide/ide-probe.c
> ===================================================================
> --- linux-2.6.git.orig/drivers/ide/ide-probe.c	2005-06-23 11:38:02.000000000 -0700
> +++ linux-2.6.git/drivers/ide/ide-probe.c	2005-07-07 10:22:02.000000000 -0700
> @@ -960,6 +960,15 @@
>  }
>  #endif /* MAX_HWIFS > 1 */
>  
> +static inline int hwif_to_node(ide_hwif_t *hwif)
> +{
> +	if (hwif && hwif->pci_dev)
> +		return pcibus_to_node(hwif->pci_dev->bus);
> +	else
> +		/* Add ways to determine the node of other busses here */
> +		return -1;
> +}

When is hwif ever NULL?

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 17:23     ` Christoph Lameter
  2005-07-07 17:31       ` Jens Axboe
@ 2005-07-07 18:57       ` Linus Torvalds
  2005-07-07 19:09         ` Christoph Lameter
  1 sibling, 1 reply; 24+ messages in thread
From: Linus Torvalds @ 2005-07-07 18:57 UTC (permalink / raw)
  To: Christoph Lameter, Bartlomiej Zolnierkiewicz
  Cc: Andi Kleen, linux-ide, Linux Kernel Mailing List



On Thu, 7 Jul 2005, Christoph Lameter wrote:
>
> On Thu, 7 Jul 2005, Linus Torvalds wrote:
> 
> > If you make it use a trivial inline function for the thing, I think that 
> > would be ok, though.
> 
> Like this?

Yes. Except that if hwif is NULL, we'll have other oopses since we access 
that in other places.

Why _is_ hwif NULL anyway? That's another, unrelated thing, and should 
probably have a separate check and an early return.

So there's two unrelated issues: hwif being NULL is a serious error
regardless of any node issues, and pci_dev being NULL just means we aren't
a PCI device.

Bartlomiej?

		Linus

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 18:57       ` Linus Torvalds
@ 2005-07-07 19:09         ` Christoph Lameter
  2005-07-07 21:15           ` Andi Kleen
  0 siblings, 1 reply; 24+ messages in thread
From: Christoph Lameter @ 2005-07-07 19:09 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Bartlomiej Zolnierkiewicz, Andi Kleen, linux-ide,
	Linux Kernel Mailing List

On Thu, 7 Jul 2005, Linus Torvalds wrote:

> Yes. Except that if hwif is NULL, we'll have other oopses since we access 
> that in other places.
> 
> Why _is_ hwif NULL anyway? That's another, unrelated thing, and should 
> probably have a separate check and an early return.

I was wondering about that one as well. Andi brought it up.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 19:09         ` Christoph Lameter
@ 2005-07-07 21:15           ` Andi Kleen
  2005-07-07 21:19             ` Bartlomiej Zolnierkiewicz
  0 siblings, 1 reply; 24+ messages in thread
From: Andi Kleen @ 2005-07-07 21:15 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Linus Torvalds, Bartlomiej Zolnierkiewicz, Andi Kleen, linux-ide,
	Linux Kernel Mailing List

On Thu, Jul 07, 2005 at 12:09:00PM -0700, Christoph Lameter wrote:
> On Thu, 7 Jul 2005, Linus Torvalds wrote:
> 
> > Yes. Except that if hwif is NULL, we'll have other oopses since we access 
> > that in other places.
> > 
> > Why _is_ hwif NULL anyway? That's another, unrelated thing, and should 
> > probably have a separate check and an early return.
> 
> I was wondering about that one as well. Andi brought it up.

I don't know why hwif was NULL, but my kernel definitely crashed.
hwif was NULL in the first function (I first misread the oops
and thought it was pci_dev NULL, but it wasn't).  For the second
I didn't verify it was hwif or pci_dev NULL, but one of them
was too.

The setup was a Intel board with 1 PATA/4 SATA onboard and only a CD-ROM 
and a external Promise PATA controller with two PATA disks.

-Andi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 21:15           ` Andi Kleen
@ 2005-07-07 21:19             ` Bartlomiej Zolnierkiewicz
  2005-07-07 21:25               ` Andi Kleen
  0 siblings, 1 reply; 24+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2005-07-07 21:19 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Christoph Lameter, Linus Torvalds, linux-ide,
	Linux Kernel Mailing List

On 7/7/05, Andi Kleen <ak@suse.de> wrote:
> On Thu, Jul 07, 2005 at 12:09:00PM -0700, Christoph Lameter wrote:
> > On Thu, 7 Jul 2005, Linus Torvalds wrote:
> >
> > > Yes. Except that if hwif is NULL, we'll have other oopses since we access
> > > that in other places.
> > >
> > > Why _is_ hwif NULL anyway? That's another, unrelated thing, and should
> > > probably have a separate check and an early return.
> >
> > I was wondering about that one as well. Andi brought it up.
> 
> I don't know why hwif was NULL, but my kernel definitely crashed.
> hwif was NULL in the first function (I first misread the oops
> and thought it was pci_dev NULL, but it wasn't).  For the second
> I didn't verify it was hwif or pci_dev NULL, but one of them
> was too.
> 
> The setup was a Intel board with 1 PATA/4 SATA onboard and only a CD-ROM
> and a external Promise PATA controller with two PATA disks.

actual OOPS would be very useful

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 21:19             ` Bartlomiej Zolnierkiewicz
@ 2005-07-07 21:25               ` Andi Kleen
  2005-07-08  4:44                 ` Christoph Lameter
  0 siblings, 1 reply; 24+ messages in thread
From: Andi Kleen @ 2005-07-07 21:25 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: Andi Kleen, Christoph Lameter, Linus Torvalds, linux-ide,
	Linux Kernel Mailing List

> > The setup was a Intel board with 1 PATA/4 SATA onboard and only a CD-ROM
> > and a external Promise PATA controller with two PATA disks.
> 
> actual OOPS would be very useful

It's difficult because I don't have serial on that machine.

-Andi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [another PATCH] Fix crash on boot in kmalloc_node IDE changes
  2005-07-07 21:25               ` Andi Kleen
@ 2005-07-08  4:44                 ` Christoph Lameter
  0 siblings, 0 replies; 24+ messages in thread
From: Christoph Lameter @ 2005-07-08  4:44 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Bartlomiej Zolnierkiewicz, Linus Torvalds, linux-ide,
	Linux Kernel Mailing List

On Thu, 7 Jul 2005, Andi Kleen wrote:

> > > The setup was a Intel board with 1 PATA/4 SATA onboard and only a CD-ROM
> > > and a external Promise PATA controller with two PATA disks.
> > 
> > actual OOPS would be very useful
> 
> It's difficult because I don't have serial on that machine.

Maybe we can settle on this patch until we know for sure that the hwif 
is also a problem. In that case we can simply patch hwif_to_node:


Index: linux-2.6.git/drivers/ide/ide-probe.c
===================================================================
--- linux-2.6.git.orig/drivers/ide/ide-probe.c	2005-06-23 11:38:02.000000000 -0700
+++ linux-2.6.git/drivers/ide/ide-probe.c	2005-07-07 20:18:22.000000000 -0700
@@ -960,6 +960,15 @@
 }
 #endif /* MAX_HWIFS > 1 */
 
+static inline int hwif_to_node(ide_hwif_t *hwif)
+{
+	if (hwif->pci_dev)
+		return pcibus_to_node(hwif->pci_dev->bus);
+	else
+		/* Add ways to determine the node of other busses here */
+		return -1;
+}
+
 /*
  * init request queue
  */
@@ -978,8 +987,7 @@
 	 *	do not.
 	 */
 
-	q = blk_init_queue_node(do_ide_request, &ide_lock,
-				pcibus_to_node(drive->hwif->pci_dev->bus));
+	q = blk_init_queue_node(do_ide_request, &ide_lock, hwif_to_node(hwif));
 	if (!q)
 		return 1;
 
@@ -1097,7 +1105,7 @@
 		spin_unlock_irq(&ide_lock);
 	} else {
 		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
-			pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus));
+					hwif_to_node(hwif->drives[0].hwif));
 		if (!hwgroup)
 	       		goto out_up;
 

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2005-07-08  4:44 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-07-06 13:30 [PATCH] Fix crash on boot in kmalloc_node IDE changes Andi Kleen
2005-07-06 14:05 ` Bartlomiej Zolnierkiewicz
2005-07-06 14:09   ` Andi Kleen
2005-07-06 14:35     ` Bartlomiej Zolnierkiewicz
2005-07-06 18:21       ` Andi Kleen
2005-07-06 16:34 ` Christoph Lameter
2005-07-06 16:45   ` Andi Kleen
2005-07-06 17:07     ` Christoph Lameter
2005-07-07 16:21 ` [another PATCH] " Christoph Lameter
2005-07-07 16:24   ` Andi Kleen
2005-07-07 16:32     ` Christoph Lameter
2005-07-07 16:36       ` Andi Kleen
2005-07-07 17:15         ` Christoph Lameter
2005-07-07 16:46       ` Bartlomiej Zolnierkiewicz
2005-07-07 16:38   ` Linus Torvalds
2005-07-07 16:40     ` Andi Kleen
2005-07-07 17:23     ` Christoph Lameter
2005-07-07 17:31       ` Jens Axboe
2005-07-07 18:57       ` Linus Torvalds
2005-07-07 19:09         ` Christoph Lameter
2005-07-07 21:15           ` Andi Kleen
2005-07-07 21:19             ` Bartlomiej Zolnierkiewicz
2005-07-07 21:25               ` Andi Kleen
2005-07-08  4:44                 ` Christoph Lameter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).