AMD 8131 MSI quirk called too late, bus_flags not inherited ?

AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Brice Goglin]

Hi Greg,

While looking at the MSI detection, I noticed that the AMD 8131 quirk
(quirk_amd_8131_ioapic) is defined as FINAL and thus executed after the
PCI hierarchy is scanned. So it looks like the bus_flags won't be
inherited at all. If there's a bridge behind the 8131, then the devices
behind this bridge won't see the bus flags and thus might try to enable
MSI anyway.
I tried to change the AMD 8131 quirk to HEADER so that it is executed
during PCI scanning. But, I don't get its message in dmesg anymore. Any
idea?

Thanks,
Brice

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Greg KH]

On Mon, May 15, 2006 at 11:11:33PM +0200, Brice Goglin wrote:
> Hi Greg,
> 
> While looking at the MSI detection, I noticed that the AMD 8131 quirk
> (quirk_amd_8131_ioapic) is defined as FINAL and thus executed after the
> PCI hierarchy is scanned. So it looks like the bus_flags won't be
> inherited at all. If there's a bridge behind the 8131, then the devices
> behind this bridge won't see the bus flags and thus might try to enable
> MSI anyway.
> I tried to change the AMD 8131 quirk to HEADER so that it is executed
> during PCI scanning. But, I don't get its message in dmesg anymore. Any
> idea?

Michael is the one who added that change, perhaps he can explain how he
tested it?

thanks,

greg k-h

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Michael S. Tsirkin]

Quoting r. Greg KH <gregkh@suse.de>:
> Subject: Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?
> 
> On Mon, May 15, 2006 at 11:11:33PM +0200, Brice Goglin wrote:
> > Hi Greg,
> > 
> > While looking at the MSI detection, I noticed that the AMD 8131 quirk
> > (quirk_amd_8131_ioapic) is defined as FINAL and thus executed after the
> > PCI hierarchy is scanned. So it looks like the bus_flags won't be
> > inherited at all. If there's a bridge behind the 8131, then the devices
> > behind this bridge won't see the bus flags and thus might try to enable
> > MSI anyway.
> > I tried to change the AMD 8131 quirk to HEADER so that it is executed
> > during PCI scanning. But, I don't get its message in dmesg anymore. Any
> > idea?
> 
> Michael is the one who added that change, perhaps he can explain how he
> tested it?

Well, I just re-tested with 2.6.17-rc4 and it does not seem to work.  No idea
what did I do wrong when testing this patch before posting it :(. Oops.
I'm sorry.

Given that its late in -rc series, that its a clear regression from 2.6.16, that
disabling MSI is always safe, and that the patch was intended to enable MSI on
Tyan motherboard which Roland used to have but doesn't anymore and which no one
else seems to be bothered to test on either - it seems the best thing is to just
revert the patch, and go back to using a global flag for now.
I'll try to revisit this for 2.6.18 if I can get my hands on such a system.
Greg, what do you think?

It's 6e325a62a0a228cd0222783802b53cce04551776 in Linus' tree:
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=6e325a62a0a228cd0222783802b53cce04551776

Here's a minimal patch against 2.6.17-rc4 - I tested it by loading mthca with
msi_x=1.

---

Revert MSI quirk change in 6e325a62a0a228cd0222783802b53cce04551776 - it breaks 
regular AMD-8131 systems, and no one seems to care enough to test on Tyan
systems where it's supposed to improve performance.

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

Index: linux-2.6.17-rc4/drivers/pci/quirks.c
===================================================================
--- linux-2.6.17-rc4.orig/drivers/pci/quirks.c	2006-05-21 12:49:52.000000000 +0300
+++ linux-2.6.17-rc4/drivers/pci/quirks.c	2006-05-21 13:04:37.000000000 +0300
@@ -575,11 +575,8 @@ static void __init quirk_amd_8131_ioapic
 { 
         unsigned char revid, tmp;
         
-	if (dev->subordinate) {
-		printk(KERN_WARNING "PCI: MSI quirk detected. "
-		       "PCI_BUS_FLAGS_NO_MSI set for subordinate bus.\n");
-		dev->subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
-	}
+	pci_msi_quirk = 1;
+	printk(KERN_WARNING "PCI: MSI quirk detected. pci_msi_quirk set.\n");
 
         if (nr_ioapics == 0) 
                 return;

-- 
MST

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Brice Goglin]

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

Michael S. Tsirkin wrote:
> Quoting r. Greg KH <gregkh@suse.de>:
>> Subject: Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?
>>
>> On Mon, May 15, 2006 at 11:11:33PM +0200, Brice Goglin wrote:
>>> Hi Greg,
>>>
>>> While looking at the MSI detection, I noticed that the AMD 8131 quirk
>>> (quirk_amd_8131_ioapic) is defined as FINAL and thus executed after the
>>> PCI hierarchy is scanned. So it looks like the bus_flags won't be
>>> inherited at all. If there's a bridge behind the 8131, then the devices
>>> behind this bridge won't see the bus flags and thus might try to enable
>>> MSI anyway.
>>> I tried to change the AMD 8131 quirk to HEADER so that it is executed
>>> during PCI scanning. But, I don't get its message in dmesg anymore. Any
>>> idea?
>> Michael is the one who added that change, perhaps he can explain how he
>> tested it?
>
> Well, I just re-tested with 2.6.17-rc4 and it does not seem to work.  No idea
> what did I do wrong when testing this patch before posting it :(. Oops.
> I'm sorry.
>
> Given that its late in -rc series, that its a clear regression from 2.6.16, that
> disabling MSI is always safe, and that the patch was intended to enable MSI on
> Tyan motherboard which Roland used to have but doesn't anymore and which no one
> else seems to be bothered to test on either - it seems the best thing is to just
> revert the patch, and go back to using a global flag for now.

I would say that it works for devices that are directly behind the
amd8131. But it won't disable MSI for devices that are behind another
bridge behind the 8131. It is great for us since we have lots of Tyan
motherboards without any device behind the 8131 :) I don't know how many
machines have a bridge behind a 8131, this bug might not hit a lot of
people.

At least, with 2.6.17-rc, we can get MSI on devices that have nothing to
do with the 8131. I consider this a very good progress. Yes, we try to
enable MSI on devices behing a bridge behind the 8131. But, if few
people have such a PCI hierarchy, I am not sure this regression is
important. What happens if you actually enable MSI there ? It fails ? Or
it crashes ? Is it worth than having no MSI at all on any devices on the
machine ?

Anyway, we are working on merging our MSI detection code based on HT
capabilities. And we think we might have to add a couple other quirks
where it will be very important there that the bus flags are inherited.
So I tried to dig more in what we could do to inherit flags properly and
here's what I found:

The amd8131 MSI quirk cannot be EARLY or HEADER because it would be
called before dev->subordinate has been set. So PCI_BUS_FLAGS_NO_MSI
could not be set in the bus flags at this time.
So we need a quirk that is called while the PCI hierarchy is scanned
(ie, before FINAL or ENABLE) and after the pci child bus of the bridge
is created (ie, after EARLY and HEADER).

The attached patch adds a new type of fixup that is called right after
the child bus is created (ie, when dev->subordinate exists). And the
patch moves the amd8131 MSI quirk to use this new quirk type.
I chose the name "bridge header" but I guess somebody will come with a
better name (I am not really familiar with all the PCI terminology).

Signed-off-by: Brice Goglin <brice@myri.com>

Brice


[-- Attachment #2: fix_amd8131_nomsi_quirk.patch --]
[-- Type: text/x-patch, Size: 4234 bytes --]

Index: brice-2.6.17-rc4/drivers/pci/quirks.c
===================================================================
--- brice-2.6.17-rc4.orig/drivers/pci/quirks.c	2006-05-17 04:16:26.000000000 -0400
+++ brice-2.6.17-rc4/drivers/pci/quirks.c	2006-05-20 08:29:55.000000000 -0400
@@ -575,12 +575,6 @@
 { 
         unsigned char revid, tmp;
         
-	if (dev->subordinate) {
-		printk(KERN_WARNING "PCI: MSI quirk detected. "
-		       "PCI_BUS_FLAGS_NO_MSI set for subordinate bus.\n");
-		dev->subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
-	}
-
         if (nr_ioapics == 0) 
                 return;
 
@@ -594,6 +588,16 @@
 } 
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8131_BRIDGE, quirk_amd_8131_ioapic);
 
+static void __init quirk_amd_8131_nomsi(struct pci_dev *dev)
+{ 
+	if (dev->subordinate) {
+		printk(KERN_WARNING "PCI: MSI quirk detected. "
+		       "PCI_BUS_FLAGS_NO_MSI set for subordinate bus.\n");
+		dev->subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
+	}
+}
+DECLARE_PCI_FIXUP_BRIDGE_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8131_BRIDGE, quirk_amd_8131_nomsi);
+
 static void __init quirk_svw_msi(struct pci_dev *dev)
 {
 	pci_msi_quirk = 1;
@@ -1407,6 +1411,8 @@
 extern struct pci_fixup __end_pci_fixups_early[];
 extern struct pci_fixup __start_pci_fixups_header[];
 extern struct pci_fixup __end_pci_fixups_header[];
+extern struct pci_fixup __start_pci_fixups_bridge_header[];
+extern struct pci_fixup __end_pci_fixups_bridge_header[];
 extern struct pci_fixup __start_pci_fixups_final[];
 extern struct pci_fixup __end_pci_fixups_final[];
 extern struct pci_fixup __start_pci_fixups_enable[];
@@ -1428,6 +1434,11 @@
 		end = __end_pci_fixups_header;
 		break;
 
+	case pci_fixup_bridge_header:
+		start = __start_pci_fixups_bridge_header;
+		end = __end_pci_fixups_bridge_header;
+		break;
+
 	case pci_fixup_final:
 		start = __start_pci_fixups_final;
 		end = __end_pci_fixups_final;
Index: brice-2.6.17-rc4/drivers/pci/probe.c
===================================================================
--- brice-2.6.17-rc4.orig/drivers/pci/probe.c	2006-05-17 04:16:29.000000000 -0400
+++ brice-2.6.17-rc4/drivers/pci/probe.c	2006-05-20 08:20:30.000000000 -0400
@@ -367,6 +367,7 @@
 		child->resource[i]->name = child->name;
 	}
 	bridge->subordinate = child;
+	pci_fixup_device(pci_fixup_bridge_header, bridge);
 
 	return child;
 }
Index: brice-2.6.17-rc4/include/asm-generic/vmlinux.lds.h
===================================================================
--- brice-2.6.17-rc4.orig/include/asm-generic/vmlinux.lds.h	2006-05-12 16:49:03.000000000 -0400
+++ brice-2.6.17-rc4/include/asm-generic/vmlinux.lds.h	2006-05-20 08:39:42.000000000 -0400
@@ -29,6 +29,9 @@
 		VMLINUX_SYMBOL(__start_pci_fixups_header) = .;		\
 		*(.pci_fixup_header)					\
 		VMLINUX_SYMBOL(__end_pci_fixups_header) = .;		\
+		VMLINUX_SYMBOL(__start_pci_fixups_bridge_header) = .;	\
+		*(.pci_fixup_bridge_header)				\
+		VMLINUX_SYMBOL(__end_pci_fixups_bridge_header) = .;	\
 		VMLINUX_SYMBOL(__start_pci_fixups_final) = .;		\
 		*(.pci_fixup_final)					\
 		VMLINUX_SYMBOL(__end_pci_fixups_final) = .;		\
Index: brice-2.6.17-rc4/include/linux/pci.h
===================================================================
--- brice-2.6.17-rc4.orig/include/linux/pci.h	2006-05-17 04:19:53.000000000 -0400
+++ brice-2.6.17-rc4/include/linux/pci.h	2006-05-20 08:01:24.000000000 -0400
@@ -750,6 +750,7 @@
 enum pci_fixup_pass {
 	pci_fixup_early,	/* Before probing BARs */
 	pci_fixup_header,	/* After reading configuration header */
+	pci_fixup_bridge_header,	/* After setting up the bridge subordinate */
 	pci_fixup_final,	/* Final phase of device fixups */
 	pci_fixup_enable,	/* pci_enable_device() time */
 };
@@ -764,6 +765,9 @@
 #define DECLARE_PCI_FIXUP_HEADER(vendor, device, hook)			\
 	DECLARE_PCI_FIXUP_SECTION(.pci_fixup_header,			\
 			vendor##device##hook, vendor, device, hook)
+#define DECLARE_PCI_FIXUP_BRIDGE_HEADER(vendor, device, hook)		\
+	DECLARE_PCI_FIXUP_SECTION(.pci_fixup_bridge_header,		\
+			vendor##device##hook, vendor, device, hook)
 #define DECLARE_PCI_FIXUP_FINAL(vendor, device, hook)			\
 	DECLARE_PCI_FIXUP_SECTION(.pci_fixup_final,			\
 			vendor##device##hook, vendor, device, hook)

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Michael S. Tsirkin]

Quoting r. Brice Goglin <brice@myri.com>:
> Subject: Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?
> 
> Michael S. Tsirkin wrote:
> > Quoting r. Greg KH <gregkh@suse.de>:
> >> Subject: Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?
> >>
> >> On Mon, May 15, 2006 at 11:11:33PM +0200, Brice Goglin wrote:
> >>> Hi Greg,
> >>>
> >>> While looking at the MSI detection, I noticed that the AMD 8131 quirk
> >>> (quirk_amd_8131_ioapic) is defined as FINAL and thus executed after the
> >>> PCI hierarchy is scanned. So it looks like the bus_flags won't be
> >>> inherited at all. If there's a bridge behind the 8131, then the devices
> >>> behind this bridge won't see the bus flags and thus might try to enable
> >>> MSI anyway.
> >>> I tried to change the AMD 8131 quirk to HEADER so that it is executed
> >>> during PCI scanning. But, I don't get its message in dmesg anymore. Any
> >>> idea?
> >> Michael is the one who added that change, perhaps he can explain how he
> >> tested it?
> >
> > Well, I just re-tested with 2.6.17-rc4 and it does not seem to work.  No idea
> > what did I do wrong when testing this patch before posting it :(. Oops.
> > I'm sorry.
> >
> > Given that its late in -rc series, that its a clear regression from 2.6.16, that
> > disabling MSI is always safe, and that the patch was intended to enable MSI on
> > Tyan motherboard which Roland used to have but doesn't anymore and which no one
> > else seems to be bothered to test on either - it seems the best thing is to just
> > revert the patch, and go back to using a global flag for now.
> 
> I would say that it works for devices that are directly behind the
> amd8131. But it won't disable MSI for devices that are behind another
> bridge behind the 8131. It is great for us since we have lots of Tyan
> motherboards without any device behind the 8131 :)

MSI is an optional feature so things are supposed to work even without MSI - are
you getting that great a benefit from MSI?

> I don't know how many
> machines have a bridge behind a 8131, this bug might not hit a lot of
> people.

All mellanox PCI-X devices have a bridge inside them, so ...

> At least, with 2.6.17-rc, we can get MSI on devices that have nothing to
> do with the 8131. I consider this a very good progress. Yes, we try to
> enable MSI on devices behing a bridge behind the 8131. But, if few
> people have such a PCI hierarchy, I am not sure this regression is
> important. What happens if you actually enable MSI there ? It fails ? Or
> it crashes ?

It fails to generate interrupts.

> Is it worth than having no MSI at all on any devices on the
> machine ?

I think it is.

> Anyway, we are working on merging our MSI detection code based on HT
> capabilities. And we think we might have to add a couple other quirks
> where it will be very important there that the bus flags are inherited.
> So I tried to dig more in what we could do to inherit flags properly and
> here's what I found:
> 
> The amd8131 MSI quirk cannot be EARLY or HEADER because it would be
> called before dev->subordinate has been set. So PCI_BUS_FLAGS_NO_MSI
> could not be set in the bus flags at this time.
> So we need a quirk that is called while the PCI hierarchy is scanned
> (ie, before FINAL or ENABLE) and after the pci child bus of the bridge
> is created (ie, after EARLY and HEADER).
> 
> The attached patch adds a new type of fixup that is called right after
> the child bus is created (ie, when dev->subordinate exists). And the
> patch moves the amd8131 MSI quirk to use this new quirk type.
> I chose the name "bridge header" but I guess somebody will come with a
> better name (I am not really familiar with all the PCI terminology).
> 
> Signed-off-by: Brice Goglin <brice@myri.com>
> 
> Brice

Doesn't seem to work for me:

ib_mthca: Initializing 0000:04:00.0
GSI 18 sharing vector 0xB9 and IRQ 18
ACPI: PCI Interrupt 0000:04:00.0[A] -> GSI 29 (level, low) -> IRQ 185
ib_mthca 0000:04:00.0: NOP command failed to generate interrupt (IRQ 217),
aborting.
ib_mthca 0000:04:00.0: Try again with MSI/MSI-X disabled.
ACPI: PCI interrupt for device 0000:04:00.0 disabled
ib_mthca: probe of 0000:04:00.0 failed with error -16

-- 
MST

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Brice Goglin]

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

Michael S. Tsirkin wrote:
> MSI is an optional feature so things are supposed to work even without MSI - are
> you getting that great a benefit from MSI?
>   

Not great, I would say small.

> All mellanox PCI-X devices have a bridge inside them, so ...
>   

Ok so you really need something for 2.6.17. What about the attached
patch to fix the fact that bus flags are not inherited ?

Signed-off-by: Brice Goglin <brice@myri.com>

> Doesn't seem to work for me:
>
> ib_mthca: Initializing 0000:04:00.0
> GSI 18 sharing vector 0xB9 and IRQ 18
> ACPI: PCI Interrupt 0000:04:00.0[A] -> GSI 29 (level, low) -> IRQ 185
> ib_mthca 0000:04:00.0: NOP command failed to generate interrupt (IRQ 217),
> aborting.
> ib_mthca 0000:04:00.0: Try again with MSI/MSI-X disabled.
> ACPI: PCI interrupt for device 0000:04:00.0 disabled
> ib_mthca: probe of 0000:04:00.0 failed with error -16
>   

Ok. Do you at least see the quirk message ?

Thanks,
Brice


[-- Attachment #2: look_at_parent_busses_flags.patch --]
[-- Type: text/x-patch, Size: 650 bytes --]

Index: linux-mm/drivers/pci/msi.c
===================================================================
--- linux-mm.orig/drivers/pci/msi.c	2006-05-21 14:25:53.000000000 +0200
+++ linux-mm/drivers/pci/msi.c	2006-05-21 14:26:56.000000000 +0200
@@ -916,6 +916,7 @@
  **/
 int pci_enable_msi(struct pci_dev* dev)
 {
+	struct pci_bus *bus;
 	int pos, temp, status = -EINVAL;
 	u16 control;
 
@@ -925,8 +926,9 @@
 	if (dev->no_msi)
 		return status;
 
-	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
-		return -EINVAL;
+	for (bus = dev->bus; bus; bus = bus->parent)
+		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+			return -EINVAL;
 
 	temp = dev->irq;
 

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Michael S. Tsirkin]

Quoting r. Brice Goglin <brice@myri.com>:
> Subject: Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?
> 
> Michael S. Tsirkin wrote:
> > MSI is an optional feature so things are supposed to work even without MSI -
> > are you getting that great a benefit from MSI?
> >   
> 
> Not great, I would say small.

Me too.

> > All mellanox PCI-X devices have a bridge inside them, so ...
> >   
> 
> Ok so you really need something for 2.6.17. What about the attached
> patch to fix the fact that bus flags are not inherited ?
> 
> Signed-off-by: Brice Goglin <brice@myri.com>

Seems to work for MSI but not for MSI-X. With MSI-X, I still see:

ib_mthca 0000:04:00.0: NOP command failed to generate interrupt (IRQ 217),
aborting.
ib_mthca 0000:04:00.0: Try again with MSI/MSI-X disabled.


> > Doesn't seem to work for me:
> >
> > ib_mthca: Initializing 0000:04:00.0
> > GSI 18 sharing vector 0xB9 and IRQ 18
> > ACPI: PCI Interrupt 0000:04:00.0[A] -> GSI 29 (level, low) -> IRQ 185
> > ib_mthca 0000:04:00.0: NOP command failed to generate interrupt (IRQ 217),
> > aborting.
> > ib_mthca 0000:04:00.0: Try again with MSI/MSI-X disabled.
> > ACPI: PCI interrupt for device 0000:04:00.0 disabled
> > ib_mthca: probe of 0000:04:00.0 failed with error -16
> >   
> 
> Ok. Do you at least see the quirk message ?

Yes.

> Thanks,
> Brice
> 
> 
> Index: linux-mm/drivers/pci/msi.c
> ===================================================================
> --- linux-mm.orig/drivers/pci/msi.c	2006-05-21 14:25:53.000000000 +0200
> +++ linux-mm/drivers/pci/msi.c	2006-05-21 14:26:56.000000000 +0200
> @@ -916,6 +916,7 @@
>   **/
>  int pci_enable_msi(struct pci_dev* dev)
>  {
> +	struct pci_bus *bus;
>  	int pos, temp, status = -EINVAL;
>  	u16 control;
>  
> @@ -925,8 +926,9 @@
>  	if (dev->no_msi)
>  		return status;
>  
> -	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> -		return -EINVAL;
> +	for (bus = dev->bus; bus; bus = bus->parent)
> +		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> +			return -EINVAL;
>  
>  	temp = dev->irq;
>  
> 
> 

It seems we must add this loop to pci_enable_msix as well.

-- 
MST

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Brice Goglin]

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

Michael S. Tsirkin wrote:
>> @@ -925,8 +926,9 @@
>>  	if (dev->no_msi)
>>  		return status;
>>  
>> -	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
>> -		return -EINVAL;
>> +	for (bus = dev->bus; bus; bus = bus->parent)
>> +		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
>> +			return -EINVAL;
>>  
>>  	temp = dev->irq;
>>     
>
> It seems we must add this loop to pci_enable_msix as well.
>   

Right, thanks. Greg, what do you think of putting the attached patch in
2.6.17 ?

By the way, do we need to check dev->no_msi in pci_enable_msix() too ?

For 2.6.18, I don't know what's the best. We could drop the fact that
bus flags should be inherited and keep looking at parent busses. It
might be good to add a pci_check_flag_in_parent_busses(dev, flag) to
provide a generic way to do my for loop.

thanks,
Brice


Signed-off-by: Brice Goglin <brice@myri.com>


[-- Attachment #2: look_at_parent_busses_flags.patch --]
[-- Type: text/x-patch, Size: 1124 bytes --]

Index: linux-mm/drivers/pci/msi.c
===================================================================
--- linux-mm.orig/drivers/pci/msi.c	2006-05-21 15:12:04.000000000 +0200
+++ linux-mm/drivers/pci/msi.c	2006-05-21 15:15:34.000000000 +0200
@@ -916,6 +916,7 @@
  **/
 int pci_enable_msi(struct pci_dev* dev)
 {
+	struct pci_bus *bus;
 	int pos, temp, status = -EINVAL;
 	u16 control;
 
@@ -925,8 +926,9 @@
 	if (dev->no_msi)
 		return status;
 
-	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
-		return -EINVAL;
+	for (bus = dev->bus; bus; bus = bus->parent)
+		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+			return -EINVAL;
 
 	temp = dev->irq;
 
@@ -1162,6 +1164,7 @@
  **/
 int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int nvec)
 {
+	struct pci_bus *bus;
 	int status, pos, nr_entries, free_vectors;
 	int i, j, temp;
 	u16 control;
@@ -1170,6 +1173,10 @@
 	if (!pci_msi_enable || !dev || !entries)
  		return -EINVAL;
 
+	for (bus = dev->bus; bus; bus = bus->parent)
+		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+			return -EINVAL;
+
 	status = msi_init();
 	if (status < 0)
 		return status;

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Greg KH]

On Sun, May 21, 2006 at 03:24:07PM +0200, Brice Goglin wrote:
> Michael S. Tsirkin wrote:
> >> @@ -925,8 +926,9 @@
> >>  	if (dev->no_msi)
> >>  		return status;
> >>  
> >> -	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> >> -		return -EINVAL;
> >> +	for (bus = dev->bus; bus; bus = bus->parent)
> >> +		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> >> +			return -EINVAL;
> >>  
> >>  	temp = dev->irq;
> >>     
> >
> > It seems we must add this loop to pci_enable_msix as well.
> >   
> 
> Right, thanks. Greg, what do you think of putting the attached patch in
> 2.6.17 ?

Ok, does everyone agree that this patch fixes the issues for them?  I've
had a few other private emails saying that the current code doesn't work
properly and hadn't been able to determine what was happening.  Thanks
for these patches.

> By the way, do we need to check dev->no_msi in pci_enable_msix() too ?

Yes, good catch, care to respin the patch and give it a good changelog
entry?

thanks,

greg k-h

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Brice Goglin]

On Mon, May 22, 2006 at 09:19:58PM -0700, Greg KH wrote:
> On Sun, May 21, 2006 at 03:24:07PM +0200, Brice Goglin wrote:
> > 
> > Right, thanks. Greg, what do you think of putting the attached patch in
> > 2.6.17 ?
> 
> Ok, does everyone agree that this patch fixes the issues for them?  I've
> had a few other private emails saying that the current code doesn't work
> properly and hadn't been able to determine what was happening.  Thanks
> for these patches.
> 
> > By the way, do we need to check dev->no_msi in pci_enable_msix() too ?
> 
> Yes, good catch, care to respin the patch and give it a good changelog
> entry?

Here you are:

The PCI_BUS_FLAGS_NO_MSI bus flags does not appear do be inherited
correctly from the amd8131 MSI quirk to its parent busses. It makes
devices behind a bridge behind amd8131 try to enable MSI while the
amd8131 does not support it.
We fix this by looking at flags of all parent busses in
pci_enable_msi() and pci_enable_msix().

By the way, also add the missing dev->no_msi check in pci_enable_msix()

Signed-off-by: Brice Goglin <brice@myri.com>

Index: linux-mm/drivers/pci/msi.c
===================================================================
--- linux-mm.orig/drivers/pci/msi.c	2006-05-21 15:12:04.000000000 +0200
+++ linux-mm/drivers/pci/msi.c	2006-05-23 08:31:02.000000000 +0200
@@ -916,6 +916,7 @@
  **/
 int pci_enable_msi(struct pci_dev* dev)
 {
+	struct pci_bus *bus;
 	int pos, temp, status = -EINVAL;
 	u16 control;
 
@@ -925,8 +926,9 @@
 	if (dev->no_msi)
 		return status;
 
-	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
-		return -EINVAL;
+	for (bus = dev->bus; bus; bus = bus->parent)
+		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+			return -EINVAL;
 
 	temp = dev->irq;
 
@@ -1162,6 +1164,7 @@
  **/
 int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int nvec)
 {
+	struct pci_bus *bus;
 	int status, pos, nr_entries, free_vectors;
 	int i, j, temp;
 	u16 control;
@@ -1170,6 +1173,13 @@
 	if (!pci_msi_enable || !dev || !entries)
  		return -EINVAL;
 
+	if (dev->no_msi)
+		return -EINVAL;
+
+	for (bus = dev->bus; bus; bus = bus->parent)
+		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+			return -EINVAL;
+
 	status = msi_init();
 	if (status < 0)
 		return status;

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Michael S. Tsirkin]

Quoting r. Greg KH <gregkh@suse.de>:
> > Right, thanks. Greg, what do you think of putting the attached patch in
> > 2.6.17 ?
> 
> Ok, does everyone agree that this patch fixes the issues for them?

Worked here on a PCI-X AMD-8131 based system.

----

Offtopic, something I wanted to bring up with respect to MSI,
but never had the time to debug:

If I do

pci_enable_msix, pci_disable_msix

then later

pci_enable_msi

on the same device fails with the following message:
PCI: 0000:08:00.0: Can't enable MSI.  Device already has MSI-X vectors assigned

This is not something new - has been happening since forever.
Looks like not all MSI-X vectors get properly unassigned by pci_disable_msix.

One way to test this is by loading mthca driver with msi_x=1, unloading
and loading with msi=1.

Someone has any idea what's wrong?

-- 
MST

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Rajesh Shah]

On Wed, May 24, 2006 at 07:59:58PM +0300, Michael S. Tsirkin wrote:
> 
> Offtopic, something I wanted to bring up with respect to MSI,
> but never had the time to debug:
> 
> If I do
> 
> pci_enable_msix, pci_disable_msix
> 
> then later
> 
> pci_enable_msi
> 
> on the same device fails with the following message:
> PCI: 0000:08:00.0: Can't enable MSI.  Device already has MSI-X vectors assigned
> 
> This is not something new - has been happening since forever.
> Looks like not all MSI-X vectors get properly unassigned by pci_disable_msix.
> 
Yes, this has been reported by others too. I've been looking at
MSI code recently to fix an unrelated problem, and noticed that
the code has policies about vector reservation that prevent
what you're trying to do. I'm planning to clean up the MSI
code shortly (patches out hopefully by next week), and will
remove such policies since many people are trying to do this.

thanks,
Rajesh

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Michael S. Tsirkin]

Quoting r. Brice Goglin <brice@myri.com>:
> Subject: Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?
> 
> Michael S. Tsirkin wrote:
> > MSI is an optional feature so things are supposed to work even without MSI - are
> > you getting that great a benefit from MSI?
> >   
> 
> Not great, I would say small.
> 
> > All mellanox PCI-X devices have a bridge inside them, so ...
> >   
> 
> Ok so you really need something for 2.6.17. What about the attached
> patch to fix the fact that bus flags are not inherited ?
> 
> Signed-off-by: Brice Goglin <brice@myri.com>

The following applies this quirk for MSI-X as well.

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

Index: linux-2.6.17-rc4/drivers/pci/msi.c
===================================================================
--- linux-2.6.17-rc4.orig/drivers/pci/msi.c	2006-05-21 12:49:52.000000000 +0300
+++ linux-2.6.17-rc4/drivers/pci/msi.c	2006-05-21 16:14:50.000000000 +0300
@@ -865,6 +865,7 @@ static int msix_capability_init(struct p
  **/
 int pci_enable_msi(struct pci_dev* dev)
 {
+	struct pci_bus *bus;
 	int pos, temp, status = -EINVAL;
 	u16 control;
 
@@ -874,8 +875,9 @@ int pci_enable_msi(struct pci_dev* dev)
 	if (dev->no_msi)
 		return status;
 
-	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
-		return -EINVAL;
+	for (bus = dev->bus; bus; bus = bus->parent)
+		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+			return -EINVAL;
 
 	temp = dev->irq;
 
@@ -1108,6 +1110,7 @@ static int reroute_msix_table(int head, 
  **/
 int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int nvec)
 {
+	struct pci_bus *bus;
 	int status, pos, nr_entries, free_vectors;
 	int i, j, temp;
 	u16 control;
@@ -1116,6 +1119,10 @@ int pci_enable_msix(struct pci_dev* dev,
 	if (!pci_msi_enable || !dev || !entries)
  		return -EINVAL;
 
+	for (bus = dev->bus; bus; bus = bus->parent)
+		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+			return -EINVAL;
+
 	status = msi_init();
 	if (status < 0)
 		return status;

-- 
MST

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Dave Olson]

On Sun, 21 May 2006, Michael S. Tsirkin wrote:
| > Michael is the one who added that change, perhaps he can explain how he
| > tested it?
| 
| Well, I just re-tested with 2.6.17-rc4 and it does not seem to work.  No idea
| what did I do wrong when testing this patch before posting it :(. Oops.
| I'm sorry.
| 
| Given that its late in -rc series, that its a clear regression from 2.6.16, that
| disabling MSI is always safe, and that the patch was intended to enable MSI on

Disabling MSI is not always safe.   The InfiniPath PCIe adapter has no
other way to interrupt, for example.

The original patch (which you are proposing reverting to, or so it
appears to me), causes our chip to not work at all on any motherboard
which has both 8131 and PCIe slots (and there are at least 2 or 3 of
those, including supermicro and HP), despite the fact that the 8131
problem is irrelevant to PCIe on Nvidia or other root complexes.

We (PathScale/QLogic) are having to tell people to patch their
kernels in order for the InfiniPath PCIe adapter to work, on such systems.

Dave Olson
olson@unixfolk.com
http://www.unixfolk.com/dave

Re: AMD 8131 MSI quirk called too late, bus_flags not inherited ?

[Dave Olson]

On Tue, 23 May 2006, Greg KH wrote:
| Ok, does everyone agree that this patch fixes the issues for them?  I've
| had a few other private emails saying that the current code doesn't work
| properly and hadn't been able to determine what was happening.  Thanks
| for these patches.

I can't speak for the people actually hitting the 8131 problem, but
Brice's latest patch does (as would be expected) work for the
InfiniPath card in systems with 8131 and PCIe (our MSI interrupts
work).

Dave Olson
olson@unixfolk.com
http://www.unixfolk.com/dave