[PATCH]: Fix assertion failure with MSI on sparc64

[PATCH]: Fix assertion failure with MSI on sparc64

[David Miller]

Hi Michael, I'm still working through the various regressions on
sparc64 added by your MSI changes :-)

The one I fixed the other day was a missed switch over to
alloc_pci_dev() in the sparc64 PCI probing code which caused an OOPS
in pci_enable_msi() because the list head of the pci dev was not
initialized.  PowerPC's OBP firmware tree based PCI probing code
was updated, sparc64's wasnt.

Today's find is a triggered assertion in msi_free_irqs() when the
system doesn't support MSI, in which case arch_setup_msi_irqs() always
returns an error.

The problem is that when this happens we branch into msi_free_irqs(),
to which you added the following assertion loop:

	list_for_each_entry(entry, &dev->msi_list, list)
		BUG_ON(irq_has_action(entry->irq));

Well, if arch_setup_msi_irqs() fails, entry->irq will be zero and
although that's never assigned to any normal devices we use that IRQ
number for the timer interrupt on sparc64 so this assertion triggers.

Better to test for zero before doing the irq_has_action() assertion
thing.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index e6740d1..d9cbd58 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -549,8 +549,10 @@ static int msi_free_irqs(struct pci_dev* dev)
 {
 	struct msi_desc *entry, *tmp;
 
-	list_for_each_entry(entry, &dev->msi_list, list)
-		BUG_ON(irq_has_action(entry->irq));
+	list_for_each_entry(entry, &dev->msi_list, list) {
+		if (entry->irq)
+			BUG_ON(irq_has_action(entry->irq));
+	}
 
 	arch_teardown_msi_irqs(dev);
 

Re: [PATCH]: Fix assertion failure with MSI on sparc64

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2437 bytes --]

On Fri, 2007-05-11 at 13:26 -0700, David Miller wrote:
> Hi Michael, I'm still working through the various regressions on
> sparc64 added by your MSI changes :-)

Hi Dave,

Guilty as charged - I did CC you on the patches though ;)

> The one I fixed the other day was a missed switch over to
> alloc_pci_dev() in the sparc64 PCI probing code which caused an OOPS
> in pci_enable_msi() because the list head of the pci dev was not
> initialized.  PowerPC's OBP firmware tree based PCI probing code
> was updated, sparc64's wasnt.

Sorry - not sure how I missed that one, it even matches
"k.alloc(.*pci_dev" - thanks for fixing it :)

> Today's find is a triggered assertion in msi_free_irqs() when the
> system doesn't support MSI, in which case arch_setup_msi_irqs() always
> returns an error.

What do you need to determine that the system can't support MSI? Could
you do that logic in arch_msi_check_device()?

> The problem is that when this happens we branch into msi_free_irqs(),
> to which you added the following assertion loop:
> 
> 	list_for_each_entry(entry, &dev->msi_list, list)
> 		BUG_ON(irq_has_action(entry->irq));
> 
> Well, if arch_setup_msi_irqs() fails, entry->irq will be zero and
> although that's never assigned to any normal devices we use that IRQ
> number for the timer interrupt on sparc64 so this assertion triggers.
> 
> Better to test for zero before doing the irq_has_action() assertion
> thing.

Yep, looks good - it matches the logic in arch_teardown_msi_irqs().

cheers


Acked-by: Michael Ellerman <michael@ellerman.id.au>

> Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index e6740d1..d9cbd58 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -549,8 +549,10 @@ static int msi_free_irqs(struct pci_dev* dev)
>  {
>  	struct msi_desc *entry, *tmp;
>  
> -	list_for_each_entry(entry, &dev->msi_list, list)
> -		BUG_ON(irq_has_action(entry->irq));
> +	list_for_each_entry(entry, &dev->msi_list, list) {
> +		if (entry->irq)
> +			BUG_ON(irq_has_action(entry->irq));
> +	}
>  
>  	arch_teardown_msi_irqs(dev);
>  
-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]